fix(proxy): preserve terminal outcome across failures

[mohammedmessaoudene-cmd]

What

Guarantee one terminal audit record for each propagating exceptional failure or
cancellation crossing call_tool, while carrying an explicit effect-boundary
disposition: not_attempted before transport and outcome_unknown once an upstream
effect can no longer be excluded. Preserve the existing post-scan response hash and any
detached external-evidence pointer when later processing or persistence fails.

Why

The existing path could let an upstream tool execute and then lose every audit record
when fallible response processing raised. A retry consumer must not confuse “possibly
committed” with “never attempted.”

Closes #571.

Security impact

This hardens audit-chain completeness around upstream side effects. The state transition
is made immediately before client.post / server.call; proven pre-send failures remain
not_attempted. A returned HTTP response is labelled
transport_response_received—not exact_response_available—because status/parsing can
still fail before the existing processed response_payload_hash is established.
KeyboardInterrupt and SystemExit are not masked. A synchronous
asyncio.CancelledError raised by fault-terminal persistence is treated as a persistence
failure so the original causal exception remains primary. No schema, raw-transport hash,
retry, or #565 correlation change is included.

The write path does not claim confirmed_occurred or confirmed_absent; it stores a
pointer to external execution evidence, not an independently verified physical outcome.
A store that commits and then raises before acknowledgement can still produce two SQLite
terminals at one sequence while memory observes one; this bounded lost-ack ambiguity is
documented and is not presented as global exactly-once.

Test plan

• pytest passes — 1,481 passed, 16 skipped, 0 failed
• Focused issue Post-execution exceptions leave a completed tool call with no audit outcome #571 matrix — 32/32 passed across memory/SQLite,
  cancellation at mutation-lock entry, session-update failure, unexpected egress failure,
  pre-send and system exceptions, concurrency, post-scan response hash, detached pointer,
  commit-then-raise, HTTP status/parse failure, persistence cancellation and post-terminal
  exception cases
• Seven new failing regression instances reproduce the P1/P2 windows on the prior
  public head; the normal-success control passes there; all eight pass after the fix
• A separate project-controlled local red-team matrix passes 12/12; eight causal
  ordering/binding mutants are killed and one benign UTF-8 equivalent survives
• ruff check src/ tests/ passes; the two modified files pass ruff format --check
• mypy src passes (64 source files)
• bandit -r src/ -c pyproject.toml passes
• Real Draft-07 validation remains outside this follow-up; no schema change is
  included

Validation status and limitations

These are project-controlled local executions, not independent validation. The existing
repository-wide formatting/debt outside the two modified files is not changed. Normal
health-check and drift-denial return paths predate this PR and remain outside its
propagating exception/cancellation finalizer; the drift detector may record a drift
observation, but this PR does not claim a per-call terminal for those normal denials. The
existing caught egress PolicyDeny terminal is also unchanged; this follow-up does not
alter its response-hash/evidence fields. Retry correlation remains owned by #565. The
commit-then-raise lost-ack case bounds, rather than proves, exactly-once behavior.

AI assistance

OpenAI Codex/ChatGPT assisted with analysis, implementation, adversarial tests, and
drafting. The human submitter reviewed the scope, evidence, and claims and accepts
responsibility for the contribution.

DCO sign-off

• I certify that I wrote or have the right to submit this contribution, and I agree
  to the Developer Certificate of Origin.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1cb9b20fe0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Compute the response hash before updating session state

When a call is cancelled while waiting for mutation_lock, or update_from_inspection() raises after the scanner has produced agt_result, execution never reaches this assignment. The outer finalizer therefore records an outcome_unknown fault with response_payload_hash=None even though the exact post-scan response is already available, defeating the new audit-preservation guarantee. Compute and store the hash immediately after selecting agt_result, before the fallible session update.

Useful? React with 👍 / 👎.

[mohammedmessaoudene-cmd]

