Skip to content

Post-execution exceptions leave a completed tool call with no audit outcome #571

Closed

Description

@Yatsuiii

Summary

CMCPProxy.call_tool writes a terminal audit entry for an unexpected exception raised before the upstream tool call, and writes nothing for one raised after it. Since the tool has already run by then, an upstream can execute a tool call and leave no record of it in the audit chain.

The invariant is not missing from this codebase. It is implemented on one side of the upstream request only.

POLICY-003 (tests/unit/test_mcp_proxy.py:556) asserts that a Cedar backend exception appends a fault entry before re-raising. That is the pre-execution case, and it works. There is no equivalent boundary on the response side, and everything between _forward_to_upstream returning and the audit append at proxy.py:1221 is fallible.

Verified end to end against main at a2893da. The reproduction below exits 0 on confirmation and includes POLICY-003 as a live control in the same run.

Reproduction result

--- A  nested text inside a valid envelope
    crash path:
        cmcp_runtime/mcp/proxy.py:1209  call_tool()
        cmcp_runtime/mcp/proxy.py:139  _extract_external_execution_evidence()
    upstream invoked    : ['test.echo']
    outcome             : RecursionError propagated out of call_tool
    audit entries       : 1 -> 1  ['session_start']
    chain tip changed   : False

--- B  nested envelope, before the size guard runs
    crash path:
        cmcp_runtime/mcp/proxy.py:975  call_tool()
        cmcp_runtime/mcp/proxy.py:525  _forward_to_upstream()
        cmcp_runtime/mcp/streamable_http.py:113  parse_response()
    upstream invoked    : ['test.echo']
    outcome             : RecursionError propagated out of call_tool
    audit entries       : 1 -> 1  ['session_start']
    chain tip changed   : False

--- C  control: pre-execution exception (POLICY-003)
    crash path:
        cmcp_runtime/mcp/proxy.py:861  call_tool()
    upstream invoked    : []
    outcome             : RuntimeError propagated out of call_tool
    audit entries       : 1 -> 2  ['session_start', 'fault']
    chain tip changed   : True

Same class of unexpected exception. One side of the upstream request records it, the other does not.

The concrete trigger

json.loads raises RecursionError on deeply nested input. RecursionError subclasses RuntimeError, not ValueError, so it is caught by neither of the two guards standing between the upstream call and the audit write:

  • proxy.py:140 catches json.JSONDecodeError only.
  • proxy.py:538 catches ValueError and maps it to UpstreamUnavailable.

There are two independent paths, which is the part that matters for how this gets fixed.

Path A, nested text inside a valid envelope. The envelope is small, shallow, and entirely valid, so parse_response handles it without complaint. The hostile shape is the result.content[].text string, which _extract_external_execution_evidence parses again at proxy.py:139. The crash lands at the call site on proxy.py:1209, twelve lines above the audit append on proxy.py:1221.

Path B, nested envelope. No inner-text trick needed. parse_response calls response.json() at streamable_http.py:113 and dies there, inside _forward_to_upstream. This one is worth separating out because it fires before the max_response_size_bytes guard at proxy.py:1016 has run at all, so that guard cannot bound it at any configured value.

Patching either guard on its own leaves the other. That is what makes this look like an ordering problem rather than a missing exception type.

Cost to trigger

Bisected on the harness below, the decoder gives out between depth 9,987 and 10,218, so roughly 60KB, which is under 3% of the 2MB response size guard. The exact threshold depends on how much C stack remains at the point of the parse, so it will differ in a real deployment. It is not a large or expensive payload in any case.

Why this is an audit problem and not only a 500

MCPServer._handle_tool_call wraps call_tool in except Exception and returns a 500 TEE_FAULT, so the process survives. What does not survive is the record.

docs/spec/threat-model.md is specific about why that matters:

  • Line 31 enumerates A3, malicious or compromised MCP server, whose stated capability is "Controls the tool's response payload" and whose stated limit is "Cannot modify runtime policy or audit entries."
  • Line 66 names the mitigation for the corresponding repudiation threat: "Tool server denies a call was made", mitigated by "Audit entry records call, tool server identity, and response hash."

A3 cannot rewrite an audit entry. It can cause one not to exist, using exactly the capability A3 is stipulated to have. The response payload is the input that decides whether the call gets recorded.

