Skip to content

feat(execution): non-operational execution-state foundation (#565) - #606

Merged
imran-siddique merged 9 commits into
agentrust-io:mainfrom
Yatsuiii:feat/565-execution-state
Sep 8, 2026
Merged

feat(execution): non-operational execution-state foundation (#565)#606
imran-siddique merged 9 commits into
agentrust-io:mainfrom
Yatsuiii:feat/565-execution-state

Conversation

@Yatsuiii

@Yatsuiii Yatsuiii commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

This PR retains standalone execution-registry primitives and a typed audit execution_id field. Execution correlation is unavailable in the running gateway.

Valid supplied execution IDs receive an audited refusal before health checks, catalog lookup, or upstream discovery. Malformed IDs, including explicit null, are rejected. Calls omitting the field retain the existing execution path.

Production registry wiring, admission/finalization calls, and provisional action-binding code are removed. Opaque binding storage/comparison remains only in the disconnected registry; it does not establish action equivalence.

Activation requires both #588’s adopted action-binding contract and requirement 7’s atomic terminal-state/audit persistence boundary, with integrated crash/recovery tests. This PR does not close #565.

Validation: focused tests, Ruff and MyPy passed. The full suite has one startup-test failure also reproduced on clean main.

Yatsuiii and others added 4 commits September 3, 2026 13:11
…st-io#565)

First implementation slice for session-independent execution correlation,
following the design merged in agentrust-io#574 (docs/spec/execution-correlation.md).

ExecutionRegistry is the authoritative execution-lifecycle record: a
SQLite-backed deep module, one instance per process, keyed by
(agent_identity, execution_id) with a PRIMARY KEY on the pair.

- admit() runs under BEGIN IMMEDIATE and either reserves a fresh in_flight
  row or classifies an existing key as a replay (in_flight / terminal /
  outcome_unknown) or a collision (changed action binding). It never
  rewrites a row and never returns "proceed" for one.
- finalize() moves in_flight to completed (transport delivered a response)
  or outcome_unknown (anything earlier). Both terminal, neither replayable.
  A second call is a no-op.
- recover() runs once at startup and seals every still-in_flight row as
  outcome_unknown, so a crash between admit and finalize can never admit
  another invocation.

The action binding reaches admit() as an opaque digest string; the registry
only stores and byte-compares it. Its canonical construction is issue agentrust-io#588's.
execution/binding.py carries provisional_action_binding, an explicit stub
with an undecided preimage that reuses the existing RFC 8785/JCS
canonicalizer (cmcp_runtime.catalog.approval) so no second serializer is
introduced. valid_execution_id bounds the identifier to 1-200 printable
non-space ASCII characters before it can reach the durable key.

Signed-off-by: Yatsuiii <battyrises@gmail.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…#565)

execution_id is a typed AuditEntry field, always serialized (null when the
caller asserted none), never a detail key. A present value came through the
validated admission path in ExecutionRegistry; null means no assertion was
made and none was synthesized. AuditChain.append gains the matching keyword.

Per the agentrust-io#574 design: the TRACE Claim does not enumerate execution_id values;
the audit entry carries the join key so bundles stay joinable offline.

Signed-off-by: Yatsuiii <battyrises@gmail.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…gentrust-io#565)

Wire ExecutionRegistry into the tool call path.

- server.py reads _cmcp.execution_id beside workflow_id; the two stay
  independent. A non-string is treated as absent.
- proxy._admit_execution reserves the execution identity at step 3a.5,
  immediately before upstream invocation. execution_id is bound-checked
  first (valid_execution_id); a malformed value is denied
  (execution_invalid_execution_id) with no reservation and is not written
  to the audit entry. The action binding comes from an injected
  action_binding_fn, so this module never encodes the agentrust-io#588 preimage.
- A replay or collision is refused here and never reaches upstream. The
  refusal is audited under the asserted execution_id.
- finalize() is called from the single terminal audit write
  (_append_call_terminal), so replay and collision policy is not spread
  across handlers. completed only when the transport delivered a response;
  anything earlier is outcome_unknown.
- cli.build_server constructs one process-wide registry, runs recover()
  before the gateway serves traffic, and injects provisional_action_binding.

A terminal transition is durable before a later request is classified as
replay, because finalize() commits its own transaction before any later
admit() runs. The terminal audit entry and the execution row are in
separate SQLite databases and do not share one transaction; a crash in the
gap leaves the row in_flight for recover() to seal. See
docs/spec/execution-correlation.md "Known limitations".

Signed-off-by: Yatsuiii <battyrises@gmail.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ntation slice

Add an "Implementation (first slice)" section to
docs/spec/execution-correlation.md describing what this slice builds:
ExecutionRegistry as the authoritative state owner, the admit / finalize /
recover contract, the opaque-digest binding seam, and execution_id ingress
validation.

The "Known limitations" subsection records that the terminal audit entry and
the execution row are in separate SQLite databases with no shared
transaction, that there is no cross-process reservation fencing beyond
SQLite BEGIN IMMEDIATE plus busy_timeout, and that this slice reports a
shared asserted execution_id without claiming exactly-once external
execution.

The action-binding construction (preimage, JCS member ordering, digest
representation) stays deferred to agentrust-io#588; this doc only describes what the
slice consumes.

Signed-off-by: Yatsuiii <battyrises@gmail.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@Yatsuiii
Yatsuiii requested review from a team as code owners September 3, 2026 07:49
@codecov-commenter

codecov-commenter commented Sep 3, 2026

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 99.24812% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/cmcp_runtime/execution/registry.py 99.03% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@Yatsuiii
Yatsuiii force-pushed the feat/565-execution-state branch from 89bf09b to 71104af Compare September 3, 2026 09:15
Yatsuiii and others added 2 commits September 3, 2026 14:49
…e rollback paths

Codecov flagged nine uncovered lines in ExecutionRegistry: the recover()
rollback path and an admit() sqlite3.IntegrityError handler for a lost insert
race.

The IntegrityError handler was unreachable. Verified against two independent
connections on one database file: the second BEGIN IMMEDIATE blocks on the
first connection's RESERVED lock, and once the first commits, the second's
SELECT runs inside its own transaction and observes the committed row, so it
classifies rather than reaching the INSERT. When the lock is held past
busy_timeout the caller gets sqlite3.OperationalError, not IntegrityError.
Within one process the registry's own threading.Lock serialises. No path
produces the exception the handler caught, so it is removed rather than tested.

The spec said "a lost insert race re-reads and classifies", which described a
path that cannot occur. It now says a competing writer either observes the
committed reservation or fails closed if the lock timeout is exceeded.

Tests added for the paths that are reachable:

- admission rollback under an injected persistence failure, asserting no row
  is left behind and that a later admit still succeeds;
- recovery rollback under an injected persistence failure, asserting the row
  stays in_flight for a later recover() rather than half-sealed;
- two independent ExecutionRegistry instances on one file admitting the same
  key simultaneously, asserting exactly one ADMITTED and one REPLAY_IN_FLIGHT.

Execution package coverage 92% to 100%.

Signed-off-by: Yatsuiii <battyrises@gmail.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Imran Siddique <imran.siddique@opaque.co>
@Yatsuiii

Yatsuiii commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for catching this. I missed that the HTTP handler treated a present non-string execution_id the same as an omitted one, which skipped execution-ID validation. The added HTTP tests cover the gap my proxy tests missed.

@imran-siddique imran-siddique left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review/test follow-up on 2579f01c6ecf.

The malformed execution-ID ingress repair remains covered, and the full local suite passed with the existing review environment. Fresh CI still fails: 20 failed, 1634 passed, 6 skipped on Python 3.13/Linux, principally Agent Manifest SDK verification returning MISMATCH, plus the TPM root-error assertion. Other matrix jobs were cancelled by fail-fast. Follow the SDK compatibility work in #616 and rerun with fresh dependencies after it lands. The provisional action binding (#588) and separate audit/registry transactions are explicitly incomplete requirements; keep that scope visible before requesting final approval. This is not ready to merge.

Full local python -m pytest -q (Windows, Python 3.12, existing review environment): 1625 passed, 16 skipped, 1 warning in 88.82s (0:01:28).

Fresh workflow evidence: 34012436620 (failure).

@Yatsuiii

Yatsuiii commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Merged main through #617 into this branch. That brings in #616 and CI is green on all six matrix rows. The Agent Manifest MISMATCH failures and the TPM root-error assertion are both gone.

The merge doesn't touch any of the 11 files here. Diff against main is still +1271/-3, and 2579f01 is unchanged.

Still incomplete, same as the PR description:

  • Action binding is a stub. admit() takes an opaque digest and never computes it. provisional_action_binding reuses the catalog canonicalizer without deciding the preimage, that's Define canonical bytes for execution action/intent bindings #588.
  • The execution row and the terminal audit entry are in separate SQLite databases, so requirement 7's single transaction boundary isn't met. The proxy writes the audit entry first, then finalizes. What's left is an audit entry showing a definite outcome while the registry says outcome_unknown.

Ready for review with that scope.

@imran-siddique imran-siddique left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 77b8057.

The current head has passing hosted CI across all six matrix rows, so the earlier dependency-related test failures are cleared. The two implementation gaps you identified still block merging this as the execution-registry implementation: admission accepts an opaque action digest without establishing its preimage, and terminal audit publication and registry finalization are separate transactions. A crash between them can leave a definite audit outcome alongside outcome_unknown. Please complete the agreed action-binding contract (#588) and requirement 7, or narrow this PR to an explicitly non-operational foundation with the incomplete execution path unavailable. Passing CI does not resolve those contract gaps.

@Yatsuiii Yatsuiii changed the title feat(execution): durable session-independent execution correlation (#565) feat(execution): non-operational execution-state foundation (#565) Sep 8, 2026
@Yatsuiii

Yatsuiii commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

Took the non-operational foundation option. The incomplete execution path is unavailable, and the provisional action-binding implementation and production registry wiring are removed. The standalone registry still stores and compares opaque bindings, but no gateway call uses it. Both #588 and requirement 7 remain prerequisites for activation.

@qubeena07 qubeena07 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read through the full diff along with the earlier review thread before writing this up. The move to a non operational foundation addresses both change requests from before. I checked the code directly and confirmed that nothing in the production path, not proxy.py, not server.py, not startup, calls admit, finalize, or recover, or constructs an ExecutionRegistry at all. Only the test suite does, and there's now a dedicated test guarding against that changing by accident.

The refusal logic in the proxy is ordered correctly, running before catalog lookup and before any upstream discovery. It also draws the right distinction: a malformed or null execution id gets refused and is never written into the audit record, while a valid but unsupported one gets refused and is recorded. Both paths have direct test coverage, including the HTTP boundary case.

I don't see anything blocking here. Approving with the expectation that the action binding contract and the atomic audit and registry boundary stay required before this gets wired into anything live.

@imran-siddique imran-siddique left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. You took the second option from my 7 September review and the narrowing is real, so this lands as a foundation rather than as the execution-registry implementation.

I checked the code rather than the section that describes it, because "non-operational" is a claim about reachability and the only way to see it is to look for the callers.

ExecutionRegistry now appears nowhere in src/cmcp_runtime/ outside its own package export. No construction, no admit(), no finalize(), no recover() in the gateway. binding.py is deleted and the CLI wiring with it, so there is no runtime opt-in to turn on by configuration either. That is what makes the two contract gaps I named harmless here: an admission path that accepts an opaque digest without establishing its preimage cannot be reached, and a crash between terminal audit publication and registry finalization cannot occur when nothing finalizes.

The ingress refusal is at the right place. _check_execution_available runs as Step 1 in the call path, before the health check, before catalog drift, and before any upstream discovery, with entry=None because no catalog entry has been consulted yet. Three behaviours, each as documented: a valid supplied execution_id is refused execution_correlation_unavailable with finalization.execution_id set first, so the audit retains the asserted identifier; a malformed one is refused execution_invalid_execution_id without that assignment, so a bad value never enters audit identity; omission returns None immediately and the legacy path is untouched. Step 0 serializes and hashes the request before any early refusal, so a refusal still carries a stable request hash with a null server.

CI is green on all twelve real checks across six matrix rows. The red gate in the rollup is a stale entry from before the approvals, with a later gate=SUCCESS beside it.

The remaining work is unchanged and now has a clean boundary: #588 defines the canonical action binding, and a single-writer store is needed before terminal audit and registry finalization can share a transaction. Neither is owed by this PR any more.

@imran-siddique
imran-siddique merged commit 6935503 into agentrust-io:main Sep 8, 2026
15 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Define session-independent execution correlation for post-session MCP

4 participants