Addressed in 1100c4924afc7fb7529cab56ecb885cea9b1222a. The exact post-scan bytes and SHA-256 are now bound immediately after agt_result, before mutation-lock acquisition or update_from_inspection. Added deterministic cancellation-at-__aenter__ and update-failure cases across memory and SQLite. The hash remains exact over the post-scan/sanitized result. The issue #571 matrix is 32/32 and the full suite is 1,481 passed / 16 skipped. Leaving the thread unresolved for reviewer verification.

[chatgpt-codex-connector]

Capture external evidence before fallible egress processing

If an allowed post-scan response contains a valid external_execution_evidence receipt but the session update or authorize_egress() raises, this assignment is never reached. _finalize_unexpected_call_failure() consequently omits the available receipt and labels it unavailable_or_unbound, producing an inaccurate terminal record for a potentially executed upstream action. Extract and bind the receipt immediately after the scanner determines agt_result so later processing failures cannot discard it.

Useful? React with 👍 / 👎.

[mohammedmessaoudene-cmd]

Addressed in 1100c4924afc7fb7529cab56ecb885cea9b1222a. _extract_external_execution_evidence(agt_result) now runs exactly once immediately after the response hash and binds the receipt on _finalization before session or egress processing. New session/update and unexpected-authorize_egress cases preserve the exact receipt; absent/invalid evidence remains unavailable, and an extractor fault retains only the already-bound hash. The issue #571 matrix is 32/32 and the full suite is 1,481 passed / 16 skipped. Leaving the thread unresolved for reviewer verification.

[imran-siddique]

Approving to release the hold gate. Not merging yet: seven workflows are sitting in action_required on this branch, so there is currently no CI verdict on it at all. Releasing them now and I will report what they say.

Read the diff rather than the description. Three things stood out.

The disposition is derived, not assigned. _EffectBoundaryState is a four-state monotonic enum and terminal_disposition is a property returning not_attempted only while the state is PRE_TRANSPORT. Nothing in the file writes either string directly. That means there is no code path where a future edit can optimistically label a call not_attempted after transport, which is the failure this whole change exists to prevent. Making it underivable rather than merely correct is the right construction.

TRANSPORT_RESPONSE_RECEIVED as a distinct state from terminal is the honest call. Collapsing "an HTTP response came back" into "the outcome is known" is the obvious simplification and it would have been wrong: status handling and parsing can still fail before the processed response_payload_hash exists. Naming it transport_response_received rather than exact_response_available, and saying so explicitly, is the difference between a label and a claim.

KeyboardInterrupt and SystemExit genuinely pass through. except BaseException as exc: with an immediate re-raise for anything that is not Exception or asyncio.CancelledError. A wide catch with an explicit filter is the correct shape here, and it is worth having in the diff rather than relying on convention, because a bare except Exception would have silently changed shutdown behaviour under a fault.

Treating a synchronous asyncio.CancelledError from fault-terminal persistence as a persistence failure, so the original causal exception stays primary, is a detail I would not have thought to ask for and would have been annoyed to find missing.

On the limits you state. Not claiming confirmed_occurred or confirmed_absent, storing a pointer to external execution evidence rather than a verified physical outcome, and documenting the bounded lost-ack ambiguity where a store commits and then raises before acknowledgement instead of presenting global exactly-once. All three are the right boundaries and all three are the kind that quietly go missing. This is the second time in two days you have marked your own work's limits without being asked.

CI report to follow.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 89.69072% with 10 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/cmcp_runtime/mcp/proxy.py	89.69%	10 Missing ⚠️

📢 Thoughts on this report? Let us know!

[imran-siddique]

Correcting my own review. I approved this to release the seven held workflows before I had read the Codex threads on it, which was the wrong order. Both findings hold, I verified each against this head rather than taking them, and both land on the guarantee this PR exists to provide.

P1 at proxy.py:1307 is real

The sequence:

1288   agt_result is in hand
1296   _finalization.failure_stage = "session_update"
1297   async with self._session.mutation_lock:        <- cancellable here
1298       self._session.update_from_inspection(...)  <- can raise here
...
1307   response_bytes = agt_result.encode()
1308   _finalization.response_payload_hash = f"sha256:{...}"