I want to be careful about the size of that claim. This is not remote code execution and it is not policy bypass. Ingress policy still ran and still decided. What is lost is the evidence that the call happened, on a gateway whose stated purpose is producing that evidence.

A buggy upstream reaches the same end state without any adversary at all.

Suggested fix

Two parts, and I think only the second one actually closes it.

1. The narrow patch. Widen the two guards. At proxy.py:140, except (json.JSONDecodeError, RecursionError) matches how that helper already treats unparseable optional evidence. At proxy.py:538, adding RecursionError alongside ValueError maps it to the existing UpstreamUnavailable path.

That stops today's reproduction. It does not establish the invariant, and the next exception class that shows up between the upstream call and the audit write reopens the same hole.

2. The finalization boundary. One exception boundary immediately after _forward_to_upstream returns successfully. If any later stage raises before a terminal outcome has been recorded, append exactly one fault entry carrying call_id, tool name, request hash, the failed stage, and the exception type, then re-raise so the server keeps its current 500 behaviour.

One boundary rather than a try around each stage. POLICY-003 already established both the entry type and the append-then-re-raise shape, so this would be applying an existing decision to the other side of the request rather than introducing a new concept.

The stages currently exposed between the upstream response and the audit append, for sizing:

Location Stage
streamable_http.py:113 envelope parse
proxy.py:561 result join and serialize
proxy.py:1016 response size check
proxy.py:1054 AGT response interception
proxy.py:1130 session mutation
proxy.py:1139 response encode
proxy.py:1142 egress authorization, catches PolicyDeny only
proxy.py:1209 execution evidence extraction

I have only proven streamable_http.py:113 and proxy.py:1209 reachable. The rest are listed as the surface the boundary would cover, not as claimed bugs.

3. Bounding the input is worth doing too, but separately. docs/spec/proxy-security.md specs MAX_JSON_NESTING_DEPTH = 64 and its fuzz target 4 is "Tool response processor. Input: arbitrary JSON as tool response. Output: processed response or error. Must not crash." That constant is not implemented anywhere in src/ or scripts/ today. It would stop this specific payload, but it is defence in depth for the ordering problem rather than a fix for it. This overlaps #562 and the request-side caps from #556 and #561, and it would want a maintainer's view on whether the response side shares those constants or carries its own.

Happy to open part 1 immediately since it is small and self-contained. Part 2 is a design decision about where the terminal-outcome boundary belongs, and I did not want to pick that unilaterally inside a bug report.

What I have not verified

  • The full HTTP server path with the real AGT scanner was not exercised. The reproduction stubs MCPGateway and MCPResponseScanner so it lands on cMCP's own parsing. Path B does not depend on that stubbing, since it crashes inside _forward_to_upstream before any scanner is reached.
  • Absence from a SQLite-backed store was not queried directly. It follows from AuditChain.append never being called, but I did not run it.
  • The other six stages in the table above are listed by inspection, not proven reachable.

Reproduction

Save as repro_audit_gap.py in the repo root and run .venv/bin/python repro_audit_gap.py. Exits 0 when confirmed.

repro_audit_gap.py
"""Reproduction: cMCP records a terminal audit outcome for an unexpected
exception raised BEFORE a tool call executes, but not for one raised AFTER it.

An upstream that has already run the tool can therefore leave no entry in the
audit chain, by returning a response whose shape crashes the response path.

The concrete crash is `json.loads` raising `RecursionError` on deeply nested
input. `RecursionError` subclasses `RuntimeError`, not `ValueError`, so it is
caught by neither of the two guards standing between the upstream call and the
audit write.

Three scenarios, all against the same proxy:

  A  nested text inside a valid envelope   -> crash at proxy.py:139,  no entry
  B  nested envelope                       -> crash at streamable_http.py:113,
                                              no entry, and this one lands
                                              before the response size guard
                                              has run at all
  C  pre-execution policy exception        -> fault entry IS written
                                              (POLICY-003, the existing control)

C is the point. The invariant already exists in this codebase. It stops at the
upstream call.

Run from the repo root with the project venv:
    .venv/bin/python repro_audit_gap.py

Exits 0 when the gap is confirmed.
"""
from __future__ import annotations

