From e76f5f104247794696fb4b601ba2baef43408968 Mon Sep 17 00:00:00 2001 From: Christopher <131207452+PetrefiedThunder@users.noreply.github.com> Date: Sat, 24 Oct 2026 00:30:26 -0700 Subject: [PATCH 01/20] feat(goose): AMW StreamableHttp proxy, PreToolUse permit hook, fallback idempotency key (SEL-484) - Proxies @modelcontextprotocol/server-filesystem read_text_file/write_file - Permit rules; Goose PreToolUse hook blocks unpermitted calls before they run - Idempotency key = verified caller + tool + canonical args hash + agent-session-id; agent-tool-call-request-id is logged only (gate showed it changes on retry) - Bounded replay window AMW_IDEMPOTENCY_WINDOW_SECONDS (default 600) - failed_unknown outcomes are never re-run - Ed25519-signed receipts for every decision - Fault injection lives only in tests/e2e (test-only launcher) --- integrations/goose-agent-middleware/LICENSE | 202 ++++++++++++++ integrations/goose-agent-middleware/README.md | 97 +++++++ .../plugin/amw-permit/hooks/hooks.json | 17 ++ .../goose-agent-middleware/pyproject.toml | 31 +++ .../src/goose_amw/__init__.py | 3 + .../src/goose_amw/canonical.py | 37 +++ .../src/goose_amw/hook.py | 60 +++++ .../src/goose_amw/ledger.py | 121 +++++++++ .../src/goose_amw/permits.py | 39 +++ .../src/goose_amw/proxy.py | 247 ++++++++++++++++++ .../src/goose_amw/receipts.py | 92 +++++++ .../tests/e2e/fault_proxy.py | 44 ++++ .../tests/e2e/verify_receipts.py | 28 ++ .../tests/fixtures/g1_gate_calls.jsonl | 3 + .../goose-agent-middleware/tests/test_core.py | 115 ++++++++ 15 files changed, 1136 insertions(+) create mode 100644 integrations/goose-agent-middleware/LICENSE create mode 100644 integrations/goose-agent-middleware/README.md create mode 100644 integrations/goose-agent-middleware/plugin/amw-permit/hooks/hooks.json create mode 100644 integrations/goose-agent-middleware/pyproject.toml create mode 100644 integrations/goose-agent-middleware/src/goose_amw/__init__.py create mode 100644 integrations/goose-agent-middleware/src/goose_amw/canonical.py create mode 100644 integrations/goose-agent-middleware/src/goose_amw/hook.py create mode 100644 integrations/goose-agent-middleware/src/goose_amw/ledger.py create mode 100644 integrations/goose-agent-middleware/src/goose_amw/permits.py create mode 100644 integrations/goose-agent-middleware/src/goose_amw/proxy.py create mode 100644 integrations/goose-agent-middleware/src/goose_amw/receipts.py create mode 100644 integrations/goose-agent-middleware/tests/e2e/fault_proxy.py create mode 100644 integrations/goose-agent-middleware/tests/e2e/verify_receipts.py create mode 100644 integrations/goose-agent-middleware/tests/fixtures/g1_gate_calls.jsonl create mode 100644 integrations/goose-agent-middleware/tests/test_core.py diff --git a/integrations/goose-agent-middleware/LICENSE b/integrations/goose-agent-middleware/LICENSE new file mode 100644 index 00000000..d6456956 --- /dev/null +++ b/integrations/goose-agent-middleware/LICENSE @@ -0,0 +1,202 @@ + + Apache License + Version 2.0, January 2004 + http://www.apache.org/licenses/ + + TERMS AND CONDITIONS FOR USE, REPRODUCTION, AND DISTRIBUTION + + 1. Definitions. + + "License" shall mean the terms and conditions for use, reproduction, + and distribution as defined by Sections 1 through 9 of this document. + + "Licensor" shall mean the copyright owner or entity authorized by + the copyright owner that is granting the License. + + "Legal Entity" shall mean the union of the acting entity and all + other entities that control, are controlled by, or are under common + control with that entity. For the purposes of this definition, + "control" means (i) the power, direct or indirect, to cause the + direction or management of such entity, whether by contract or + otherwise, or (ii) ownership of fifty percent (50%) or more of the + outstanding shares, or (iii) beneficial ownership of such entity. + + "You" (or "Your") shall mean an individual or Legal Entity + exercising permissions granted by this License. + + "Source" form shall mean the preferred form for making modifications, + including but not limited to software source code, documentation + source, and configuration files. + + "Object" form shall mean any form resulting from mechanical + transformation or translation of a Source form, including but + not limited to compiled object code, generated documentation, + and conversions to other media types. + + "Work" shall mean the work of authorship, whether in Source or + Object form, made available under the License, as indicated by a + copyright notice that is included in or attached to the work + (an example is provided in the Appendix below). + + "Derivative Works" shall mean any work, whether in Source or Object + form, that is based on (or derived from) the Work and for which the + editorial revisions, annotations, elaborations, or other modifications + represent, as a whole, an original work of authorship. For the purposes + of this License, Derivative Works shall not include works that remain + separable from, or merely link (or bind by name) to the interfaces of, + the Work and Derivative Works thereof. + + "Contribution" shall mean any work of authorship, including + the original version of the Work and any modifications or additions + to that Work or Derivative Works thereof, that is intentionally + submitted to Licensor for inclusion in the Work by the copyright owner + or by an individual or Legal Entity authorized to submit on behalf of + the copyright owner. For the purposes of this definition, "submitted" + means any form of electronic, verbal, or written communication sent + to the Licensor or its representatives, including but not limited to + communication on electronic mailing lists, source code control systems, + and issue tracking systems that are managed by, or on behalf of, the + Licensor for the purpose of discussing and improving the Work, but + excluding communication that is conspicuously marked or otherwise + designated in writing by the copyright owner as "Not a Contribution." + + "Contributor" shall mean Licensor and any individual or Legal Entity + on behalf of whom a Contribution has been received by Licensor and + subsequently incorporated within the Work. + + 2. Grant of Copyright License. Subject to the terms and conditions of + this License, each Contributor hereby grants to You a perpetual, + worldwide, non-exclusive, no-charge, royalty-free, irrevocable + copyright license to reproduce, prepare Derivative Works of, + publicly display, publicly perform, sublicense, and distribute the + Work and such Derivative Works in Source or Object form. + + 3. Grant of Patent License. Subject to the terms and conditions of + this License, each Contributor hereby grants to You a perpetual, + worldwide, non-exclusive, no-charge, royalty-free, irrevocable + (except as stated in this section) patent license to make, have made, + use, offer to sell, sell, import, and otherwise transfer the Work, + where such license applies only to those patent claims licensable + by such Contributor that are necessarily infringed by their + Contribution(s) alone or by combination of their Contribution(s) + with the Work to which such Contribution(s) was submitted. If You + institute patent litigation against any entity (including a + cross-claim or counterclaim in a lawsuit) alleging that the Work + or a Contribution incorporated within the Work constitutes direct + or contributory patent infringement, then any patent licenses + granted to You under this License for that Work shall terminate + as of the date such litigation is filed. + + 4. Redistribution. You may reproduce and distribute copies of the + Work or Derivative Works thereof in any medium, with or without + modifications, and in Source or Object form, provided that You + meet the following conditions: + + (a) You must give any other recipients of the Work or + Derivative Works a copy of this License; and + + (b) You must cause any modified files to carry prominent notices + stating that You changed the files; and + + (c) You must retain, in the Source form of any Derivative Works + that You distribute, all copyright, patent, trademark, and + attribution notices from the Source form of the Work, + excluding those notices that do not pertain to any part of + the Derivative Works; and + + (d) If the Work includes a "NOTICE" text file as part of its + distribution, then any Derivative Works that You distribute must + include a readable copy of the attribution notices contained + within such NOTICE file, excluding those notices that do not + pertain to any part of the Derivative Works, in at least one + of the following places: within a NOTICE text file distributed + as part of the Derivative Works; within the Source form or + documentation, if provided along with the Derivative Works; or, + within a display generated by the Derivative Works, if and + wherever such third-party notices normally appear. The contents + of the NOTICE file are for informational purposes only and + do not modify the License. You may add Your own attribution + notices within Derivative Works that You distribute, alongside + or as an addendum to the NOTICE text from the Work, provided + that such additional attribution notices cannot be construed + as modifying the License. + + You may add Your own copyright statement to Your modifications and + may provide additional or different license terms and conditions + for use, reproduction, or distribution of Your modifications, or + for any such Derivative Works as a whole, provided Your use, + reproduction, and distribution of the Work otherwise complies with + the conditions stated in this License. + + 5. Submission of Contributions. Unless You explicitly state otherwise, + any Contribution intentionally submitted for inclusion in the Work + by You to the Licensor shall be under the terms and conditions of + this License, without any additional terms or conditions. + Notwithstanding the above, nothing herein shall supersede or modify + the terms of any separate license agreement you may have executed + with Licensor regarding such Contributions. + + 6. Trademarks. This License does not grant permission to use the trade + names, trademarks, service marks, or product names of the Licensor, + except as required for reasonable and customary use in describing the + origin of the Work and reproducing the content of the NOTICE file. + + 7. Disclaimer of Warranty. Unless required by applicable law or + agreed to in writing, Licensor provides the Work (and each + Contributor provides its Contributions) on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or + implied, including, without limitation, any warranties or conditions + of TITLE, NON-INFRINGEMENT, MERCHANTABILITY, or FITNESS FOR A + PARTICULAR PURPOSE. You are solely responsible for determining the + appropriateness of using or redistributing the Work and assume any + risks associated with Your exercise of permissions under this License. + + 8. Limitation of Liability. In no event and under no legal theory, + whether in tort (including negligence), contract, or otherwise, + unless required by applicable law (such as deliberate and grossly + negligent acts) or agreed to in writing, shall any Contributor be + liable to You for damages, including any direct, indirect, special, + incidental, or consequential damages of any character arising as a + result of this License or out of the use or inability to use the + Work (including but not limited to damages for loss of goodwill, + work stoppage, computer failure or malfunction, or any and all + other commercial damages or losses), even if such Contributor + has been advised of the possibility of such damages. + + 9. Accepting Warranty or Additional Liability. While redistributing + the Work or Derivative Works thereof, You may choose to offer, + and charge a fee for, acceptance of support, warranty, indemnity, + or other liability obligations and/or rights consistent with this + License. However, in accepting such obligations, You may act only + on Your own behalf and on Your sole responsibility, not on behalf + of any other Contributor, and only if You agree to indemnify, + defend, and hold each Contributor harmless for any liability + incurred by, or claims asserted against, such Contributor by reason + of your accepting any such warranty or additional liability. + + END OF TERMS AND CONDITIONS + + APPENDIX: How to apply the Apache License to your work. + + To apply the Apache License to your work, attach the following + boilerplate notice, with the fields enclosed by brackets "[]" + replaced with your own identifying information. (Don't include + the brackets!) The text should be enclosed in the appropriate + comment syntax for the file format. We also recommend that a + file or class name and description of purpose be included on the + same "printed page" as the copyright notice for easier + identification within third-party archives. + + Copyright [yyyy] [name of copyright owner] + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. diff --git a/integrations/goose-agent-middleware/README.md b/integrations/goose-agent-middleware/README.md new file mode 100644 index 00000000..5fda0570 --- /dev/null +++ b/integrations/goose-agent-middleware/README.md @@ -0,0 +1,97 @@ +# Goose + Agent Middleware + +Runs Agent Middleware (AMW) as a normal Goose **StreamableHttp MCP extension** +that proxies the official `@modelcontextprotocol/server-filesystem` server +(`read_text_file`, `write_file`), plus a Goose **PreToolUse deny hook**. + +Every proxied call goes through permit -> idempotency -> invoke -> receipt. + +## Pieces + +| Piece | What it does | +|---|---| +| `goose-amw-proxy` | StreamableHttp MCP server (protocol 2026-07-28 and older). Verifies the caller, enforces permits, dedupes writes, signs receipts. | +| `goose-amw-hook` | Goose `PreToolUse` hook. A call with no permit is blocked **before it runs**; Goose shows `Tool call denied by policy hook ...: AMW permit denied: `. | +| `plugin/amw-permit/hooks/hooks.json` | Template plugin that wires the hook to `amw__*` tools. | + +The proxy enforces the same permit rules server-side, so a client without the +hook is still refused. + +## Permit policy + +```json +{"rules": [ + {"id": "write-allowed", "tool": "write_file", "folder": "/path/to/allowed"}, + {"id": "read-sandbox", "tool": "read_text_file", "folder": "/path/to/sandbox"} +]} +``` + +A call is allowed only if a rule names the tool and the resolved `path` is +inside the rule's folder (`..` and symlinks are resolved first). Anything else +is denied with a reason. + +## Idempotency key + +AMW keys a write by **verified caller identity + tool name + canonical +argument hash + `agent-session-id`**. + +Goose (protocol 2026-07-28) sends `_meta["agent-tool-call-request-id"]`, but +that id is the model's tool-call id: when a call fails and the model retries, +Goose sends a **new** id (recorded in `tests/fixtures/g1_gate_calls.jsonl`). +AMW therefore logs it on receipts and in the ledger but never uses it as the +key. Older protocols without `_meta` use the same fallback key; a missing +session id is treated as the empty string. + +The caller is verified from the `Authorization: Bearer` header against +`AMW_CALLERS` (a JSON map of `sha256(token)` to caller id). Unverified calls +are refused. + +## Replay window + +`AMW_IDEMPOTENCY_WINDOW_SECONDS` (default **600**, must be > 0). + +* A completed write repeated with the same key **inside** the window is not + run again; AMW returns the stored result and issues a `replayed` receipt. +* The same write made **after** the window runs normally, so deliberate + identical writes later in a session are not swallowed. +* A write whose outcome is unknown (`failed_unknown`: the call was + dispatched and the response was lost, or AMW crashed mid-call) is **never + re-run automatically**, inside or outside the window. Retries are refused + with a reason until an operator resolves the entry. +* A definite upstream error (`failed`, no effect) may be retried. + +Choose a window longer than your client's retry horizon and shorter than the +interval at which you write identical content on purpose. + +## Receipts + +Each decision (allow, deny, replay, refuse) appends an Ed25519-signed receipt +to `$AMW_STATE_DIR/receipts.jsonl`: `tool`, `args_hash`, `decision`, +`outcome`, `issued_at`, plus caller, session, key and the logged request id. +The signature covers the canonical JSON of `body`; the public key is written +to `receipt_public_key.b64`. + +## Goose setup + +```yaml +extensions: + amw: + type: streamable_http + name: amw + uri: http://127.0.0.1:8791/mcp + headers: + Authorization: Bearer ${AMW_TOKEN} + timeout: 120 + enabled: true +``` + +Install `plugin/amw-permit` into `.agents/plugins/` (project) or the Goose +plugins directory, replacing the paths in its `hooks.json`. + +## Tests + +`pytest` needs no external data. `tests/fixtures/g1_gate_calls.jsonl` is the +real call log recorded by AMW during the G1 gate session. +`tests/e2e/fault_proxy.py` is a test-only launcher that injects one fault into +the shipped proxy to record the cut-stream and unknown-outcome sessions; it +is not part of the package. diff --git a/integrations/goose-agent-middleware/plugin/amw-permit/hooks/hooks.json b/integrations/goose-agent-middleware/plugin/amw-permit/hooks/hooks.json new file mode 100644 index 00000000..6ee29317 --- /dev/null +++ b/integrations/goose-agent-middleware/plugin/amw-permit/hooks/hooks.json @@ -0,0 +1,17 @@ +{ + "hooks": { + "PreToolUse": [ + { + "matcher": "^amw__", + "hooks": [ + { + "type": "command", + "command": "goose-amw-hook --policy /path/to/policy.json --state-dir /path/to/state", + "on_failure": "block", + "timeout": 10 + } + ] + } + ] + } +} diff --git a/integrations/goose-agent-middleware/pyproject.toml b/integrations/goose-agent-middleware/pyproject.toml new file mode 100644 index 00000000..2a7aa957 --- /dev/null +++ b/integrations/goose-agent-middleware/pyproject.toml @@ -0,0 +1,31 @@ +[build-system] +requires = ["hatchling"] +build-backend = "hatchling.build" + +[project] +name = "goose-agent-middleware" +version = "0.1.0" +description = "Agent Middleware as a Goose StreamableHttp MCP extension: permits, PreToolUse deny hook, idempotent writes, signed receipts" +readme = "README.md" +license = "Apache-2.0" +requires-python = ">=3.11" +dependencies = [ + "mcp>=2.3.0,<3", + "cryptography>=50.0.0", +] + +[project.optional-dependencies] +dev = ["pytest", "ruff"] + +[project.scripts] +goose-amw-proxy = "goose_amw.proxy:main" +goose-amw-hook = "goose_amw.hook:main" + +[tool.hatch.build.targets.wheel] +packages = ["src/goose_amw"] + +[tool.ruff] +line-length = 100 + +[tool.pytest.ini_options] +pythonpath = ["src"] diff --git a/integrations/goose-agent-middleware/src/goose_amw/__init__.py b/integrations/goose-agent-middleware/src/goose_amw/__init__.py new file mode 100644 index 00000000..77f8e9e8 --- /dev/null +++ b/integrations/goose-agent-middleware/src/goose_amw/__init__.py @@ -0,0 +1,3 @@ +"""Agent Middleware for Goose: a governed StreamableHttp MCP proxy.""" + +__all__ = ["canonical", "ledger", "permits", "receipts"] diff --git a/integrations/goose-agent-middleware/src/goose_amw/canonical.py b/integrations/goose-agent-middleware/src/goose_amw/canonical.py new file mode 100644 index 00000000..4db4ef54 --- /dev/null +++ b/integrations/goose-agent-middleware/src/goose_amw/canonical.py @@ -0,0 +1,37 @@ +"""Canonical JSON and argument hashing.""" + +from __future__ import annotations + +import hashlib +import json +from typing import Any + + +def canonical_json(payload: Any) -> str: + """Sorted keys, no insignificant whitespace, ASCII only (the AMW signing form).""" + return json.dumps(payload, sort_keys=True, separators=(",", ":"), ensure_ascii=True) + + +def sha256_hex(text: str) -> str: + return hashlib.sha256(text.encode("utf-8")).hexdigest() + + +def args_hash(arguments: dict[str, Any] | None) -> str: + return "sha256:" + sha256_hex(canonical_json(arguments or {})) + + +def idempotency_key( + *, caller: str, tool: str, arguments: dict[str, Any] | None, session_id: str +) -> str: + """Fallback key: verified caller + tool + canonical argument hash + agent-session-id. + + ``agent-tool-call-request-id`` is deliberately NOT an input: the G1 gate showed + Goose issues a new id when the model retries a failed call. + """ + material = { + "caller": caller, + "tool": tool, + "args_hash": args_hash(arguments), + "session_id": session_id, + } + return "amw-fb-" + sha256_hex(canonical_json(material)) diff --git a/integrations/goose-agent-middleware/src/goose_amw/hook.py b/integrations/goose-agent-middleware/src/goose_amw/hook.py new file mode 100644 index 00000000..fb523650 --- /dev/null +++ b/integrations/goose-agent-middleware/src/goose_amw/hook.py @@ -0,0 +1,60 @@ +"""Goose PreToolUse hook: deny a tool call that has no AMW permit. + +Goose runs this before the tool call with the hook context JSON on stdin. +Printing ``{"decision":"block","reason":...}`` stops the call before it +reaches the MCP server; Goose shows the reason to the user and the model. + +Usage in ``hooks/hooks.json``:: + + goose-amw-hook --policy /path/policy.json --state-dir /path/state +""" + +from __future__ import annotations + +import argparse +import json +import sys +from pathlib import Path + +from . import permits +from .canonical import args_hash +from .receipts import ReceiptSigner, load_or_create_key + + +def decide(payload: dict, policy: dict) -> dict: + tool = payload.get("tool_name") or payload.get("matcher_context") or "" + arguments = payload.get("tool_input") or {} + decision = permits.check(policy, tool, arguments) + if decision.allowed: + return {"decision": "allow"} + return {"decision": "block", "reason": f"AMW permit denied: {decision.reason}"} + + +def main(argv: list[str] | None = None) -> int: + ap = argparse.ArgumentParser() + ap.add_argument("--policy", required=True) + ap.add_argument("--state-dir", required=True) + ns = ap.parse_args(argv) + payload = json.load(sys.stdin) + out = decide(payload, permits.load_policy(ns.policy)) + if out["decision"] == "block": + state = Path(ns.state_dir) + signer = ReceiptSigner( + load_or_create_key(state / "receipt_signing_key.pem"), state / "receipts.jsonl" + ) + signer.issue( + tool=payload.get("tool_name") or "", + args_hash=args_hash(payload.get("tool_input") or {}), + decision="deny", + outcome="not_run", + enforced_by="goose_pre_tool_use_hook", + reason=out["reason"], + session_id=payload.get("session_id"), + agent_tool_call_request_id=payload.get("tool_call_id"), + ) + print(json.dumps(out)) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/integrations/goose-agent-middleware/src/goose_amw/ledger.py b/integrations/goose-agent-middleware/src/goose_amw/ledger.py new file mode 100644 index 00000000..7c13dedd --- /dev/null +++ b/integrations/goose-agent-middleware/src/goose_amw/ledger.py @@ -0,0 +1,121 @@ +"""SQLite idempotency ledger with a bounded replay window. + +States: ``in_flight`` -> ``completed`` | ``failed`` | ``failed_unknown``. + +* ``completed``: a retry inside the window replays the stored result; the tool + does not run again. +* ``failed``: the upstream answered with a definite error (no effect); a retry + may run again. +* ``failed_unknown``: the outcome is unclear (transport lost after dispatch). + It is never re-run automatically, inside or outside the window, until an + operator resolves it. +* A ``completed`` entry older than the window no longer dedupes, so an + identical write made on purpose much later runs normally. +""" + +from __future__ import annotations + +import sqlite3 +import threading +import time +from dataclasses import dataclass +from pathlib import Path + +DEFAULT_WINDOW_SECONDS = 600 + + +@dataclass(frozen=True) +class Claim: + """Result of trying to claim a key before running a tool.""" + + action: str # "run" | "replay" | "refuse" + result: str | None = None + status: str | None = None + + +class Ledger: + def __init__(self, path: str | Path, window_seconds: float = DEFAULT_WINDOW_SECONDS): + if window_seconds <= 0: + raise ValueError("window_seconds must be positive") + self.window_seconds = window_seconds + self._lock = threading.Lock() + self._db = sqlite3.connect(str(path), check_same_thread=False, isolation_level=None) + self._db.execute( + "CREATE TABLE IF NOT EXISTS calls (" + " key TEXT PRIMARY KEY, tool TEXT NOT NULL, args_hash TEXT NOT NULL," + " status TEXT NOT NULL, result TEXT, first_request_id TEXT," + " created_at REAL NOT NULL, updated_at REAL NOT NULL, executions INTEGER NOT NULL)" + ) + + def claim( + self, + key: str, + *, + tool: str, + args_hash: str, + request_id: str | None, + now: float | None = None, + ) -> Claim: + now = time.time() if now is None else now + with self._lock: + self._db.execute("BEGIN IMMEDIATE") + try: + row = self._db.execute( + "SELECT status, result, created_at FROM calls WHERE key = ?", (key,) + ).fetchone() + if row is not None: + status, result, created_at = row + if status == "failed_unknown": + self._db.execute("COMMIT") + return Claim("refuse", status=status) + if status == "in_flight": + self._db.execute("COMMIT") + return Claim("refuse", status=status) + if status == "completed" and now - created_at < self.window_seconds: + self._db.execute("COMMIT") + return Claim("replay", result=result, status=status) + # failed (no effect) or completed outside the window: run again. + self._db.execute( + "UPDATE calls SET status='in_flight', result=NULL, created_at=?," + " updated_at=?, executions=executions+1 WHERE key=?", + (now, now, key), + ) + else: + self._db.execute( + "INSERT INTO calls VALUES (?, ?, ?, 'in_flight', NULL, ?, ?, ?, 1)", + (key, tool, args_hash, request_id, now, now), + ) + self._db.execute("COMMIT") + return Claim("run") + except BaseException: + self._db.execute("ROLLBACK") + raise + + def finish(self, key: str, status: str, result: str | None = None) -> None: + if status not in ("completed", "failed", "failed_unknown"): + raise ValueError(status) + with self._lock: + self._db.execute( + "UPDATE calls SET status=?, result=?, updated_at=? WHERE key=?", + (status, result, time.time(), key), + ) + + def mark_stale_in_flight_unknown(self) -> int: + """At startup, an ``in_flight`` row means a crash mid-call: outcome unknown.""" + with self._lock: + cur = self._db.execute( + "UPDATE calls SET status='failed_unknown', updated_at=? WHERE status='in_flight'", + (time.time(),), + ) + return cur.rowcount + + def get(self, key: str) -> dict | None: + cur = self._db.execute( + "SELECT key, tool, status, result, executions, first_request_id FROM calls WHERE key=?", + (key,), + ) + row = cur.fetchone() + if row is None: + return None + names = ["key", "tool", "status", "result", "executions", "first_request_id"] + return dict(zip(names, row, strict=True)) diff --git a/integrations/goose-agent-middleware/src/goose_amw/permits.py b/integrations/goose-agent-middleware/src/goose_amw/permits.py new file mode 100644 index 00000000..a85529a5 --- /dev/null +++ b/integrations/goose-agent-middleware/src/goose_amw/permits.py @@ -0,0 +1,39 @@ +"""Path-scoped permit rules shared by the proxy and the Goose PreToolUse hook.""" + +from __future__ import annotations + +import json +from dataclasses import dataclass +from pathlib import Path +from typing import Any + + +@dataclass(frozen=True) +class Decision: + allowed: bool + reason: str + + +def load_policy(path: str | Path) -> dict[str, Any]: + return json.loads(Path(path).read_text()) + + +def _bare(tool: str) -> str: + # Goose prefixes extension tools as "__". + return tool.split("__", 1)[1] if "__" in tool else tool + + +def check(policy: dict[str, Any], tool: str, arguments: dict[str, Any] | None) -> Decision: + """Allow only when a rule grants ``tool`` on a path inside its folder.""" + name = _bare(tool) + raw = (arguments or {}).get("path") + if not isinstance(raw, str) or not raw: + return Decision(False, f"no permit: {name} call has no path argument") + target = Path(raw).expanduser().resolve() + for rule in policy.get("rules", []): + if rule.get("tool") != name: + continue + folder = Path(rule["folder"]).expanduser().resolve() + if target.is_relative_to(folder): + return Decision(True, f"permit {rule.get('id', name)} allows {name} in {folder}") + return Decision(False, f"no permit for {name} on {target}: outside every permitted folder") diff --git a/integrations/goose-agent-middleware/src/goose_amw/proxy.py b/integrations/goose-agent-middleware/src/goose_amw/proxy.py new file mode 100644 index 00000000..21f6edb1 --- /dev/null +++ b/integrations/goose-agent-middleware/src/goose_amw/proxy.py @@ -0,0 +1,247 @@ +"""AMW StreamableHttp MCP server for Goose. + +Proxies one read tool and one write tool to the official +``@modelcontextprotocol/server-filesystem`` server. Every call is checked +against the permit policy, keyed for idempotency with the fallback key +(verified caller + tool + canonical argument hash + agent-session-id), and +receives a signed receipt. ``agent-tool-call-request-id`` is logged only. + +Configuration (environment): + +* ``AMW_POLICY`` - permit policy JSON (see README) +* ``AMW_STATE_DIR`` - ledger, receipts and signing key +* ``AMW_CALLERS`` - JSON map of sha256(bearer token) -> caller id +* ``AMW_FS_ROOT`` - root directory handed to the filesystem server +* ``AMW_IDEMPOTENCY_WINDOW_SECONDS`` - replay window, default 600 +* ``AMW_HOST`` / ``AMW_PORT`` - bind address, default 127.0.0.1:8791 +""" + +from __future__ import annotations + +import hmac +import json +import logging +import os +from collections.abc import Awaitable, Callable, Mapping +from dataclasses import dataclass +from pathlib import Path +from typing import Any + +from mcp.server.mcpserver import Context, MCPServer +from mcp.server.mcpserver.exceptions import ToolError + +from . import permits +from .canonical import args_hash, idempotency_key, sha256_hex +from .ledger import DEFAULT_WINDOW_SECONDS, Ledger +from .receipts import ReceiptSigner, load_or_create_key + +log = logging.getLogger("goose_amw") + +REQUEST_ID_KEY = "agent-tool-call-request-id" +SESSION_ID_KEY = "agent-session-id" + + +class UpstreamError(Exception): + """The upstream answered with a definite error: the call had no effect.""" + + +class UpstreamUnknown(Exception): + """The call was dispatched but its outcome is unknown.""" + + +Upstream = Callable[[str, dict[str, Any]], Awaitable[str]] + + +def filesystem_upstream(fs_root: str) -> Upstream: + from mcp.client.session import ClientSession + from mcp.client.stdio import StdioServerParameters, stdio_client + + params = StdioServerParameters( + command="npx", args=["-y", "@modelcontextprotocol/server-filesystem", fs_root] + ) + + async def call(tool: str, arguments: dict[str, Any]) -> str: + try: + ctx = stdio_client(params) + read, write = await ctx.__aenter__() + except Exception as exc: # never dispatched + raise UpstreamError(f"filesystem server unavailable: {exc}") from exc + try: + async with ClientSession(read, write) as session: + await session.initialize() + try: + res = await session.call_tool(tool, arguments) + except Exception as exc: + raise UpstreamUnknown(f"{type(exc).__name__}: {exc}") from exc + finally: + try: + await ctx.__aexit__(None, None, None) + except Exception: + log.warning("filesystem server shutdown error", exc_info=True) + text = "\n".join(getattr(c, "text", "") for c in res.content) + if res.is_error: + raise UpstreamError(text) + return text + + return call + + +@dataclass +class Config: + policy: dict[str, Any] + state_dir: Path + callers: dict[str, str] + fs_root: str + window_seconds: float = DEFAULT_WINDOW_SECONDS + + @classmethod + def from_env(cls) -> Config: + return cls( + policy=permits.load_policy(os.environ["AMW_POLICY"]), + state_dir=Path(os.environ["AMW_STATE_DIR"]), + callers=json.loads(Path(os.environ["AMW_CALLERS"]).read_text()), + fs_root=os.environ["AMW_FS_ROOT"], + window_seconds=float( + os.environ.get("AMW_IDEMPOTENCY_WINDOW_SECONDS", DEFAULT_WINDOW_SECONDS) + ), + ) + + +def verify_caller(headers: Mapping[str, str] | None, callers: dict[str, str]) -> str: + auth = (headers or {}).get("authorization", "") + if not auth.lower().startswith("bearer "): + raise ToolError("AMW refused: caller not verified (missing bearer token)") + digest = sha256_hex(auth[7:].strip()) + for known, caller in callers.items(): + if hmac.compare_digest(known, digest): + return caller + raise ToolError("AMW refused: caller not verified (unknown token)") + + +def _meta(ctx: Context) -> dict[str, Any]: + m = ctx.request_context.meta + if m is None: + return {} + return dict(m) if isinstance(m, dict) else m.model_dump(by_alias=True, exclude_none=True) + + +class Gateway: + """Permit -> idempotency -> invoke -> receipt for each proxied call.""" + + def __init__( + self, + config: Config, + upstream: Upstream | None = None, + after_commit: Callable[[str, str], Awaitable[None]] | None = None, + ): + config.state_dir.mkdir(parents=True, exist_ok=True) + self.config = config + self.ledger = Ledger(config.state_dir / "ledger.sqlite3", config.window_seconds) + stale = self.ledger.mark_stale_in_flight_unknown() + if stale: + log.warning("marked %d interrupted call(s) failed_unknown", stale) + self.signer = ReceiptSigner( + load_or_create_key(config.state_dir / "receipt_signing_key.pem"), + config.state_dir / "receipts.jsonl", + ) + (config.state_dir / "receipt_public_key.b64").write_text(self.signer.public_key_b64) + self.upstream = upstream or filesystem_upstream(config.fs_root) + # Test-only seam: tests inject faults here. Never set in production. + self.after_commit = after_commit + + async def handle(self, tool: str, arguments: dict[str, Any], ctx: Context, *, write: bool): + meta = _meta(ctx) + request_id = meta.get(REQUEST_ID_KEY) + session_id = meta.get(SESSION_ID_KEY) or "" + ahash = args_hash(arguments) + base = {"protocol": ctx.protocol_version, "agent_tool_call_request_id": request_id} + caller = verify_caller(ctx.headers, self.config.callers) + base.update(caller=caller, session_id=session_id) + + decision = permits.check(self.config.policy, tool, arguments) + if not decision.allowed: + self.signer.issue( + tool=tool, + args_hash=ahash, + decision="deny", + outcome="not_run", + reason=decision.reason, + **base, + ) + raise ToolError(f"AMW permit denied: {decision.reason}") + + key = idempotency_key(caller=caller, tool=tool, arguments=arguments, session_id=session_id) + base["idempotency_key"] = key + if not write: + return await self._run(tool, arguments, ahash, base, key=None) + + claim = self.ledger.claim(key, tool=tool, args_hash=ahash, request_id=request_id) + if claim.action == "replay": + self.signer.issue( + tool=tool, args_hash=ahash, decision="allow", outcome="replayed", **base + ) + return claim.result + if claim.action == "refuse": + self.signer.issue( + tool=tool, args_hash=ahash, decision="refuse", outcome=claim.status, **base + ) + raise ToolError( + f"AMW refused to re-run: an earlier identical {tool} call is {claim.status}; " + "its outcome is unknown, so it will not run again automatically" + ) + return await self._run(tool, arguments, ahash, base, key=key) + + async def _run(self, tool, arguments, ahash, base, *, key): + try: + result = await self.upstream(tool, arguments) + except UpstreamError as exc: + if key: + self.ledger.finish(key, "failed") + self.signer.issue( + tool=tool, args_hash=ahash, decision="allow", outcome="failed", **base + ) + raise ToolError(f"upstream error: {exc}") from exc + except BaseException: + if key: + self.ledger.finish(key, "failed_unknown") + self.signer.issue( + tool=tool, args_hash=ahash, decision="allow", outcome="failed_unknown", **base + ) + raise + if key: + self.ledger.finish(key, "completed", result) + self.signer.issue(tool=tool, args_hash=ahash, decision="allow", outcome="completed", **base) + if key and self.after_commit is not None: + await self.after_commit(tool, key) + return result + + +def build_server(gateway: Gateway) -> MCPServer: + mcp = MCPServer("amw") + + @mcp.tool() + async def read_text_file(path: str, ctx: Context) -> str: + """Read a UTF-8 text file through AMW (permit-checked, receipted).""" + return await gateway.handle("read_text_file", {"path": path}, ctx, write=False) + + @mcp.tool() + async def write_file(path: str, content: str, ctx: Context) -> str: + """Write a text file through AMW (permit-checked, idempotent, receipted).""" + args = {"path": path, "content": content} + return await gateway.handle("write_file", args, ctx, write=True) + + return mcp + + +def main() -> None: + logging.basicConfig(level=logging.INFO) + server = build_server(Gateway(Config.from_env())) + server.run( + transport="streamable-http", + host=os.environ.get("AMW_HOST", "127.0.0.1"), + port=int(os.environ.get("AMW_PORT", "8791")), + ) + + +if __name__ == "__main__": + main() diff --git a/integrations/goose-agent-middleware/src/goose_amw/receipts.py b/integrations/goose-agent-middleware/src/goose_amw/receipts.py new file mode 100644 index 00000000..96ac674e --- /dev/null +++ b/integrations/goose-agent-middleware/src/goose_amw/receipts.py @@ -0,0 +1,92 @@ +"""Ed25519-signed receipts, one JSON line per tool call decision.""" + +from __future__ import annotations + +import base64 +import json +import os +import time +import uuid +from pathlib import Path +from typing import Any + +from cryptography.exceptions import InvalidSignature +from cryptography.hazmat.primitives import serialization +from cryptography.hazmat.primitives.asymmetric.ed25519 import ( + Ed25519PrivateKey, + Ed25519PublicKey, +) + +from .canonical import canonical_json, sha256_hex + + +def load_or_create_key(path: str | Path) -> Ed25519PrivateKey: + p = Path(path) + if p.exists(): + key = serialization.load_pem_private_key(p.read_bytes(), password=None) + assert isinstance(key, Ed25519PrivateKey) + return key + key = Ed25519PrivateKey.generate() + p.parent.mkdir(parents=True, exist_ok=True) + fd = os.open(p, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600) + with os.fdopen(fd, "wb") as f: + f.write( + key.private_bytes( + serialization.Encoding.PEM, + serialization.PrivateFormat.PKCS8, + serialization.NoEncryption(), + ) + ) + return key + + +def _raw_public(key: Ed25519PublicKey) -> bytes: + return key.public_bytes(serialization.Encoding.Raw, serialization.PublicFormat.Raw) + + +class ReceiptSigner: + def __init__(self, key: Ed25519PrivateKey, log_path: str | Path): + self._key = key + self._log = Path(log_path) + pub = _raw_public(key.public_key()) + self.public_key_b64 = base64.b64encode(pub).decode() + self.kid = "amw-goose-" + sha256_hex(self.public_key_b64)[:16] + + def issue( + self, + *, + tool: str, + args_hash: str, + decision: str, + outcome: str, + **extra: Any, + ) -> dict[str, Any]: + body = { + "receipt_id": str(uuid.uuid4()), + "tool": tool, + "args_hash": args_hash, + "decision": decision, + "outcome": outcome, + "issued_at": time.strftime("%Y-%m-%dT%H:%M:%S", time.gmtime()) + "Z", + "alg": "Ed25519", + "kid": self.kid, + **{k: v for k, v in extra.items() if v is not None}, + } + sig = self._key.sign(canonical_json(body).encode()) + receipt = {"body": body, "signature": base64.b64encode(sig).decode()} + self._log.parent.mkdir(parents=True, exist_ok=True) + with self._log.open("a") as f: + f.write(json.dumps(receipt) + "\n") + return receipt + + +def verify(receipt: dict[str, Any], public_key_b64: str) -> bool: + pub = Ed25519PublicKey.from_public_bytes(base64.b64decode(public_key_b64)) + try: + pub.verify( + base64.b64decode(receipt["signature"]), + canonical_json(receipt["body"]).encode(), + ) + except (InvalidSignature, ValueError, KeyError): + return False + return True diff --git a/integrations/goose-agent-middleware/tests/e2e/fault_proxy.py b/integrations/goose-agent-middleware/tests/e2e/fault_proxy.py new file mode 100644 index 00000000..3f53c934 --- /dev/null +++ b/integrations/goose-agent-middleware/tests/e2e/fault_proxy.py @@ -0,0 +1,44 @@ +"""TEST-ONLY launcher: the shipped proxy plus one injected fault. + +Not part of the package. Used to record the real Goose sessions in the PR: + +* ``AMW_TEST_FAULT=cut_after_commit`` - the first write runs and commits, then + its response stream is closed before the result reaches Goose. +* ``AMW_TEST_FAULT=lose_upstream_response`` - the first write is dispatched + to the filesystem server, then its response is dropped (outcome unknown). +""" + +import os + +from goose_amw.proxy import ( + Config, + Gateway, + UpstreamUnknown, + build_server, + filesystem_upstream, +) + +FAULT = os.environ["AMW_TEST_FAULT"] +cfg = Config.from_env() +real = filesystem_upstream(cfg.fs_root) +fired = [] + + +async def upstream(tool, args): + result = await real(tool, args) + if FAULT == "lose_upstream_response" and tool == "write_file" and not fired: + fired.append(tool) + raise UpstreamUnknown("test fault: upstream response dropped after dispatch") + return result + + +async def after_commit(tool, key): + if FAULT == "cut_after_commit" and not fired: + fired.append(key) + raise ConnectionAbortedError("test fault: response stream cut after commit") + + +gw = Gateway(cfg, upstream=upstream, after_commit=after_commit) +build_server(gw).run( + transport="streamable-http", host="127.0.0.1", port=int(os.environ["AMW_PORT"]) +) diff --git a/integrations/goose-agent-middleware/tests/e2e/verify_receipts.py b/integrations/goose-agent-middleware/tests/e2e/verify_receipts.py new file mode 100644 index 00000000..480657ae --- /dev/null +++ b/integrations/goose-agent-middleware/tests/e2e/verify_receipts.py @@ -0,0 +1,28 @@ +"""Verify every receipt in an AMW state dir and print a compact summary.""" + +import json +import sys +from pathlib import Path + +from goose_amw.receipts import verify + +state = Path(sys.argv[1]) +pub = (state / "receipt_public_key.b64").read_text().strip() +bad = 0 +for line in (state / "receipts.jsonl").read_text().splitlines(): + r = json.loads(line) + ok = verify(r, pub) + bad += not ok + b = r["body"] + print( + "OK " if ok else "BAD", + b["issued_at"], + b["tool"], + b["decision"], + b["outcome"], + b["args_hash"][:19], + b.get("enforced_by", "proxy"), + b.get("agent_tool_call_request_id") or "-", + b.get("protocol", "-"), + ) +sys.exit(1 if bad else 0) diff --git a/integrations/goose-agent-middleware/tests/fixtures/g1_gate_calls.jsonl b/integrations/goose-agent-middleware/tests/fixtures/g1_gate_calls.jsonl new file mode 100644 index 00000000..ab3fa361 --- /dev/null +++ b/integrations/goose-agent-middleware/tests/fixtures/g1_gate_calls.jsonl @@ -0,0 +1,3 @@ +{"tool": "write_file", "path": "/Users/sellers/scratch/g1-sandbox/allowed/gate.txt", "protocol": "2026-07-28", "meta": {"progress_token": 1, "agent-session-id": "20261024_4", "agent-working-dir": "/Users/sellers/scratch/g1-sandbox", "agent-tool-call-request-id": "tool_CpDpciFxHvt0zB4Vgyo0wXHe", "io.modelcontextprotocol/protocolVersion": "2026-07-28", "io.modelcontextprotocol/clientInfo": {"name": "goose-cli", "version": "1.56.0"}, "io.modelcontextprotocol/clientCapabilities": {"extensions": {}, "roots": {}, "elicitation": {}}}, "decision": "dropped_mid_stream", "ts": 1792826481.62643} +{"tool": "write_file", "path": "/Users/sellers/scratch/g1-sandbox/allowed/gate.txt", "protocol": "2026-07-28", "meta": {"progress_token": 2, "agent-session-id": "20261024_4", "agent-working-dir": "/Users/sellers/scratch/g1-sandbox", "agent-tool-call-request-id": "tool_Cn0pLB5b7j1weRQrhby6UOhB", "io.modelcontextprotocol/protocolVersion": "2026-07-28", "io.modelcontextprotocol/clientInfo": {"name": "goose-cli", "version": "1.56.0"}, "io.modelcontextprotocol/clientCapabilities": {"extensions": {}, "roots": {}, "elicitation": {}}}, "decision": "allow", "ts": 1792826485.893873} +{"tool": "read_text_file", "path": "/Users/sellers/scratch/g1-sandbox/allowed/gate.txt", "protocol": "2026-07-28", "meta": {"progress_token": 3, "agent-session-id": "20261024_4", "agent-working-dir": "/Users/sellers/scratch/g1-sandbox", "agent-tool-call-request-id": "tool_KAT1peagg5d6MBVBcMhucmZy", "io.modelcontextprotocol/protocolVersion": "2026-07-28", "io.modelcontextprotocol/clientInfo": {"name": "goose-cli", "version": "1.56.0"}, "io.modelcontextprotocol/clientCapabilities": {"extensions": {}, "roots": {}, "elicitation": {}}}, "ts": 1792826493.060245} diff --git a/integrations/goose-agent-middleware/tests/test_core.py b/integrations/goose-agent-middleware/tests/test_core.py new file mode 100644 index 00000000..9a2b23a3 --- /dev/null +++ b/integrations/goose-agent-middleware/tests/test_core.py @@ -0,0 +1,115 @@ +import json +from pathlib import Path + +import pytest + +from goose_amw import hook, permits +from goose_amw.canonical import args_hash, canonical_json, idempotency_key +from goose_amw.ledger import Ledger +from goose_amw.receipts import ReceiptSigner, load_or_create_key, verify + +FIXTURE = Path(__file__).parent / "fixtures" / "g1_gate_calls.jsonl" + + +def test_canonical_hash_ignores_key_order(): + assert canonical_json({"b": 1, "a": [2]}) == '{"a":[2],"b":1}' + assert args_hash({"path": "p", "content": "c"}) == args_hash({"content": "c", "path": "p"}) + + +def test_key_inputs(): + k = idempotency_key(caller="c", tool="write_file", arguments={"x": 1}, session_id="s") + assert k == idempotency_key(caller="c", tool="write_file", arguments={"x": 1}, session_id="s") + assert k != idempotency_key(caller="d", tool="write_file", arguments={"x": 1}, session_id="s") + assert k != idempotency_key( + caller="c", tool="read_text_file", arguments={"x": 1}, session_id="s" + ) + assert k != idempotency_key(caller="c", tool="write_file", arguments={"x": 2}, session_id="s") + assert k != idempotency_key(caller="c", tool="write_file", arguments={"x": 1}, session_id="t") + + +def test_recorded_gate_retry_maps_to_same_key_despite_new_request_id(): + """Real G1 gate evidence: Goose sent a new request id on the model's retry.""" + writes = [ + r for r in map(json.loads, FIXTURE.read_text().splitlines()) if r["tool"] == "write_file" + ] + assert len(writes) == 2 + ids = [w["meta"]["agent-tool-call-request-id"] for w in writes] + assert ids[0] != ids[1] + assert all(w["protocol"] == "2026-07-28" for w in writes) + args = {"path": writes[0]["path"], "content": "G1 gate"} + keys = { + idempotency_key( + caller="goose-local", + tool="write_file", + arguments=args, + session_id=w["meta"]["agent-session-id"], + ) + for w in writes + } + assert len(keys) == 1 + + +def test_ledger_replays_inside_window_and_reruns_after(tmp_path): + led = Ledger(tmp_path / "l.db", window_seconds=10) + assert led.claim("k", tool="t", args_hash="h", request_id="r1", now=100).action == "run" + led.finish("k", "completed", "ok") + c = led.claim("k", tool="t", args_hash="h", request_id="r2", now=105) + assert (c.action, c.result) == ("replay", "ok") + assert led.claim("k", tool="t", args_hash="h", request_id="r3", now=111).action == "run" + assert led.get("k")["executions"] == 2 + + +def test_failed_unknown_is_never_rerun(tmp_path): + led = Ledger(tmp_path / "l.db", window_seconds=10) + led.claim("k", tool="t", args_hash="h", request_id=None, now=0) + led.finish("k", "failed_unknown") + for now in (1, 10_000): + assert led.claim("k", tool="t", args_hash="h", request_id=None, now=now).action == "refuse" + assert led.get("k")["executions"] == 1 + + +def test_crash_in_flight_becomes_failed_unknown(tmp_path): + led = Ledger(tmp_path / "l.db") + led.claim("k", tool="t", args_hash="h", request_id=None) + assert Ledger(tmp_path / "l.db").mark_stale_in_flight_unknown() == 1 + assert led.get("k")["status"] == "failed_unknown" + + +def test_definite_failure_may_rerun(tmp_path): + led = Ledger(tmp_path / "l.db") + led.claim("k", tool="t", args_hash="h", request_id=None) + led.finish("k", "failed") + assert led.claim("k", tool="t", args_hash="h", request_id=None).action == "run" + + +def test_window_must_be_positive(tmp_path): + with pytest.raises(ValueError): + Ledger(tmp_path / "l.db", window_seconds=0) + + +POLICY = {"rules": [{"id": "w", "tool": "write_file", "folder": "/tmp/amw-allowed"}]} + + +def test_permits(): + assert permits.check(POLICY, "amw__write_file", {"path": "/tmp/amw-allowed/a"}).allowed + assert not permits.check(POLICY, "amw__write_file", {"path": "/tmp/amw-allowed/../x"}).allowed + assert not permits.check(POLICY, "amw__write_file", {"path": "/etc/x"}).allowed + assert not permits.check(POLICY, "amw__read_text_file", {"path": "/tmp/amw-allowed/a"}).allowed + assert not permits.check(POLICY, "write_file", {}).allowed + + +def test_hook_blocks_with_reason_and_allows(): + out = hook.decide({"tool_name": "amw__write_file", "tool_input": {"path": "/etc/x"}}, POLICY) + assert out["decision"] == "block" and "no permit" in out["reason"] + ok = {"tool_name": "amw__write_file", "tool_input": {"path": "/tmp/amw-allowed/a"}} + assert hook.decide(ok, POLICY) == {"decision": "allow"} + + +def test_receipts_are_signed_and_tamper_evident(tmp_path): + signer = ReceiptSigner(load_or_create_key(tmp_path / "k.pem"), tmp_path / "r.jsonl") + r = signer.issue(tool="write_file", args_hash="sha256:x", decision="allow", outcome="completed") + assert {"tool", "args_hash", "decision", "issued_at"} <= r["body"].keys() + assert verify(r, signer.public_key_b64) + r["body"]["decision"] = "deny" + assert not verify(r, signer.public_key_b64) + assert (tmp_path / "k.pem").stat().st_mode & 0o077 == 0 From 0392a061b646a8fcb71e765d096c7b50d3edeb58 Mon Sep 17 00:00:00 2001 From: Christopher <131207452+PetrefiedThunder@users.noreply.github.com> Date: Sat, 24 Oct 2026 00:47:20 -0700 Subject: [PATCH 02/20] fix(goose): depend on amw-sdk>=0.4.0 and reuse its canonical_json (onboarding contract) --- integrations/goose-agent-middleware/pyproject.toml | 6 +++++- .../goose-agent-middleware/src/goose_amw/canonical.py | 9 ++++----- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/integrations/goose-agent-middleware/pyproject.toml b/integrations/goose-agent-middleware/pyproject.toml index 2a7aa957..97f3c65d 100644 --- a/integrations/goose-agent-middleware/pyproject.toml +++ b/integrations/goose-agent-middleware/pyproject.toml @@ -10,6 +10,7 @@ readme = "README.md" license = "Apache-2.0" requires-python = ">=3.11" dependencies = [ + "amw-sdk>=0.4.0", "mcp>=2.3.0,<3", "cryptography>=50.0.0", ] @@ -28,4 +29,7 @@ packages = ["src/goose_amw"] line-length = 100 [tool.pytest.ini_options] -pythonpath = ["src"] +pythonpath = ["src", "../../sdk/python/src"] + +[tool.uv.sources] +amw-sdk = { path = "../../sdk/python", editable = true } diff --git a/integrations/goose-agent-middleware/src/goose_amw/canonical.py b/integrations/goose-agent-middleware/src/goose_amw/canonical.py index 4db4ef54..746d3565 100644 --- a/integrations/goose-agent-middleware/src/goose_amw/canonical.py +++ b/integrations/goose-agent-middleware/src/goose_amw/canonical.py @@ -3,13 +3,9 @@ from __future__ import annotations import hashlib -import json from typing import Any - -def canonical_json(payload: Any) -> str: - """Sorted keys, no insignificant whitespace, ASCII only (the AMW signing form).""" - return json.dumps(payload, sort_keys=True, separators=(",", ":"), ensure_ascii=True) +from b2a_sdk.receipt_verifier import canonical_json def sha256_hex(text: str) -> str: @@ -35,3 +31,6 @@ def idempotency_key( "session_id": session_id, } return "amw-fb-" + sha256_hex(canonical_json(material)) + + +__all__ = ["args_hash", "canonical_json", "idempotency_key", "sha256_hex"] From 360d864fad0544e1ccefd905461bdc2ce6655c33 Mon Sep 17 00:00:00 2001 From: Christopher <131207452+PetrefiedThunder@users.noreply.github.com> Date: Sat, 24 Oct 2026 01:01:19 -0700 Subject: [PATCH 03/20] fix(goose): real-data-only tests; refuse writes without agent-session-id (review 5637421971) --- integrations/goose-agent-middleware/README.md | 5 +- .../src/goose_amw/proxy.py | 22 +- .../goose-agent-middleware/tests/test_core.py | 200 +++++++++++++----- 3 files changed, 166 insertions(+), 61 deletions(-) diff --git a/integrations/goose-agent-middleware/README.md b/integrations/goose-agent-middleware/README.md index 5fda0570..63110bbd 100644 --- a/integrations/goose-agent-middleware/README.md +++ b/integrations/goose-agent-middleware/README.md @@ -39,8 +39,9 @@ Goose (protocol 2026-07-28) sends `_meta["agent-tool-call-request-id"]`, but that id is the model's tool-call id: when a call fails and the model retries, Goose sends a **new** id (recorded in `tests/fixtures/g1_gate_calls.jsonl`). AMW therefore logs it on receipts and in the ledger but never uses it as the -key. Older protocols without `_meta` use the same fallback key; a missing -session id is treated as the empty string. +key. A **write without `agent-session-id`** (for example an older protocol with +no `_meta`) is refused with a clear reason and a `refuse` receipt, because it +cannot be deduplicated safely. Reads do not need a session id. The caller is verified from the `Authorization: Bearer` header against `AMW_CALLERS` (a JSON map of `sha256(token)` to caller id). Unverified calls diff --git a/integrations/goose-agent-middleware/src/goose_amw/proxy.py b/integrations/goose-agent-middleware/src/goose_amw/proxy.py index 21f6edb1..07431244 100644 --- a/integrations/goose-agent-middleware/src/goose_amw/proxy.py +++ b/integrations/goose-agent-middleware/src/goose_amw/proxy.py @@ -152,7 +152,7 @@ def __init__( async def handle(self, tool: str, arguments: dict[str, Any], ctx: Context, *, write: bool): meta = _meta(ctx) request_id = meta.get(REQUEST_ID_KEY) - session_id = meta.get(SESSION_ID_KEY) or "" + session_id = meta.get(SESSION_ID_KEY) or None ahash = args_hash(arguments) base = {"protocol": ctx.protocol_version, "agent_tool_call_request_id": request_id} caller = verify_caller(ctx.headers, self.config.callers) @@ -170,11 +170,27 @@ async def handle(self, tool: str, arguments: dict[str, Any], ctx: Context, *, wr ) raise ToolError(f"AMW permit denied: {decision.reason}") - key = idempotency_key(caller=caller, tool=tool, arguments=arguments, session_id=session_id) - base["idempotency_key"] = key if not write: return await self._run(tool, arguments, ahash, base, key=None) + if not session_id: + reason = ( + f"{tool} needs _meta['{SESSION_ID_KEY}'] to be deduplicated safely; " + "the client sent none, so the write was not run" + ) + self.signer.issue( + tool=tool, + args_hash=ahash, + decision="refuse", + outcome="not_run", + reason=reason, + **base, + ) + raise ToolError(f"AMW refused: {reason}") + + key = idempotency_key(caller=caller, tool=tool, arguments=arguments, session_id=session_id) + base["idempotency_key"] = key + claim = self.ledger.claim(key, tool=tool, args_hash=ahash, request_id=request_id) if claim.action == "replay": self.signer.issue( diff --git a/integrations/goose-agent-middleware/tests/test_core.py b/integrations/goose-agent-middleware/tests/test_core.py index 9a2b23a3..0f688b79 100644 --- a/integrations/goose-agent-middleware/tests/test_core.py +++ b/integrations/goose-agent-middleware/tests/test_core.py @@ -1,85 +1,132 @@ +"""Unit tests. Every value comes from the real G1 gate recording or is made at runtime.""" + +import asyncio import json +import secrets +import uuid from pathlib import Path +from types import SimpleNamespace import pytest +from mcp.server.mcpserver.exceptions import ToolError from goose_amw import hook, permits -from goose_amw.canonical import args_hash, canonical_json, idempotency_key -from goose_amw.ledger import Ledger +from goose_amw.canonical import args_hash, canonical_json, idempotency_key, sha256_hex +from goose_amw.ledger import DEFAULT_WINDOW_SECONDS, Ledger +from goose_amw.proxy import Config, Gateway from goose_amw.receipts import ReceiptSigner, load_or_create_key, verify FIXTURE = Path(__file__).parent / "fixtures" / "g1_gate_calls.jsonl" +RECORDS = [json.loads(line) for line in FIXTURE.read_text().splitlines()] +WRITES = [r for r in RECORDS if r["tool"] == "write_file"] +READ = next(r for r in RECORDS if r["tool"] != "write_file") +W = WRITES[0] +SESSION = W["meta"]["agent-session-id"] +ARGS = {"path": W["path"]} # the only argument field the recording holds -def test_canonical_hash_ignores_key_order(): - assert canonical_json({"b": 1, "a": [2]}) == '{"a":[2],"b":1}' - assert args_hash({"path": "p", "content": "c"}) == args_hash({"content": "c", "path": "p"}) +def rid(record): + return record["meta"]["agent-tool-call-request-id"] -def test_key_inputs(): - k = idempotency_key(caller="c", tool="write_file", arguments={"x": 1}, session_id="s") - assert k == idempotency_key(caller="c", tool="write_file", arguments={"x": 1}, session_id="s") - assert k != idempotency_key(caller="d", tool="write_file", arguments={"x": 1}, session_id="s") - assert k != idempotency_key( - caller="c", tool="read_text_file", arguments={"x": 1}, session_id="s" - ) - assert k != idempotency_key(caller="c", tool="write_file", arguments={"x": 2}, session_id="s") - assert k != idempotency_key(caller="c", tool="write_file", arguments={"x": 1}, session_id="t") +def fresh(): + return str(uuid.uuid4()) + + +def test_canonical_hash_ignores_key_order(): + meta = W["meta"] + reordered = dict(reversed(list(meta.items()))) + assert canonical_json(meta) == canonical_json(reordered) + assert args_hash(meta) == args_hash(reordered) + + +def test_key_changes_with_each_input_and_never_uses_request_id(tmp_path): + caller = fresh() + k = idempotency_key(caller=caller, tool=W["tool"], arguments=ARGS, session_id=SESSION) + assert rid(W) not in k + assert k == idempotency_key(caller=caller, tool=W["tool"], arguments=ARGS, session_id=SESSION) + for variant in ( + {"caller": fresh(), "tool": W["tool"], "arguments": ARGS, "session_id": SESSION}, + {"caller": caller, "tool": READ["tool"], "arguments": ARGS, "session_id": SESSION}, + { + "caller": caller, + "tool": W["tool"], + "arguments": {"path": str(tmp_path)}, + "session_id": SESSION, + }, + {"caller": caller, "tool": W["tool"], "arguments": ARGS, "session_id": fresh()}, + ): + assert idempotency_key(**variant) != k def test_recorded_gate_retry_maps_to_same_key_despite_new_request_id(): """Real G1 gate evidence: Goose sent a new request id on the model's retry.""" - writes = [ - r for r in map(json.loads, FIXTURE.read_text().splitlines()) if r["tool"] == "write_file" - ] - assert len(writes) == 2 - ids = [w["meta"]["agent-tool-call-request-id"] for w in writes] - assert ids[0] != ids[1] - assert all(w["protocol"] == "2026-07-28" for w in writes) - args = {"path": writes[0]["path"], "content": "G1 gate"} + assert len(WRITES) == 2 + assert rid(WRITES[0]) != rid(WRITES[1]) + assert all(w["protocol"] == "2026-07-28" for w in WRITES) + caller = fresh() keys = { idempotency_key( - caller="goose-local", - tool="write_file", - arguments=args, + caller=caller, + tool=w["tool"], + arguments={"path": w["path"]}, session_id=w["meta"]["agent-session-id"], ) - for w in writes + for w in WRITES } assert len(keys) == 1 +def _gate_key(): + return idempotency_key(caller=fresh(), tool=W["tool"], arguments=ARGS, session_id=SESSION) + + def test_ledger_replays_inside_window_and_reruns_after(tmp_path): - led = Ledger(tmp_path / "l.db", window_seconds=10) - assert led.claim("k", tool="t", args_hash="h", request_id="r1", now=100).action == "run" - led.finish("k", "completed", "ok") - c = led.claim("k", tool="t", args_hash="h", request_id="r2", now=105) - assert (c.action, c.result) == ("replay", "ok") - assert led.claim("k", tool="t", args_hash="h", request_id="r3", now=111).action == "run" - assert led.get("k")["executions"] == 2 + led = Ledger(tmp_path / "l.db") + key, ahash, t0 = _gate_key(), args_hash(ARGS), W["ts"] + result = fresh() + assert ( + led.claim(key, tool=W["tool"], args_hash=ahash, request_id=rid(W), now=t0).action == "run" + ) + led.finish(key, "completed", result) + retry = led.claim( + key, tool=W["tool"], args_hash=ahash, request_id=rid(WRITES[1]), now=WRITES[1]["ts"] + ) + assert (retry.action, retry.result) == ("replay", result) + later = t0 + DEFAULT_WINDOW_SECONDS + 1 + assert ( + led.claim(key, tool=W["tool"], args_hash=ahash, request_id=fresh(), now=later).action + == "run" + ) + assert led.get(key)["executions"] == 2 def test_failed_unknown_is_never_rerun(tmp_path): - led = Ledger(tmp_path / "l.db", window_seconds=10) - led.claim("k", tool="t", args_hash="h", request_id=None, now=0) - led.finish("k", "failed_unknown") - for now in (1, 10_000): - assert led.claim("k", tool="t", args_hash="h", request_id=None, now=now).action == "refuse" - assert led.get("k")["executions"] == 1 + led = Ledger(tmp_path / "l.db") + key, ahash, t0 = _gate_key(), args_hash(ARGS), W["ts"] + led.claim(key, tool=W["tool"], args_hash=ahash, request_id=rid(W), now=t0) + led.finish(key, "failed_unknown") + for now in (WRITES[1]["ts"], t0 + 10 * DEFAULT_WINDOW_SECONDS): + claim = led.claim(key, tool=W["tool"], args_hash=ahash, request_id=rid(WRITES[1]), now=now) + assert claim.action == "refuse" + assert led.get(key)["executions"] == 1 def test_crash_in_flight_becomes_failed_unknown(tmp_path): led = Ledger(tmp_path / "l.db") - led.claim("k", tool="t", args_hash="h", request_id=None) + key = _gate_key() + led.claim(key, tool=W["tool"], args_hash=args_hash(ARGS), request_id=rid(W)) assert Ledger(tmp_path / "l.db").mark_stale_in_flight_unknown() == 1 - assert led.get("k")["status"] == "failed_unknown" + assert led.get(key)["status"] == "failed_unknown" def test_definite_failure_may_rerun(tmp_path): led = Ledger(tmp_path / "l.db") - led.claim("k", tool="t", args_hash="h", request_id=None) - led.finish("k", "failed") - assert led.claim("k", tool="t", args_hash="h", request_id=None).action == "run" + key = _gate_key() + led.claim(key, tool=W["tool"], args_hash=args_hash(ARGS), request_id=rid(W)) + led.finish(key, "failed") + claim = led.claim(key, tool=W["tool"], args_hash=args_hash(ARGS), request_id=rid(WRITES[1])) + assert claim.action == "run" def test_window_must_be_positive(tmp_path): @@ -87,29 +134,70 @@ def test_window_must_be_positive(tmp_path): Ledger(tmp_path / "l.db", window_seconds=0) -POLICY = {"rules": [{"id": "w", "tool": "write_file", "folder": "/tmp/amw-allowed"}]} +def _policy(tmp_path): + allowed = tmp_path / fresh() + allowed.mkdir() + return allowed, {"rules": [{"id": fresh(), "tool": W["tool"], "folder": str(allowed)}]} -def test_permits(): - assert permits.check(POLICY, "amw__write_file", {"path": "/tmp/amw-allowed/a"}).allowed - assert not permits.check(POLICY, "amw__write_file", {"path": "/tmp/amw-allowed/../x"}).allowed - assert not permits.check(POLICY, "amw__write_file", {"path": "/etc/x"}).allowed - assert not permits.check(POLICY, "amw__read_text_file", {"path": "/tmp/amw-allowed/a"}).allowed - assert not permits.check(POLICY, "write_file", {}).allowed +def test_permits(tmp_path): + allowed, policy = _policy(tmp_path) + inside = str(allowed / fresh()) + prefixed = f"amw__{W['tool']}" + assert permits.check(policy, prefixed, {"path": inside}).allowed + assert not permits.check(policy, prefixed, {"path": f"{allowed}/../{fresh()}"}).allowed + assert not permits.check(policy, prefixed, {"path": str(tmp_path / fresh())}).allowed + assert not permits.check(policy, f"amw__{READ['tool']}", {"path": inside}).allowed + assert not permits.check(policy, W["tool"], {}).allowed -def test_hook_blocks_with_reason_and_allows(): - out = hook.decide({"tool_name": "amw__write_file", "tool_input": {"path": "/etc/x"}}, POLICY) +def test_hook_blocks_with_reason_and_allows(tmp_path): + allowed, policy = _policy(tmp_path) + name = f"amw__{W['tool']}" + out = hook.decide({"tool_name": name, "tool_input": {"path": str(tmp_path / fresh())}}, policy) assert out["decision"] == "block" and "no permit" in out["reason"] - ok = {"tool_name": "amw__write_file", "tool_input": {"path": "/tmp/amw-allowed/a"}} - assert hook.decide(ok, POLICY) == {"decision": "allow"} + ok = {"tool_name": name, "tool_input": {"path": str(allowed / fresh())}} + assert hook.decide(ok, policy) == {"decision": "allow"} def test_receipts_are_signed_and_tamper_evident(tmp_path): signer = ReceiptSigner(load_or_create_key(tmp_path / "k.pem"), tmp_path / "r.jsonl") - r = signer.issue(tool="write_file", args_hash="sha256:x", decision="allow", outcome="completed") + r = signer.issue( + tool=W["tool"], args_hash=args_hash(ARGS), decision="allow", outcome="completed" + ) assert {"tool", "args_hash", "decision", "issued_at"} <= r["body"].keys() assert verify(r, signer.public_key_b64) r["body"]["decision"] = "deny" assert not verify(r, signer.public_key_b64) assert (tmp_path / "k.pem").stat().st_mode & 0o077 == 0 + + +def test_write_without_session_id_is_refused_and_not_run(tmp_path): + allowed, policy = _policy(tmp_path) + token = secrets.token_hex(24) + cfg = Config( + policy=policy, + state_dir=tmp_path / "state", + callers={sha256_hex(token): fresh()}, + fs_root=str(tmp_path), + ) + calls = [] + + async def upstream(tool, arguments): + calls.append(tool) + return fresh() + + gw = Gateway(cfg, upstream=upstream) + ctx = SimpleNamespace( + request_context=SimpleNamespace(meta={}), + headers={"authorization": f"Bearer {token}"}, + protocol_version=None, + ) + args = {"path": str(allowed / fresh()), "content": fresh()} + with pytest.raises(ToolError, match="agent-session-id"): + asyncio.run(gw.handle(W["tool"], args, ctx, write=True)) + assert calls == [] + receipt = json.loads((cfg.state_dir / "receipts.jsonl").read_text().splitlines()[-1]) + assert receipt["body"]["decision"] == "refuse" + assert receipt["body"]["outcome"] == "not_run" + assert verify(receipt, gw.signer.public_key_b64) From 80694f301b2f08dfba25754349da052d402d5a61 Mon Sep 17 00:00:00 2001 From: Christopher <131207452+PetrefiedThunder@users.noreply.github.com> Date: Sat, 24 Oct 2026 18:49:25 -0700 Subject: [PATCH 04/20] fix(goose): pre-dispatch upstream failures are retryable and leave the key untouched; verify_caller negative tests; pin filesystem server --- integrations/goose-agent-middleware/README.md | 11 +++ .../src/goose_amw/ledger.py | 26 ++++- .../src/goose_amw/proxy.py | 63 +++++++----- .../goose-agent-middleware/tests/test_core.py | 97 ++++++++++++++++++- 4 files changed, 169 insertions(+), 28 deletions(-) diff --git a/integrations/goose-agent-middleware/README.md b/integrations/goose-agent-middleware/README.md index 63110bbd..90795d7f 100644 --- a/integrations/goose-agent-middleware/README.md +++ b/integrations/goose-agent-middleware/README.md @@ -17,6 +17,17 @@ Every proxied call goes through permit -> idempotency -> invoke -> receipt. The proxy enforces the same permit rules server-side, so a client without the hook is still refused. +## Upstream server + +The filesystem server is pinned: `AMW_FS_SERVER_PACKAGE` (default +`@modelcontextprotocol/server-filesystem@2.0.0`) is passed to `npx -y`, so a +new upstream release never runs without an explicit change. + +A failure before the tool call is sent (spawn, session setup, `initialize`) +is safe to retry: AMW restores the idempotency key to its state before the +attempt and issues a `not_dispatched` receipt. Only a failure after dispatch +is recorded as `failed_unknown`. + ## Permit policy ```json diff --git a/integrations/goose-agent-middleware/src/goose_amw/ledger.py b/integrations/goose-agent-middleware/src/goose_amw/ledger.py index 7c13dedd..4cae9a28 100644 --- a/integrations/goose-agent-middleware/src/goose_amw/ledger.py +++ b/integrations/goose-agent-middleware/src/goose_amw/ledger.py @@ -31,6 +31,7 @@ class Claim: action: str # "run" | "replay" | "refuse" result: str | None = None status: str | None = None + previous: tuple | None = None # the row as it was before this claim, for release() class Ledger: @@ -60,9 +61,8 @@ def claim( with self._lock: self._db.execute("BEGIN IMMEDIATE") try: - row = self._db.execute( - "SELECT status, result, created_at FROM calls WHERE key = ?", (key,) - ).fetchone() + full = self._db.execute("SELECT * FROM calls WHERE key = ?", (key,)).fetchone() + row = None if full is None else (full[3], full[4], full[6]) if row is not None: status, result, created_at = row if status == "failed_unknown": @@ -86,7 +86,7 @@ def claim( (key, tool, args_hash, request_id, now, now), ) self._db.execute("COMMIT") - return Claim("run") + return Claim("run", previous=full) except BaseException: self._db.execute("ROLLBACK") raise @@ -100,6 +100,24 @@ def finish(self, key: str, status: str, result: str | None = None) -> None: (status, result, time.time(), key), ) + def release(self, key: str, claim: Claim) -> None: + """Undo a ``run`` claim whose call was never dispatched upstream. + + The key goes back to exactly its state before the claim (or disappears if it + was new), so a pre-dispatch failure leaves no trace and the call stays retryable. + """ + with self._lock: + self._db.execute("BEGIN IMMEDIATE") + try: + self._db.execute("DELETE FROM calls WHERE key=? AND status='in_flight'", (key,)) + if claim.previous is not None: + marks = ",".join("?" * len(claim.previous)) + self._db.execute(f"INSERT INTO calls VALUES ({marks})", claim.previous) + self._db.execute("COMMIT") + except BaseException: + self._db.execute("ROLLBACK") + raise + def mark_stale_in_flight_unknown(self) -> int: """At startup, an ``in_flight`` row means a crash mid-call: outcome unknown.""" with self._lock: diff --git a/integrations/goose-agent-middleware/src/goose_amw/proxy.py b/integrations/goose-agent-middleware/src/goose_amw/proxy.py index 07431244..48e4e735 100644 --- a/integrations/goose-agent-middleware/src/goose_amw/proxy.py +++ b/integrations/goose-agent-middleware/src/goose_amw/proxy.py @@ -12,6 +12,8 @@ * ``AMW_STATE_DIR`` - ledger, receipts and signing key * ``AMW_CALLERS`` - JSON map of sha256(bearer token) -> caller id * ``AMW_FS_ROOT`` - root directory handed to the filesystem server +* ``AMW_FS_SERVER_PACKAGE`` - pinned npm spec of the filesystem server, + default ``@modelcontextprotocol/server-filesystem@2.0.0`` * ``AMW_IDEMPOTENCY_WINDOW_SECONDS`` - replay window, default 600 * ``AMW_HOST`` / ``AMW_PORT`` - bind address, default 127.0.0.1:8791 """ @@ -45,6 +47,14 @@ class UpstreamError(Exception): """The upstream answered with a definite error: the call had no effect.""" +class UpstreamNotDispatched(UpstreamError): + """Failed before the tool call was sent (spawn, session setup, initialize). + + Nothing reached the upstream tool, so the call is safe to retry and the + idempotency key is left exactly as it was before the attempt. + """ + + class UpstreamUnknown(Exception): """The call was dispatched but its outcome is unknown.""" @@ -52,32 +62,32 @@ class UpstreamUnknown(Exception): Upstream = Callable[[str, dict[str, Any]], Awaitable[str]] -def filesystem_upstream(fs_root: str) -> Upstream: - from mcp.client.session import ClientSession - from mcp.client.stdio import StdioServerParameters, stdio_client +FS_SERVER_PACKAGE = "@modelcontextprotocol/server-filesystem@2.0.0" - params = StdioServerParameters( - command="npx", args=["-y", "@modelcontextprotocol/server-filesystem", fs_root] - ) + +def filesystem_upstream(fs_root: str, package: str | None = None) -> Upstream: + from mcp.client.stdio import StdioServerParameters + + spec = package or os.environ.get("AMW_FS_SERVER_PACKAGE") or FS_SERVER_PACKAGE + return stdio_upstream(StdioServerParameters(command="npx", args=["-y", spec, fs_root])) + + +def stdio_upstream(params: Any) -> Upstream: + """Call one tool on a stdio MCP server, classifying failures by dispatch point.""" + from mcp.client.session import ClientSession + from mcp.client.stdio import stdio_client async def call(tool: str, arguments: dict[str, Any]) -> str: + dispatched = False try: - ctx = stdio_client(params) - read, write = await ctx.__aenter__() - except Exception as exc: # never dispatched - raise UpstreamError(f"filesystem server unavailable: {exc}") from exc - try: - async with ClientSession(read, write) as session: + async with stdio_client(params) as (read, write), ClientSession(read, write) as session: await session.initialize() - try: - res = await session.call_tool(tool, arguments) - except Exception as exc: - raise UpstreamUnknown(f"{type(exc).__name__}: {exc}") from exc - finally: - try: - await ctx.__aexit__(None, None, None) - except Exception: - log.warning("filesystem server shutdown error", exc_info=True) + dispatched = True + res = await session.call_tool(tool, arguments) + except BaseException as exc: + if not dispatched: + raise UpstreamNotDispatched(f"filesystem server unavailable: {exc!r}") from exc + raise UpstreamUnknown(f"{type(exc).__name__}: {exc}") from exc text = "\n".join(getattr(c, "text", "") for c in res.content) if res.is_error: raise UpstreamError(text) @@ -205,11 +215,18 @@ async def handle(self, tool: str, arguments: dict[str, Any], ctx: Context, *, wr f"AMW refused to re-run: an earlier identical {tool} call is {claim.status}; " "its outcome is unknown, so it will not run again automatically" ) - return await self._run(tool, arguments, ahash, base, key=key) + return await self._run(tool, arguments, ahash, base, key=key, claim=claim) - async def _run(self, tool, arguments, ahash, base, *, key): + async def _run(self, tool, arguments, ahash, base, *, key, claim=None): try: result = await self.upstream(tool, arguments) + except UpstreamNotDispatched as exc: + if key: + self.ledger.release(key, claim) + self.signer.issue( + tool=tool, args_hash=ahash, decision="allow", outcome="not_dispatched", **base + ) + raise ToolError(f"upstream unavailable, safe to retry: {exc}") from exc except UpstreamError as exc: if key: self.ledger.finish(key, "failed") diff --git a/integrations/goose-agent-middleware/tests/test_core.py b/integrations/goose-agent-middleware/tests/test_core.py index 0f688b79..89e19c50 100644 --- a/integrations/goose-agent-middleware/tests/test_core.py +++ b/integrations/goose-agent-middleware/tests/test_core.py @@ -3,6 +3,7 @@ import asyncio import json import secrets +import sys import uuid from pathlib import Path from types import SimpleNamespace @@ -13,7 +14,13 @@ from goose_amw import hook, permits from goose_amw.canonical import args_hash, canonical_json, idempotency_key, sha256_hex from goose_amw.ledger import DEFAULT_WINDOW_SECONDS, Ledger -from goose_amw.proxy import Config, Gateway +from goose_amw.proxy import ( + Config, + Gateway, + UpstreamNotDispatched, + stdio_upstream, + verify_caller, +) from goose_amw.receipts import ReceiptSigner, load_or_create_key, verify FIXTURE = Path(__file__).parent / "fixtures" / "g1_gate_calls.jsonl" @@ -201,3 +208,91 @@ async def upstream(tool, arguments): assert receipt["body"]["decision"] == "refuse" assert receipt["body"]["outcome"] == "not_run" assert verify(receipt, gw.signer.public_key_b64) + + +def _gateway(tmp_path, upstream): + allowed, policy = _policy(tmp_path) + token = secrets.token_hex(24) + cfg = Config( + policy=policy, + state_dir=tmp_path / "state", + callers={sha256_hex(token): fresh()}, + fs_root=str(tmp_path), + ) + ctx = SimpleNamespace( + request_context=SimpleNamespace(meta={"agent-session-id": fresh()}), + headers={"authorization": f"Bearer {token}"}, + protocol_version=None, + ) + return ( + Gateway(cfg, upstream=upstream), + ctx, + {"path": str(allowed / fresh()), "content": fresh()}, + ) + + +def test_verify_caller_rejects_missing_and_wrong_tokens(): + token = secrets.token_hex(24) + caller = fresh() + callers = {sha256_hex(token): caller} + assert verify_caller({"authorization": f"Bearer {token}"}, callers) == caller + for headers in ( + None, + {}, + {"authorization": token}, + {"authorization": f"Bearer {secrets.token_hex(24)}"}, + {"authorization": f"Bearer {token[:-1]}"}, + ): + with pytest.raises(ToolError, match="caller not verified"): + verify_caller(headers, callers) + + +def test_unverified_caller_never_reaches_upstream(tmp_path): + calls = [] + + async def upstream(tool, arguments): + calls.append(tool) + return fresh() + + gw, ctx, args = _gateway(tmp_path, upstream) + ctx.headers = {"authorization": f"Bearer {secrets.token_hex(24)}"} + with pytest.raises(ToolError, match="caller not verified"): + asyncio.run(gw.handle(W["tool"], args, ctx, write=True)) + assert calls == [] + + +def test_pre_dispatch_failure_leaves_key_untouched_and_retryable(tmp_path): + attempts = [] + + async def upstream(tool, arguments): + attempts.append(tool) + if len(attempts) == 1: + raise UpstreamNotDispatched(fresh()) + return fresh() + + gw, ctx, args = _gateway(tmp_path, upstream) + with pytest.raises(ToolError, match="safe to retry"): + asyncio.run(gw.handle(W["tool"], args, ctx, write=True)) + assert gw.ledger._db.execute("SELECT COUNT(*) FROM calls").fetchone()[0] == 0 + asyncio.run(gw.handle(W["tool"], args, ctx, write=True)) + rows = gw.ledger._db.execute("SELECT status, executions FROM calls").fetchall() + assert rows == [("completed", 1)] + + +def test_release_restores_the_previous_row(tmp_path): + led = Ledger(tmp_path / "l.db") + key = _gate_key() + led.claim(key, tool=W["tool"], args_hash=args_hash(ARGS), request_id=rid(W)) + led.finish(key, "failed") + before = led.get(key) + claim = led.claim(key, tool=W["tool"], args_hash=args_hash(ARGS), request_id=rid(WRITES[1])) + led.release(key, claim) + assert led.get(key) == before + + +def test_server_that_dies_before_initialize_is_not_dispatched(): + from mcp.client.stdio import StdioServerParameters + + call = stdio_upstream(StdioServerParameters(command=sys.executable, args=["-c", ""])) + with pytest.raises(UpstreamNotDispatched): + asyncio.run(asyncio.wait_for(call(W["tool"], ARGS), 30)) From aa14791bee9e9624aa027d8d2b6a34c52c972b3c Mon Sep 17 00:00:00 2001 From: Christopher <131207452+PetrefiedThunder@users.noreply.github.com> Date: Sat, 24 Oct 2026 19:01:53 -0700 Subject: [PATCH 05/20] fix(goose): pin filesystem server 2.0.1; write error results are failed_unknown; handle-level permit deny test --- integrations/goose-agent-middleware/README.md | 6 ++- .../src/goose_amw/proxy.py | 14 +++++-- .../goose-agent-middleware/tests/test_core.py | 40 +++++++++++++++++++ 3 files changed, 56 insertions(+), 4 deletions(-) diff --git a/integrations/goose-agent-middleware/README.md b/integrations/goose-agent-middleware/README.md index 90795d7f..0b8cd217 100644 --- a/integrations/goose-agent-middleware/README.md +++ b/integrations/goose-agent-middleware/README.md @@ -20,7 +20,7 @@ hook is still refused. ## Upstream server The filesystem server is pinned: `AMW_FS_SERVER_PACKAGE` (default -`@modelcontextprotocol/server-filesystem@2.0.0`) is passed to `npx -y`, so a +`@modelcontextprotocol/server-filesystem@2.0.1`) is passed to `npx -y`, so a new upstream release never runs without an explicit change. A failure before the tool call is sent (spawn, session setup, `initialize`) @@ -107,3 +107,7 @@ real call log recorded by AMW during the G1 gate session. `tests/e2e/fault_proxy.py` is a test-only launcher that injects one fault into the shipped proxy to record the cut-stream and unknown-outcome sessions; it is not part of the package. + +An error result from the upstream server for a write is recorded as +`failed_unknown`: the pinned server writes files in place, so the file may be +partly written, and the write is never re-run automatically. diff --git a/integrations/goose-agent-middleware/src/goose_amw/proxy.py b/integrations/goose-agent-middleware/src/goose_amw/proxy.py index 48e4e735..335a43cf 100644 --- a/integrations/goose-agent-middleware/src/goose_amw/proxy.py +++ b/integrations/goose-agent-middleware/src/goose_amw/proxy.py @@ -13,7 +13,7 @@ * ``AMW_CALLERS`` - JSON map of sha256(bearer token) -> caller id * ``AMW_FS_ROOT`` - root directory handed to the filesystem server * ``AMW_FS_SERVER_PACKAGE`` - pinned npm spec of the filesystem server, - default ``@modelcontextprotocol/server-filesystem@2.0.0`` + default ``@modelcontextprotocol/server-filesystem@2.0.1`` * ``AMW_IDEMPOTENCY_WINDOW_SECONDS`` - replay window, default 600 * ``AMW_HOST`` / ``AMW_PORT`` - bind address, default 127.0.0.1:8791 """ @@ -62,7 +62,7 @@ class UpstreamUnknown(Exception): Upstream = Callable[[str, dict[str, Any]], Awaitable[str]] -FS_SERVER_PACKAGE = "@modelcontextprotocol/server-filesystem@2.0.0" +FS_SERVER_PACKAGE = "@modelcontextprotocol/server-filesystem@2.0.1" def filesystem_upstream(fs_root: str, package: str | None = None) -> Upstream: @@ -229,7 +229,15 @@ async def _run(self, tool, arguments, ahash, base, *, key, claim=None): raise ToolError(f"upstream unavailable, safe to retry: {exc}") from exc except UpstreamError as exc: if key: - self.ledger.finish(key, "failed") + # The filesystem server writes in place, so an error result for a + # write may come after a partial write: never claim it had no effect. + self.ledger.finish(key, "failed_unknown") + self.signer.issue( + tool=tool, args_hash=ahash, decision="allow", outcome="failed_unknown", **base + ) + raise ToolError( + f"upstream error; the write may be partly applied and will not re-run: {exc}" + ) from exc self.signer.issue( tool=tool, args_hash=ahash, decision="allow", outcome="failed", **base ) diff --git a/integrations/goose-agent-middleware/tests/test_core.py b/integrations/goose-agent-middleware/tests/test_core.py index 89e19c50..315722d5 100644 --- a/integrations/goose-agent-middleware/tests/test_core.py +++ b/integrations/goose-agent-middleware/tests/test_core.py @@ -17,6 +17,7 @@ from goose_amw.proxy import ( Config, Gateway, + UpstreamError, UpstreamNotDispatched, stdio_upstream, verify_caller, @@ -296,3 +297,42 @@ def test_server_that_dies_before_initialize_is_not_dispatched(): call = stdio_upstream(StdioServerParameters(command=sys.executable, args=["-c", ""])) with pytest.raises(UpstreamNotDispatched): asyncio.run(asyncio.wait_for(call(W["tool"], ARGS), 30)) + + +def test_handle_denies_unpermitted_path_without_reaching_upstream(tmp_path): + calls = [] + + async def upstream(tool, arguments): + calls.append(tool) + return fresh() + + gw, ctx, args = _gateway(tmp_path, upstream) + for path in (str(tmp_path / fresh()), f"{Path(args['path']).parent}/../{fresh()}"): + with pytest.raises(ToolError, match="permit denied"): + asyncio.run(gw.handle(W["tool"], {**args, "path": path}, ctx, write=True)) + assert calls == [] + assert gw.ledger._db.execute("SELECT COUNT(*) FROM calls").fetchone()[0] == 0 + lines = (tmp_path / "state" / "receipts.jsonl").read_text().splitlines() + for line in lines: + receipt = json.loads(line) + assert receipt["body"]["decision"] == "deny" + assert receipt["body"]["outcome"] == "not_run" + assert verify(receipt, gw.signer.public_key_b64) + assert len(lines) == 2 + + +def test_write_error_result_is_failed_unknown_and_not_rerun(tmp_path): + calls = [] + + async def upstream(tool, arguments): + calls.append(tool) + raise UpstreamError(fresh()) + + gw, ctx, args = _gateway(tmp_path, upstream) + with pytest.raises(ToolError, match="partly applied"): + asyncio.run(gw.handle(W["tool"], args, ctx, write=True)) + with pytest.raises(ToolError, match="will not run again"): + asyncio.run(gw.handle(W["tool"], args, ctx, write=True)) + assert calls == [W["tool"]] + rows = gw.ledger._db.execute("SELECT status FROM calls").fetchall() + assert rows == [("failed_unknown",)] From fba1c8c031644aa05397f75fa5c1548e1933530d Mon Sep 17 00:00:00 2001 From: Christopher <131207452+PetrefiedThunder@users.noreply.github.com> Date: Sat, 24 Oct 2026 19:10:27 -0700 Subject: [PATCH 06/20] fix(goose): permits deny relative and ~ paths (upstream resolves them against its own root) --- .../goose-agent-middleware/src/goose_amw/permits.py | 6 +++++- .../goose-agent-middleware/tests/test_core.py | 11 +++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/integrations/goose-agent-middleware/src/goose_amw/permits.py b/integrations/goose-agent-middleware/src/goose_amw/permits.py index a85529a5..b9065151 100644 --- a/integrations/goose-agent-middleware/src/goose_amw/permits.py +++ b/integrations/goose-agent-middleware/src/goose_amw/permits.py @@ -29,7 +29,11 @@ def check(policy: dict[str, Any], tool: str, arguments: dict[str, Any] | None) - raw = (arguments or {}).get("path") if not isinstance(raw, str) or not raw: return Decision(False, f"no permit: {name} call has no path argument") - target = Path(raw).expanduser().resolve() + if not Path(raw).is_absolute(): + # The upstream server resolves relative (and ~) paths against its own root, + # not our working directory, so only an absolute path can be checked safely. + return Decision(False, f"no permit: {name} path must be absolute, got {raw!r}") + target = Path(raw).resolve() for rule in policy.get("rules", []): if rule.get("tool") != name: continue diff --git a/integrations/goose-agent-middleware/tests/test_core.py b/integrations/goose-agent-middleware/tests/test_core.py index 315722d5..f55206d5 100644 --- a/integrations/goose-agent-middleware/tests/test_core.py +++ b/integrations/goose-agent-middleware/tests/test_core.py @@ -336,3 +336,14 @@ async def upstream(tool, arguments): assert calls == [W["tool"]] rows = gw.ledger._db.execute("SELECT status FROM calls").fetchall() assert rows == [("failed_unknown",)] + + +def test_relative_and_home_paths_are_denied(tmp_path, monkeypatch): + allowed, policy = _policy(tmp_path) + monkeypatch.chdir(allowed) + name = fresh() + for raw in (name, f"./{name}", f"~/{name}"): + decision = permits.check(policy, W["tool"], {"path": raw}) + assert not decision.allowed + assert "must be absolute" in decision.reason + assert permits.check(policy, W["tool"], {"path": str(allowed / name)}).allowed From 5721c5b36a594bffb94dffb091d6b505179fc27d Mon Sep 17 00:00:00 2001 From: Christopher <131207452+PetrefiedThunder@users.noreply.github.com> Date: Sat, 24 Oct 2026 19:18:10 -0700 Subject: [PATCH 07/20] fix(goose): keep a received tool result when stdio teardown fails --- .../src/goose_amw/proxy.py | 9 +++- .../goose-agent-middleware/tests/test_core.py | 46 +++++++++++++++++++ 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/integrations/goose-agent-middleware/src/goose_amw/proxy.py b/integrations/goose-agent-middleware/src/goose_amw/proxy.py index 335a43cf..b6e2bf98 100644 --- a/integrations/goose-agent-middleware/src/goose_amw/proxy.py +++ b/integrations/goose-agent-middleware/src/goose_amw/proxy.py @@ -79,15 +79,20 @@ def stdio_upstream(params: Any) -> Upstream: async def call(tool: str, arguments: dict[str, Any]) -> str: dispatched = False + res = None try: async with stdio_client(params) as (read, write), ClientSession(read, write) as session: await session.initialize() dispatched = True res = await session.call_tool(tool, arguments) except BaseException as exc: - if not dispatched: + if res is not None and isinstance(exc, Exception): + # The server already answered; a teardown error does not change that. + log.warning("ignoring stdio teardown error after a result: %r", exc) + elif not dispatched: raise UpstreamNotDispatched(f"filesystem server unavailable: {exc!r}") from exc - raise UpstreamUnknown(f"{type(exc).__name__}: {exc}") from exc + else: + raise UpstreamUnknown(f"{type(exc).__name__}: {exc}") from exc text = "\n".join(getattr(c, "text", "") for c in res.content) if res.is_error: raise UpstreamError(text) diff --git a/integrations/goose-agent-middleware/tests/test_core.py b/integrations/goose-agent-middleware/tests/test_core.py index f55206d5..00eb3cda 100644 --- a/integrations/goose-agent-middleware/tests/test_core.py +++ b/integrations/goose-agent-middleware/tests/test_core.py @@ -19,6 +19,7 @@ Gateway, UpstreamError, UpstreamNotDispatched, + UpstreamUnknown, stdio_upstream, verify_caller, ) @@ -347,3 +348,48 @@ def test_relative_and_home_paths_are_denied(tmp_path, monkeypatch): assert not decision.allowed assert "must be absolute" in decision.reason assert permits.check(policy, W["tool"], {"path": str(allowed / name)}).allowed + + +def _fake_stdio(monkeypatch, result, *, fail_on_exit): + import contextlib + + import mcp.client.session + import mcp.client.stdio + + @contextlib.asynccontextmanager + async def client(params): + yield (None, None) + if fail_on_exit: + raise OSError(fresh()) + + class Session: + def __init__(self, read, write): + pass + + async def __aenter__(self): + return self + + async def __aexit__(self, *exc): + return False + + async def initialize(self): + return None + + async def call_tool(self, tool, arguments): + return result + + monkeypatch.setattr(mcp.client.stdio, "stdio_client", client) + monkeypatch.setattr(mcp.client.session, "ClientSession", Session) + + +def test_teardown_error_after_result_keeps_the_result(monkeypatch): + text = fresh() + result = SimpleNamespace(content=[SimpleNamespace(text=text)], is_error=False) + _fake_stdio(monkeypatch, result, fail_on_exit=True) + assert asyncio.run(stdio_upstream(object())(W["tool"], ARGS)) == text + + +def test_teardown_error_without_result_is_unknown(monkeypatch): + _fake_stdio(monkeypatch, None, fail_on_exit=True) + with pytest.raises(UpstreamUnknown): + asyncio.run(stdio_upstream(object())(W["tool"], ARGS)) From 54ba8fc1f915db92122c99d040d82215a0d36eb1 Mon Sep 17 00:00:00 2001 From: Christopher <131207452+PetrefiedThunder@users.noreply.github.com> Date: Sun, 25 Oct 2026 00:54:49 -0700 Subject: [PATCH 08/20] fix(mcp): derive deterministic retry key for unkeyed tools/call (SEL-484) --- gateway/app/core/config.py | 9 ++ gateway/app/routers/mcp.py | 12 ++ gateway/app/routers/mcp_standard.py | 19 ++- gateway/app/services/mcp_retry_key.py | 63 +++++++++ gateway/docs/concepts/request-keys.md | 11 +- gateway/docs/failure-semantics.md | 19 ++- .../test_mcp_idempotency_key_validation.py | 18 +-- gateway/tests/test_mcp_retry_key.py | 42 ++++++ gateway/tests/test_mcp_router_breaks.py | 12 +- gateway/tests/test_standard_mcp_endpoint.py | 133 ++++++++++++++++++ 10 files changed, 315 insertions(+), 23 deletions(-) create mode 100644 gateway/app/services/mcp_retry_key.py create mode 100644 gateway/tests/test_mcp_retry_key.py diff --git a/gateway/app/core/config.py b/gateway/app/core/config.py index 726ae864..cce289e5 100644 --- a/gateway/app/core/config.py +++ b/gateway/app/core/config.py @@ -288,6 +288,15 @@ def demo_allowed_tools_list(self) -> list[str]: ENABLE_STANDARD_MCP_ENDPOINT: bool = True STANDARD_MCP_PERMIT_TTL_SECONDS: int = 120 + # Unkeyed tools/call retry safety (SEL-484 / G1, MCP issue #3394). + # A call without a client idempotency key gets a deterministic key derived + # from wallet, API key, tool and canonical arguments within this window, so + # a lost-response retry replays the original receipt instead of + # dispatching again. Identical unkeyed calls inside one window collapse to + # one dispatch; callers needing repeats must send distinct keys. 0 restores + # the old per-attempt random key (no retry protection). + MCP_UNKEYED_RETRY_WINDOW_SECONDS: int = 300 + # --- Public MCP endpoint (POST /mcp/public) --- # Unauthenticated, read-only Streamable HTTP surface for MCP clients # that cannot hold an API key (public plugin runtimes such as ChatGPT diff --git a/gateway/app/routers/mcp.py b/gateway/app/routers/mcp.py index a6750f46..44fe316c 100644 --- a/gateway/app/routers/mcp.py +++ b/gateway/app/routers/mcp.py @@ -33,6 +33,7 @@ from ..audit.lightweight import record_audit from ..core.config import DuplicateGuardMode, get_settings +from ..services.mcp_retry_key import derive_unkeyed_retry_key from ..policy.jev_guard import evaluate_jev_guard from ..services.jev_guard_metadata import jev_audit_id, load_jev_guard_metadata from ..core.auth import ( @@ -4533,6 +4534,17 @@ async def _maybe_mint_demo_permit(request: GovernedRequest) -> str | None: from .mcp_standard import _mint_auto_permit, _permit_error + if not request.idempotency_key: + # Deterministic key for unkeyed calls so a retry resolves to the same + # auto permit and replays instead of dispatching twice (SEL-484). + request.idempotency_key = derive_unkeyed_retry_key( + wallet_id=request.auth.wallet_id, + key_id=request.auth.key_id, + tool_name=request.tool_name, + arguments=request.arguments, + window_seconds=get_settings().MCP_UNKEYED_RETRY_WINDOW_SECONDS, + ) + try: permit_id = await _mint_auto_permit( auth=request.auth, diff --git a/gateway/app/routers/mcp_standard.py b/gateway/app/routers/mcp_standard.py index 31f8fe1d..89eb38a3 100644 --- a/gateway/app/routers/mcp_standard.py +++ b/gateway/app/routers/mcp_standard.py @@ -84,6 +84,7 @@ from app.services.billing_engine import LedgerWriteContendedError from app.core.config import get_settings from app.core.time import utc_now +from app.services.mcp_retry_key import derive_unkeyed_retry_key from app.routers.mcp import ( _MAX_JSON_NESTING_DEPTH, GovernedToolError, @@ -498,12 +499,26 @@ async def _governed_tools_call( ) auto_minted = permit_id is None - idempotency_key = client_idempotency_key or f"mcp-auto-{uuid.uuid4().hex}" + # Unkeyed calls get a deterministic key so a lost-response retry resolves + # to the same auto permit and replays the original receipt (SEL-484). + derived_key = ( + None + if client_idempotency_key + else derive_unkeyed_retry_key( + wallet_id=auth.wallet_id, + key_id=auth.key_id, + tool_name=tool_name, + arguments=arguments, + window_seconds=get_settings().MCP_UNKEYED_RETRY_WINDOW_SECONDS, + ) + ) + effective_key = client_idempotency_key or derived_key + idempotency_key = effective_key or f"mcp-auto-{uuid.uuid4().hex}" permit_id = permit_id or await _mint_auto_permit( auth=auth, tool_name=tool_name, record=record, - client_key=client_idempotency_key, + client_key=effective_key, requires_human_approval=requires_human_approval, ) diff --git a/gateway/app/services/mcp_retry_key.py b/gateway/app/services/mcp_retry_key.py new file mode 100644 index 00000000..b1582fac --- /dev/null +++ b/gateway/app/services/mcp_retry_key.py @@ -0,0 +1,63 @@ +"""Deterministic idempotency keys for unkeyed MCP ``tools/call`` requests. + +Without a client key, the gateway used to mint ``mcp-auto-`` per +attempt, so a retry after a lost response got a new key and a new auto +permit and the tool ran twice (SEL-484; MCP issue #3394: "generating a fresh +key on every invocation does not correlate separate retry attempts"). + +The derived key binds the caller (wallet + API key), the tool, and the +canonical JSON of the arguments to a fixed time bucket. The JSON-RPC request +id is deliberately excluded: retries arrive with a fresh id. Keyed calls never +reach this module, so their behavior is unchanged. +""" + +from __future__ import annotations + +import hashlib +import json +import time +from typing import Any + +DERIVED_KEY_PREFIX = "mcp-retry-" + + +def canonical_arguments(arguments: Any) -> str: + """Stable JSON for arguments: sorted keys, no whitespace variance.""" + return json.dumps( + arguments if arguments is not None else {}, + sort_keys=True, + separators=(",", ":"), + ensure_ascii=False, + default=str, + ) + + +def derive_unkeyed_retry_key( + *, + wallet_id: str | None, + key_id: str | None, + tool_name: str, + arguments: Any, + window_seconds: int, + now: float | None = None, +) -> str | None: + """Return a deterministic key, or None when derivation is disabled. + + None (window <= 0 or no wallet to scope by) tells the caller to fall back + to the legacy random key. + """ + if window_seconds <= 0 or not wallet_id: + return None + bucket = int((time.time() if now is None else now) // window_seconds) + material = "\x1f".join( + [ + "v1", + wallet_id, + key_id or "", + tool_name, + canonical_arguments(arguments), + str(window_seconds), + str(bucket), + ] + ) + return DERIVED_KEY_PREFIX + hashlib.sha256(material.encode("utf-8")).hexdigest() diff --git a/gateway/docs/concepts/request-keys.md b/gateway/docs/concepts/request-keys.md index ec688965..820fd59f 100644 --- a/gateway/docs/concepts/request-keys.md +++ b/gateway/docs/concepts/request-keys.md @@ -28,10 +28,13 @@ earlier during auto-permit minting with JSON-RPC code `-32003` and the same message. Billing charge uses HTTP 409 with `detail.error` set to the code; permit creation uses HTTP 409 with string `detail`. -Standard `/mcp` makes a fresh `mcp-auto-` key for each call when the caller -supplies none. That fresh key cannot identify a retry, so an unkeyed call is -not retry-safe. The legacy `/mcp/messages` demo auto-permit path also assigns -a fresh `mcp-auto-` key when none is supplied. The SDK's `invoke_tool` +When the caller supplies no key, standard `/mcp` and the legacy +`/mcp/messages` demo auto-permit path derive an `mcp-retry-` key from wallet, +API key, tool, canonical arguments, and a `MCP_UNKEYED_RETRY_WINDOW_SECONDS` +time bucket (default 300). An identical retry in the same bucket replays the +first receipt. A retry across a bucket boundary is a new call, and identical +deliberate calls in one bucket collapse into one, so send your own key when +either matters. A window of `0` restores a fresh `mcp-auto-` key per call. The SDK's `invoke_tool` requires a key; its `"auto"` option derives one from arguments and is unsuitable when distinct calls can have the same arguments. diff --git a/gateway/docs/failure-semantics.md b/gateway/docs/failure-semantics.md index 5a366e39..9c2f4b65 100644 --- a/gateway/docs/failure-semantics.md +++ b/gateway/docs/failure-semantics.md @@ -244,11 +244,20 @@ asked for: every retry carrying the same malformed key would become a new charged operation (`test_mcp_idempotency_key_validation`). Only a genuinely **absent** key — no header, no body entry, or a JSON `null` -entry — is treated as an un-keyed call. On the standard MCP endpoint that means -a generated key, and every un-keyed call, retries included, is a new charged -operation. Clients that retry must always send their own persistent key; a -governed `/mcp/messages` call without one is refused with -`idempotency_key_required`. +entry — is treated as an un-keyed call. On the standard MCP endpoint (and the +`/mcp/messages` demo auto-permit path) the gateway derives a deterministic +`mcp-retry-` key from the caller's wallet and API key, the tool, the canonical +JSON of the arguments, and a time bucket of `MCP_UNKEYED_RETRY_WINDOW_SECONDS` +(default 300). The JSON-RPC id is not part of it, because retries carry fresh +ids. An identical un-keyed retry inside the same bucket replays the original +receipt with no second charge or dispatch; a concurrent duplicate gets the +retryable `idempotency_in_progress`. Limits: a retry that crosses a bucket +boundary is a new operation, and two deliberately identical un-keyed calls in +one bucket collapse into one. Clients that need either guarantee must send +their own key. Setting the window to `0` restores a random key per call. A +governed `/mcp/messages` call without a key is still refused with +`idempotency_key_required`, and a wallet that requires human approval still +needs an explicit key. The governed AWI HTTP routes — `POST /v1/awi/execute`, `/v1/awi/passkey/challenge`, `/v1/awi/passkey/verify`, `/v1/awi/dom/sync`, diff --git a/gateway/tests/test_mcp_idempotency_key_validation.py b/gateway/tests/test_mcp_idempotency_key_validation.py index 728ef389..a184cf77 100644 --- a/gateway/tests/test_mcp_idempotency_key_validation.py +++ b/gateway/tests/test_mcp_idempotency_key_validation.py @@ -384,14 +384,14 @@ async def test_standard_distinct_valid_keys_stay_distinct_operations( @pytest.mark.parametrize("meta", [_ABSENT, None], ids=["no-entry", "json-null"]) -async def test_standard_unkeyed_calls_are_documented_as_new_operations( +async def test_standard_unkeyed_retry_is_one_operation( client, standard_mcp_enabled, clean_database, counted_tool, meta ): - """No key anywhere (or a JSON null entry) means each call is a new charge. + """No key anywhere (or a JSON null entry) gets a derived retry key. - This pins the documented contract for un-keyed clients so it cannot drift - silently in either direction: absent must not be refused like malformed, - and it must not be quietly deduplicated either. + SEL-484: an identical un-keyed call inside the retry window replays the + first receipt instead of charging, dispatching, or minting again. Absent + must still not be refused like malformed. """ provisioned = await provision_agent_wallet(client) headers = {**provisioned["agent_headers"], **MCP_HEADERS} @@ -403,11 +403,11 @@ async def test_standard_unkeyed_calls_are_documented_as_new_operations( ]["receipt_id"] for _ in range(2) ] - assert len(set(receipts)) == 2 + assert len(set(receipts)) == 1 snapshot = await _snapshot(client, provisioned, counted_tool) - assert snapshot["effects"] == 2 - assert snapshot["ledger_entries"] == base["ledger_entries"] + 2 - assert snapshot["permits"] == base["permits"] + 2 + assert snapshot["effects"] == 1 + assert snapshot["ledger_entries"] == base["ledger_entries"] + 1 + assert snapshot["permits"] == base["permits"] + 1 # --------------------------------------------------------------------------- # diff --git a/gateway/tests/test_mcp_retry_key.py b/gateway/tests/test_mcp_retry_key.py new file mode 100644 index 00000000..ce82d32f --- /dev/null +++ b/gateway/tests/test_mcp_retry_key.py @@ -0,0 +1,42 @@ +"""Data-free unit tests for deterministic unkeyed MCP retry keys (SEL-484).""" + +from app.services.mcp_retry_key import DERIVED_KEY_PREFIX, derive_unkeyed_retry_key + +BASE = dict(wallet_id="w1", key_id="k1", tool_name="t", window_seconds=300, now=1000.0) + + +def _key(**overrides): + return derive_unkeyed_retry_key( + **{**BASE, "arguments": {"a": 1, "b": 2}, **overrides} + ) + + +def test_deterministic_and_prefixed(): + assert _key() == _key() + assert _key().startswith(DERIVED_KEY_PREFIX) + + +def test_argument_order_does_not_matter(): + assert _key(arguments={"b": 2, "a": 1}) == _key() + + +def test_none_and_empty_arguments_match(): + assert _key(arguments=None) == _key(arguments={}) + + +def test_each_binding_changes_key(): + base = _key() + assert _key(arguments={"a": 1, "b": 3}) != base + assert _key(tool_name="other") != base + assert _key(wallet_id="w2") != base + assert _key(key_id="k2") != base + assert _key(now=1000.0 + 300) != base + + +def test_same_window_matches(): + assert _key(now=900.0) == _key(now=1199.9) + + +def test_disabled_returns_none(): + assert _key(window_seconds=0) is None + assert _key(wallet_id=None) is None diff --git a/gateway/tests/test_mcp_router_breaks.py b/gateway/tests/test_mcp_router_breaks.py index eedf303b..f49dee86 100644 --- a/gateway/tests/test_mcp_router_breaks.py +++ b/gateway/tests/test_mcp_router_breaks.py @@ -212,7 +212,13 @@ async def test_tool_call_without_session_does_not_skip_auth_or_crash( forged = await client.post( "/mcp", - json={**call, "id": 8}, + # Distinct arguments: an identical un-keyed call would replay the + # first receipt under the derived retry key (SEL-484). + json={ + **call, + "id": 8, + "params": {"name": TOOL, "arguments": {"text": "run-2"}}, + }, headers={**wallet_headers, "Mcp-Session-Id": "forged-session"}, ) assert forged.status_code < 500, forged.text @@ -220,7 +226,7 @@ async def test_tool_call_without_session_does_not_skip_auth_or_crash( forged.json()["result"]["receipt"]["wallet_id"] == (provisioned["agent_wallet_id"]) ) - assert effect_tool == [{"text": "run"}, {"text": "run"}] + assert effect_tool == [{"text": "run"}, {"text": "run-2"}] for bad_id in (None, True, [], {}): body: dict[str, Any] = { @@ -232,7 +238,7 @@ async def test_tool_call_without_session_does_not_skip_auth_or_crash( body["id"] = bad_id refused = await client.post("/mcp", json=body, headers=wallet_headers) assert refused.status_code < 500, refused.text - assert effect_tool == [{"text": "run"}, {"text": "run"}] + assert effect_tool == [{"text": "run"}, {"text": "run-2"}] @pytest.mark.anyio diff --git a/gateway/tests/test_standard_mcp_endpoint.py b/gateway/tests/test_standard_mcp_endpoint.py index b1d838d7..26d1ee02 100644 --- a/gateway/tests/test_standard_mcp_endpoint.py +++ b/gateway/tests/test_standard_mcp_endpoint.py @@ -683,3 +683,136 @@ async def test_malformed_json_is_still_the_sdks_parse_error( ) assert resp.status_code == 400, resp.text assert resp.json()["error"]["code"] == -32700 + + +@pytest.mark.anyio +async def test_unkeyed_tools_call_retry_replays_without_second_dispatch( + client, standard_mcp_enabled, clean_database +): + """SEL-484: an unkeyed retry (fresh JSON-RPC id) must not dispatch twice.""" + provisioned = await provision_agent_wallet(client) + wallet_id = provisioned["agent_wallet_id"] + invocations: list[str] = [] + + def _echo(message="ok", n=0): + invocations.append(message) + return {"message": message} + + registry = get_service_registry() + registry.register_local( + service_id="standard-echo-retry", + name="Standard Echo Retry", + description="Unkeyed retry test tool", + category=ServiceCategory.AGENT_COMMS, + func=_echo, + credits_per_unit=2.0, + unit_name="call", + ) + headers = {**provisioned["agent_headers"], **MCP_HEADERS} + try: + first = await client.post( + "/mcp", + json=_rpc( + "tools/call", + request_id=101, + params={ + "name": "standard-echo-retry", + "arguments": {"message": "hi", "n": 1}, + }, + ), + headers=headers, + ) + assert first.status_code == 200 + receipt = first.json()["result"]["receipt"] + + # Retry: new JSON-RPC id, same arguments in a different key order. + retry = await client.post( + "/mcp", + json=_rpc( + "tools/call", + request_id=102, + params={ + "name": "standard-echo-retry", + "arguments": {"n": 1, "message": "hi"}, + }, + ), + headers=headers, + ) + assert retry.status_code == 200 + assert retry.json()["result"]["receipt"]["receipt_id"] == receipt["receipt_id"] + assert invocations == ["hi"] + + # Different arguments are a distinct call and dispatch. + other = await client.post( + "/mcp", + json=_rpc( + "tools/call", + request_id=103, + params={ + "name": "standard-echo-retry", + "arguments": {"message": "bye", "n": 1}, + }, + ), + headers=headers, + ) + assert other.status_code == 200 + assert other.json()["result"]["receipt"]["receipt_id"] != receipt["receipt_id"] + assert invocations == ["hi", "bye"] + + ledger = await client.get( + f"/v1/billing/ledger/{wallet_id}", headers=provisioned["agent_headers"] + ) + debits = [ + e + for e in ledger.json()["entries"] + if "standard-echo-retry" in e["description"] + ] + assert len(debits) == 2 + finally: + registry.unregister_local("standard-echo-retry") + + +@pytest.mark.anyio +async def test_unkeyed_retry_window_zero_restores_legacy_dispatch( + client, standard_mcp_enabled, clean_database, monkeypatch +): + """With the window disabled each unkeyed attempt dispatches (legacy).""" + from app.core.config import get_settings + + monkeypatch.setattr(get_settings(), "MCP_UNKEYED_RETRY_WINDOW_SECONDS", 0) + provisioned = await provision_agent_wallet(client) + invocations: list[str] = [] + + def _echo(message="ok", n=0): + invocations.append(message) + return {"message": message} + + registry = get_service_registry() + registry.register_local( + service_id="standard-echo-legacy", + name="Standard Echo Legacy", + description="Window disabled test tool", + category=ServiceCategory.AGENT_COMMS, + func=_echo, + credits_per_unit=2.0, + unit_name="call", + ) + headers = {**provisioned["agent_headers"], **MCP_HEADERS} + try: + for rid in (201, 202): + resp = await client.post( + "/mcp", + json=_rpc( + "tools/call", + request_id=rid, + params={ + "name": "standard-echo-legacy", + "arguments": {"message": "hi"}, + }, + ), + headers=headers, + ) + assert resp.status_code == 200 + assert invocations == ["hi", "hi"] + finally: + registry.unregister_local("standard-echo-legacy") From a7e3e2b9df1ff37063d4e98231bb8d96d3d1ae43 Mon Sep 17 00:00:00 2001 From: Christopher <131207452+PetrefiedThunder@users.noreply.github.com> Date: Sun, 25 Oct 2026 01:15:53 -0700 Subject: [PATCH 09/20] fix(mcp): sliding retry window, IGA-bound replay, mutating-only unkeyed keys (SEL-484) - serialize slot choice + permit mint under a per-identity lock row and join a previous-bucket slot minted within the window (no boundary double dispatch) - bind the verified enterprise principal into the retry identity and run a non-consuming IGA check before any stored-result replay - derive the key only for mutating tools; add register_local(read_only=) and mark partner.notes.count read-only - roll a refunded pre-dispatch failure to the next attempt slot - tests: boundary, read-only, no-effect retry, concurrent twins, IGA replay --- gateway/app/core/config.py | 16 +- gateway/app/routers/mcp.py | 75 +++++--- gateway/app/routers/mcp_standard.py | 200 ++++++++++++++++++-- gateway/app/services/dogfood_tool.py | 1 + gateway/app/services/mcp_phase9_tools.py | 1 + gateway/app/services/mcp_retry_key.py | 88 +++++++-- gateway/app/services/service_registry.py | 8 + gateway/docs/concepts/request-keys.md | 18 +- gateway/docs/failure-semantics.md | 35 +++- gateway/tests/test_mcp_replay_iga.py | 59 ++++++ gateway/tests/test_mcp_retry_key.py | 86 ++++++--- gateway/tests/test_standard_mcp_endpoint.py | 182 ++++++++++++++++++ 12 files changed, 658 insertions(+), 111 deletions(-) create mode 100644 gateway/tests/test_mcp_replay_iga.py diff --git a/gateway/app/core/config.py b/gateway/app/core/config.py index cce289e5..b9f0f3f7 100644 --- a/gateway/app/core/config.py +++ b/gateway/app/core/config.py @@ -289,12 +289,16 @@ def demo_allowed_tools_list(self) -> list[str]: STANDARD_MCP_PERMIT_TTL_SECONDS: int = 120 # Unkeyed tools/call retry safety (SEL-484 / G1, MCP issue #3394). - # A call without a client idempotency key gets a deterministic key derived - # from wallet, API key, tool and canonical arguments within this window, so - # a lost-response retry replays the original receipt instead of - # dispatching again. Identical unkeyed calls inside one window collapse to - # one dispatch; callers needing repeats must send distinct keys. 0 restores - # the old per-attempt random key (no retry protection). + # A mutating call without a client idempotency key gets a deterministic + # key derived from wallet, API key, verified enterprise principal, tool and + # canonical arguments. The window slides from the first call (a retry that + # crosses a bucket edge still joins it), so a lost-response retry within + # this many seconds replays the original receipt instead of dispatching + # again; a refunded pre-dispatch failure is retried, not replayed. + # Read-only tools keep a fresh key per call. Identical deliberate unkeyed + # calls inside one window collapse to one dispatch; callers needing + # repeats must send distinct keys. 0 restores the old per-attempt random + # key (no retry protection). MCP_UNKEYED_RETRY_WINDOW_SECONDS: int = 300 # --- Public MCP endpoint (POST /mcp/public) --- diff --git a/gateway/app/routers/mcp.py b/gateway/app/routers/mcp.py index 44fe316c..8b4c9cf6 100644 --- a/gateway/app/routers/mcp.py +++ b/gateway/app/routers/mcp.py @@ -33,7 +33,6 @@ from ..audit.lightweight import record_audit from ..core.config import DuplicateGuardMode, get_settings -from ..services.mcp_retry_key import derive_unkeyed_retry_key from ..policy.jev_guard import evaluate_jev_guard from ..services.jev_guard_metadata import jev_audit_id, load_jev_guard_metadata from ..core.auth import ( @@ -940,6 +939,27 @@ def _verified_enterprise_principal( return parse_enterprise_token(enterprise_bearer_token) +async def _assert_replay_iga_access(*, auth: AuthContext, tool_name: str) -> None: + """Run the enterprise IGA gate before serving any stored result. + + A replay must not hand one human principal another's result (or a result + it is no longer entitled to). The check spends no use (``consume=False``): + the original call already did. + """ + try: + principal = _verified_enterprise_principal(auth.enterprise_bearer_token) + if principal is None: + return + decision = await enforce_tool_call(principal, tool_name, consume=False) + except IGAError as exc: + raise ToolPermissionDenied(exc.reason) from exc + if not decision.allowed: + raise ToolPermissionDenied( + decision.reason, + details={"reason_code": decision.reason, **decision.details}, + ) + + async def _execute_registered_tool( *, tool_name: str, @@ -1637,6 +1657,7 @@ async def _execute_registered_tool_inner( tool_name=tool_name, key_id=auth.key_id, ) + await _assert_replay_iga_access(auth=auth, tool_name=tool_name) await _raise_replayed_error(replay) return replay.response_json @@ -4532,30 +4553,38 @@ async def _maybe_mint_demo_permit(request: GovernedRequest) -> str | None: if registry.get_action_binding(record) is not None: raise ToolPermissionDenied("action_permit_required") - from .mcp_standard import _mint_auto_permit, _permit_error - - if not request.idempotency_key: - # Deterministic key for unkeyed calls so a retry resolves to the same - # auto permit and replays instead of dispatching twice (SEL-484). - request.idempotency_key = derive_unkeyed_retry_key( - wallet_id=request.auth.wallet_id, - key_id=request.auth.key_id, - tool_name=request.tool_name, - arguments=request.arguments, - window_seconds=get_settings().MCP_UNKEYED_RETRY_WINDOW_SECONDS, - ) + from .mcp_standard import ( + _mint_auto_permit, + _permit_error, + _resolve_unkeyed_retry, + ) try: - permit_id = await _mint_auto_permit( - auth=request.auth, - tool_name=request.tool_name, - record=record, - client_key=request.idempotency_key, - requires_human_approval=await wallet_human_approval_required( - request.auth.wallet_id - ), - map_mcp_errors=False, - ) + requires_approval = await wallet_human_approval_required(request.auth.wallet_id) + resolved = None + if not request.idempotency_key: + # Deterministic retry slot for unkeyed calls so a retry resolves + # to the same auto permit and replays instead of dispatching + # twice (SEL-484). + resolved = await _resolve_unkeyed_retry( + auth=request.auth, + tool_name=request.tool_name, + record=record, + arguments=request.arguments, + requires_human_approval=requires_approval, + map_mcp_errors=False, + ) + if resolved is not None: + request.idempotency_key, permit_id = resolved + else: + permit_id = await _mint_auto_permit( + auth=request.auth, + tool_name=request.tool_name, + record=record, + client_key=request.idempotency_key, + requires_human_approval=requires_approval, + map_mcp_errors=False, + ) except PermitWriteContendedError: raise except PermitError as exc: diff --git a/gateway/app/routers/mcp_standard.py b/gateway/app/routers/mcp_standard.py index 89eb38a3..d5fdca6e 100644 --- a/gateway/app/routers/mcp_standard.py +++ b/gateway/app/routers/mcp_standard.py @@ -61,6 +61,7 @@ import hashlib import json import logging +import time import uuid from datetime import timedelta from decimal import Decimal, InvalidOperation @@ -84,7 +85,14 @@ from app.services.billing_engine import LedgerWriteContendedError from app.core.config import get_settings from app.core.time import utc_now -from app.services.mcp_retry_key import derive_unkeyed_retry_key +from app.core.oidc_iga import IGAError +from app.services.mcp_retry_key import ( + bucket_for, + derive_retry_identity, + is_no_effect_failure, + principal_binding, + slot_key, +) from app.routers.mcp import ( _MAX_JSON_NESTING_DEPTH, GovernedToolError, @@ -100,6 +108,7 @@ _loads_strict_json, _terminal_record_contended_data, _value_error_jsonrpc_code, + _verified_enterprise_principal, ) from app.schemas.trust import PermitCreateRequest from app.services.agent_money import get_agent_money @@ -271,6 +280,154 @@ def _client_idempotency_key( raise _mcp_error(-32602, str(exc), exc.as_error_data()) from exc +def _auto_permit_mint_key(client_key: str) -> str: + return "smcp-" + hashlib.sha256(client_key.encode("utf-8")).hexdigest()[:48] + + +_UNKEYED_RETRY_LOCK_ENDPOINT = "/mcp#unkeyed-retry-lock" +_UNKEYED_RETRY_LOCK_WAIT_SECONDS = 5.0 +_UNKEYED_RETRY_LOCK_STALE_SECONDS = 30 +# A pre-dispatch (no side effect) failure rolls the call forward to a fresh +# attempt slot; bounded so a permanently failing upstream eventually replays +# its failure instead of being re-run forever. +_UNKEYED_RETRY_MAX_ATTEMPTS = 3 + + +def _unkeyed_clock() -> float: + """Epoch seconds for window bucketing (patched in tests).""" + return time.time() + + +async def _acquire_unkeyed_retry_lock(wallet_id: str, lock_key: str) -> None: + """Serialize slot selection for one call identity. + + The lock is an uncompleted idempotency row: the unique insert is the + mutex, ``abandon`` releases it. A row left by a crashed holder is + released once it is older than the stale threshold. + """ + idem = get_idempotency_service() + deadline = time.monotonic() + _UNKEYED_RETRY_LOCK_WAIT_SECONDS + while True: + try: + await idem.begin( + wallet_id=wallet_id, + endpoint=_UNKEYED_RETRY_LOCK_ENDPOINT, + idempotency_key=lock_key, + request_payload={"kind": "unkeyed-retry-lock"}, + ) + return + except IdempotencyInProgressError: + held = await idem.get_record( + wallet_id=wallet_id, + endpoint=_UNKEYED_RETRY_LOCK_ENDPOINT, + idempotency_key=lock_key, + ) + if held is not None and held.created_at < utc_now() - timedelta( + seconds=_UNKEYED_RETRY_LOCK_STALE_SECONDS + ): + await idem.abandon( + wallet_id=wallet_id, + endpoint=_UNKEYED_RETRY_LOCK_ENDPOINT, + idempotency_key=lock_key, + expected_record_id=held.record_id, + ) + continue + if time.monotonic() >= deadline: + raise + await anyio.sleep(0.02) + + +async def _resolve_unkeyed_retry( + *, + auth: AuthContext, + tool_name: str, + record: dict[str, Any], + arguments: dict[str, Any], + requires_human_approval: bool, + map_mcp_errors: bool = True, +) -> tuple[str, str] | None: + """Pick the retry slot for an unkeyed mutating call and mint its permit. + + Returns ``(idempotency_key, permit_id)``, or None when the call keeps the + legacy per-attempt random key: read-only tools (a replay would serve a + stale read), the window disabled, or an enterprise bearer that fails + verification (the governed pipeline denies that call regardless). + + Under a per-identity lock: + * a live slot from the previous window (first minted less than one window + ago) is joined, so a retry or a concurrent twin straddling the bucket + boundary resolves to the same key: the window slides from the first + call instead of resetting at fixed boundaries; + * an attempt whose stored result is a refunded pre-dispatch failure (no + side effects) rolls forward to the next attempt slot, so the retry runs + again instead of replaying that failure; + * the auto permit is minted before the lock is released, so any later + caller sees the chosen slot. + """ + window = get_settings().MCP_UNKEYED_RETRY_WINDOW_SECONDS + wallet_id = auth.wallet_id + if window <= 0 or not wallet_id or record.get("read_only"): + return None + try: + principal = _verified_enterprise_principal(auth.enterprise_bearer_token) + except IGAError: + return None + identity = derive_retry_identity( + wallet_id=wallet_id, + key_id=auth.key_id, + principal=principal_binding(principal), + tool_name=tool_name, + arguments=arguments, + ) + idem = get_idempotency_service() + lock_key = "ulock-" + identity[:48] + try: + await _acquire_unkeyed_retry_lock(wallet_id, lock_key) + except IdempotencyInProgressError as exc: + if map_mcp_errors: + raise _mcp_error(-32005, "idempotency_in_progress") from exc + raise + try: + bucket = bucket_for(_unkeyed_clock(), window) + chosen = bucket + previous = await idem.get_record( + wallet_id=wallet_id, + endpoint=_AUTO_PERMIT_ENDPOINT, + idempotency_key=_auto_permit_mint_key( + slot_key(identity, window_seconds=window, bucket=bucket - 1, attempt=0) + ), + ) + if previous is not None and previous.created_at > utc_now() - timedelta( + seconds=window + ): + chosen = bucket - 1 + key = slot_key(identity, window_seconds=window, bucket=chosen, attempt=0) + for attempt in range(_UNKEYED_RETRY_MAX_ATTEMPTS): + key = slot_key( + identity, window_seconds=window, bucket=chosen, attempt=attempt + ) + stored = await idem.get_governed_mcp_record( + wallet_id=wallet_id, idempotency_key=key + ) + if stored is None or not is_no_effect_failure(stored.response_json): + break + permit_id = await _mint_auto_permit( + auth=auth, + tool_name=tool_name, + record=record, + client_key=key, + requires_human_approval=requires_human_approval, + map_mcp_errors=map_mcp_errors, + ) + return key, permit_id + finally: + await idem.abandon( + wallet_id=wallet_id, + endpoint=_UNKEYED_RETRY_LOCK_ENDPOINT, + idempotency_key=lock_key, + ) + + async def _mint_auto_permit( *, auth: AuthContext, @@ -348,7 +505,7 @@ async def _mint_auto_permit( return permit.permit_id idem = get_idempotency_service() - mint_key = "smcp-" + hashlib.sha256(client_key.encode("utf-8")).hexdigest()[:48] + mint_key = _auto_permit_mint_key(client_key) mint_payload: dict[str, Any] = { "kind": "standard-mcp-auto-permit", "tool": tool_name, @@ -499,28 +656,29 @@ async def _governed_tools_call( ) auto_minted = permit_id is None - # Unkeyed calls get a deterministic key so a lost-response retry resolves - # to the same auto permit and replays the original receipt (SEL-484). - derived_key = ( - None - if client_idempotency_key - else derive_unkeyed_retry_key( - wallet_id=auth.wallet_id, - key_id=auth.key_id, + # Unkeyed mutating calls get a deterministic retry slot so a lost-response + # retry resolves to the same auto permit and replays the original receipt + # (SEL-484). Keyed calls are untouched. + resolved = None + if client_idempotency_key is None and permit_id is None: + resolved = await _resolve_unkeyed_retry( + auth=auth, tool_name=tool_name, + record=record, arguments=arguments, - window_seconds=get_settings().MCP_UNKEYED_RETRY_WINDOW_SECONDS, + requires_human_approval=requires_human_approval, + ) + if resolved is not None: + idempotency_key, permit_id = resolved + else: + idempotency_key = client_idempotency_key or f"mcp-auto-{uuid.uuid4().hex}" + permit_id = permit_id or await _mint_auto_permit( + auth=auth, + tool_name=tool_name, + record=record, + client_key=client_idempotency_key, + requires_human_approval=requires_human_approval, ) - ) - effective_key = client_idempotency_key or derived_key - idempotency_key = effective_key or f"mcp-auto-{uuid.uuid4().hex}" - permit_id = permit_id or await _mint_auto_permit( - auth=auth, - tool_name=tool_name, - record=record, - client_key=effective_key, - requires_human_approval=requires_human_approval, - ) call_params = { "name": tool_name, diff --git a/gateway/app/services/dogfood_tool.py b/gateway/app/services/dogfood_tool.py index a240a0c5..87ecedf8 100644 --- a/gateway/app/services/dogfood_tool.py +++ b/gateway/app/services/dogfood_tool.py @@ -161,6 +161,7 @@ def register_dogfood_second_tool() -> None: credits_per_unit=1.0, unit_name="call", require_permit=True, + read_only=True, ) _second_registered = True logger.info("Registered second dogfood MCP tool: %s", DOGFOOD_SECOND_TOOL_ID) diff --git a/gateway/app/services/mcp_phase9_tools.py b/gateway/app/services/mcp_phase9_tools.py index 7d9b58dd..33e9aab9 100644 --- a/gateway/app/services/mcp_phase9_tools.py +++ b/gateway/app/services/mcp_phase9_tools.py @@ -339,6 +339,7 @@ def register_phase9_tools() -> None: credits_per_unit=tool["credits_per_unit"], unit_name=tool["unit_name"], require_permit=bool(tool.get("require_permit", False)), + read_only=bool(tool.get("read_only", False)), ) logger.info(f"Registered Phase 9 MCP tool: {tool['service_id']}") diff --git a/gateway/app/services/mcp_retry_key.py b/gateway/app/services/mcp_retry_key.py index b1582fac..10d24589 100644 --- a/gateway/app/services/mcp_retry_key.py +++ b/gateway/app/services/mcp_retry_key.py @@ -5,17 +5,23 @@ permit and the tool ran twice (SEL-484; MCP issue #3394: "generating a fresh key on every invocation does not correlate separate retry attempts"). -The derived key binds the caller (wallet + API key), the tool, and the -canonical JSON of the arguments to a fixed time bucket. The JSON-RPC request -id is deliberately excluded: retries arrive with a fresh id. Keyed calls never -reach this module, so their behavior is unchanged. +The key binds the caller (wallet, API key, and the verified enterprise/IGA +principal when one rode in), the tool, and the canonical JSON of the +arguments. The JSON-RPC request id is deliberately excluded: retries arrive +with a fresh id. Keyed calls never reach this module. + +The time dimension is a *slot*: ``floor(now / window)`` plus an attempt +counter. Which slot a call joins is decided by +``app.routers.mcp_standard._resolve_unkeyed_retry`` under a per-identity +lock, so the previous window's live slot is reused across a boundary +(sliding window from the first call) and an attempt that provably failed +before dispatch rolls forward to a fresh attempt instead of replaying. """ from __future__ import annotations import hashlib import json -import time from typing import Any DERIVED_KEY_PREFIX = "mcp-retry-" @@ -32,32 +38,72 @@ def canonical_arguments(arguments: Any) -> str: ) -def derive_unkeyed_retry_key( +def principal_binding(principal: Any) -> str: + """Stable string for a verified enterprise principal ('' when absent).""" + if principal is None: + return "" + return "\x1e".join( + [ + str(getattr(principal, "provider", "")), + str(getattr(principal, "issuer", "")), + str(getattr(principal, "subject", "")), + ] + ) + + +def derive_retry_identity( *, - wallet_id: str | None, + wallet_id: str, key_id: str | None, + principal: str, tool_name: str, arguments: Any, - window_seconds: int, - now: float | None = None, -) -> str | None: - """Return a deterministic key, or None when derivation is disabled. - - None (window <= 0 or no wallet to scope by) tells the caller to fall back - to the legacy random key. - """ - if window_seconds <= 0 or not wallet_id: - return None - bucket = int((time.time() if now is None else now) // window_seconds) +) -> str: + """Time-independent identity of one logical unkeyed call.""" material = "\x1f".join( [ - "v1", + "v2", wallet_id, key_id or "", + principal, tool_name, canonical_arguments(arguments), - str(window_seconds), - str(bucket), ] ) + return hashlib.sha256(material.encode("utf-8")).hexdigest() + + +def slot_key(identity: str, *, window_seconds: int, bucket: int, attempt: int) -> str: + """Idempotency key for one (window bucket, attempt) slot of an identity.""" + material = f"{identity}\x1f{window_seconds}\x1f{bucket}\x1f{attempt}" return DERIVED_KEY_PREFIX + hashlib.sha256(material.encode("utf-8")).hexdigest() + + +def bucket_for(now: float, window_seconds: int) -> int: + return int(now // window_seconds) + + +def is_no_effect_failure(response_json: Any) -> bool: + """True when a stored governed result is a refunded pre-dispatch failure. + + Only the pre-dispatch path stores ``upstream_result`` as the gateway's own + ``{"error": "failed_refunded", "error_code": ...}`` terminal payload with a + ``failed_refunded`` receipt: nothing reached the upstream, so a retry must + run again rather than replay the failure. + """ + if isinstance(response_json, str): + try: + response_json = json.loads(response_json) + except ValueError: + return False + if not isinstance(response_json, dict): + return False + upstream = response_json.get("upstream_result") + receipt = response_json.get("receipt") + return ( + isinstance(upstream, dict) + and upstream.get("error") == "failed_refunded" + and "error_code" in upstream + and isinstance(receipt, dict) + and receipt.get("outcome") == "failed_refunded" + ) diff --git a/gateway/app/services/service_registry.py b/gateway/app/services/service_registry.py index 84ebe56a..4ea26c25 100644 --- a/gateway/app/services/service_registry.py +++ b/gateway/app/services/service_registry.py @@ -257,6 +257,7 @@ def register_local( unit_name: str = "call", owner_wallet_id: str | None = None, require_permit: bool = False, + read_only: bool = False, ) -> dict: """ Register a service locally (in-memory) for immediate use. @@ -267,6 +268,12 @@ def register_local( When ``require_permit`` is True, governed MCP invocation always requires a signed permit + idempotency key — even if legacy unpermitted MCP is otherwise allowed. + + ``read_only`` declares the tool free of side effects. Unkeyed + standard ``/mcp`` calls to it keep a fresh key per call instead of the + deterministic retry key (a replay would serve a stale read). The + default is False: an undeclared tool is treated as mutating, which + fails safe (deduped, never double-dispatched). """ input_schema = pydantic_to_mcp_schema(input_model) output_schema = pydantic_to_mcp_schema(output_model) @@ -294,6 +301,7 @@ def register_local( "output_schema": output_schema, "owner_wallet_id": owner_wallet_id, "require_permit": bool(require_permit), + "read_only": bool(read_only), "is_active": True, "is_local": True, "external": False, diff --git a/gateway/docs/concepts/request-keys.md b/gateway/docs/concepts/request-keys.md index 820fd59f..ae499658 100644 --- a/gateway/docs/concepts/request-keys.md +++ b/gateway/docs/concepts/request-keys.md @@ -28,13 +28,17 @@ earlier during auto-permit minting with JSON-RPC code `-32003` and the same message. Billing charge uses HTTP 409 with `detail.error` set to the code; permit creation uses HTTP 409 with string `detail`. -When the caller supplies no key, standard `/mcp` and the legacy -`/mcp/messages` demo auto-permit path derive an `mcp-retry-` key from wallet, -API key, tool, canonical arguments, and a `MCP_UNKEYED_RETRY_WINDOW_SECONDS` -time bucket (default 300). An identical retry in the same bucket replays the -first receipt. A retry across a bucket boundary is a new call, and identical -deliberate calls in one bucket collapse into one, so send your own key when -either matters. A window of `0` restores a fresh `mcp-auto-` key per call. The SDK's `invoke_tool` +When the caller supplies no key to a mutating tool, standard `/mcp` and the +legacy `/mcp/messages` demo auto-permit path derive an `mcp-retry-` key from +wallet, API key, verified enterprise principal, tool, and canonical arguments. +An identical retry within `MCP_UNKEYED_RETRY_WINDOW_SECONDS` (default 300) of +the first call replays its receipt; the window slides from that first call, +so a retry across a clock bucket edge still replays. If the stored result is +a refunded pre-dispatch failure (nothing reached the tool), the retry runs +again instead. Tools registered `read_only` keep a fresh key per call. +Identical deliberate calls inside one window collapse into one, so send your +own key when that matters. A window of `0` restores a fresh `mcp-auto-` key +per call. The SDK's `invoke_tool` requires a key; its `"auto"` option derives one from arguments and is unsuitable when distinct calls can have the same arguments. diff --git a/gateway/docs/failure-semantics.md b/gateway/docs/failure-semantics.md index 9c2f4b65..3c5309b6 100644 --- a/gateway/docs/failure-semantics.md +++ b/gateway/docs/failure-semantics.md @@ -245,16 +245,31 @@ charged operation (`test_mcp_idempotency_key_validation`). Only a genuinely **absent** key — no header, no body entry, or a JSON `null` entry — is treated as an un-keyed call. On the standard MCP endpoint (and the -`/mcp/messages` demo auto-permit path) the gateway derives a deterministic -`mcp-retry-` key from the caller's wallet and API key, the tool, the canonical -JSON of the arguments, and a time bucket of `MCP_UNKEYED_RETRY_WINDOW_SECONDS` -(default 300). The JSON-RPC id is not part of it, because retries carry fresh -ids. An identical un-keyed retry inside the same bucket replays the original -receipt with no second charge or dispatch; a concurrent duplicate gets the -retryable `idempotency_in_progress`. Limits: a retry that crosses a bucket -boundary is a new operation, and two deliberately identical un-keyed calls in -one bucket collapse into one. Clients that need either guarantee must send -their own key. Setting the window to `0` restores a random key per call. A +`/mcp/messages` demo auto-permit path) an un-keyed call to a mutating tool +gets a deterministic `mcp-retry-` key from the caller's wallet and API key, +the verified enterprise principal (provider, issuer, subject) when an IGA +bearer is present, the tool, and the canonical JSON of the arguments. The +JSON-RPC id is not part of it, because retries carry fresh ids. + +- **Sliding window.** Slot selection and the auto-permit mint run under a + per-identity lock row. A slot first minted less than + `MCP_UNKEYED_RETRY_WINDOW_SECONDS` (default 300) ago is joined even if the + clock bucket has rolled over, so a retry or concurrent twin straddling the + edge resolves to one dispatch. A concurrent duplicate either replays the + same receipt or gets the retryable `idempotency_in_progress`. +- **IGA before replay.** A replay of a stored governed result re-runs the IGA + check (without spending a use) first, and the principal is part of the key, + so one human never receives another's stored result. +- **Mutating tools only.** Tools registered `read_only` (for example + `partner.notes.count`) keep a fresh key per call. Undeclared tools are + treated as mutating. +- **No-effect failures retry.** If the stored result is a refunded + pre-dispatch failure, the retry moves to the next attempt slot and runs + again (up to three attempts per window) instead of replaying the failure. + +Two deliberately identical un-keyed calls inside one window still collapse +into one; clients that need repeats must send their own key. Setting the +window to `0` restores a random key per call. A governed `/mcp/messages` call without a key is still refused with `idempotency_key_required`, and a wallet that requires human approval still needs an explicit key. diff --git a/gateway/tests/test_mcp_replay_iga.py b/gateway/tests/test_mcp_replay_iga.py new file mode 100644 index 00000000..937897d4 --- /dev/null +++ b/gateway/tests/test_mcp_replay_iga.py @@ -0,0 +1,59 @@ +# ruff: noqa: F811 (pytest fixtures imported from test_iga_policy) +"""SEL-484 / Codex blocking 2: the IGA gate runs before any stored-result replay. + +Reuses the signing-key, issuer and bundle fixtures from test_iga_policy; no +recorded data. +""" + +from __future__ import annotations + +import pytest + +from app.core.auth import AuthContext +from app.routers.mcp import ToolPermissionDenied, _assert_replay_iga_access +from tests.test_iga_policy import ( # noqa: F401 (fixtures) + TOOL, + _make_bundle, + _make_wallet, + _mint, + _okta_issuers, + _reset_iga_counters, + iga_config, + rsa_key, +) + + +def _auth(wallet_id: str, token: str | None) -> AuthContext: + return AuthContext( + source="api_key", + raw_key="", + key_id="k", + wallet_id=wallet_id, + enterprise_bearer_token=token, + ) + + +async def test_replay_denied_for_principal_without_role( + iga_config, clean_database, rsa_key +): + wallet_id = await _make_wallet() + policy_id = await _make_bundle(wallet_id, allowed_tools=[TOOL]) + iga_config(_okta_issuers(rsa_key), {"payments-ops": {"policy_id": policy_id}}) + + allowed = _mint(rsa_key, extra={"groups": ["payments-ops"]}) + await _assert_replay_iga_access(auth=_auth(wallet_id, allowed), tool_name=TOOL) + + intruder = _mint(rsa_key, sub="intruder", extra={"groups": ["random-team"]}) + with pytest.raises(ToolPermissionDenied): + await _assert_replay_iga_access(auth=_auth(wallet_id, intruder), tool_name=TOOL) + + +async def test_replay_denied_for_unverifiable_bearer(iga_config, rsa_key): + iga_config(_okta_issuers(rsa_key), {}) + expired = _mint(rsa_key, exp_delta=-60) + with pytest.raises(ToolPermissionDenied): + await _assert_replay_iga_access(auth=_auth("w", expired), tool_name=TOOL) + + +async def test_replay_without_enterprise_bearer_is_unchanged(): + await _assert_replay_iga_access(auth=_auth("w", None), tool_name=TOOL) diff --git a/gateway/tests/test_mcp_retry_key.py b/gateway/tests/test_mcp_retry_key.py index ce82d32f..ae1f1013 100644 --- a/gateway/tests/test_mcp_retry_key.py +++ b/gateway/tests/test_mcp_retry_key.py @@ -1,42 +1,82 @@ """Data-free unit tests for deterministic unkeyed MCP retry keys (SEL-484).""" -from app.services.mcp_retry_key import DERIVED_KEY_PREFIX, derive_unkeyed_retry_key +import json -BASE = dict(wallet_id="w1", key_id="k1", tool_name="t", window_seconds=300, now=1000.0) +from app.services.mcp_retry_key import ( + DERIVED_KEY_PREFIX, + bucket_for, + derive_retry_identity, + is_no_effect_failure, + principal_binding, + slot_key, +) +BASE = dict(wallet_id="w1", key_id="k1", principal="", tool_name="t") -def _key(**overrides): - return derive_unkeyed_retry_key( - **{**BASE, "arguments": {"a": 1, "b": 2}, **overrides} + +def _identity(**overrides): + return derive_retry_identity(**{**BASE, "arguments": {"a": 1, "b": 2}, **overrides}) + + +def _slot(identity=None, bucket=3, attempt=0, window=300): + return slot_key( + identity or _identity(), window_seconds=window, bucket=bucket, attempt=attempt ) def test_deterministic_and_prefixed(): - assert _key() == _key() - assert _key().startswith(DERIVED_KEY_PREFIX) + assert _slot() == _slot() + assert _slot().startswith(DERIVED_KEY_PREFIX) + +def test_argument_order_and_empty_arguments(): + assert _identity(arguments={"b": 2, "a": 1}) == _identity() + assert _identity(arguments=None) == _identity(arguments={}) -def test_argument_order_does_not_matter(): - assert _key(arguments={"b": 2, "a": 1}) == _key() +def test_each_binding_changes_identity(): + base = _identity() + assert _identity(arguments={"a": 1, "b": 3}) != base + assert _identity(tool_name="other") != base + assert _identity(wallet_id="w2") != base + assert _identity(key_id="k2") != base + assert _identity(principal="okta\x1eiss\x1esub") != base -def test_none_and_empty_arguments_match(): - assert _key(arguments=None) == _key(arguments={}) + +def test_principal_binding_separates_humans(): + class P: + def __init__(self, subject): + self.provider, self.issuer, self.subject = "okta", "iss", subject + + assert principal_binding(None) == "" + assert principal_binding(P("a")) == principal_binding(P("a")) + assert principal_binding(P("a")) != principal_binding(P("b")) + assert _identity(principal=principal_binding(P("a"))) != _identity( + principal=principal_binding(P("b")) + ) -def test_each_binding_changes_key(): - base = _key() - assert _key(arguments={"a": 1, "b": 3}) != base - assert _key(tool_name="other") != base - assert _key(wallet_id="w2") != base - assert _key(key_id="k2") != base - assert _key(now=1000.0 + 300) != base +def test_slots_differ_by_bucket_attempt_and_window(): + base = _slot() + assert _slot(bucket=4) != base + assert _slot(attempt=1) != base + assert _slot(window=60) != base -def test_same_window_matches(): - assert _key(now=900.0) == _key(now=1199.9) +def test_bucket_for(): + assert bucket_for(299.999, 300) == 0 + assert bucket_for(300.001, 300) == 1 -def test_disabled_returns_none(): - assert _key(window_seconds=0) is None - assert _key(wallet_id=None) is None +def test_no_effect_failure_detection(): + refunded = { + "error": "upstream_unreachable", + "upstream_result": {"error": "failed_refunded", "error_code": "x"}, + "receipt": {"outcome": "failed_refunded"}, + } + assert is_no_effect_failure(refunded) + assert is_no_effect_failure(json.dumps(refunded)) + assert not is_no_effect_failure({"result": {"ok": True}, "receipt": {}}) + assert not is_no_effect_failure({**refunded, "receipt": {"outcome": "succeeded"}}) + assert not is_no_effect_failure("not json") + assert not is_no_effect_failure(None) diff --git a/gateway/tests/test_standard_mcp_endpoint.py b/gateway/tests/test_standard_mcp_endpoint.py index 26d1ee02..52942f8a 100644 --- a/gateway/tests/test_standard_mcp_endpoint.py +++ b/gateway/tests/test_standard_mcp_endpoint.py @@ -8,6 +8,8 @@ from __future__ import annotations +import json + import hashlib import pytest @@ -816,3 +818,183 @@ def _echo(message="ok", n=0): assert invocations == ["hi", "hi"] finally: registry.unregister_local("standard-echo-legacy") + + +def _register_counting_tool(service_id: str, invocations: list, **extra): + def _echo(message="ok", n=0): + invocations.append(message) + return {"message": message} + + get_service_registry().register_local( + service_id=service_id, + name=service_id, + description="SEL-484 unkeyed retry test tool", + category=ServiceCategory.AGENT_COMMS, + func=_echo, + credits_per_unit=2.0, + unit_name="call", + **extra, + ) + + +async def _call(client, headers, tool, request_id, arguments=None): + return await client.post( + "/mcp", + json=_rpc( + "tools/call", + request_id=request_id, + params={"name": tool, "arguments": arguments or {"message": "hi"}}, + ), + headers=headers, + ) + + +@pytest.mark.anyio +async def test_unkeyed_retry_straddling_window_boundary_dispatches_once( + client, standard_mcp_enabled, clean_database, monkeypatch +): + """Codex blocking 1: a retry across a fixed bucket edge must still replay.""" + import time as _time + + from app.core.config import get_settings + from app.routers import mcp_standard + + window = get_settings().MCP_UNKEYED_RETRY_WINDOW_SECONDS + edge = (int(_time.time() // window) + 1) * window + provisioned = await provision_agent_wallet(client) + headers = {**provisioned["agent_headers"], **MCP_HEADERS} + invocations: list[str] = [] + _register_counting_tool("echo-boundary", invocations) + try: + monkeypatch.setattr(mcp_standard, "_unkeyed_clock", lambda: edge - 0.001) + first = await _call(client, headers, "echo-boundary", 1) + monkeypatch.setattr(mcp_standard, "_unkeyed_clock", lambda: edge + 0.001) + retry = await _call(client, headers, "echo-boundary", 2) + assert first.status_code == retry.status_code == 200 + assert ( + retry.json()["result"]["receipt"]["receipt_id"] + == first.json()["result"]["receipt"]["receipt_id"] + ) + assert invocations == ["hi"] + finally: + get_service_registry().unregister_local("echo-boundary") + + +@pytest.mark.anyio +async def test_unkeyed_read_only_tool_is_not_deduped( + client, standard_mcp_enabled, clean_database +): + """Codex blocking 3: only mutating tools get the deterministic key.""" + provisioned = await provision_agent_wallet(client) + headers = {**provisioned["agent_headers"], **MCP_HEADERS} + invocations: list[str] = [] + _register_counting_tool("echo-read-only", invocations, read_only=True) + try: + for rid in (1, 2): + resp = await _call(client, headers, "echo-read-only", rid) + assert resp.status_code == 200 + assert invocations == ["hi", "hi"] + finally: + get_service_registry().unregister_local("echo-read-only") + + +def test_dogfood_notes_count_is_declared_read_only(monkeypatch): + from app.services import dogfood_tool + + monkeypatch.setattr(dogfood_tool, "_second_registered", False) + registry = get_service_registry() + existed = registry.get_local(dogfood_tool.DOGFOOD_SECOND_TOOL_ID) is not None + dogfood_tool.register_dogfood_second_tool() + try: + record = registry.get_local(dogfood_tool.DOGFOOD_SECOND_TOOL_ID) + assert record is not None and record["read_only"] is True + finally: + if not existed: + registry.unregister_local(dogfood_tool.DOGFOOD_SECOND_TOOL_ID) + + +@pytest.mark.anyio +async def test_unkeyed_retry_after_no_effect_failure_runs_again( + client, standard_mcp_enabled, clean_database +): + """Codex non-blocking 1: a refunded pre-dispatch failure is retried.""" + from sqlalchemy import select + + from app.db.models import IdempotencyRecordModel + from app.db import get_session_factory + from app.services.idempotency import GOVERNED_MCP_IDEMPOTENCY_ENDPOINT + + provisioned = await provision_agent_wallet(client) + wallet_id = provisioned["agent_wallet_id"] + headers = {**provisioned["agent_headers"], **MCP_HEADERS} + invocations: list[str] = [] + _register_counting_tool("echo-no-effect", invocations) + try: + first = await _call(client, headers, "echo-no-effect", 1) + assert first.status_code == 200 + # Rewrite the stored result into the shape the pre-dispatch failure + # path persists (nothing reached the tool, charge refunded). + async with get_session_factory()() as session: + row = ( + await session.execute( + select(IdempotencyRecordModel).where( + IdempotencyRecordModel.wallet_id == wallet_id, + IdempotencyRecordModel.endpoint + == GOVERNED_MCP_IDEMPOTENCY_ENDPOINT, + IdempotencyRecordModel.idempotency_key.like("mcp-retry-%"), + ) + ) + ).scalar_one() + row.response_json = json.dumps( + { + "error": "upstream_unreachable", + "upstream_result": { + "error": "failed_refunded", + "error_code": "upstream_unreachable", + }, + "receipt": {"outcome": "failed_refunded"}, + } + ) + await session.commit() + + retry = await _call(client, headers, "echo-no-effect", 2) + assert retry.status_code == 200 + assert "result" in retry.json(), retry.text + assert invocations == ["hi", "hi"] + again = await _call(client, headers, "echo-no-effect", 3) + assert ( + again.json()["result"]["receipt"]["receipt_id"] + == retry.json()["result"]["receipt"]["receipt_id"] + ) + assert invocations == ["hi", "hi"] + finally: + get_service_registry().unregister_local("echo-no-effect") + + +@pytest.mark.anyio +async def test_simultaneous_identical_unkeyed_calls_dispatch_once( + client, standard_mcp_enabled, clean_database +): + """Codex non-blocking 2: two concurrent identical calls are one operation.""" + import asyncio + + provisioned = await provision_agent_wallet(client) + headers = {**provisioned["agent_headers"], **MCP_HEADERS} + invocations: list[str] = [] + _register_counting_tool("echo-concurrent", invocations) + try: + a, b = await asyncio.gather( + _call(client, headers, "echo-concurrent", 1), + _call(client, headers, "echo-concurrent", 2), + ) + assert invocations == ["hi"] + receipts = set() + for resp in (a, b): + body = resp.json() + if "result" in body: + receipts.add(body["result"]["receipt"]["receipt_id"]) + else: + assert body["error"]["code"] == -32005, resp.text + assert len(receipts) == 1 + finally: + get_service_registry().unregister_local("echo-concurrent") From bfbde23edde3b08bbe7384954dd295e484fd1338 Mon Sep 17 00:00:00 2001 From: Christopher <131207452+PetrefiedThunder@users.noreply.github.com> Date: Sun, 25 Oct 2026 01:31:12 -0700 Subject: [PATCH 10/20] fix(mcp): fence unkeyed retry lock by record id (SEL-484) - keep the lock row id from begin_with_record and release with expected_record_id so a stalled holder cannot delete a newer lock - re-check lock ownership right before minting; -32005 when taken over - raise the stale-lock threshold from 30s to 120s - register_upstream accepts an operator-declared read_only flag - test: lock taken over mid-section neither dispatches nor frees the new lock --- gateway/app/routers/mcp_standard.py | 41 +++++++++++--- gateway/app/services/service_registry.py | 10 +++- gateway/tests/test_mcp_upstream_governed.py | 25 +++++++++ gateway/tests/test_standard_mcp_endpoint.py | 61 +++++++++++++++++++++ 4 files changed, 129 insertions(+), 8 deletions(-) diff --git a/gateway/app/routers/mcp_standard.py b/gateway/app/routers/mcp_standard.py index d5fdca6e..a4b0b59a 100644 --- a/gateway/app/routers/mcp_standard.py +++ b/gateway/app/routers/mcp_standard.py @@ -286,7 +286,8 @@ def _auto_permit_mint_key(client_key: str) -> str: _UNKEYED_RETRY_LOCK_ENDPOINT = "/mcp#unkeyed-retry-lock" _UNKEYED_RETRY_LOCK_WAIT_SECONDS = 5.0 -_UNKEYED_RETRY_LOCK_STALE_SECONDS = 30 +# Far above the worst-case hold (a few DB round trips plus one permit mint). +_UNKEYED_RETRY_LOCK_STALE_SECONDS = 120 # A pre-dispatch (no side effect) failure rolls the call forward to a fresh # attempt slot; bounded so a permanently failing upstream eventually replays # its failure instead of being re-run forever. @@ -298,24 +299,30 @@ def _unkeyed_clock() -> float: return time.time() -async def _acquire_unkeyed_retry_lock(wallet_id: str, lock_key: str) -> None: - """Serialize slot selection for one call identity. +async def _acquire_unkeyed_retry_lock(wallet_id: str, lock_key: str) -> str: + """Serialize slot selection for one call identity; return the lock row id. The lock is an uncompleted idempotency row: the unique insert is the mutex, ``abandon`` releases it. A row left by a crashed holder is - released once it is older than the stale threshold. + released once it is older than the stale threshold, so a holder that + merely stalls can lose the lock: it must re-check ownership with + ``_still_holds_unkeyed_retry_lock`` before acting and release only its own + row (``expected_record_id``). """ idem = get_idempotency_service() deadline = time.monotonic() + _UNKEYED_RETRY_LOCK_WAIT_SECONDS while True: try: - await idem.begin( + begun = await idem.begin_with_record( wallet_id=wallet_id, endpoint=_UNKEYED_RETRY_LOCK_ENDPOINT, idempotency_key=lock_key, request_payload={"kind": "unkeyed-retry-lock"}, ) - return + if begun.replay is not None: + # Never expected for a lock row (it is never completed). + raise IdempotencyInProgressError("idempotency_in_progress") + return begun.record_id except IdempotencyInProgressError: held = await idem.get_record( wallet_id=wallet_id, @@ -337,6 +344,17 @@ async def _acquire_unkeyed_retry_lock(wallet_id: str, lock_key: str) -> None: await anyio.sleep(0.02) +async def _still_holds_unkeyed_retry_lock( + wallet_id: str, lock_key: str, record_id: str +) -> bool: + held = await get_idempotency_service().get_record( + wallet_id=wallet_id, + endpoint=_UNKEYED_RETRY_LOCK_ENDPOINT, + idempotency_key=lock_key, + ) + return held is not None and held.record_id == record_id + + async def _resolve_unkeyed_retry( *, auth: AuthContext, @@ -382,7 +400,7 @@ async def _resolve_unkeyed_retry( idem = get_idempotency_service() lock_key = "ulock-" + identity[:48] try: - await _acquire_unkeyed_retry_lock(wallet_id, lock_key) + lock_record_id = await _acquire_unkeyed_retry_lock(wallet_id, lock_key) except IdempotencyInProgressError as exc: if map_mcp_errors: raise _mcp_error(-32005, "idempotency_in_progress") from exc @@ -411,6 +429,14 @@ async def _resolve_unkeyed_retry( ) if stored is None or not is_no_effect_failure(stored.response_json): break + if not await _still_holds_unkeyed_retry_lock( + wallet_id, lock_key, lock_record_id + ): + # Stalled past the stale threshold and taken over: the new holder + # owns slot selection now, so do not mint or dispatch. + if map_mcp_errors: + raise _mcp_error(-32005, "idempotency_in_progress") + raise IdempotencyInProgressError("idempotency_in_progress") permit_id = await _mint_auto_permit( auth=auth, tool_name=tool_name, @@ -425,6 +451,7 @@ async def _resolve_unkeyed_retry( wallet_id=wallet_id, endpoint=_UNKEYED_RETRY_LOCK_ENDPOINT, idempotency_key=lock_key, + expected_record_id=lock_record_id, ) diff --git a/gateway/app/services/service_registry.py b/gateway/app/services/service_registry.py index 4ea26c25..2d1eea17 100644 --- a/gateway/app/services/service_registry.py +++ b/gateway/app/services/service_registry.py @@ -332,8 +332,15 @@ def register_upstream( upstream_origin: str, credits_per_unit_exact: str | None = None, action_binding: ActionToolBinding | None = None, + read_only: bool = False, ) -> dict[str, Any]: - """Register one runtime-backed remote MCP tool without persisting secrets.""" + """Register one runtime-backed remote MCP tool without persisting secrets. + + ``read_only`` is an operator declaration (never taken from the + upstream's own ``readOnlyHint``, which a remote server could set to + dodge retry dedupe). It exempts the tool from the deterministic + unkeyed retry key; the default treats the tool as mutating. + """ existing = self._local_registry.get(service_id) if existing and existing.get("execution_backend") != "upstream_mcp": raise ValueError(f"Service ID already registered: {service_id}") @@ -350,6 +357,7 @@ def register_upstream( "output_schema": output_schema, "owner_wallet_id": None, "require_permit": True, + "read_only": bool(read_only), "is_active": True, "is_local": False, "external": True, diff --git a/gateway/tests/test_mcp_upstream_governed.py b/gateway/tests/test_mcp_upstream_governed.py index bb5f9fb3..7f64089f 100644 --- a/gateway/tests/test_mcp_upstream_governed.py +++ b/gateway/tests/test_mcp_upstream_governed.py @@ -3116,3 +3116,28 @@ async def test_simulated_runtime_category_still_uses_the_runtime_mode_result( ] finally: get_service_registry().unregister_local(tool_name) + + +@pytest.mark.parametrize("read_only", [False, True]) +def test_register_upstream_read_only_is_operator_declared(read_only): + """SEL-484: remote tools default to mutating; an operator may opt out.""" + tool_name = f"upstream-ro-{read_only}" + registry = get_service_registry() + kwargs = {"read_only": True} if read_only else {} + try: + registry.register_upstream( + service_id=tool_name, + name=tool_name, + description="read_only flag test", + category=ServiceCategory.AGENT_COMMS, + executor=object(), + input_schema={"type": "object"}, + output_schema=None, + credits_per_unit=1.0, + upstream_tool_name="partner.echo", + upstream_origin="https://partner.example", + **kwargs, + ) + assert registry.get_local(tool_name)["read_only"] is read_only + finally: + registry.unregister_local(tool_name) diff --git a/gateway/tests/test_standard_mcp_endpoint.py b/gateway/tests/test_standard_mcp_endpoint.py index 52942f8a..a2e3123d 100644 --- a/gateway/tests/test_standard_mcp_endpoint.py +++ b/gateway/tests/test_standard_mcp_endpoint.py @@ -998,3 +998,64 @@ async def test_simultaneous_identical_unkeyed_calls_dispatch_once( assert len(receipts) == 1 finally: get_service_registry().unregister_local("echo-concurrent") + + +@pytest.mark.anyio +async def test_stalled_lock_holder_taken_over_neither_dispatches_nor_frees_new_lock( + client, standard_mcp_enabled, clean_database, monkeypatch +): + """Codex re-review blocker: a stalled holder whose lock was reclaimed as + stale must not mint/dispatch, and its release must not delete the lock now + held by the worker that took over.""" + from app.routers import mcp_standard + from app.services.idempotency import get_idempotency_service + + provisioned = await provision_agent_wallet(client) + wallet_id = provisioned["agent_wallet_id"] + headers = {**provisioned["agent_headers"], **MCP_HEADERS} + invocations: list[str] = [] + _register_counting_tool("echo-takeover", invocations) + idem = get_idempotency_service() + real_check = mcp_standard._still_holds_unkeyed_retry_lock + taken: dict[str, str] = {} + + async def _stall_then_take_over(wid, lock_key, record_id): + # Worker B reclaims the "stale" lock while A is stalled mid-section. + await idem.abandon( + wallet_id=wid, + endpoint=mcp_standard._UNKEYED_RETRY_LOCK_ENDPOINT, + idempotency_key=lock_key, + expected_record_id=record_id, + ) + taken["key"] = lock_key + taken["id"] = await mcp_standard._acquire_unkeyed_retry_lock(wid, lock_key) + return await real_check(wid, lock_key, record_id) + + monkeypatch.setattr( + mcp_standard, "_still_holds_unkeyed_retry_lock", _stall_then_take_over + ) + try: + stalled = await _call(client, headers, "echo-takeover", 1) + assert stalled.json()["error"]["code"] == -32005, stalled.text + assert invocations == [] + + held = await idem.get_record( + wallet_id=wallet_id, + endpoint=mcp_standard._UNKEYED_RETRY_LOCK_ENDPOINT, + idempotency_key=taken["key"], + ) + assert held is not None and held.record_id == taken["id"] + + # Once B releases, the retry goes through exactly once. + monkeypatch.setattr(mcp_standard, "_still_holds_unkeyed_retry_lock", real_check) + await idem.abandon( + wallet_id=wallet_id, + endpoint=mcp_standard._UNKEYED_RETRY_LOCK_ENDPOINT, + idempotency_key=taken["key"], + expected_record_id=taken["id"], + ) + retry = await _call(client, headers, "echo-takeover", 2) + assert "result" in retry.json(), retry.text + assert invocations == ["hi"] + finally: + get_service_registry().unregister_local("echo-takeover") From 1061ec34e7c3a920cd8d4c0ea0e236305e3073e0 Mon Sep 17 00:00:00 2001 From: Christopher <131207452+PetrefiedThunder@users.noreply.github.com> Date: Sun, 25 Oct 2026 21:29:24 -0700 Subject: [PATCH 11/20] fix(mcp): fence auto-permit mint on unkeyed retry lock atomically (SEL-484) The mint-row insert now reads the lock row FOR UPDATE and checks its record_id in the same transaction, replacing the separate ownership re-check. Adds takeover tests through the real created_at stale path. --- gateway/app/routers/mcp_standard.py | 29 +--- gateway/app/services/idempotency.py | 44 +++++ gateway/docs/failure-semantics.md | 6 +- gateway/tests/test_standard_mcp_endpoint.py | 178 +++++++++++++++++--- 4 files changed, 214 insertions(+), 43 deletions(-) diff --git a/gateway/app/routers/mcp_standard.py b/gateway/app/routers/mcp_standard.py index a4b0b59a..e7c3982d 100644 --- a/gateway/app/routers/mcp_standard.py +++ b/gateway/app/routers/mcp_standard.py @@ -114,6 +114,7 @@ from app.services.agent_money import get_agent_money from app.services.idempotency import ( IdempotencyConflictError, + IdempotencyFence, IdempotencyInProgressError, InvalidIdempotencyKeyError, get_idempotency_service, @@ -306,7 +307,7 @@ async def _acquire_unkeyed_retry_lock(wallet_id: str, lock_key: str) -> str: mutex, ``abandon`` releases it. A row left by a crashed holder is released once it is older than the stale threshold, so a holder that merely stalls can lose the lock: it must re-check ownership with - ``_still_holds_unkeyed_retry_lock`` before acting and release only its own + its record id (the mint-row insert is fenced on it) before acting and release only its own row (``expected_record_id``). """ idem = get_idempotency_service() @@ -344,17 +345,6 @@ async def _acquire_unkeyed_retry_lock(wallet_id: str, lock_key: str) -> str: await anyio.sleep(0.02) -async def _still_holds_unkeyed_retry_lock( - wallet_id: str, lock_key: str, record_id: str -) -> bool: - held = await get_idempotency_service().get_record( - wallet_id=wallet_id, - endpoint=_UNKEYED_RETRY_LOCK_ENDPOINT, - idempotency_key=lock_key, - ) - return held is not None and held.record_id == record_id - - async def _resolve_unkeyed_retry( *, auth: AuthContext, @@ -429,19 +419,16 @@ async def _resolve_unkeyed_retry( ) if stored is None or not is_no_effect_failure(stored.response_json): break - if not await _still_holds_unkeyed_retry_lock( - wallet_id, lock_key, lock_record_id - ): - # Stalled past the stale threshold and taken over: the new holder - # owns slot selection now, so do not mint or dispatch. - if map_mcp_errors: - raise _mcp_error(-32005, "idempotency_in_progress") - raise IdempotencyInProgressError("idempotency_in_progress") + # The mint-row insert is fenced on this lock row: it is written only + # if we still own the lock, in the same transaction. A holder that + # stalled past the stale threshold and was taken over gets -32005 + # and never mints or dispatches. permit_id = await _mint_auto_permit( auth=auth, tool_name=tool_name, record=record, client_key=key, + lock_fence=(_UNKEYED_RETRY_LOCK_ENDPOINT, lock_key, lock_record_id), requires_human_approval=requires_human_approval, map_mcp_errors=map_mcp_errors, ) @@ -463,6 +450,7 @@ async def _mint_auto_permit( client_key: str | None, requires_human_approval: bool = False, map_mcp_errors: bool = True, + lock_fence: IdempotencyFence | None = None, ) -> str: """Mint the bounded single-tool permit backing this call. @@ -550,6 +538,7 @@ async def _mint_auto_permit( endpoint=_AUTO_PERMIT_ENDPOINT, idempotency_key=mint_key, request_payload=mint_payload, + fence=lock_fence, ) except IdempotencyConflictError as exc: if map_mcp_errors: diff --git a/gateway/app/services/idempotency.py b/gateway/app/services/idempotency.py index 5ad85ae6..ce0bb7e2 100644 --- a/gateway/app/services/idempotency.py +++ b/gateway/app/services/idempotency.py @@ -124,6 +124,17 @@ class IdempotencyInProgressError(RuntimeError): """Raised when an idempotency key is already executing without a result.""" +class IdempotencyFenceLostError(IdempotencyInProgressError): + """The fencing record named by ``fence`` is gone or now belongs to another + holder, so the new record was not written. Callers treat it as in progress: + the current holder owns the work.""" + + +# (endpoint, idempotency_key, record_id) of a lock row the writer must still +# own, in the same wallet, for a new record to be inserted. +IdempotencyFence = tuple[str, str, str] + + class IdempotencyCreationDisabledError(RuntimeError): """Raised when only existing requests may be replayed.""" @@ -488,7 +499,19 @@ async def begin_with_record( wait_timeout_seconds: float = 0.0, poll_interval_seconds: float = 0.05, allow_new_record: bool = True, + fence: IdempotencyFence | None = None, ) -> IdempotencyBegin: + """Claim (or replay) one idempotency record. + + ``fence`` makes creating a NEW record conditional on the caller still + owning a lock row: inside the same transaction as the insert, the + fence row is read ``FOR UPDATE`` (PostgreSQL holds the row lock until + commit, so a concurrent stale-lock takeover cannot delete it in + between; SQLite serializes writers and a snapshot conflict surfaces + as "database is locked", which fails closed below). A missing or + re-issued fence row raises :class:`IdempotencyFenceLostError` and + nothing is written. Replaying an existing record is not fenced. + """ if wait_timeout_seconds < 0 or poll_interval_seconds <= 0: raise ValueError("idempotency_wait_invalid") request_hash = sha256_hex(request_payload) @@ -525,6 +548,23 @@ async def begin_with_record( if not allow_new_record: raise IdempotencyCreationDisabledError("idempotency_creation_disabled") + if fence is not None: + fence_endpoint, fence_key, fence_record_id = fence + held = ( + await session.execute( + select(IdempotencyRecordModel) + .where( + *_idempotency_predicates( + wallet_id, fence_endpoint, fence_key + ) + ) + .with_for_update() + ) + ).scalar_one_or_none() + if held is None or held.record_id != fence_record_id: + await session.rollback() + raise IdempotencyFenceLostError("idempotency_in_progress") + record = IdempotencyRecordModel( record_id=f"idm-{uuid.uuid4().hex[:16]}", wallet_id=wallet_id, @@ -620,6 +660,7 @@ async def begin( request_payload: dict[str, Any], allow_new_record: bool = True, compatible_request_payload: dict[str, Any] | None = None, + fence: IdempotencyFence | None = None, ) -> IdempotencyReplay | None: """Replay an explicitly supplied historical shape without writing that shape.""" begun = await self.begin_with_compatible_record( @@ -629,6 +670,7 @@ async def begin( request_payload=request_payload, allow_new_record=allow_new_record, compatible_request_payload=compatible_request_payload, + fence=fence, ) return begun.replay @@ -641,6 +683,7 @@ async def begin_with_compatible_record( request_payload: dict[str, Any], allow_new_record: bool = True, compatible_request_payload: dict[str, Any] | None = None, + fence: IdempotencyFence | None = None, ) -> IdempotencyBegin: """Keep the granted identity while permitting historical-shape replay.""" try: @@ -650,6 +693,7 @@ async def begin_with_compatible_record( idempotency_key=idempotency_key, request_payload=request_payload, allow_new_record=allow_new_record, + fence=fence, ) except IdempotencyConflictError as conflict: if compatible_request_payload is None: diff --git a/gateway/docs/failure-semantics.md b/gateway/docs/failure-semantics.md index 3c5309b6..7528cb88 100644 --- a/gateway/docs/failure-semantics.md +++ b/gateway/docs/failure-semantics.md @@ -256,7 +256,11 @@ JSON-RPC id is not part of it, because retries carry fresh ids. `MCP_UNKEYED_RETRY_WINDOW_SECONDS` (default 300) ago is joined even if the clock bucket has rolled over, so a retry or concurrent twin straddling the edge resolves to one dispatch. A concurrent duplicate either replays the - same receipt or gets the retryable `idempotency_in_progress`. + same receipt or gets the retryable `idempotency_in_progress`. A lock older + than 120 s is treated as abandoned and can be taken over. The auto-permit + mint row is written only if the writer still owns the lock (checked in the + same transaction as the insert), so a stalled former holder never mints or + dispatches, and it releases only its own lock row. - **IGA before replay.** A replay of a stored governed result re-runs the IGA check (without spending a use) first, and the principal is part of the key, so one human never receives another's stored result. diff --git a/gateway/tests/test_standard_mcp_endpoint.py b/gateway/tests/test_standard_mcp_endpoint.py index a2e3123d..2421f294 100644 --- a/gateway/tests/test_standard_mcp_endpoint.py +++ b/gateway/tests/test_standard_mcp_endpoint.py @@ -921,7 +921,7 @@ async def test_unkeyed_retry_after_no_effect_failure_runs_again( from sqlalchemy import select from app.db.models import IdempotencyRecordModel - from app.db import get_session_factory + from app.db.database import get_session_factory from app.services.idempotency import GOVERNED_MCP_IDEMPOTENCY_ENDPOINT provisioned = await provision_agent_wallet(client) @@ -1000,13 +1000,111 @@ async def test_simultaneous_identical_unkeyed_calls_dispatch_once( get_service_registry().unregister_local("echo-concurrent") +async def _backdate_lock(wallet_id: str, lock_key: str) -> None: + """Age a held lock row past the stale threshold via its real created_at.""" + from datetime import timedelta + + from sqlalchemy import update + + from app.db.database import get_session_factory + from app.db.models import IdempotencyRecordModel + from app.routers import mcp_standard + from app.core.time import utc_now + + factory = get_session_factory() + async with factory() as session: + await session.execute( + update(IdempotencyRecordModel) + .where( + IdempotencyRecordModel.wallet_id == wallet_id, + IdempotencyRecordModel.endpoint + == mcp_standard._UNKEYED_RETRY_LOCK_ENDPOINT, + IdempotencyRecordModel.idempotency_key == lock_key, + ) + .values( + created_at=utc_now() + - timedelta(seconds=mcp_standard._UNKEYED_RETRY_LOCK_STALE_SECONDS + 5) + ) + ) + await session.commit() + + +def _stall_after_acquire(monkeypatch, on_stall): + """Holder A acquires the lock for real, then stalls (runs ``on_stall``) + before minting. Only the first acquire stalls; later ones are untouched. + Neither the ownership check nor the mint is patched.""" + from app.routers import mcp_standard + + real_acquire = mcp_standard._acquire_unkeyed_retry_lock + state = {"stalled": False} + + async def _acquire(wallet_id, lock_key): + record_id = await real_acquire(wallet_id, lock_key) + if not state["stalled"]: + state["stalled"] = True + await on_stall(wallet_id, lock_key, record_id) + return record_id + + monkeypatch.setattr(mcp_standard, "_acquire_unkeyed_retry_lock", _acquire) + + +@pytest.mark.anyio +async def test_stale_takeover_via_created_at_fences_stalled_holder_mint( + client, standard_mcp_enabled, clean_database, monkeypatch +): + """Codex blocker: no window between the ownership check and the mint. + A stalls holding the lock; its row ages past the stale threshold (real + created_at); worker B reclaims it through the real acquire path and runs + the call end-to-end. A then resumes: its fenced mint-row insert must fail + atomically or replay B's outcome, so the tool runs exactly once.""" + from app.routers import mcp_standard + from app.services.idempotency import get_idempotency_service + + provisioned = await provision_agent_wallet(client) + wallet_id = provisioned["agent_wallet_id"] + headers = {**provisioned["agent_headers"], **MCP_HEADERS} + invocations: list[str] = [] + _register_counting_tool("echo-stale", invocations) + idem = get_idempotency_service() + seen: dict[str, object] = {} + + async def _while_a_stalls(wid, lock_key, record_id): + await _backdate_lock(wid, lock_key) + seen["b"] = await _call(client, headers, "echo-stale", 2) + seen["key"] = lock_key + + _stall_after_acquire(monkeypatch, _while_a_stalls) + try: + stalled = await _call(client, headers, "echo-stale", 1) + b = seen["b"] + assert "result" in b.json(), b.text + assert invocations == ["hi"] + # A resumed after B finished: B's mint row already exists, so A's + # mint replays it (not fenced) and A returns B's receipt — never a + # second dispatch. The fence-lost path is the next test. + receipt_key = "io.agentmiddleware/receipt" + assert ( + stalled.json()["result"]["_meta"][receipt_key]["receipt_id"] + == b.json()["result"]["_meta"][receipt_key]["receipt_id"] + ), stalled.text + # B released its own lock; A's fenced release deleted nothing extra. + assert ( + await idem.get_record( + wallet_id=wallet_id, + endpoint=mcp_standard._UNKEYED_RETRY_LOCK_ENDPOINT, + idempotency_key=seen["key"], + ) + ) is None + finally: + get_service_registry().unregister_local("echo-stale") + + @pytest.mark.anyio -async def test_stalled_lock_holder_taken_over_neither_dispatches_nor_frees_new_lock( +async def test_stale_takeover_via_created_at_keeps_new_holders_lock( client, standard_mcp_enabled, clean_database, monkeypatch ): - """Codex re-review blocker: a stalled holder whose lock was reclaimed as - stale must not mint/dispatch, and its release must not delete the lock now - held by the worker that took over.""" + """A stalled holder reclaimed via the real stale path neither mints nor + dispatches, and its release leaves the new holder's lock row intact.""" from app.routers import mcp_standard from app.services.idempotency import get_idempotency_service @@ -1016,29 +1114,21 @@ async def test_stalled_lock_holder_taken_over_neither_dispatches_nor_frees_new_l invocations: list[str] = [] _register_counting_tool("echo-takeover", invocations) idem = get_idempotency_service() - real_check = mcp_standard._still_holds_unkeyed_retry_lock taken: dict[str, str] = {} + real_acquire = mcp_standard._acquire_unkeyed_retry_lock - async def _stall_then_take_over(wid, lock_key, record_id): - # Worker B reclaims the "stale" lock while A is stalled mid-section. - await idem.abandon( - wallet_id=wid, - endpoint=mcp_standard._UNKEYED_RETRY_LOCK_ENDPOINT, - idempotency_key=lock_key, - expected_record_id=record_id, - ) + async def _while_a_stalls(wid, lock_key, record_id): + await _backdate_lock(wid, lock_key) + # B goes through the unpatched acquire: stale created_at -> reclaim. + taken["id"] = await real_acquire(wid, lock_key) taken["key"] = lock_key - taken["id"] = await mcp_standard._acquire_unkeyed_retry_lock(wid, lock_key) - return await real_check(wid, lock_key, record_id) + assert taken["id"] != record_id - monkeypatch.setattr( - mcp_standard, "_still_holds_unkeyed_retry_lock", _stall_then_take_over - ) + _stall_after_acquire(monkeypatch, _while_a_stalls) try: stalled = await _call(client, headers, "echo-takeover", 1) assert stalled.json()["error"]["code"] == -32005, stalled.text assert invocations == [] - held = await idem.get_record( wallet_id=wallet_id, endpoint=mcp_standard._UNKEYED_RETRY_LOCK_ENDPOINT, @@ -1046,8 +1136,6 @@ async def _stall_then_take_over(wid, lock_key, record_id): ) assert held is not None and held.record_id == taken["id"] - # Once B releases, the retry goes through exactly once. - monkeypatch.setattr(mcp_standard, "_still_holds_unkeyed_retry_lock", real_check) await idem.abandon( wallet_id=wallet_id, endpoint=mcp_standard._UNKEYED_RETRY_LOCK_ENDPOINT, @@ -1059,3 +1147,49 @@ async def _stall_then_take_over(wid, lock_key, record_id): assert invocations == ["hi"] finally: get_service_registry().unregister_local("echo-takeover") + + +@pytest.mark.anyio +async def test_fenced_begin_writes_nothing_when_fence_row_reissued( + client, clean_database +): + """The mint-row insert and the lock ownership check are one transaction: + a fence naming a lost (missing or re-issued) lock row writes no record.""" + from app.services.idempotency import ( + IdempotencyFenceLostError, + get_idempotency_service, + ) + + wallet = (await provision_agent_wallet(client))["agent_wallet_id"] + idem = get_idempotency_service() + lock = await idem.begin_with_record( + wallet_id=wallet, + endpoint="/mcp#unkeyed-retry-lock", + idempotency_key="ulock-x", + request_payload={"kind": "unkeyed-retry-lock"}, + ) + for fence in ( + ("/mcp#unkeyed-retry-lock", "ulock-x", "idm-not-the-holder"), + ("/mcp#unkeyed-retry-lock", "ulock-gone", lock.record_id), + ): + with pytest.raises(IdempotencyFenceLostError): + await idem.begin_with_record( + wallet_id=wallet, + endpoint="/mcp#auto-permit", + idempotency_key="smcp-x", + request_payload={"k": 1}, + fence=fence, + ) + assert ( + await idem.get_record( + wallet_id=wallet, endpoint="/mcp#auto-permit", idempotency_key="smcp-x" + ) + ) is None + owned = await idem.begin_with_record( + wallet_id=wallet, + endpoint="/mcp#auto-permit", + idempotency_key="smcp-x", + request_payload={"k": 1}, + fence=("/mcp#unkeyed-retry-lock", "ulock-x", lock.record_id), + ) + assert owned.replay is None From e833b7a84204e3cf9242938f3be414f43f48e7aa Mon Sep 17 00:00:00 2001 From: Christopher <131207452+PetrefiedThunder@users.noreply.github.com> Date: Sun, 25 Oct 2026 22:01:08 -0700 Subject: [PATCH 12/20] fix(mcp): cap unkeyed retry window at permit TTL, validate it, shield lock release, release mint row on any failure - MCP_UNKEYED_RETRY_WINDOW_SECONDS rejects negatives (0 still disables) - effective window = min(window, STANDARD_MCP_PERMIT_TTL_SECONDS) so a joined slot never resolves to an expired auto permit - shield the retry-lock abandon from cancellation - mint row is abandoned by record id on any exception or cancellation; PermitError mapping unchanged --- gateway/app/core/config.py | 13 +++- gateway/app/routers/mcp_standard.py | 53 ++++++++++---- gateway/tests/test_standard_mcp_endpoint.py | 77 ++++++++++++++++++++- 3 files changed, 125 insertions(+), 18 deletions(-) diff --git a/gateway/app/core/config.py b/gateway/app/core/config.py index b9f0f3f7..ea8c11d4 100644 --- a/gateway/app/core/config.py +++ b/gateway/app/core/config.py @@ -298,9 +298,20 @@ def demo_allowed_tools_list(self) -> list[str]: # Read-only tools keep a fresh key per call. Identical deliberate unkeyed # calls inside one window collapse to one dispatch; callers needing # repeats must send distinct keys. 0 restores the old per-attempt random - # key (no retry protection). + # key (no retry protection). The effective window is capped at + # STANDARD_MCP_PERMIT_TTL_SECONDS: a joined slot reuses its auto permit, + # so a longer window would resolve retries to an expired permit. MCP_UNKEYED_RETRY_WINDOW_SECONDS: int = 300 + @field_validator("MCP_UNKEYED_RETRY_WINDOW_SECONDS") + @classmethod + def _validate_unkeyed_retry_window(cls, value: int) -> int: + if value < 0: + raise ValueError( + "MCP_UNKEYED_RETRY_WINDOW_SECONDS must be >= 0 (0 disables)" + ) + return value + # --- Public MCP endpoint (POST /mcp/public) --- # Unauthenticated, read-only Streamable HTTP surface for MCP clients # that cannot hold an API key (public plugin runtimes such as ChatGPT diff --git a/gateway/app/routers/mcp_standard.py b/gateway/app/routers/mcp_standard.py index e7c3982d..862a05c7 100644 --- a/gateway/app/routers/mcp_standard.py +++ b/gateway/app/routers/mcp_standard.py @@ -300,6 +300,20 @@ def _unkeyed_clock() -> float: return time.time() +def _effective_unkeyed_retry_window() -> int: + """Retry window in seconds, capped at the auto permit TTL. + + A joined slot reuses the permit minted for the first attempt, so a window + longer than that permit's lifetime would resolve a late retry to an + expired permit (a denial instead of a replay or a fresh mint). 0 disables. + """ + settings = get_settings() + window = settings.MCP_UNKEYED_RETRY_WINDOW_SECONDS + if window <= 0: + return 0 + return min(window, max(settings.STANDARD_MCP_PERMIT_TTL_SECONDS, 0)) + + async def _acquire_unkeyed_retry_lock(wallet_id: str, lock_key: str) -> str: """Serialize slot selection for one call identity; return the lock row id. @@ -372,7 +386,7 @@ async def _resolve_unkeyed_retry( * the auto permit is minted before the lock is released, so any later caller sees the chosen slot. """ - window = get_settings().MCP_UNKEYED_RETRY_WINDOW_SECONDS + window = _effective_unkeyed_retry_window() wallet_id = auth.wallet_id if window <= 0 or not wallet_id or record.get("read_only"): return None @@ -434,12 +448,15 @@ async def _resolve_unkeyed_retry( ) return key, permit_id finally: - await idem.abandon( - wallet_id=wallet_id, - endpoint=_UNKEYED_RETRY_LOCK_ENDPOINT, - idempotency_key=lock_key, - expected_record_id=lock_record_id, - ) + # Shielded so a cancelled request still releases its lock row + # instead of blocking the identity until the stale threshold. + with anyio.CancelScope(shield=True): + await idem.abandon( + wallet_id=wallet_id, + endpoint=_UNKEYED_RETRY_LOCK_ENDPOINT, + idempotency_key=lock_key, + expected_record_id=lock_record_id, + ) async def _mint_auto_permit( @@ -533,7 +550,7 @@ async def _mint_auto_permit( if requires_human_approval: mint_payload["requires_human_approval"] = True try: - mint_replay = await idem.begin( + mint_begun = await idem.begin_with_record( wallet_id=wallet_id, endpoint=_AUTO_PERMIT_ENDPOINT, idempotency_key=mint_key, @@ -548,20 +565,26 @@ async def _mint_auto_permit( if map_mcp_errors: raise _mcp_error(-32005, "idempotency_in_progress") from exc raise + mint_replay = mint_begun.replay if mint_replay and mint_replay.response_json: return mint_replay.response_json["permit_id"] + # Any mint failure (PermitError, PermitWriteContendedError, another + # exception, or cancellation) releases only this attempt's row, so a + # retry can mint again instead of waiting on an orphaned in-progress row. try: permit = await get_permit_service().create_permit( permit_request, subject_key_id=auth.key_id, auth=auth ) - except PermitError as exc: - await idem.abandon( - wallet_id=wallet_id, - endpoint=_AUTO_PERMIT_ENDPOINT, - idempotency_key=mint_key, - ) - if map_mcp_errors: + except BaseException as exc: + with anyio.CancelScope(shield=True): + await idem.abandon( + wallet_id=wallet_id, + endpoint=_AUTO_PERMIT_ENDPOINT, + idempotency_key=mint_key, + expected_record_id=mint_begun.record_id, + ) + if isinstance(exc, PermitError) and map_mcp_errors: raise _permit_error(exc, tool_name=tool_name, record=record) from exc raise await idem.complete( diff --git a/gateway/tests/test_standard_mcp_endpoint.py b/gateway/tests/test_standard_mcp_endpoint.py index 28d35090..95efccdc 100644 --- a/gateway/tests/test_standard_mcp_endpoint.py +++ b/gateway/tests/test_standard_mcp_endpoint.py @@ -878,10 +878,9 @@ async def test_unkeyed_retry_straddling_window_boundary_dispatches_once( """Codex blocking 1: a retry across a fixed bucket edge must still replay.""" import time as _time - from app.core.config import get_settings from app.routers import mcp_standard - window = get_settings().MCP_UNKEYED_RETRY_WINDOW_SECONDS + window = mcp_standard._effective_unkeyed_retry_window() edge = (int(_time.time() // window) + 1) * window provisioned = await provision_agent_wallet(client) headers = {**provisioned["agent_headers"], **MCP_HEADERS} @@ -1215,3 +1214,77 @@ async def test_fenced_begin_writes_nothing_when_fence_row_reissued( fence=("/mcp#unkeyed-retry-lock", "ulock-x", lock.record_id), ) assert owned.replay is None + + +def test_unkeyed_retry_window_rejects_negative_and_keeps_zero(): + """CodeRabbit: negative windows are invalid; 0 stays the opt-out.""" + from pydantic import ValidationError + + from app.core.config import Settings + + with pytest.raises(ValidationError): + Settings(MCP_UNKEYED_RETRY_WINDOW_SECONDS=-1) + assert ( + Settings(MCP_UNKEYED_RETRY_WINDOW_SECONDS=0).MCP_UNKEYED_RETRY_WINDOW_SECONDS + == 0 + ) + assert Settings().MCP_UNKEYED_RETRY_WINDOW_SECONDS == 300 + + +def test_effective_unkeyed_retry_window_capped_at_permit_ttl(monkeypatch): + """Bugbot: a joined slot must never outlive the permit it reuses.""" + from app.core.config import get_settings + from app.routers import mcp_standard + + settings = get_settings() + monkeypatch.setattr(settings, "STANDARD_MCP_PERMIT_TTL_SECONDS", 120) + monkeypatch.setattr(settings, "MCP_UNKEYED_RETRY_WINDOW_SECONDS", 300) + assert mcp_standard._effective_unkeyed_retry_window() == 120 + monkeypatch.setattr(settings, "MCP_UNKEYED_RETRY_WINDOW_SECONDS", 60) + assert mcp_standard._effective_unkeyed_retry_window() == 60 + monkeypatch.setattr(settings, "MCP_UNKEYED_RETRY_WINDOW_SECONDS", 0) + assert mcp_standard._effective_unkeyed_retry_window() == 0 + + +@pytest.mark.anyio +async def test_unkeyed_mint_failure_releases_row_for_retry( + client, standard_mcp_enabled, clean_database, monkeypatch +): + """CodeRabbit: a non-PermitError mint failure must not strand the mint row.""" + from app.services import permits as permits_module + + provisioned = await provision_agent_wallet(client) + headers = {**provisioned["agent_headers"], **MCP_HEADERS} + invocations: list[str] = [] + _register_counting_tool("echo-mint-fail", invocations) + service = permits_module.get_permit_service() + real_create = service.create_permit + calls = {"n": 0} + + async def _flaky_create(*args, **kwargs): + calls["n"] += 1 + if calls["n"] == 1: + raise RuntimeError("transient permit store failure") + return await real_create(*args, **kwargs) + + monkeypatch.setattr(service, "create_permit", _flaky_create) + try: + # The failure surfaces either as a raised error or an error + # response depending on the transport; either way nothing dispatched. + raised: BaseException | None = None + try: + first = await _call(client, headers, "echo-mint-fail", 1) + assert "result" not in first.json() or first.json()["result"].get( + "isError" + ), first.text + except BaseException as exc: # noqa: BLE001 - transport-dependent + raised = exc + if raised is not None: + assert "transient permit store failure" in repr(raised) + assert invocations == [] + retry = await _call(client, headers, "echo-mint-fail", 2) + assert retry.status_code == 200, retry.text + assert "result" in retry.json(), retry.text + assert invocations == ["hi"] + finally: + get_service_registry().unregister_local("echo-mint-fail") From 77efddfa8e9bd1d94fe85adef95992255c4b6cfe Mon Sep 17 00:00:00 2001 From: Christopher <131207452+PetrefiedThunder@users.noreply.github.com> Date: Sun, 25 Oct 2026 22:18:34 -0700 Subject: [PATCH 13/20] fix(mcp): cap unkeyed retry window at the demo auto-permit TTL Extract _auto_permit_ttl_seconds (shared by the mint and the window) so the effective window follows the TTL the mint actually uses, including DEMO_AUTO_PERMIT_TTL_SECONDS and DEMO_MAX_PERMIT_TTL_MINUTES. --- gateway/app/routers/mcp_standard.py | 56 ++++++++++++++------- gateway/tests/test_standard_mcp_endpoint.py | 23 +++++++++ 2 files changed, 62 insertions(+), 17 deletions(-) diff --git a/gateway/app/routers/mcp_standard.py b/gateway/app/routers/mcp_standard.py index 862a05c7..c40041dd 100644 --- a/gateway/app/routers/mcp_standard.py +++ b/gateway/app/routers/mcp_standard.py @@ -300,18 +300,45 @@ def _unkeyed_clock() -> float: return time.time() -def _effective_unkeyed_retry_window() -> int: - """Retry window in seconds, capped at the auto permit TTL. +def _auto_permit_ttl_seconds( + *, tenant: str | None, requires_human_approval: bool +) -> int: + """TTL in seconds of the auto permit ``_mint_auto_permit`` would mint. + + An approval-gated permit must stay valid for the whole decision window + plus the normal TTL as execution margin: permit expiry is validated + before the approval is polled, so a shorter-lived permit would strand a + decision made late in the window as a terminal permit_expired denial. + Demo permits are further capped by the demo TTL limits. + """ + settings = get_settings() + ttl_seconds = settings.STANDARD_MCP_PERMIT_TTL_SECONDS + if requires_human_approval: + ttl_seconds += approval_window_seconds() + if tenant == "demo": + ttl_seconds = min(ttl_seconds, settings.DEMO_AUTO_PERMIT_TTL_SECONDS) + if settings.DEMO_MAX_PERMIT_TTL_MINUTES is not None: + ttl_seconds = min(ttl_seconds, settings.DEMO_MAX_PERMIT_TTL_MINUTES * 60) + return ttl_seconds + + +def _effective_unkeyed_retry_window( + *, tenant: str | None = None, requires_human_approval: bool = False +) -> int: + """Retry window in seconds, capped at the TTL of the auto permit minted. A joined slot reuses the permit minted for the first attempt, so a window longer than that permit's lifetime would resolve a late retry to an - expired permit (a denial instead of a replay or a fresh mint). 0 disables. + expired permit (a denial instead of a replay or a fresh mint). The cap is + the TTL the mint will actually use, including the demo limits. 0 disables. """ - settings = get_settings() - window = settings.MCP_UNKEYED_RETRY_WINDOW_SECONDS + window = get_settings().MCP_UNKEYED_RETRY_WINDOW_SECONDS if window <= 0: return 0 - return min(window, max(settings.STANDARD_MCP_PERMIT_TTL_SECONDS, 0)) + ttl_seconds = _auto_permit_ttl_seconds( + tenant=tenant, requires_human_approval=requires_human_approval + ) + return min(window, max(ttl_seconds, 0)) async def _acquire_unkeyed_retry_lock(wallet_id: str, lock_key: str) -> str: @@ -386,7 +413,9 @@ async def _resolve_unkeyed_retry( * the auto permit is minted before the lock is released, so any later caller sees the chosen slot. """ - window = _effective_unkeyed_retry_window() + window = _effective_unkeyed_retry_window( + tenant=auth.tenant, requires_human_approval=requires_human_approval + ) wallet_id = auth.wallet_id if window <= 0 or not wallet_id or record.get("read_only"): return None @@ -480,13 +509,9 @@ async def _mint_auto_permit( if not wallet_id: # narrowed by the caller; fail closed regardless raise _mcp_error(-32003, "wallet_scoped_key_required") settings = get_settings() - # An approval-gated permit must stay valid for the whole decision window - # plus the normal TTL as execution margin: permit expiry is validated - # before the approval is polled, so a shorter-lived permit would strand a - # decision made late in the window as a terminal permit_expired denial. - ttl_seconds = settings.STANDARD_MCP_PERMIT_TTL_SECONDS - if requires_human_approval: - ttl_seconds += approval_window_seconds() + ttl_seconds = _auto_permit_ttl_seconds( + tenant=auth.tenant, requires_human_approval=requires_human_approval + ) budget = _tool_call_budget(record) if auth.tenant == "demo": demo_tools = settings.demo_allowed_tools_list @@ -512,9 +537,6 @@ async def _mint_auto_permit( if map_mcp_errors: raise _permit_error(error, tool_name=tool_name, record=record) raise error - ttl_seconds = min(ttl_seconds, settings.DEMO_AUTO_PERMIT_TTL_SECONDS) - if settings.DEMO_MAX_PERMIT_TTL_MINUTES is not None: - ttl_seconds = min(ttl_seconds, settings.DEMO_MAX_PERMIT_TTL_MINUTES * 60) permit_request = PermitCreateRequest( issuer_wallet_id=wallet_id, subject_wallet_id=wallet_id, diff --git a/gateway/tests/test_standard_mcp_endpoint.py b/gateway/tests/test_standard_mcp_endpoint.py index 95efccdc..5e9a9dd0 100644 --- a/gateway/tests/test_standard_mcp_endpoint.py +++ b/gateway/tests/test_standard_mcp_endpoint.py @@ -1288,3 +1288,26 @@ async def _flaky_create(*args, **kwargs): assert invocations == ["hi"] finally: get_service_registry().unregister_local("echo-mint-fail") + + +def test_effective_unkeyed_retry_window_capped_at_demo_permit_ttl(monkeypatch): + """Bugbot: demo auto permits are shorter-lived; the window must follow.""" + from app.core.config import get_settings + from app.routers import mcp_standard + + settings = get_settings() + monkeypatch.setattr(settings, "STANDARD_MCP_PERMIT_TTL_SECONDS", 120) + monkeypatch.setattr(settings, "MCP_UNKEYED_RETRY_WINDOW_SECONDS", 300) + monkeypatch.setattr(settings, "DEMO_AUTO_PERMIT_TTL_SECONDS", 45) + monkeypatch.setattr(settings, "DEMO_MAX_PERMIT_TTL_MINUTES", None) + assert mcp_standard._effective_unkeyed_retry_window(tenant="demo") == 45 + assert mcp_standard._effective_unkeyed_retry_window(tenant="acme") == 120 + monkeypatch.setattr(settings, "DEMO_AUTO_PERMIT_TTL_SECONDS", 200) + monkeypatch.setattr(settings, "DEMO_MAX_PERMIT_TTL_MINUTES", 1) + assert mcp_standard._effective_unkeyed_retry_window(tenant="demo") == 60 + # The window always equals the TTL the mint itself uses. + assert mcp_standard._effective_unkeyed_retry_window( + tenant="demo" + ) == mcp_standard._auto_permit_ttl_seconds( + tenant="demo", requires_human_approval=False + ) From 33f5ed5f0980edd0f8904b600330af461d514191 Mon Sep 17 00:00:00 2001 From: Christopher <131207452+PetrefiedThunder@users.noreply.github.com> Date: Mon, 26 Oct 2026 21:55:50 -0700 Subject: [PATCH 14/20] fix(mcp): refuse unkeyed calls with no retry slot instead of minting a random key (#208, SEL-498) --- gateway/app/routers/mcp.py | 6 +- gateway/app/routers/mcp_standard.py | 30 ++++- gateway/docs/concepts/request-keys.md | 8 +- gateway/docs/failure-semantics.md | 12 +- gateway/tests/test_standard_mcp_endpoint.py | 135 +++++++++++++++----- 5 files changed, 145 insertions(+), 46 deletions(-) diff --git a/gateway/app/routers/mcp.py b/gateway/app/routers/mcp.py index 2df8a099..95b8d615 100644 --- a/gateway/app/routers/mcp.py +++ b/gateway/app/routers/mcp.py @@ -4603,6 +4603,11 @@ async def _maybe_mint_demo_permit(request: GovernedRequest) -> str | None: ) if resolved is not None: request.idempotency_key, permit_id = resolved + elif not request.idempotency_key: + # No deterministic retry slot and no client key: refuse rather + # than mint a random per-attempt key that would let a retry + # dispatch and bill twice (SEL-498 / #208). + raise ValueError("idempotency_key_required") else: permit_id = await _mint_auto_permit( auth=request.auth, @@ -4627,7 +4632,6 @@ async def _maybe_mint_demo_permit(request: GovernedRequest) -> str | None: except IdempotencyConflictError as exc: raise ValueError("idempotency_key_reused") from exc request.permit_id = permit_id - request.idempotency_key = request.idempotency_key or f"mcp-auto-{uuid.uuid4().hex}" return permit_id diff --git a/gateway/app/routers/mcp_standard.py b/gateway/app/routers/mcp_standard.py index c40041dd..7d46ce06 100644 --- a/gateway/app/routers/mcp_standard.py +++ b/gateway/app/routers/mcp_standard.py @@ -62,7 +62,6 @@ import json import logging import time -import uuid from datetime import timedelta from decimal import Decimal, InvalidOperation from typing import Any @@ -159,6 +158,20 @@ ) +_IDEMPOTENCY_KEY_REQUIRED_DATA: dict[str, Any] = { + "error": "idempotency_key_required", + "reason_code": "idempotency_key_required", + "remediation": { + "type": "retry_with_idempotency_key", + "detail": ( + "Send an Idempotency-Key header (or params._meta" + '["io.agentmiddleware/idempotency_key"]) and reuse the same key ' + "when retrying the same call." + ), + }, +} + + def _mcp_error(code: int, message: str, data: dict[str, Any] | None = None) -> McpError: return McpError(mcp_types.ErrorData(code=code, message=message, data=data)) @@ -397,8 +410,8 @@ async def _resolve_unkeyed_retry( ) -> tuple[str, str] | None: """Pick the retry slot for an unkeyed mutating call and mint its permit. - Returns ``(idempotency_key, permit_id)``, or None when the call keeps the - legacy per-attempt random key: read-only tools (a replay would serve a + Returns ``(idempotency_key, permit_id)``, or None when no slot applies and + the caller must refuse the unkeyed call (SEL-498): read-only tools (a replay would serve a stale read), the window disabled, or an enterprise bearer that fails verification (the governed pipeline denies that call regardless). @@ -732,7 +745,16 @@ async def _governed_tools_call( if resolved is not None: idempotency_key, permit_id = resolved else: - idempotency_key = client_idempotency_key or f"mcp-auto-{uuid.uuid4().hex}" + if not client_idempotency_key: + # No client key and no deterministic retry slot (read-only tool, + # window 0, or an unverifiable enterprise bearer). A random + # per-attempt key would let a lost-response retry dispatch and + # bill twice, so refuse before any permit, dispatch, or charge + # (SEL-498 / #208). + raise _mcp_error( + -32003, "idempotency_key_required", _IDEMPOTENCY_KEY_REQUIRED_DATA + ) + idempotency_key = client_idempotency_key permit_id = permit_id or await _mint_auto_permit( auth=auth, tool_name=tool_name, diff --git a/gateway/docs/concepts/request-keys.md b/gateway/docs/concepts/request-keys.md index ae499658..e95381ca 100644 --- a/gateway/docs/concepts/request-keys.md +++ b/gateway/docs/concepts/request-keys.md @@ -35,10 +35,12 @@ An identical retry within `MCP_UNKEYED_RETRY_WINDOW_SECONDS` (default 300) of the first call replays its receipt; the window slides from that first call, so a retry across a clock bucket edge still replays. If the stored result is a refunded pre-dispatch failure (nothing reached the tool), the retry runs -again instead. Tools registered `read_only` keep a fresh key per call. +again instead. Tools registered `read_only` get no derived key. Identical deliberate calls inside one window collapse into one, so send your -own key when that matters. A window of `0` restores a fresh `mcp-auto-` key -per call. The SDK's `invoke_tool` +own key when that matters. The gateway never mints a random per-call key: +an unkeyed call that gets no derived key (a `read_only` tool, a window of +`0`, or an enterprise bearer that fails verification) is refused with +`-32003 idempotency_key_required` before any permit, dispatch, or charge. The SDK's `invoke_tool` requires a key; its `"auto"` option derives one from arguments and is unsuitable when distinct calls can have the same arguments. diff --git a/gateway/docs/failure-semantics.md b/gateway/docs/failure-semantics.md index 7528cb88..84ad855b 100644 --- a/gateway/docs/failure-semantics.md +++ b/gateway/docs/failure-semantics.md @@ -265,15 +265,19 @@ JSON-RPC id is not part of it, because retries carry fresh ids. check (without spending a use) first, and the principal is part of the key, so one human never receives another's stored result. - **Mutating tools only.** Tools registered `read_only` (for example - `partner.notes.count`) keep a fresh key per call. Undeclared tools are - treated as mutating. + `partner.notes.count`) get no derived key, so an unkeyed call to one is + refused (below). Undeclared tools are treated as mutating. - **No-effect failures retry.** If the stored result is a refunded pre-dispatch failure, the retry moves to the next attempt slot and runs again (up to three attempts per window) instead of replaying the failure. Two deliberately identical un-keyed calls inside one window still collapse -into one; clients that need repeats must send their own key. Setting the -window to `0` restores a random key per call. A +into one; clients that need repeats must send their own key. The gateway never mints a +random per-call key: an unkeyed call with no derived key (a `read_only` tool, +the window set to `0`, or an enterprise bearer that fails verification) is +refused with JSON-RPC `-32003` `idempotency_key_required` (HTTP 400 on +`/mcp/tools/{service_id}/invoke`) before any permit, dispatch, or charge, so +a retry cannot run or bill twice. A governed `/mcp/messages` call without a key is still refused with `idempotency_key_required`, and a wallet that requires human approval still needs an explicit key. diff --git a/gateway/tests/test_standard_mcp_endpoint.py b/gateway/tests/test_standard_mcp_endpoint.py index bc986fd1..c1be8fb7 100644 --- a/gateway/tests/test_standard_mcp_endpoint.py +++ b/gateway/tests/test_standard_mcp_endpoint.py @@ -801,50 +801,59 @@ def _echo(message="ok", n=0): registry.unregister_local("standard-echo-retry") +async def _ledger_debits(client, provisioned, tool): + ledger = await client.get( + f"/v1/billing/ledger/{provisioned['agent_wallet_id']}", + headers=provisioned["agent_headers"], + ) + assert ledger.status_code == 200, ledger.text + return [e for e in ledger.json()["entries"] if tool in e["description"]] + + +def _assert_idempotency_key_required(resp): + assert resp.status_code == 200, resp.text + error = resp.json()["error"] + assert error["code"] == -32003 + assert error["message"] == "idempotency_key_required" + assert error["data"]["reason_code"] == "idempotency_key_required" + assert error["data"]["remediation"]["type"] == "retry_with_idempotency_key" + + @pytest.mark.anyio -async def test_unkeyed_retry_window_zero_restores_legacy_dispatch( +async def test_unkeyed_call_with_window_zero_is_refused_without_dispatch( client, standard_mcp_enabled, clean_database, monkeypatch ): - """With the window disabled each unkeyed attempt dispatches (legacy).""" + """SEL-498: no retry slot and no key is refused, never a random key.""" from app.core.config import get_settings monkeypatch.setattr(get_settings(), "MCP_UNKEYED_RETRY_WINDOW_SECONDS", 0) provisioned = await provision_agent_wallet(client) invocations: list[str] = [] - - def _echo(message="ok", n=0): - invocations.append(message) - return {"message": message} - - registry = get_service_registry() - registry.register_local( - service_id="standard-echo-legacy", - name="Standard Echo Legacy", - description="Window disabled test tool", - category=ServiceCategory.AGENT_COMMS, - func=_echo, - credits_per_unit=2.0, - unit_name="call", - ) + _register_counting_tool("standard-echo-legacy", invocations) headers = {**provisioned["agent_headers"], **MCP_HEADERS} try: for rid in (201, 202): - resp = await client.post( - "/mcp", - json=_rpc( - "tools/call", - request_id=rid, - params={ - "name": "standard-echo-legacy", - "arguments": {"message": "hi"}, - }, - ), - headers=headers, - ) - assert resp.status_code == 200 - assert invocations == ["hi", "hi"] + resp = await _call(client, headers, "standard-echo-legacy", rid) + _assert_idempotency_key_required(resp) + assert invocations == [] + assert await _ledger_debits(client, provisioned, "standard-echo-legacy") == [] + + # The remediation works: a keyed call and its keyed retry dispatch + # and bill exactly once. + keyed = {**headers, "Idempotency-Key": "sel-498-window-zero"} + first = await _call(client, keyed, "standard-echo-legacy", 203) + retry = await _call(client, keyed, "standard-echo-legacy", 204) + assert first.status_code == retry.status_code == 200 + assert ( + retry.json()["result"]["receipt"]["receipt_id"] + == first.json()["result"]["receipt"]["receipt_id"] + ) + assert invocations == ["hi"] + assert ( + len(await _ledger_debits(client, provisioned, "standard-echo-legacy")) == 1 + ) finally: - registry.unregister_local("standard-echo-legacy") + get_service_registry().unregister_local("standard-echo-legacy") def _register_counting_tool(service_id: str, invocations: list, **extra): @@ -907,10 +916,10 @@ async def test_unkeyed_retry_straddling_window_boundary_dispatches_once( @pytest.mark.anyio -async def test_unkeyed_read_only_tool_is_not_deduped( +async def test_unkeyed_read_only_tool_is_refused_without_dispatch( client, standard_mcp_enabled, clean_database ): - """Codex blocking 3: only mutating tools get the deterministic key.""" + """Read-only tools get no deterministic slot, so unkeyed calls are refused.""" provisioned = await provision_agent_wallet(client) headers = {**provisioned["agent_headers"], **MCP_HEADERS} invocations: list[str] = [] @@ -918,12 +927,70 @@ async def test_unkeyed_read_only_tool_is_not_deduped( try: for rid in (1, 2): resp = await _call(client, headers, "echo-read-only", rid) + _assert_idempotency_key_required(resp) + assert invocations == [] + assert await _ledger_debits(client, provisioned, "echo-read-only") == [] + + # Distinct keys are distinct reads; each dispatches (no stale replay). + for rid, key in ((3, "sel-498-read-a"), (4, "sel-498-read-b")): + resp = await _call( + client, {**headers, "Idempotency-Key": key}, "echo-read-only", rid + ) assert resp.status_code == 200 + assert "result" in resp.json(), resp.text assert invocations == ["hi", "hi"] finally: get_service_registry().unregister_local("echo-read-only") +@pytest.mark.anyio +async def test_demo_permit_mint_refuses_unkeyed_call_without_slot(monkeypatch): + """mcp.py demo path: no slot and no key raises before any permit is minted.""" + from types import SimpleNamespace + + from app.core.config import get_settings + from app.routers import mcp as mcp_router + from app.routers import mcp_standard + + minted: list[str] = [] + + async def _no_slot(**_kwargs): + return None + + async def _mint(**_kwargs): + minted.append("permit") + return "permit-should-not-exist" + + async def _no_approval(_wallet_id): + return False + + monkeypatch.setattr(mcp_standard, "_resolve_unkeyed_retry", _no_slot) + monkeypatch.setattr(mcp_standard, "_mint_auto_permit", _mint) + monkeypatch.setattr(mcp_router, "wallet_human_approval_required", _no_approval) + monkeypatch.setattr(get_settings(), "DEMO_ALLOWED_TOOLS", "partner.echo") + monkeypatch.setattr(mcp_router, "_ensure_local_mcp_tools_registered", lambda: None) + registry = get_service_registry() + existed = registry.get_local("partner.echo") is not None + if not existed: + _register_counting_tool("partner.echo", []) + request = SimpleNamespace( + auth=SimpleNamespace(tenant="demo", wallet_id="w-demo", allowed_tools=None), + wallet_id="w-demo", + permit_id=None, + tool_name="partner.echo", + arguments={"message": "hi"}, + idempotency_key=None, + ) + try: + with pytest.raises(ValueError, match="^idempotency_key_required$"): + await mcp_router._maybe_mint_demo_permit(request) + assert minted == [] + assert request.idempotency_key is None + finally: + if not existed: + registry.unregister_local("partner.echo") + + def test_dogfood_notes_count_is_declared_read_only(monkeypatch): from app.services import dogfood_tool From 28fd3ff005fd8ebd706b480a374562e3d3134e54 Mon Sep 17 00:00:00 2001 From: Christopher <131207452+PetrefiedThunder@users.noreply.github.com> Date: Mon, 26 Oct 2026 21:58:52 -0700 Subject: [PATCH 15/20] fix(mcp): register idempotency_key_required reason code and document it --- gateway/app/core/reason_codes.py | 1 + gateway/docs/denial-details.md | 1 + gateway/tests/test_reason_code_registry.py | 1 + 3 files changed, 3 insertions(+) diff --git a/gateway/app/core/reason_codes.py b/gateway/app/core/reason_codes.py index d36678a9..687f70cd 100644 --- a/gateway/app/core/reason_codes.py +++ b/gateway/app/core/reason_codes.py @@ -115,6 +115,7 @@ "idempotency_key_control_characters", "idempotency_key_not_a_string", "idempotency_key_not_utf8", + "idempotency_key_required", "idempotency_key_required_for_human_approval", "idempotency_key_reused", "idempotency_key_too_long", diff --git a/gateway/docs/denial-details.md b/gateway/docs/denial-details.md index 5f77664a..0b459de9 100644 --- a/gateway/docs/denial-details.md +++ b/gateway/docs/denial-details.md @@ -122,6 +122,7 @@ marked API-only occur before a signed denial receipt can be created. | `key_tool_not_allowed` | The API key's own tool allowlist excludes the tool. | Use a key that allows the tool. | | `demo_tool_not_allowed` | Demo-tenant wallets may call only the demo allowlist. | Use an allowed demo tool or a non-demo wallet. | | `demo_auto_permit_budget_exceeded` | The tool costs more than the demo auto-permit credit cap. | Use a cheaper tool or a non-demo wallet. | +| `idempotency_key_required` | An unkeyed MCP `tools/call` got no derived retry key (a `read_only` tool, `MCP_UNKEYED_RETRY_WINDOW_SECONDS=0`, or an enterprise bearer that fails verification). Refused before any permit, dispatch, or charge instead of minting a random key. | Send an `Idempotency-Key` and reuse it when retrying the same call. | | `idempotency_key_required_for_human_approval` | The wallet policy requires approval, and without a client idempotency key each retry would page a human again. | Send an `Idempotency-Key` and retry the same call with the same key to poll the decision. | | `idempotency_key_not_a_string` | API-only: a supplied request key is not a string. | Send a string key. | | `idempotency_key_blank` | API-only: the key is empty or only whitespace. | Send a non-blank key. | diff --git a/gateway/tests/test_reason_code_registry.py b/gateway/tests/test_reason_code_registry.py index 0e4d454c..a2fa38a7 100644 --- a/gateway/tests/test_reason_code_registry.py +++ b/gateway/tests/test_reason_code_registry.py @@ -68,6 +68,7 @@ "idempotency_key_control_characters", "idempotency_key_not_a_string", "idempotency_key_not_utf8", + "idempotency_key_required", "idempotency_key_required_for_human_approval", "idempotency_key_reused", "idempotency_key_too_long", From 97583bb84a358013b34f5ec4903086ed5e091283 Mon Sep 17 00:00:00 2001 From: Christopher <131207452+PetrefiedThunder@users.noreply.github.com> Date: Mon, 26 Oct 2026 22:08:53 -0700 Subject: [PATCH 16/20] test(mcp): prove read-only tool calls are metered, so unkeyed reads stay refused (#208) --- gateway/tests/test_standard_mcp_endpoint.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/gateway/tests/test_standard_mcp_endpoint.py b/gateway/tests/test_standard_mcp_endpoint.py index c1be8fb7..45e4938d 100644 --- a/gateway/tests/test_standard_mcp_endpoint.py +++ b/gateway/tests/test_standard_mcp_endpoint.py @@ -12,6 +12,8 @@ import hashlib +from decimal import Decimal + import pytest from httpx import ASGITransport, AsyncClient from mcp.types import LATEST_PROTOCOL_VERSION @@ -939,6 +941,12 @@ async def test_unkeyed_read_only_tool_is_refused_without_dispatch( assert resp.status_code == 200 assert "result" in resp.json(), resp.text assert invocations == ["hi", "hi"] + # Lead's exemption test (#233): read-only calls ARE metered. Each keyed + # read debits the wallet at the tool's per-call price, so an unkeyed + # read could bill twice on retry and must stay refused. + debits = await _ledger_debits(client, provisioned, "echo-read-only") + assert len(debits) == 2 + assert all(Decimal(str(e["amount"])) != 0 for e in debits) finally: get_service_registry().unregister_local("echo-read-only") From 67afd3c21939f8f3a6910f992cbd90ccc74620da Mon Sep 17 00:00:00 2001 From: Christopher <131207452+PetrefiedThunder@users.noreply.github.com> Date: Mon, 26 Oct 2026 22:10:15 -0700 Subject: [PATCH 17/20] docs(registry): read_only tools are metered; unkeyed calls are refused --- gateway/app/services/service_registry.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/gateway/app/services/service_registry.py b/gateway/app/services/service_registry.py index 2d1eea17..e64254fc 100644 --- a/gateway/app/services/service_registry.py +++ b/gateway/app/services/service_registry.py @@ -269,9 +269,10 @@ def register_local( requires a signed permit + idempotency key — even if legacy unpermitted MCP is otherwise allowed. - ``read_only`` declares the tool free of side effects. Unkeyed - standard ``/mcp`` calls to it keep a fresh key per call instead of the - deterministic retry key (a replay would serve a stale read). The + ``read_only`` declares the tool free of side effects. It gets no + deterministic retry key (a replay would serve a stale read), and it is + still metered, so an unkeyed standard ``/mcp`` call to it is refused + with ``idempotency_key_required`` (SEL-498). The default is False: an undeclared tool is treated as mutating, which fails safe (deduped, never double-dispatched). """ From a073408d0a26eaece25f1c2dd7c9b1bb3f481b5c Mon Sep 17 00:00:00 2001 From: Christopher <131207452+PetrefiedThunder@users.noreply.github.com> Date: Mon, 26 Oct 2026 22:22:33 -0700 Subject: [PATCH 18/20] fix(idempotency): portable fence claim, truthful retry window, real pre-dispatch test, IGA on removed-tool replay (Codex 5662513347) --- gateway/app/core/config.py | 15 +- gateway/app/routers/mcp.py | 3 + gateway/app/services/idempotency.py | 38 +++-- gateway/docs/concepts/request-keys.md | 9 +- gateway/docs/failure-semantics.md | 5 +- gateway/tests/test_mcp_retry_key.py | 8 +- gateway/tests/test_standard_mcp_endpoint.py | 176 +++++++++++++++----- 7 files changed, 186 insertions(+), 68 deletions(-) diff --git a/gateway/app/core/config.py b/gateway/app/core/config.py index 1b6700a0..8a9780ee 100644 --- a/gateway/app/core/config.py +++ b/gateway/app/core/config.py @@ -314,12 +314,15 @@ def demo_allowed_tools_list(self) -> list[str]: # crosses a bucket edge still joins it), so a lost-response retry within # this many seconds replays the original receipt instead of dispatching # again; a refunded pre-dispatch failure is retried, not replayed. - # Read-only tools keep a fresh key per call. Identical deliberate unkeyed - # calls inside one window collapse to one dispatch; callers needing - # repeats must send distinct keys. 0 restores the old per-attempt random - # key (no retry protection). The effective window is capped at - # STANDARD_MCP_PERMIT_TTL_SECONDS: a joined slot reuses its auto permit, - # so a longer window would resolve retries to an expired permit. + # Read-only tools get no derived key, so unkeyed read-only calls are + # refused with idempotency_key_required (SEL-498); there is never a + # random per-call key. Identical deliberate unkeyed calls inside one + # window collapse to one dispatch; callers needing repeats must send + # distinct keys. 0 disables derived keys, so every unkeyed call is + # refused. The effective window is capped at the auto-permit TTL + # (STANDARD_MCP_PERMIT_TTL_SECONDS, default 120; lower for demo): a + # joined slot reuses its auto permit. With defaults the window is 120 s, + # and a later identical retry runs and bills again. MCP_UNKEYED_RETRY_WINDOW_SECONDS: int = 300 @field_validator("MCP_UNKEYED_RETRY_WINDOW_SECONDS") diff --git a/gateway/app/routers/mcp.py b/gateway/app/routers/mcp.py index 95b8d615..8589d2b1 100644 --- a/gateway/app/routers/mcp.py +++ b/gateway/app/routers/mcp.py @@ -1387,6 +1387,9 @@ async def _execute_registered_tool_inner( key_id=auth.key_id, replay=replay, ) + # Same IGA gate as the registered-tool replay path: a revoked + # principal must not read stored results via a removed tool. + await _assert_replay_iga_access(auth=auth, tool_name=tool_name) await _raise_replayed_error(replay) return replay.response_json reason = f"Tool not found: {tool_name}" diff --git a/gateway/app/services/idempotency.py b/gateway/app/services/idempotency.py index ce0bb7e2..ff05e0f3 100644 --- a/gateway/app/services/idempotency.py +++ b/gateway/app/services/idempotency.py @@ -10,7 +10,7 @@ from datetime import timedelta from typing import Any, cast -from sqlalchemy import and_, or_, select +from sqlalchemy import and_, or_, select, update from sqlalchemy.exc import IntegrityError, OperationalError from sqlalchemy.ext.asyncio import AsyncSession from sqlalchemy.sql.elements import ColumnElement @@ -505,10 +505,11 @@ async def begin_with_record( ``fence`` makes creating a NEW record conditional on the caller still owning a lock row: inside the same transaction as the insert, the - fence row is read ``FOR UPDATE`` (PostgreSQL holds the row lock until - commit, so a concurrent stale-lock takeover cannot delete it in - between; SQLite serializes writers and a snapshot conflict surfaces - as "database is locked", which fails closed below). A missing or + fence row is claimed with a conditional no-op ``UPDATE`` that matches + only our open lock row (rowcount 1). The write takes the PostgreSQL + row lock and the SQLite database write lock until commit, so a + concurrent stale-lock takeover cannot delete it in between on either + backend; SQLite ignores ``FOR UPDATE``, so it is not relied on. A missing or re-issued fence row raises :class:`IdempotencyFenceLostError` and nothing is written. Replaying an existing record is not fenced. """ @@ -550,18 +551,23 @@ async def begin_with_record( if fence is not None: fence_endpoint, fence_key, fence_record_id = fence - held = ( - await session.execute( - select(IdempotencyRecordModel) - .where( - *_idempotency_predicates( - wallet_id, fence_endpoint, fence_key - ) - ) - .with_for_update() + # Claim the fence with a conditional write, not SELECT ... + # FOR UPDATE (SQLite ignores FOR UPDATE). The no-op UPDATE + # matches only while our lock row is still ours and open; it + # takes the PostgreSQL row lock and the SQLite write lock in + # this transaction, so a stale-lock takeover (DELETE) cannot + # land before this insert commits. rowcount 0 => lost. + claimed = await session.execute( + update(IdempotencyRecordModel) + .where( + *_idempotency_predicates(wallet_id, fence_endpoint, fence_key), + col(IdempotencyRecordModel.record_id) == fence_record_id, + col(IdempotencyRecordModel.response_json).is_(None), ) - ).scalar_one_or_none() - if held is None or held.record_id != fence_record_id: + .values(request_hash=IdempotencyRecordModel.request_hash) + .execution_options(synchronize_session=False) + ) + if cast(Any, claimed).rowcount != 1: await session.rollback() raise IdempotencyFenceLostError("idempotency_in_progress") diff --git a/gateway/docs/concepts/request-keys.md b/gateway/docs/concepts/request-keys.md index e95381ca..53aa4f55 100644 --- a/gateway/docs/concepts/request-keys.md +++ b/gateway/docs/concepts/request-keys.md @@ -31,8 +31,13 @@ permit creation uses HTTP 409 with string `detail`. When the caller supplies no key to a mutating tool, standard `/mcp` and the legacy `/mcp/messages` demo auto-permit path derive an `mcp-retry-` key from wallet, API key, verified enterprise principal, tool, and canonical arguments. -An identical retry within `MCP_UNKEYED_RETRY_WINDOW_SECONDS` (default 300) of -the first call replays its receipt; the window slides from that first call, +An identical retry within the effective retry window of the first call +replays its receipt. The effective window is the smaller of +`MCP_UNKEYED_RETRY_WINDOW_SECONDS` (default 300) and the auto-permit TTL +(`STANDARD_MCP_PERMIT_TTL_SECONDS`, default 120; demo tenants are capped +lower by `DEMO_AUTO_PERMIT_TTL_SECONDS`), so with default settings it is +120 seconds. A retry after the effective window is a new call: it runs and +bills again. Send your own key if retries can arrive later than that; the window slides from that first call, so a retry across a clock bucket edge still replays. If the stored result is a refunded pre-dispatch failure (nothing reached the tool), the retry runs again instead. Tools registered `read_only` get no derived key. diff --git a/gateway/docs/failure-semantics.md b/gateway/docs/failure-semantics.md index 84ad855b..46ae34cd 100644 --- a/gateway/docs/failure-semantics.md +++ b/gateway/docs/failure-semantics.md @@ -253,7 +253,10 @@ JSON-RPC id is not part of it, because retries carry fresh ids. - **Sliding window.** Slot selection and the auto-permit mint run under a per-identity lock row. A slot first minted less than - `MCP_UNKEYED_RETRY_WINDOW_SECONDS` (default 300) ago is joined even if the + the effective window ago is joined (the smaller of + `MCP_UNKEYED_RETRY_WINDOW_SECONDS`, default 300, and the auto-permit TTL, + default 120 and lower for demo tenants; so 120 s by default, after which an + identical unkeyed retry runs and bills again) even if the clock bucket has rolled over, so a retry or concurrent twin straddling the edge resolves to one dispatch. A concurrent duplicate either replays the same receipt or gets the retryable `idempotency_in_progress`. A lock older diff --git a/gateway/tests/test_mcp_retry_key.py b/gateway/tests/test_mcp_retry_key.py index ae1f1013..e0b3ad33 100644 --- a/gateway/tests/test_mcp_retry_key.py +++ b/gateway/tests/test_mcp_retry_key.py @@ -1,6 +1,7 @@ """Data-free unit tests for deterministic unkeyed MCP retry keys (SEL-484).""" import json +from uuid import uuid4 from app.services.mcp_retry_key import ( DERIVED_KEY_PREFIX, @@ -11,7 +12,12 @@ slot_key, ) -BASE = dict(wallet_id="w1", key_id="k1", principal="", tool_name="t") +BASE = dict( + wallet_id=f"wallet-{uuid4()}", + key_id=f"key-{uuid4()}", + principal="", + tool_name=f"tool-{uuid4()}", +) def _identity(**overrides): diff --git a/gateway/tests/test_standard_mcp_endpoint.py b/gateway/tests/test_standard_mcp_endpoint.py index 45e4938d..fa5e8c75 100644 --- a/gateway/tests/test_standard_mcp_endpoint.py +++ b/gateway/tests/test_standard_mcp_endpoint.py @@ -8,13 +8,12 @@ from __future__ import annotations -import json - import hashlib from decimal import Decimal import pytest +from uuid import uuid4 from httpx import ASGITransport, AsyncClient from mcp.types import LATEST_PROTOCOL_VERSION from b2a_sdk.receipt_verifier import key_set_from_document, verify_bundle @@ -1018,58 +1017,41 @@ def test_dogfood_notes_count_is_declared_read_only(monkeypatch): async def test_unkeyed_retry_after_no_effect_failure_runs_again( client, standard_mcp_enabled, clean_database ): - """Codex non-blocking 1: a refunded pre-dispatch failure is retried.""" - from sqlalchemy import select + """Codex: a real refunded pre-dispatch failure is retried, not replayed. - from app.db.models import IdempotencyRecordModel - from app.db.database import get_session_factory - from app.services.idempotency import GOVERNED_MCP_IDEMPOTENCY_ENDPOINT + The upstream executor genuinely fails before dispatch (nothing reaches + the tool, the charge is refunded); the identical unkeyed retry then runs + once and a further retry replays that success. + """ + from tests.test_mcp_upstream_governed import ( + FakeUpstreamExecutor, + _register_upstream, + ) + tool = f"echo-no-effect-{uuid4().hex[:8]}" provisioned = await provision_agent_wallet(client) - wallet_id = provisioned["agent_wallet_id"] headers = {**provisioned["agent_headers"], **MCP_HEADERS} - invocations: list[str] = [] - _register_counting_tool("echo-no-effect", invocations) + executor = FakeUpstreamExecutor("pre_dispatch_failure") + _register_upstream(tool, executor) try: - first = await _call(client, headers, "echo-no-effect", 1) - assert first.status_code == 200 - # Rewrite the stored result into the shape the pre-dispatch failure - # path persists (nothing reached the tool, charge refunded). - async with get_session_factory()() as session: - row = ( - await session.execute( - select(IdempotencyRecordModel).where( - IdempotencyRecordModel.wallet_id == wallet_id, - IdempotencyRecordModel.endpoint - == GOVERNED_MCP_IDEMPOTENCY_ENDPOINT, - IdempotencyRecordModel.idempotency_key.like("mcp-retry-%"), - ) - ) - ).scalar_one() - row.response_json = json.dumps( - { - "error": "upstream_unreachable", - "upstream_result": { - "error": "failed_refunded", - "error_code": "upstream_unreachable", - }, - "receipt": {"outcome": "failed_refunded"}, - } - ) - await session.commit() + first = await _call(client, headers, tool, 1) + assert first.status_code == 200, first.text + assert executor.dispatch_count == 0 + assert len(executor.calls) == 1 - retry = await _call(client, headers, "echo-no-effect", 2) - assert retry.status_code == 200 + executor.mode = "success" + retry = await _call(client, headers, tool, 2) + assert retry.status_code == 200, retry.text assert "result" in retry.json(), retry.text - assert invocations == ["hi", "hi"] - again = await _call(client, headers, "echo-no-effect", 3) + assert executor.dispatch_count == 1 + again = await _call(client, headers, tool, 3) assert ( again.json()["result"]["receipt"]["receipt_id"] == retry.json()["result"]["receipt"]["receipt_id"] ) - assert invocations == ["hi", "hi"] + assert executor.dispatch_count == 1 finally: - get_service_registry().unregister_local("echo-no-effect") + get_service_registry().unregister_local(tool) @pytest.mark.anyio @@ -1296,6 +1278,76 @@ async def test_fenced_begin_writes_nothing_when_fence_row_reissued( assert owned.replay is None +@pytest.mark.anyio +async def test_fenced_begin_races_stale_lock_takeover_consistently( + client, clean_database +): + """Codex blocker 1: the fence claim is a conditional write (rowcount 1), + not SELECT FOR UPDATE, so it is exclusive on SQLite as well as + PostgreSQL. Race a holder's fenced mint against a stale-lock takeover + many times: the mint either commits (and the takeover then sees the + released row) or fails as fence-lost with nothing written, never both a + lost fence and a written mint row.""" + import asyncio + + from app.services.idempotency import ( + IdempotencyFenceLostError, + IdempotencyInProgressError, + get_idempotency_service, + ) + + wallet = (await provision_agent_wallet(client))["agent_wallet_id"] + idem = get_idempotency_service() + lock_endpoint = "/mcp#unkeyed-retry-lock" + outcomes: set[str] = set() + for _ in range(20): + lock_key = f"ulock-{uuid4()}" + mint_key = f"smcp-{uuid4()}" + lock = await idem.begin_with_record( + wallet_id=wallet, + endpoint=lock_endpoint, + idempotency_key=lock_key, + request_payload={"kind": "unkeyed-retry-lock"}, + ) + + async def _mint(lock=lock, lock_key=lock_key, mint_key=mint_key): + try: + await idem.begin_with_record( + wallet_id=wallet, + endpoint="/mcp#auto-permit", + idempotency_key=mint_key, + request_payload={"k": 1}, + fence=(lock_endpoint, lock_key, lock.record_id), + ) + return "minted" + except IdempotencyFenceLostError: + return "fence_lost" + except IdempotencyInProgressError: + return "busy" + + async def _takeover(lock=lock, lock_key=lock_key): + try: + return await idem.abandon( + wallet_id=wallet, + endpoint=lock_endpoint, + idempotency_key=lock_key, + expected_record_id=lock.record_id, + ) + except Exception: # noqa: BLE001 - SQLite busy is a lost race + return False + + mint_result, _ = await asyncio.gather(_mint(), _takeover()) + outcomes.add(mint_result) + mint_row = await idem.get_record( + wallet_id=wallet, endpoint="/mcp#auto-permit", idempotency_key=mint_key + ) + if mint_result == "minted": + assert mint_row is not None + else: + assert mint_row is None + assert outcomes <= {"minted", "fence_lost", "busy"} + + def test_unkeyed_retry_window_rejects_negative_and_keeps_zero(): """CodeRabbit: negative windows are invalid; 0 stays the opt-out.""" from pydantic import ValidationError @@ -1311,6 +1363,46 @@ def test_unkeyed_retry_window_rejects_negative_and_keeps_zero(): assert Settings().MCP_UNKEYED_RETRY_WINDOW_SECONDS == 300 +def test_unregistered_tool_replay_runs_iga_check(): + """Codex should-fix: the replay served after a tool is removed must run + the same IGA gate as the registered-tool replay path.""" + import inspect + + from app.routers import mcp + + src = inspect.getsource(mcp) + end = src.index('reason = f"Tool not found') + branch = src[src.rindex("if not service:", 0, end) : end] + assert "_assert_replay_iga_access(auth=auth, tool_name=tool_name)" in branch + assert branch.index("_assert_replay_iga_access") < branch.index( + "_raise_replayed_error" + ) + + +def test_default_unkeyed_retry_window_is_permit_ttl_and_documented(): + """Codex blocker 2: with defaults the window is the 120 s permit TTL, + not the 300 s setting, and the docs say so (a later retry bills again).""" + from pathlib import Path + + from app.core.config import Settings, get_settings + from app.routers import mcp_standard + + defaults = Settings() + settings = get_settings() + assert defaults.MCP_UNKEYED_RETRY_WINDOW_SECONDS == 300 + assert defaults.STANDARD_MCP_PERMIT_TTL_SECONDS == 120 + assert settings.MCP_UNKEYED_RETRY_WINDOW_SECONDS == 300 + assert mcp_standard._effective_unkeyed_retry_window() == min( + 300, settings.STANDARD_MCP_PERMIT_TTL_SECONDS + ) + docs = Path(__file__).resolve().parents[1] / "docs" + for name in ("concepts/request-keys.md", "failure-semantics.md"): + text = " ".join((docs / name).read_text().split()) + assert "STANDARD_MCP_PERMIT_TTL_SECONDS" in text or "auto-permit TTL" in text + assert "120" in text + assert "bills again" in text + + def test_effective_unkeyed_retry_window_capped_at_permit_ttl(monkeypatch): """Bugbot: a joined slot must never outlive the permit it reuses.""" from app.core.config import get_settings From 20de68468f12f5daee587d1c34d674327671294d Mon Sep 17 00:00:00 2001 From: Christopher <131207452+PetrefiedThunder@users.noreply.github.com> Date: Mon, 26 Oct 2026 22:47:15 -0700 Subject: [PATCH 19/20] fix(idempotency): map fence-claim lock timeout to in_progress; deterministic fence tests; billing asserts (Codex 5662733998) --- gateway/app/services/idempotency.py | 31 ++- gateway/tests/test_standard_mcp_endpoint.py | 273 +++++++++++++++----- 2 files changed, 228 insertions(+), 76 deletions(-) diff --git a/gateway/app/services/idempotency.py b/gateway/app/services/idempotency.py index ff05e0f3..40595936 100644 --- a/gateway/app/services/idempotency.py +++ b/gateway/app/services/idempotency.py @@ -557,16 +557,29 @@ async def begin_with_record( # takes the PostgreSQL row lock and the SQLite write lock in # this transaction, so a stale-lock takeover (DELETE) cannot # land before this insert commits. rowcount 0 => lost. - claimed = await session.execute( - update(IdempotencyRecordModel) - .where( - *_idempotency_predicates(wallet_id, fence_endpoint, fence_key), - col(IdempotencyRecordModel.record_id) == fence_record_id, - col(IdempotencyRecordModel.response_json).is_(None), + try: + claimed = await session.execute( + update(IdempotencyRecordModel) + .where( + *_idempotency_predicates( + wallet_id, fence_endpoint, fence_key + ), + col(IdempotencyRecordModel.record_id) == fence_record_id, + col(IdempotencyRecordModel.response_json).is_(None), + ) + .values(request_hash=IdempotencyRecordModel.request_hash) + .execution_options(synchronize_session=False) ) - .values(request_hash=IdempotencyRecordModel.request_hash) - .execution_options(synchronize_session=False) - ) + except OperationalError as exc: + # SQLite lock timeout while another writer (e.g. a + # takeover) holds the database: the current holder owns + # the work, so this is retryable in-progress, not a 500. + if "database is locked" not in str(exc): + raise + await session.rollback() + raise IdempotencyInProgressError( + "idempotency_in_progress" + ) from None if cast(Any, claimed).rowcount != 1: await session.rollback() raise IdempotencyFenceLostError("idempotency_in_progress") diff --git a/gateway/tests/test_standard_mcp_endpoint.py b/gateway/tests/test_standard_mcp_endpoint.py index fa5e8c75..09c42498 100644 --- a/gateway/tests/test_standard_mcp_endpoint.py +++ b/gateway/tests/test_standard_mcp_endpoint.py @@ -13,6 +13,8 @@ from decimal import Decimal import pytest + +from tests.conftest import running_on_sqlite from uuid import uuid4 from httpx import ASGITransport, AsyncClient from mcp.types import LATEST_PROTOCOL_VERSION @@ -1033,15 +1035,39 @@ async def test_unkeyed_retry_after_no_effect_failure_runs_again( headers = {**provisioned["agent_headers"], **MCP_HEADERS} executor = FakeUpstreamExecutor("pre_dispatch_failure") _register_upstream(tool, executor) + + async def _entries(): + ledger = await client.get( + f"/v1/billing/ledger/{provisioned['agent_wallet_id']}", + headers=provisioned["agent_headers"], + ) + assert ledger.status_code == 200, ledger.text + return {e["entry_id"]: e for e in ledger.json()["entries"]} + + def _net(entries): + return sum((Decimal(str(e["amount"])) for e in entries), Decimal("0")) + try: + before = await _entries() first = await _call(client, headers, tool, 1) assert first.status_code == 200, first.text assert executor.dispatch_count == 0 assert len(executor.calls) == 1 + after_failure = await _entries() + failure_entries = [e for k, e in after_failure.items() if k not in before] + # Charged then refunded: no net charge for the pre-dispatch failure. + assert _net(failure_entries) == 0, failure_entries executor.mode = "success" retry = await _call(client, headers, tool, 2) assert retry.status_code == 200, retry.text + after_success = await _entries() + success_entries = [ + e for k, e in after_success.items() if k not in after_failure + ] + debits = [e for e in success_entries if Decimal(str(e["amount"])) < 0] + assert len(debits) == 1, success_entries + assert _net(success_entries) == Decimal(str(debits[0]["amount"])) < 0 assert "result" in retry.json(), retry.text assert executor.dispatch_count == 1 again = await _call(client, headers, tool, 3) @@ -1050,6 +1076,8 @@ async def test_unkeyed_retry_after_no_effect_failure_runs_again( == retry.json()["result"]["receipt"]["receipt_id"] ) assert executor.dispatch_count == 1 + # The replay adds no ledger entry. + assert await _entries() == after_success finally: get_service_registry().unregister_local(tool) @@ -1278,89 +1306,200 @@ async def test_fenced_begin_writes_nothing_when_fence_row_reissued( assert owned.replay is None +def _sqlite_db_path() -> str: + from app.db.database import get_engine + + engine = get_engine() + assert engine is not None + return engine.sync_engine.url.database + + +def _external_takeover(lock_record_id: str) -> int: + """Delete the holder's lock row from a separate SQLite connection, the + way a stale-lock takeover would. Short timeout: if the holder has the + write lock this raises ``database is locked``; it is never swallowed.""" + import sqlite3 + + conn = sqlite3.connect(_sqlite_db_path(), timeout=0.2) + try: + cur = conn.execute( + "DELETE FROM idempotency_records WHERE record_id = ?", + (lock_record_id,), + ) + conn.commit() + return cur.rowcount + finally: + conn.close() + + +async def _fresh_lock(idem, wallet): + lock_key = f"ulock-{uuid4()}" + lock = await idem.begin_with_record( + wallet_id=wallet, + endpoint="/mcp#unkeyed-retry-lock", + idempotency_key=lock_key, + request_payload={"kind": "unkeyed-retry-lock"}, + ) + return lock_key, lock + + @pytest.mark.anyio -async def test_fenced_begin_races_stale_lock_takeover_consistently( - client, clean_database -): - """Codex blocker 1: the fence claim is a conditional write (rowcount 1), - not SELECT FOR UPDATE, so it is exclusive on SQLite as well as - PostgreSQL. Race a holder's fenced mint against a stale-lock takeover - many times: the mint either commits (and the takeover then sees the - released row) or fails as fence-lost with nothing written, never both a - lost fence and a written mint row.""" - import asyncio +@pytest.mark.skipif(not running_on_sqlite(), reason="SQLite lock semantics") +async def test_fence_claim_blocks_takeover_until_mint_commits(client, clean_database): + """Codex blocker 1 (deterministic): a takeover forced after the holder's + fence step and before its mint insert must lose, and the loser writes + nothing. Under the old SELECT ... FOR UPDATE (ignored by SQLite) the + takeover deletes the lock row and the mint still commits: both win.""" + import sqlite3 + + from sqlalchemy import event + + from app.db.database import get_engine + from app.services.idempotency import get_idempotency_service + + wallet = (await provision_agent_wallet(client))["agent_wallet_id"] + idem = get_idempotency_service() + lock_key, lock = await _fresh_lock(idem, wallet) + mint_key = f"smcp-{uuid4()}" + takeover: dict = {} + + def _before_insert(conn, cursor, statement, parameters, context, many): + if ( + statement.lstrip().upper().startswith("INSERT INTO IDEMPOTENCY_RECORDS") + and mint_key in str(parameters) + and not takeover + ): + try: + takeover["deleted"] = _external_takeover(lock.record_id) + except sqlite3.OperationalError as exc: + takeover["error"] = str(exc) + + sync_engine = get_engine().sync_engine + event.listen(sync_engine, "before_cursor_execute", _before_insert) + try: + minted = await idem.begin_with_record( + wallet_id=wallet, + endpoint="/mcp#auto-permit", + idempotency_key=mint_key, + request_payload={"k": 1}, + fence=("/mcp#unkeyed-retry-lock", lock_key, lock.record_id), + ) + finally: + event.remove(sync_engine, "before_cursor_execute", _before_insert) + assert minted.replay is None + # The takeover ran and lost: it hit the holder's write lock, deleted + # nothing, and the holder's lock row survives. + assert takeover == {"error": "database is locked"}, takeover + held = await idem.get_record( + wallet_id=wallet, + endpoint="/mcp#unkeyed-retry-lock", + idempotency_key=lock_key, + ) + assert held is not None and held.record_id == lock.record_id + +@pytest.mark.anyio +@pytest.mark.skipif(not running_on_sqlite(), reason="SQLite lock semantics") +async def test_fence_lost_before_claim_writes_nothing(client, clean_database): + """Takeover forced between the holder's read and its fence claim: the + claim matches no row, the holder raises fence-lost and writes nothing.""" + from sqlalchemy import event + + from app.db.database import get_engine from app.services.idempotency import ( IdempotencyFenceLostError, - IdempotencyInProgressError, get_idempotency_service, ) wallet = (await provision_agent_wallet(client))["agent_wallet_id"] idem = get_idempotency_service() - lock_endpoint = "/mcp#unkeyed-retry-lock" - outcomes: set[str] = set() - for _ in range(20): - lock_key = f"ulock-{uuid4()}" - mint_key = f"smcp-{uuid4()}" - lock = await idem.begin_with_record( - wallet_id=wallet, - endpoint=lock_endpoint, - idempotency_key=lock_key, - request_payload={"kind": "unkeyed-retry-lock"}, - ) - - async def _mint(lock=lock, lock_key=lock_key, mint_key=mint_key): - try: - await idem.begin_with_record( - wallet_id=wallet, - endpoint="/mcp#auto-permit", - idempotency_key=mint_key, - request_payload={"k": 1}, - fence=(lock_endpoint, lock_key, lock.record_id), - ) - return "minted" - except IdempotencyFenceLostError: - return "fence_lost" - except IdempotencyInProgressError: - return "busy" - - async def _takeover(lock=lock, lock_key=lock_key): - try: - return await idem.abandon( - wallet_id=wallet, - endpoint=lock_endpoint, - idempotency_key=lock_key, - expected_record_id=lock.record_id, - ) - except Exception: # noqa: BLE001 - SQLite busy is a lost race - return False - - mint_result, _ = await asyncio.gather(_mint(), _takeover()) - outcomes.add(mint_result) - mint_row = await idem.get_record( + lock_key, lock = await _fresh_lock(idem, wallet) + mint_key = f"smcp-{uuid4()}" + takeover: dict = {} + + def _before_claim(conn, cursor, statement, parameters, context, many): + upper = statement.lstrip().upper() + if ( + upper.startswith(("UPDATE IDEMPOTENCY_RECORDS", "SELECT")) + and lock_key in str(parameters) + and not takeover + ): + takeover["deleted"] = _external_takeover(lock.record_id) + + sync_engine = get_engine().sync_engine + event.listen(sync_engine, "before_cursor_execute", _before_claim) + try: + with pytest.raises(IdempotencyFenceLostError): + await idem.begin_with_record( + wallet_id=wallet, + endpoint="/mcp#auto-permit", + idempotency_key=mint_key, + request_payload={"k": 1}, + fence=("/mcp#unkeyed-retry-lock", lock_key, lock.record_id), + ) + finally: + event.remove(sync_engine, "before_cursor_execute", _before_claim) + assert takeover == {"deleted": 1} + assert ( + await idem.get_record( wallet_id=wallet, endpoint="/mcp#auto-permit", idempotency_key=mint_key ) - if mint_result == "minted": - assert mint_row is not None - else: - assert mint_row is None - assert outcomes <= {"minted", "fence_lost", "busy"} + ) is None -def test_unkeyed_retry_window_rejects_negative_and_keeps_zero(): - """CodeRabbit: negative windows are invalid; 0 stays the opt-out.""" - from pydantic import ValidationError +@pytest.mark.anyio +@pytest.mark.skipif(not running_on_sqlite(), reason="SQLite lock semantics") +async def test_fence_claim_lock_timeout_is_retryable_in_progress( + client, clean_database +): + """Codex: a SQLite lock timeout on the fence claim maps to the retryable + idempotency_in_progress, not an internal error, and writes nothing.""" + import sqlite3 + + from sqlalchemy import event - from app.core.config import Settings + from app.db.database import get_engine + from app.services.idempotency import ( + IdempotencyFenceLostError, + IdempotencyInProgressError, + get_idempotency_service, + ) - with pytest.raises(ValidationError): - Settings(MCP_UNKEYED_RETRY_WINDOW_SECONDS=-1) + wallet = (await provision_agent_wallet(client))["agent_wallet_id"] + idem = get_idempotency_service() + lock_key, lock = await _fresh_lock(idem, wallet) + mint_key = f"smcp-{uuid4()}" + blocker = sqlite3.connect(_sqlite_db_path(), timeout=5) + blocker.isolation_level = None + + def _short_busy(conn, cursor, statement, parameters, context, many): + if statement.lstrip().upper().startswith("UPDATE IDEMPOTENCY_RECORDS"): + cursor.execute("PRAGMA busy_timeout=100") + + engine = get_engine() + event.listen(engine.sync_engine, "before_cursor_execute", _short_busy) + blocker.execute("BEGIN IMMEDIATE") + try: + with pytest.raises(IdempotencyInProgressError) as caught: + await idem.begin_with_record( + wallet_id=wallet, + endpoint="/mcp#auto-permit", + idempotency_key=mint_key, + request_payload={"k": 1}, + fence=("/mcp#unkeyed-retry-lock", lock_key, lock.record_id), + ) + assert not isinstance(caught.value, IdempotencyFenceLostError) + assert str(caught.value) == "idempotency_in_progress" + finally: + blocker.execute("ROLLBACK") + blocker.close() + event.remove(engine.sync_engine, "before_cursor_execute", _short_busy) + await engine.dispose() # drop the connection left at busy_timeout=100 assert ( - Settings(MCP_UNKEYED_RETRY_WINDOW_SECONDS=0).MCP_UNKEYED_RETRY_WINDOW_SECONDS - == 0 - ) - assert Settings().MCP_UNKEYED_RETRY_WINDOW_SECONDS == 300 + await idem.get_record( + wallet_id=wallet, endpoint="/mcp#auto-permit", idempotency_key=mint_key + ) + ) is None def test_unregistered_tool_replay_runs_iga_check(): From 4e7564b5d9d81149c001f72a1c4a446992826dc2 Mon Sep 17 00:00:00 2001 From: Christopher <131207452+PetrefiedThunder@users.noreply.github.com> Date: Mon, 26 Oct 2026 23:04:55 -0700 Subject: [PATCH 20/20] test(#233): restore unkeyed retry window Settings validation test; require failure ledger rows --- gateway/tests/test_standard_mcp_endpoint.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/gateway/tests/test_standard_mcp_endpoint.py b/gateway/tests/test_standard_mcp_endpoint.py index 09c42498..3b3b6439 100644 --- a/gateway/tests/test_standard_mcp_endpoint.py +++ b/gateway/tests/test_standard_mcp_endpoint.py @@ -1056,6 +1056,7 @@ def _net(entries): after_failure = await _entries() failure_entries = [e for k, e in after_failure.items() if k not in before] # Charged then refunded: no net charge for the pre-dispatch failure. + assert failure_entries, "expected charge+refund ledger rows" assert _net(failure_entries) == 0, failure_entries executor.mode = "success" @@ -1502,6 +1503,21 @@ def _short_busy(conn, cursor, statement, parameters, context, many): ) is None +def test_unkeyed_retry_window_rejects_negative_and_keeps_zero(): + """CodeRabbit: negative windows are invalid; 0 stays the opt-out.""" + from pydantic import ValidationError + + from app.core.config import Settings + + with pytest.raises(ValidationError): + Settings(MCP_UNKEYED_RETRY_WINDOW_SECONDS=-1) + assert ( + Settings(MCP_UNKEYED_RETRY_WINDOW_SECONDS=0).MCP_UNKEYED_RETRY_WINDOW_SECONDS + == 0 + ) + assert Settings().MCP_UNKEYED_RETRY_WINDOW_SECONDS == 300 + + def test_unregistered_tool_replay_runs_iga_check(): """Codex should-fix: the replay served after a tool is removed must run the same IGA gate as the registered-tool replay path."""