A cancellation while waiting on mutation_lock, or anything raised inside update_from_inspection, never reaches 1308. The finalizer then writes an outcome_unknown terminal with response_payload_hash=None while the exact post-scan response was sitting in agt_result the whole time.

Your own summary says the change will "preserve the existing post-scan response hash and any detached external-evidence pointer when later processing or persistence fails". This is a window where later processing fails and it is not preserved.

P2 at proxy.py:1383 is the same shape

_extract_external_execution_evidence(agt_result) runs at 1382, after the same fallible session update and after authorize_egress(). If either raises, _finalization.external_execution_evidence is still None and the finalizer writes external_execution_evidence_state: "unavailable_or_unbound" at line 1151.

That is the worse of the two. An unavailable receipt and a discarded one produce the same terminal record, so a reader cannot distinguish "the upstream returned no evidence" from "the evidence existed and we dropped it". For a possibly-executed action that is an inaccurate record rather than an incomplete one, which is the distinction the rest of this PR is careful about.

What I would like

Both bindings moved to immediately after the scanner determines agt_result, before anything fallible runs. Same fix for both, and it is the one Codex names.

Please add a case for each. The suite is green at 1,473 passed and neither of these is covered, because nothing exercises a cancellation at that lock or a raise inside update_from_inspection. Both are reachable with the same technique your 24-case matrix already uses. A guarantee with no test for its own boundary is the shape you spent 622 lines closing elsewhere in this PR.

Not in question

Everything in my earlier review stands: the derived disposition, the monotonic state enum, transport_response_received as a state distinct from terminal, the BaseException filter re-raising KeyboardInterrupt and SystemExit, and the honest limits on lost-ack ambiguity. This is a strict improvement over main and I want it in. These two windows are the same defect class at earlier points in the pipeline, and closing them is what makes the guarantee hold end to end rather than from the transport boundary onward.

CI report, since I promised one: releasing the held runs took this from 1 check to 12, all green. benchmark skipped, gate is our maintainer hold. Green CI does not clear either finding above.

[imran-siddique]

Both findings are fixed. Verified the ordering at this head rather than taking the commit message, since the ordering is the whole substance of the change:

1278   agt_result = scan.content if scan.content is not None else response_text
1285   _finalization.response_payload_hash = f"sha256:{...}"
1287   _finalization.external_execution_evidence = _extract_external_execution_evidence(agt_result)
1306   async with self._session.mutation_lock:
1307       self._session.update_from_inspection(...)
1317       egress_decision = self._policy.authorize_egress(...)

Everything cancellable or fallible now runs after both bindings. That closes both windows.

You ordered the two bindings against each other, which neither the review nor I asked for. Your comment says it:

The hash is bound first so an evidence-parser failure still preserves the exact response bytes that were available.

_extract_external_execution_evidence parses attacker-influenced content, so it is the more likely of the two to raise, and binding the hash ahead of it means a malformed receipt costs the receipt rather than the receipt and the hash. That is the same failure-window reasoning one level down from the finding, and it is the kind of thing that only gets done by someone who understood why the original ordering was wrong rather than just where.

Adding deterministic cancellation-at-__aenter__ and update-failure cases across both memory and SQLite is the right coverage. The reason the original defect survived 1,473 passing tests is that nothing exercised a cancellation at that lock, so the cases matter more than the count.

A process correction of mine, on the record. My approval on this PR yesterday went out to release eight held workflows, before I had read the Codex threads sitting on it. That was the wrong order and it is why an approval briefly stood on a PR with two live findings. The findings were real and yours, not the bot's, to fix, but the sequencing error was mine.

Releasing the eight held runs now. CI report to follow before I merge.

[imran-siddique]

CI report, as promised. Releasing the held workflows took this from one check to twelve, all green: tests on 3.11, 3.12 and 3.13 across ubuntu and windows, CodeQL, governance and the Docker build. gate is our maintainer hold rather than a result.

Merging. This closes #571.

Worth recording what the round trip produced: the first version guaranteed the terminal record from the transport boundary onward, and the guarantee now holds from the moment the post-scan bytes exist. Two windows narrower than where it started, both found by review rather than by a failing test, which is the shape of defect this PR exists to make impossible.