import asyncio
import json
import threading
import traceback
from http.server import BaseHTTPRequestHandler, HTTPServer
from unittest.mock import MagicMock, patch

from cmcp_runtime.audit.chain import AuditChain
from cmcp_runtime.catalog.loader import (
    ApprovedDefinition,
    CatalogEntry,
    ServerIdentity,
    ToolCatalog,
)
from cmcp_runtime.config import AttestationConfig, Config, EnforcementMode
from cmcp_runtime.session.state import SessionState

# Depth is not the interesting number, reachability is. Bisected on this
# harness, the decoder gives out between depth 9,987 and 10,218, i.e. about
# 60KB, which is under 3% of the 2MB response size guard. The exact threshold
# depends on how much C stack is left at the point of the parse, so it will
# differ in a real deployment. 60,000 is used below to stay clear of that edge.
NEST_DEPTH = 60_000

upstream_invocations: list[str] = []


def _nested(depth: int) -> str:
    return '{"a":' * depth + "1" + "}" * depth


class _HostileUpstream(BaseHTTPRequestHandler):
    """Serves whatever `_HostileUpstream.mode` is set to.

    In both hostile modes every envelope-level check the gateway performs
    before the crash passes cleanly. Nothing here is a malformed HTTP response
    or a malformed JSON-RPC envelope in the sense the gateway checks for.
    """

    mode = "inner"

    def do_POST(self):  # noqa: N802
        length = int(self.headers.get("Content-Length", 0))
        request = json.loads(self.rfile.read(length))

        if request.get("method", "") == "tools/list":
            # Provenance check the gateway runs before forwarding.
            body = json.dumps({
                "jsonrpc": "2.0",
                "id": request.get("id"),
                "result": {"tools": [{
                    "name": "test.echo",
                    "description": "echo",
                    "inputSchema": {"type": "object"},
                }]},
            })
        else:
            upstream_invocations.append(request["params"]["name"])
            if self.mode == "inner":
                # Small, shallow, entirely valid envelope. The hostile shape is
                # the text content inside it, which the gateway parses again.
                body = json.dumps({
                    "jsonrpc": "2.0",
                    "id": request["id"],
                    "result": {"content": [{"type": "text", "text": _nested(NEST_DEPTH)}]},
                })
            else:
                # The envelope itself is the hostile shape.
                body = ('{"jsonrpc":"2.0","id":"' + str(request["id"])
                        + '","result":{"deep":' + _nested(NEST_DEPTH) + "}}")

        payload = body.encode()
        self.send_response(200)
        self.send_header("Content-Type", "application/json")
        self.send_header("Content-Length", str(len(payload)))
        self.end_headers()
        self.wfile.write(payload)

    def log_message(self, *args):
        pass


def _make_proxy(server_url: str, audit_chain: AuditChain, evaluator=None):
    from cmcp_runtime.mcp.proxy import CMCPProxy

    entry = CatalogEntry(
        tool_name="test.echo",
        server=ServerIdentity(
            display_name="Hostile",
            url=server_url,
            tls_fingerprint="SHA256:" + "A" * 43 + "=",
            spiffe_id=None,
            transport="http-sse",
            rotation_mode="key-pinned",
        ),
        approved_definition=ApprovedDefinition(
            description="echo", input_schema={"type": "object"}, output_schema=None
        ),
        definition_hash="sha256:" + "0" * 64,
        compliance_domain="public",
        requires_baa=False,
        sensitivity_level="public",
        added_at="2026-06-10T00:00:00Z",
        approved_by="test",
    )
    catalog = ToolCatalog(entries={"test.echo": entry}, catalog_hash="sha256:" + "1" * 64)
    config = Config(attestation=AttestationConfig(enforcement_mode=EnforcementMode.ENFORCING))

    # AGT's gateway and scanner are stubbed so this lands on cMCP's own parsing
    # rather than on whatever the vendored scanner does with the payload. See
    # the caveat printed at the end.
    with patch("cmcp_runtime.mcp.proxy.MCPGateway") as gw, \
         patch("cmcp_runtime.mcp.proxy.MCPResponseScanner"):
        scan = MagicMock()
        scan.threats = []
        scan.allowed = True
        scan.content = None  # not sanitized, so agt_result is the upstream text
        gw.return_value.intercept_tool_response.return_value = scan
        gw.return_value.intercept_tool_call.return_value = (True, None)

        return CMCPProxy(
            catalog=catalog,
            policy_evaluator=evaluator if evaluator is not None else MagicMock(),
            session=SessionState(session_id="repro"),
            audit_chain=audit_chain,
            config=config,
        )


async def _scenario(label: str, mode: str, evaluator=None) -> dict:
    _HostileUpstream.mode = mode
    upstream_invocations.clear()

    server = HTTPServer(("127.0.0.1", 0), _HostileUpstream)
    threading.Thread(target=server.serve_forever, daemon=True).start()
    chain = AuditChain("repro")
    proxy = _make_proxy(f"http://127.0.0.1:{server.server_port}/mcp", chain, evaluator)

    before, tip_before = len(chain.entries), chain.chain_tip
    frames: list[str] = []
    try:
        result = await proxy.call_tool("repro-call-1", "test.echo", {"x": 1})
        outcome = f"returned normally (allowed={result.allowed})"
    except BaseException as exc:  # noqa: BLE001
        outcome = f"{type(exc).__name__} propagated out of call_tool"
        for f in traceback.extract_tb(exc.__traceback__):
            if "cmcp_runtime" in f.filename:
                rel = f.filename.split("cmcp_runtime/")[-1]
                frames.append(f"cmcp_runtime/{rel}:{f.lineno}  {f.name}()")
    server.shutdown()

    entry_types = [e.entry_type for e in chain.entries]
    print(f"--- {label}")
    if frames:
        print("    crash path:")
        for fr in frames:
            print(f"        {fr}")
    print(f"    upstream invoked    : {upstream_invocations}")
    print(f"    outcome             : {outcome}")
    print(f"    audit entries       : {before} -> {len(chain.entries)}  {entry_types}")
    print(f"    chain tip changed   : {tip_before != chain.chain_tip}")
    print()
    return {
        "executed": bool(upstream_invocations),
        "new_entries": len(chain.entries) - before,
        "fault": "fault" in entry_types,
    }


async def main() -> int:
    print(f"nesting depth {NEST_DEPTH:,}, payload {6 * NEST_DEPTH + 1:,} bytes "
          f"({100 * (6 * NEST_DEPTH + 1) / (2 * 1024 * 1024):.1f}% of the 2MB response cap)\n")

    a = await _scenario(
        "A  nested text inside a valid envelope", "inner")
    b = await _scenario(
        "B  nested envelope, before the size guard runs", "envelope")

    # POLICY-003: an unexpected exception raised BEFORE the call does write an
    # entry. Same exception class, opposite side of the upstream request.
    evaluator = MagicMock()
    evaluator.evaluate.side_effect = RuntimeError("malformed Cedar policy")
    evaluator.authorize_egress.return_value = MagicMock(would_have_denied=False)
    evaluator.bundle_hash = "sha256:" + "0" * 64
    evaluator.enforcement_mode = EnforcementMode.ENFORCING
    c = await _scenario(
        "C  control: pre-execution exception (POLICY-003)", "inner", evaluator)

    gap_a = a["executed"] and a["new_entries"] == 0
    gap_b = b["executed"] and b["new_entries"] == 0
    control_ok = (not c["executed"]) and c["fault"]

    print("=" * 62)
    print(f"A  executed and unaudited            : {gap_a}")
    print(f"B  executed and unaudited            : {gap_b}")
    print(f"C  not executed and fault recorded   : {control_ok}")
    print()

    if gap_a and gap_b and control_ok:
        print("RESULT: CONFIRMED.")
        print("  An unexpected exception before the upstream call is recorded.")
        print("  The same class of exception after it is not, and the tool has")
        print("  already run by then.")
        rc = 0
    else:
        print("RESULT: NOT confirmed, see the per-scenario output above.")
        rc = 1

    print()
    print("Caveat: AGT's MCPGateway and MCPResponseScanner are stubbed here so")
    print("this exercises cMCP's own response path rather than the vendored")
    print("scanner. Scenario B does not depend on that stubbing at all: it")
    print("crashes inside _forward_to_upstream, before any scanner is reached.")
    return rc


if __name__ == "__main__":
    raise SystemExit(asyncio.run(main()))

Activity

  1. mohammedmessaoudene-cmd commented on Aug 25, 2026

    @mohammedmessaoudene-cmd
    Contributor

    One semantic boundary seems worth preserving in the finalization fix: a pre-upstream fault and a post-upstream fault are not the same audit outcome.

    Once the request may have crossed the upstream boundary, the terminal record should distinguish at least:

    • not_attempted — the upstream invocation was not made;
    • outcome_unknown — the invocation was made, but no authoritative evidence establishes whether the external effect occurred;
    • confirmed_occurred / confirmed_absent — only when provider/system-of-record evidence supports that conclusion.

    The proposed single finalization boundary closes the missing-record invariant. But a generic fault carrying only stage and exception type can still let retry logic collapse “possibly committed” into “never attempted,” which is unsafe for irreversible tools.

    A minimal two-sided conformance pair would use the same exception class on opposite sides of the boundary:

    1. exception before forwarding → terminal fault + not_attempted;
    2. exception after upstream invocation, without provider evidence → terminal fault + outcome_unknown.

    A separate pair could add authoritative reconciliation to reach confirmed_occurred or confirmed_absent. This could initially live in structured detail rather than prejudging a schema change.

    I have not rerun the #571 harness; this is a semantic review of the reported reproduction and the current AuditEntry shape. I can contribute a bounded fixture pack after the maintainer chooses the representation.

    AI-assistance disclosure: ChatGPT assisted with repository review and drafting; Mohammed Messaoudene reviewed the claim and remains responsible for it.

  2. Yatsuiii commented on Aug 25, 2026

    @Yatsuiii
    ContributorAuthor

    Agree on the distinction, and it's a real gap in what I wrote. Stage plus exception type leaves a consumer of the chain inferring commit uncertainty from a stage name string. not_attempted versus outcome_unknown should be explicit.

    Where the crash lands makes your absent case the sharp one here. _extract_external_execution_evidence populates the audit entry's evidence field, and proxy.py:139 inside it is one of the two crash sites in this issue. It's called at proxy.py:1209, twelve lines above the audit append.

    I tested that pairing rather than assuming it. A response carrying a well-formed receipt next to a deeply nested sibling loses both, because the parse dies before it reads either:

    CONTROL  receipt, no deep nesting     -> 1 audit entry,  receipt recorded: True
    HOSTILE  same receipt + deep sibling  -> 0 audit entries, receipt recorded: False
    

    So the call best equipped to prove its own outcome is the one that leaves no record of having happened.

    On confirmed_occurred and confirmed_absent I'd push back, because of what the audit entry actually holds:

    # #301: optional independent execution evidence, e.g. a controller-signed
    # receipt of a physical outcome. Distinct from response_payload_hash:
    # response_payload_hash is what the gateway forwarded, this is what an
    # independent authority attested. Serialized uniformly (null when absent),
    # so receipt-less entries remain deterministic and schema-stable.
    external_execution_evidence: dict[str, str] | None = None

    Six fields: issuer, issuer_key_id, signature, evidence_hash, evidence_type, linked_call_id. A signed envelope binding the call to a receipt by hash. The outcome isn't in it. terminal_state, verdict and completeness_claim live in the detached payload, and verify_embodied_action_evidence(audit_entry, detached_payload, ...) takes that as a separate argument, entered through cmcp_runtime/cli.py as its own verification command. Nothing on the request path calls it.

    So the write path holds a pointer, not an outcome. A confirmed state asks the writer to record something only a reader can determine.

    The read side doesn't get there either, which is the part I'd actually argue. verdict is only accepted or rejected, and embodied_action.py fails any receipt whose physical_completion_claim isn't none. The aborted fixture verifies cleanly and still carries completeness_claim: not_proven. So a rejected receipt isn't confirmed_absent, it's an issuer verdict with completion unproven in either direction.

    Which makes outcome_unknown the correct terminal state rather than a fallback. At fault time the gateway can't know, and structurally can't.

    On detail: typed dict[str, str | int | float] | None, so this fits without a schema change. Agreed that's where to start.

    Next I'll extend the reproduction to exercise both write-side states against the real AuditEntry shape and post what comes back.

  3. mohammedmessaoudene-cmd commented on Aug 25, 2026

    @mohammedmessaoudene-cmd
    Contributor

    Separate current-HEAD reproduction is now complete against 0d3cb8eb8f63e6dbd79963ac22313e3e30b42a18 (tree a249c18ee50db68daba7d5107e9c64b63db9e932). This is project-controlled execution, not independent validation.

    Results:

    • reproduced the receipt-preserving control and receipt-losing hostile case in both memory and SQLite;
    • same exception before the upstream boundary → exactly one terminal fault with detail.terminal_disposition = not_attempted;
    • same exception after upstream invocation/possible effect → exactly one terminal fault with detail.terminal_disposition = outcome_unknown;
    • detail was sufficient; no schema change was needed;
    • 15/15 targeted gates, 10/10 causal mutations killed and restored, and the full suite passed: 1435 passed, 16 skipped, 0 failed;
    • Ruff, scoped MyPy and Bandit passed.

    The reader-side checks support your correction: signed accepted and rejected detached receipts with completeness_claim: not_proven and physical_completion_claim: none both remain outcome-unknown. Nothing in the executed matrix justified confirmed_occurred or confirmed_absent.

    One residual guidance gap remains: two calls sharing the same call_id after an ambiguous outcome invoke the upstream twice and write two outcome_unknown faults. cMCP currently has no retry/idempotency surface that makes such a retry safe.

    I have a one-file local candidate plus regression tests, but have not pushed it. Before opening a PR I am doing one final hardening pass on async cancellation and on naming the boundary conservatively as “effect may have occurred,” since the proxy cannot always prove bytes crossed the transport. If the maintainers prefer a draft PR now, I can submit it with DCO sign-off and the full test matrix.

    AI-assistance disclosure: ChatGPT and Codex assisted with analysis, implementation and testing; Mohammed Messaoudene reviewed the evidence and remains responsible for the claims.

  4. Yatsuiii commented on Aug 25, 2026

    @Yatsuiii
    ContributorAuthor

    Good result, and it lines up with what the read-side analysis predicted: signed accepted and rejected receipts both staying outcome-unknown is the behaviour the completeness_claim: not_proven pairing implies. Useful to see it hold in SQLite too, since absence from a persistent store was the part of my original report I hadn't actually exercised.

    On the residual gap you named, I think it's larger than two calls sharing a call_id. call_id is minted server side at server.py:569 as a fresh uuid.uuid4(), and nothing in the MCP path lets a caller supply one. So a client retrying after an outcome_unknown fault doesn't collide on call_id, it gets a new one, and the chain ends up with two entries that nothing relates as the same intent. The gateway can't recognise a retry as a retry at all.

    Nothing in cmcp_runtime carries an idempotency, dedup or replay surface today, and call_id is recorded without ever being checked for reuse. So outcome_unknown is a state a caller can read but not yet act on safely, which for irreversible tools is where the actual damage sits.

    I'd like to take that piece if nobody else is on it, starting with an issue that writes up the retry semantics rather than a PR, since it needs a decision on whether callers get a supplied idempotency key.

    @imran-siddique two questions so this doesn't get built in the wrong order. Do you want the finalization boundary landed narrow first, and should the retry surface be a separate issue rather than folded into it?

  5. mohammedmessaoudene-cmd commented on Aug 25, 2026

    @mohammedmessaoudene-cmd
    Contributor

    Agreed, and a correction to my residual-gap wording: the same-call_id probe was only a synthetic lower-bound at the local API, not the real MCP retry path. On the public server path, fresh server-minted UUIDs make the gap larger: repeated intent is uncorrelated rather than deduplicated.

    There is already a maintainer-authored tracker, #565, whose acceptance evidence includes correlation across retries and multi-request work, with explicit collision, replay, and missing-context failures. That looks like the natural coordination point for the execution-identity part. It may still need separate semantics for irreversible outcome_unknown, but I will not open a competing issue or implement that surface.

    I will keep the #571 candidate narrow: terminal finalization, explicit not_attempted / outcome_unknown, exactly one terminal record, and no schema change. I’ll wait for @imran-siddique’s routing before opening any PR.

    AI-assistance disclosure: ChatGPT assisted with cross-reference review and drafting; Mohammed Messaoudene reviewed the correction and remains responsible for it.

  6. imran-siddique commented on Aug 25, 2026

    @imran-siddique
    Member

    Confirmed, and yes, please send it.

    I verified the defect against main at pinned 80d74397 rather than reading the report. _forward_to_upstream is invoked at proxy.py:975, _extract_external_execution_evidence runs at 1209, and the terminal self._audit.append("tool_call", ...) is at 1220. That is roughly 245 lines of fallible code between the point the side effect may have committed and the point anything is written down, with no exception boundary anywhere in it, while the pre-upstream path at 910-918 does append a fault before re-raising.

    @Yatsuiii's sentence is the right description and I want to repeat it back because it is the part that makes this a bug rather than a hardening request: the invariant is not missing from this codebase, it is implemented on one side of the upstream request only. POLICY-003 exists, passes, and covers the half that was never the dangerous half.

    For a gateway whose entire premise is that a governed action leaves evidence, a tool that executed with zero audit entries is the worst failure mode available to us. It is strictly worse than a tool that was wrongly denied, because a denial is visible and this is not.

    Scope I am accepting

    Exactly what you narrowed it to in the last two comments:

    • one terminal record per call_tool, on both sides of the upstream boundary;
    • detail.terminal_disposition of not_attempted when the invocation was not made, outcome_unknown when it may have committed;
    • no schema change;
    • the two-sided conformance pair, same exception class on opposite sides of _forward_to_upstream, so the pairing is what is tested rather than two independent assertions.

    @mohammedmessaoudene-cmd, the not_attempted versus outcome_unknown distinction is the part of this I would have insisted on if it had not been proposed. A generic fault carrying stage and exception type lets a retry layer read "possibly committed" as "never attempted", and on an irreversible tool that is how you get a double-charge with a clean-looking chain behind it. Naming commit uncertainty in the record instead of leaving a consumer to infer it from a stage string is correct.

    confirmed_occurred / confirmed_absent I would leave out of this PR. They require provider or system-of-record evidence we do not have a channel for, and adding the vocabulary before the evidence exists invites someone to populate it optimistically.

    The receipt-loss finding is worth calling out separately, because it is the sharpest consequence and it was measured rather than argued:

    CONTROL  receipt, no deep nesting     -> 1 audit entry,  receipt recorded: True
    HOSTILE  same receipt + deep sibling  -> 0 audit entries, receipt recorded: False
    

    A well-formed receipt sitting next to a hostile sibling loses both, because the parse dies before it reads either. That is an attacker-controlled way to make an execution unrecordable, not just an unlucky crash.

    Execution identity stays out

    Agreed, and thank you for routing it to #565 rather than opening a competing issue. The retry-correlation gap @Yatsuiii identified is real and larger than a call_id collision: server.py:569 mints a fresh uuid.uuid4() server side with no caller-supplied path, so a retry after an outcome_unknown fault produces two entries nothing relates as the same intent. That belongs in #565 with the irreversible-outcome_unknown semantics as a named requirement, not bolted onto this fix.

    Two process notes, both meant as credit rather than correction. Labelling your own reproduction "project-controlled execution, not independent validation" is the right standard and I wish more reports drew that line. And correcting your own residual-gap wording once you saw the real MCP path, rather than leaving the stronger claim standing, is why I am taking the rest of this at face value.

    Labelled bug. Open the PR whenever it is ready.

  7. Yatsuiii commented on Aug 26, 2026

    @Yatsuiii
    ContributorAuthor

    @imran-siddique you routed the execution-identity piece to #565, so: #574 is a draft proposal on that issue, opened about 40 minutes before your comment rather than in response to it. It is now out of draft.

    It already carries the requirement you named. Replay of an execution_id after a terminal outcome is flagged as the case worth refusing, with reuse after outcome_unknown named specifically, and the retry gap is tied back to #571: server.py:569 mints a fresh call_id per call with no caller-supplied path, so two attempts at one intent are uncorrelated.

    Five decisions are left open rather than settled, including whether execution_id is an AuditEntry field or lives in detail. Happy to implement whichever way they land, or to close it if the direction is wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions