From 18d0aff5d45a5df013f38fecc83d1a042f79e125 Mon Sep 17 00:00:00 2001 From: Albert Mavashev Date: Wed, 13 May 2026 08:48:02 -0400 Subject: [PATCH 1/7] feat: initial v0.1.0 scaffold for AP2 runtime authority guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add `cycles_guard_payment` sync context manager wrapping a Cycles reserve / commit / release lifecycle around a Google AP2 PaymentMandate. AP2 proves intent; Cycles enforces runtime exposure before that intent is exercised — preventing mandate reuse, double-spend, and concurrent checkout attempts under retries and fan-out. What this lands: - `runcycles_ap2` package: `cycles_guard_payment`, `AP2Mandate` adapter, `RuntimeAuthorityReceipt`, deterministic idempotency keys (`ap2:{transaction_id}:reserve|commit|release:{ExcType}`). - Built-in `payment.charge` action kind — no protocol PR required. - USD-only in v0.1; raises `AP2CurrencyError` on non-USD. - Mirrors sibling `cycles-client-python` conventions: hatchling, ruff (E,F,I,UP; line 120), mypy --strict, pytest, ≥95% coverage gate. - CI parity: ci.yml, python-publish.yml, dependabot-auto-merge.yml, scorecard.yml. - SEO-tuned pyproject.toml (31 keywords, 11 classifiers, 7 project URLs). - End-to-end example: examples/ap2_human_not_present.py. Test posture: - 53 tests, 97.89% coverage. - mypy --strict clean (7 files). - ruff check + ruff format --check clean (18 files). Receipt note: - `RuntimeAuthorityReceipt` is client-side derived from the ALLOW + COMMIT responses and is NOT signed by the Cycles server in protocol v0.1.26. Server-verifiable variant lands in v0.3 once cycles-protocol adds a signed-receipt field. --- .github/workflows/ci.yml | 16 + .github/workflows/dependabot-auto-merge.yml | 26 ++ .github/workflows/python-publish.yml | 151 +++++++++ .github/workflows/scorecard.yml | 59 ++++ AUDIT.md | 23 ++ CHANGELOG.md | 24 ++ CLAUDE.md | 25 ++ MAINTAINERS.md | 14 + README.md | 247 +++++++++++++++ examples/README.md | 16 + examples/ap2_human_not_present.py | 59 ++++ pyproject.toml | 116 +++++++ runcycles_ap2/__init__.py | 21 ++ runcycles_ap2/_constants.py | 35 ++ runcycles_ap2/exceptions.py | 28 ++ runcycles_ap2/guard.py | 333 ++++++++++++++++++++ runcycles_ap2/mapping.py | 174 ++++++++++ runcycles_ap2/models.py | 121 +++++++ runcycles_ap2/py.typed | 0 runcycles_ap2/receipt.py | 44 +++ tests/__init__.py | 0 tests/conftest.py | 93 ++++++ tests/test_double_spend_same_transaction.py | 48 +++ tests/test_dry_run.py | 64 ++++ tests/test_guard_clean_commit.py | 81 +++++ tests/test_guard_denial.py | 52 +++ tests/test_guard_release_on_exception.py | 58 ++++ tests/test_idempotency.py | 54 ++++ tests/test_mapping.py | 320 +++++++++++++++++++ tests/test_receipt.py | 82 +++++ 30 files changed, 2384 insertions(+) create mode 100644 .github/workflows/ci.yml create mode 100644 .github/workflows/dependabot-auto-merge.yml create mode 100644 .github/workflows/python-publish.yml create mode 100644 .github/workflows/scorecard.yml create mode 100644 AUDIT.md create mode 100644 CHANGELOG.md create mode 100644 CLAUDE.md create mode 100644 MAINTAINERS.md create mode 100644 README.md create mode 100644 examples/README.md create mode 100644 examples/ap2_human_not_present.py create mode 100644 pyproject.toml create mode 100644 runcycles_ap2/__init__.py create mode 100644 runcycles_ap2/_constants.py create mode 100644 runcycles_ap2/exceptions.py create mode 100644 runcycles_ap2/guard.py create mode 100644 runcycles_ap2/mapping.py create mode 100644 runcycles_ap2/models.py create mode 100644 runcycles_ap2/py.typed create mode 100644 runcycles_ap2/receipt.py create mode 100644 tests/__init__.py create mode 100644 tests/conftest.py create mode 100644 tests/test_double_spend_same_transaction.py create mode 100644 tests/test_dry_run.py create mode 100644 tests/test_guard_clean_commit.py create mode 100644 tests/test_guard_denial.py create mode 100644 tests/test_guard_release_on_exception.py create mode 100644 tests/test_idempotency.py create mode 100644 tests/test_mapping.py create mode 100644 tests/test_receipt.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..58949f2 --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,16 @@ +name: CI + +on: + push: + branches: [main] + pull_request: + branches: [main] + +permissions: + contents: read + +jobs: + ci: + uses: runcycles/.github/.github/workflows/ci-python.yml@v1 + with: + mypy-target: runcycles_ap2 diff --git a/.github/workflows/dependabot-auto-merge.yml b/.github/workflows/dependabot-auto-merge.yml new file mode 100644 index 0000000..6ad171c --- /dev/null +++ b/.github/workflows/dependabot-auto-merge.yml @@ -0,0 +1,26 @@ +name: Dependabot auto-merge + +on: pull_request + +# Default to read-all at top level; the automerge job below escalates only the +# narrow scopes it actually needs. Per OpenSSF Scorecard's Token-Permissions +# criterion: avoid blanket write at the workflow level. +permissions: read-all + +jobs: + automerge: + runs-on: ubuntu-latest + if: github.event.pull_request.user.login == 'dependabot[bot]' + permissions: + contents: write # required to enable auto-merge + pull-requests: write # required to mark the PR as auto-merge + steps: + - name: Fetch Dependabot metadata + id: meta + uses: dependabot/fetch-metadata@25dd0e34f4fe68f24cc83900b1fe3fe149efef98 # v3 + - name: Enable auto-merge for patch updates + if: steps.meta.outputs.update-type == 'version-update:semver-patch' + run: gh pr merge --auto --merge "$PR_URL" + env: + PR_URL: ${{ github.event.pull_request.html_url }} + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} diff --git a/.github/workflows/python-publish.yml b/.github/workflows/python-publish.yml new file mode 100644 index 0000000..8afe066 --- /dev/null +++ b/.github/workflows/python-publish.yml @@ -0,0 +1,151 @@ +name: Publish Python package + +on: + push: + tags: + - "v*" + workflow_dispatch: + inputs: + target: + description: "Where to publish" + required: true + default: "testpypi" + type: choice + options: + - testpypi + - pypi + +# Default least-privilege; publish-* jobs override with id-token: write. +permissions: + contents: read + +jobs: + build: + name: Build distributions + runs-on: ubuntu-latest + + steps: + - name: Check out source + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6 + - name: Set up Python + uses: actions/setup-python@a309ff8b426b58ec0e2a45f0f869d46889d02405 # v6 + with: + python-version: "3.12" + + # Fail-fast before build/publish if (a) pyproject.toml declares a pre-release + # version (e.g. 1.0.0.dev0, 1.0.0a1) when a final-version tag (vX.Y.Z) was + # pushed, or (b) the tag version doesn't match pyproject.toml. PyPI rejects + # duplicate versions server-side, but a local guard surfaces the operator + # error before the upload phase. + - name: Verify pyproject version matches tag + if: startsWith(github.ref, 'refs/tags/v') + run: | + python -c "import tomllib,sys; v=tomllib.load(open('pyproject.toml','rb'))['project']['version']; print(v)" > /tmp/pyproject_version + PYPROJECT_VERSION=$(cat /tmp/pyproject_version) + TAG_VERSION="${GITHUB_REF_NAME#v}" + echo "pyproject.toml version: ${PYPROJECT_VERSION}" + echo "Git tag version: ${TAG_VERSION}" + if [ "${PYPROJECT_VERSION}" != "${TAG_VERSION}" ]; then + echo "::error::Version mismatch: pyproject.toml has '${PYPROJECT_VERSION}' but tag is 'v${TAG_VERSION}'. Bump pyproject.toml or retag." + exit 1 + fi + + - name: Install build tool + run: python -m pip install --upgrade pip build + + - name: Build sdist and wheel + run: python -m build + + - name: Upload distribution artifacts + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 + with: + name: python-package-distributions + path: dist/ + + publish-to-testpypi: + name: Publish to TestPyPI + needs: build + runs-on: ubuntu-latest + if: github.event_name == 'workflow_dispatch' && inputs.target == 'testpypi' + environment: + name: testpypi + url: https://test.pypi.org/p/runcycles-ap2 + permissions: + id-token: write + contents: read + + steps: + - name: Download distribution artifacts + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8 + with: + name: python-package-distributions + path: dist/ + + - name: Publish to TestPyPI + uses: pypa/gh-action-pypi-publish@cef221092ed1bacb1cc03d23a2d87d1d172e277b # release/v1 + with: + repository-url: https://test.pypi.org/legacy/ + + publish-to-pypi: + name: Publish to PyPI + needs: build + runs-on: ubuntu-latest + if: startsWith(github.ref, 'refs/tags/v') || (github.event_name == 'workflow_dispatch' && inputs.target == 'pypi') + environment: + name: pypi + url: https://pypi.org/p/runcycles-ap2 + permissions: + id-token: write + contents: read + + steps: + - name: Download distribution artifacts + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8 + with: + name: python-package-distributions + path: dist/ + + - name: Publish to PyPI + uses: pypa/gh-action-pypi-publish@cef221092ed1bacb1cc03d23a2d87d1d172e277b # release/v1 + + create-release: + name: Create GitHub Release + needs: publish-to-pypi + runs-on: ubuntu-latest + if: startsWith(github.ref, 'refs/tags/v') + permissions: + contents: write + + steps: + - name: Check out source + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6 + + - name: Extract release notes from CHANGELOG + id: notes + run: | + version="${GITHUB_REF_NAME#v}" + # Extract the section between this version's heading and the next "## [" heading. + # Uses string functions instead of regex to stay portable across awk variants. + notes=$(awk -v v="$version" ' + BEGIN { start = "## [" v "]"; flag = 0 } + substr($0, 1, length(start)) == start { flag = 1; next } + substr($0, 1, 4) == "## [" { flag = 0 } + flag { print } + ' CHANGELOG.md) + if [ -z "$(printf '%s' "$notes" | tr -d '[:space:]')" ]; then + echo "::warning::No CHANGELOG entry found for v${version} — using fallback body" + notes="Release v${version}. See commit history for changes." + fi + { + echo "notes<> "$GITHUB_OUTPUT" + + - name: Create GitHub Release + uses: softprops/action-gh-release@b4309332981a82ec1c5618f44dd2e27cc8bfbfda # v3.0.0 + with: + name: ${{ github.ref_name }} + body: ${{ steps.notes.outputs.notes }} + draft: false + prerelease: ${{ contains(github.ref_name, '-') }} diff --git a/.github/workflows/scorecard.yml b/.github/workflows/scorecard.yml new file mode 100644 index 0000000..45779c6 --- /dev/null +++ b/.github/workflows/scorecard.yml @@ -0,0 +1,59 @@ +# OpenSSF Scorecard — supply-chain security analysis. +# +# Runs on push to main, on branch-protection-rule changes, and weekly so the +# score stays current as the repo evolves. Results are uploaded to GitHub's +# code-scanning surface (visible in Security tab) and published to the public +# OpenSSF metric API at api.scorecard.dev so the badge auto-updates. +# +# What it scores: branch protection, signed commits, dependency review, +# pinned dependencies, token permissions, vulnerability disclosure, fuzzing, +# SAST, and ~14 other supply-chain practices. Scores 0-10. +# +# Setup notes: +# - publish_results: true requires the repo to be public (it is). +# - id-token: write is required to mint the OIDC token used for publishing. +# - Workflow MUST live on the default branch for results to publish. +name: Scorecard supply-chain security + +on: + branch_protection_rule: + schedule: + - cron: '0 6 * * 1' # Monday 06:00 UTC — weekly refresh + push: + branches: [main] + +permissions: read-all + +jobs: + analysis: + name: Scorecard analysis + runs-on: ubuntu-latest + permissions: + security-events: write # Upload SARIF to code-scanning + id-token: write # Mint OIDC token for publish to api.scorecard.dev + contents: read + actions: read + steps: + - name: Checkout code + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + persist-credentials: false + + - name: Run analysis + uses: ossf/scorecard-action@4eaacf0543bb3f2c246792bd56e8cdeffafb205a # v2.4.3 + with: + results_file: results.sarif + results_format: sarif + publish_results: true + + - name: Upload artifact + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: SARIF file + path: results.sarif + retention-days: 5 + + - name: Upload to code-scanning + uses: github/codeql-action/upload-sarif@68bde559dea0fdcac2102bfdf6230c5f70eb485e # v4.35.4 + with: + sarif_file: results.sarif diff --git a/AUDIT.md b/AUDIT.md new file mode 100644 index 0000000..a6f92e0 --- /dev/null +++ b/AUDIT.md @@ -0,0 +1,23 @@ +# AUDIT + +Per `CLAUDE.md`: this file records material changes to the repo (server, admin, client). For a client package, that means public API, on-the-wire request shape, and protocol-conformance posture. + +## 2026-05-13 — v0.1.0 initial scaffold + +**Author:** initial commit +**Scope:** new repo, no protocol changes + +- Created `runcycles_ap2` package: `cycles_guard_payment` sync context manager wrapping a Cycles `reserve / commit / release` lifecycle around an AP2 Payment Mandate. +- AP2 → Cycles wire mapping: + - `Action.kind = "payment.charge"` (built-in high-risk kind from `cycles-action-kinds-v0.1.26.yaml:1562-1574`; **no custom kinds required, no protocol change**). + - `Amount.unit = USD_MICROCENTS`; USD only in v0.1, non-USD raises `ValueError` client-side. + - `Subject.dimensions` keys: `run_id`, `ap2_transaction_id`, `checkout_hash`, `open_mandate_hash` (all under 256 chars, ≤ 16 dims). + - `action.policy_keys.host = payee_website`; `policy_keys.custom = {payment_protocol: "ap2", currency: "USD"}`. Attached at the reservation request body level (raw dict path through `CyclesClient.create_reservation`) since the v0.4.1 `runcycles.models.Action` does not yet surface `policy_keys` as a typed field. +- Idempotency policy: client computes deterministic keys per phase from `transaction_id` — `ap2:{txid}:reserve`, `ap2:{txid}:commit`, `ap2:{txid}:release:{ExcType}`. This is the consume-once defense against mandate reuse across retries / concurrent attempts. +- Runtime-authority receipt: client-side derivation only (schema `runtime_authority.ap2.payment.charge.v1`). **Not server-verifiable in protocol v0.1.26.** Promoted to a signed protocol field in v0.3. +- Test coverage: ≥ 95% enforced via `pyproject.toml: fail_under = 95`. CI workflow mirrors `cycles-client-python`. + +**Risks acknowledged in this version:** +- PSP failure *after* clean `__exit__` will commit budget against a failed charge. Mitigation: README mandates raising inside the `with` block on PSP error. +- TTL timeout is server-side expiry, not a client release. +- AP2 SDK is not yet on PyPI; `AP2Mandate` is our adapter layer so upstream field renames touch only `models.py` + `mapping.py`. diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..8dc05bc --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,24 @@ +# Changelog + +All notable changes to this project will be documented in this file. + +The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), +and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). + +## [0.1.0] — 2026-05-13 + +### Added +- `cycles_guard_payment(...)` sync context manager: `reserve` on enter, `commit` on clean exit, `release` on exception. +- `AP2Mandate` adapter type insulating the wrapper from upstream AP2 schema churn. +- `RuntimeAuthorityReceipt` (`runtime_authority.ap2.payment.charge.v1`) — client-side, informational. +- Deterministic idempotency keys (`ap2:{transaction_id}:reserve|commit|release:{...}`). +- USD-only enforcement; non-USD raises `ValueError`. +- 8 tests, ≥ 95% coverage, ruff + mypy strict. + +### Planned for v0.2 +- `AsyncGuardedPayment` (asyncio). +- Multi-currency. +- `payment.refund` helper. + +### Planned for v0.3 +- Server-verifiable runtime-authority receipt (requires `cycles-protocol` signed-receipt field). diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000..e0a7f36 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,25 @@ +## Git Rules — STRICT +- ALWAYS use native git for ALL commits and pushes +- NEVER use mcp__github__ tools for committing or pushing +- Use mcp__github__ ONLY for: PRs, Issues, GitHub Actions +- Write commit messages to a temp file, then: `git commit -F ` +- NEVER use --no-gpg-sign flag + +# Cycles strict rules +- yaml API specs always the authority (cycles-protocol, AP2) +- always update AUDIT.md when making changes that affect public API, wire-level payload shape, or AP2/Cycles conformance posture +- maintain at least 95% or higher test coverage for all code repos + +# AP2 strict rules +- this package is a Cycles-side guard; it MUST NOT verify or sign AP2 mandates — that is the upstream SDK's job +- `AP2Mandate` is the adapter layer; upstream field renames touch only `models.py` and `mapping.py` +- never default to a custom action kind that requires server-side registration; default is built-in `payment.charge` +- USD only in v0.1 — non-USD raises `AP2CurrencyError` client-side +- idempotency keys are derived deterministically from `transaction_id`; callers MUST NOT pass their own + +# Build & Test +- Install: `pip install -e ".[dev]"` +- Test: `pytest` +- Test with coverage: `pytest --cov=runcycles_ap2` +- Lint & format: `ruff check` / `ruff format` +- Type check: `mypy runcycles_ap2` diff --git a/MAINTAINERS.md b/MAINTAINERS.md new file mode 100644 index 0000000..d79fbaf --- /dev/null +++ b/MAINTAINERS.md @@ -0,0 +1,14 @@ +# Maintainers + +This package is part of the [runcycles](https://github.com/runcycles) ecosystem. + +- Issues: https://github.com/runcycles/cycles-ap2-python/issues +- Sibling: https://github.com/runcycles/cycles-client-python +- Upstream protocol being wrapped: https://github.com/google-agentic-commerce/AP2 + +## Release process + +1. Bump `version` in `pyproject.toml`. +2. Update `CHANGELOG.md` and `AUDIT.md`. +3. Tag: `git tag v0.1.0 && git push --tags`. +4. CI builds and publishes to PyPI. diff --git a/README.md b/README.md new file mode 100644 index 0000000..4931115 --- /dev/null +++ b/README.md @@ -0,0 +1,247 @@ +[![PyPI](https://img.shields.io/pypi/v/runcycles-ap2)](https://pypi.org/project/runcycles-ap2/) +[![PyPI Downloads](https://img.shields.io/pypi/dm/runcycles-ap2)](https://pypi.org/project/runcycles-ap2/) +[![CI](https://github.com/runcycles/cycles-ap2-python/actions/workflows/ci.yml/badge.svg)](https://github.com/runcycles/cycles-ap2-python/actions) +[![License](https://img.shields.io/badge/license-Apache%202.0-blue)](LICENSE) +[![Coverage](https://img.shields.io/badge/coverage-98%25-brightgreen)](https://github.com/runcycles/cycles-ap2-python/actions) +[![OpenSSF Scorecard](https://api.scorecard.dev/projects/github.com/runcycles/cycles-ap2-python/badge)](https://scorecard.dev/viewer/?uri=github.com/runcycles/cycles-ap2-python) + +# Cycles AP2 Guard — Runtime authority for AP2 agent payments + +**Cycles runtime authority guard for [AP2](https://github.com/google-agentic-commerce/AP2) (Agent Payments Protocol) — wrap every AP2 payment moment in a `reserve / commit / release` lifecycle so a valid mandate cannot be over-exercised under retries, fan-out, or concurrent checkout attempts.** Works with Google's AP2 spec and any AP2-compatible SDK (Python samples today; A2A / MCP / UCP roadmap). + +AP2 answers *"was this payment authorized?"* with signed Open/Closed mandates, Payment Mandates, and Checkout Mandates. Cycles answers the complementary question: *"should this agent be allowed to attempt this payment **right now**?"* — pre-execution authority over reservation, idempotency, quota, and consume-once semantics. Install via `pip install runcycles-ap2`. + +## The problem AP2 itself flags + +From the AP2 spec, human-not-present flows let the agent act autonomously using an open mandate and sign a closed mandate on the user's behalf. AP2 warns: + +> "A shopping agent must avoid presenting subsequent open mandates without a rejection receipt to prevent multiple checkouts using the same open mandate." + +That is a **runtime-state** problem: concurrency, retries, in-flight attempts, quota counters, consume-once. AP2 mandates are cryptographic *authorization*. Cycles adds the missing runtime enforcement. + +## Installation + +```bash +pip install runcycles-ap2 +``` + +Needs a running Cycles server (see [`cycles-client-python`](https://github.com/runcycles/cycles-client-python) for setup) and a signed AP2 PaymentMandate. + +## Quickstart + +```python +from runcycles import CyclesClient, CyclesConfig +from runcycles_ap2 import AP2Mandate, cycles_guard_payment + +config = CyclesConfig.from_env() # CYCLES_BASE_URL, CYCLES_API_KEY, CYCLES_TENANT + +with CyclesClient(config) as client: + mandate = AP2Mandate( + transaction_id="ap2-tx-9f3c", + amount_value="199.00", + currency="USD", + payee_website="merchant.example", + checkout_hash="ch_b1a9...", + ) + with cycles_guard_payment( + client, + mandate=mandate, + run_id="run_abc123", + tenant="acme", + agent="checkout-bot", + ) as guard: + # Real PSP call goes here — protected by reserve / commit / release. + psp_receipt = psp.charge(mandate) + guard.attach_receipt_fields(psp_ref=psp_receipt.id) + + print(guard.receipt) # client-side runtime-authority receipt +``` + +## From an existing AP2 SDK object + +If you already hold a `PaymentMandate` (and optional `CheckoutMandate`) from the AP2 Python SDK, build an `AP2Mandate` adapter in one line. Field renames in AP2 only touch this adapter — your guard code stays stable. + +```python +from runcycles_ap2 import AP2Mandate + +mandate = AP2Mandate.from_ap2(payment_mandate, checkout_mandate) +``` + +Required upstream attributes: `payment_mandate.transaction_id`, `payment_mandate.payment_amount.value`, `payment_mandate.payment_amount.currency`, `payment_mandate.payee.website` (or `.identifier`). Optional: `checkout_mandate.hash`. + +## How the guard responds + +| Scenario | Outcome | Detail | +|---|---|---| +| `Decision.ALLOW`, body completes | **Commit** | Server idempotency key `ap2:{transaction_id}:commit` | +| `Decision.ALLOW`, body raises | **Release** | Reason `ap2_guard_failed:{ExcType}`, key `ap2:{transaction_id}:release:{ExcType}` | +| `Decision.DENY` | **Neither** | `AP2GuardDenied` raised in `__enter__`; real money never moves | +| HTTP / transport error on reserve | **Neither** | `AP2GuardDenied` raised; caller can retry — same `transaction_id` ⇒ same reserve key | +| Commit returns `RESERVATION_FINALIZED` / `RESERVATION_EXPIRED` / `IDEMPOTENCY_MISMATCH` | **Neither** | Logged at warning; we never auto-release after these (a previous commit may already have charged) | +| Commit returns other 4xx | **Release** | Reason `ap2_commit_rejected:{code}` | +| `guard.abort(reason)` called inside `with` | **Release** | Reason `ap2_guard_aborted:{reason}` | +| `dry_run=True` | **Neither** | Returns decision only; no reservation_id, no commit/release on exit | + +`AP2GuardDenied` carries `reason_code` and `request_id` for upstream logging. + +## AP2 → Cycles wire mapping + +| AP2 source | Cycles destination | Notes | +|---|---|---| +| `PaymentMandate.transaction_id` | `Subject.dimensions["ap2_transaction_id"]` | also feeds idempotency keys | +| `PaymentMandate.payment_amount.value` | `Amount.amount` | `int(round(value * 1e8))` USD micro-cents | +| `PaymentMandate.payment_amount.currency` | `Action.policy_keys.custom["currency"]` | MVP enforces `"USD"` | +| `PaymentMandate.payee.website` | `Action.policy_keys.host` | required for policy routing | +| `CheckoutMandate.hash` | `Subject.dimensions["checkout_hash"]` | optional | +| `sha256(open_mandate_canonical)` | `Subject.dimensions["open_mandate_hash"]` | optional, human-not-present | +| caller `run_id` | `Subject.dimensions["run_id"]` | required | +| const `"ap2"` | `Action.policy_keys.custom["payment_protocol"]` | marker | +| const `"payment.charge"` | `Action.kind` | built-in `high_risk` kind in `cycles-action-kinds-v0.1.26.yaml` | +| const `USD_MICROCENTS` | `Amount.unit` | single-unit per reservation | + +No protocol changes required for v0.1 — `payment.charge` and `payment.refund` already exist as `high_risk` action kinds in the Cycles protocol registry. + +## Deterministic idempotency keys + +The wrapper computes idempotency keys from `transaction_id`; callers MUST NOT pass their own. This is the consume-once defense — retried `__enter__`s on the same mandate, from any process, return the original reservation: + +| Phase | Key shape | +|---|---| +| Reserve | `ap2:{transaction_id}:reserve` | +| Commit | `ap2:{transaction_id}:commit` | +| Release | `ap2:{transaction_id}:release:{ExcType}` | + +Keys are truncated to 256 chars and sanitized to alphanumeric + `_-.` per protocol header rules. + +## Runtime authority receipt + +After a successful commit, the guard exposes a client-side receipt that can be persisted alongside AP2 dispute evidence: + +```json +{ + "schema": "runtime_authority.ap2.payment.charge.v1", + "decision": "ALLOW", + "reservation_id": "rsv_...", + "tenant": "acme", + "ap2_transaction_id": "ap2-tx-9f3c", + "checkout_hash": "ch_b1a9...", + "action_kind": "payment.charge", + "amount_unit": "USD_MICROCENTS", + "amount_micros": 19900000000, + "policy_keys": {"host": "merchant.example", "custom": {"payment_protocol": "ap2", "currency": "USD"}}, + "issued_at_ms": 1715600000000, + "committed": true, + "psp_ref": "psp_abc" +} +``` + +> **Important.** The receipt is built client-side from the Cycles ALLOW + COMMIT responses. It is **not** signed by the Cycles server in protocol v0.1.26 and must not be relied on as cryptographic evidence by third parties. A server-verifiable variant lands in v0.3 once `cycles-protocol` adds a signed-receipt field. + +Disable with `emit_receipt=False` if you don't need it. + +## Error handling + +```python +from runcycles_ap2 import AP2GuardDenied, AP2CurrencyError, AP2MandateError, cycles_guard_payment + +try: + with cycles_guard_payment(client, mandate=mandate, run_id="r", tenant="acme") as guard: + psp.charge(mandate) +except AP2GuardDenied as e: + # Cycles refused the attempt. Real money has NOT moved. + log.warning("denied", reason_code=e.reason_code, request_id=e.request_id) +except AP2CurrencyError: + # v0.1 supports USD only. + log.error("non-usd mandate") +except AP2MandateError: + # Adapter input is malformed (missing payee, non-decimal amount, etc.). + log.error("malformed mandate") +``` + +Exception hierarchy: + +| Exception | When | +|---|---| +| `AP2GuardError` | Base for all AP2-guard errors | +| `AP2GuardDenied` | Cycles returned `DENY` or the reserve POST failed | +| `AP2CurrencyError` | Non-USD mandate in v0.1 (subclass of `ValueError`) | +| `AP2MandateError` | Adapter input is malformed (subclass of `ValueError`) | + +## Features + +- **One context manager** — `cycles_guard_payment` wraps a single AP2 payment moment in reserve → commit / release. +- **Deterministic idempotency** — no caller-supplied keys; retries replay the same reservation. +- **Consume-once defense** — duplicate workers on the same mandate collapse onto one reservation server-side. +- **Built-in `payment.charge` action** — no custom action-kind registration, no protocol PR required. +- **Adapter layer** (`AP2Mandate`) insulates from upstream AP2 SDK churn. +- **Pydantic v2 models** with strict validation. +- **Client-side runtime-authority receipt** alongside AP2 dispute evidence (server-verifiable in v0.3). +- **Typed** (`py.typed`) and mypy-strict clean. +- **≥ 95% test coverage** enforced in CI. + +## Scope of v0.1 + +| In scope | Out of scope (v0.2+) | +|---|---| +| Sync context manager | Async API (`AsyncGuardedPayment`) | +| USD payments | Multi-currency | +| `payment.charge`, with override for `payment.refund` | `payment.refund` convenience helper | +| Caller-passed signed mandates | Mandate signing or signature verification | +| Built-in action kinds | Custom action kinds requiring server registration | +| Single-charge flows | Partial capture, multi-shipment, split-tender | + +## Example + +End-to-end runnable sample in [`examples/ap2_human_not_present.py`](examples/ap2_human_not_present.py). Set the env vars and run: + +```bash +CYCLES_BASE_URL=http://localhost:7878 \ +CYCLES_API_KEY=test-key \ +CYCLES_TENANT=acme \ +python examples/ap2_human_not_present.py +``` + +Set `DRY_RUN=1` to evaluate the policy decision without creating a reservation. Run twice with the same `transaction_id` to see the idempotent replay (server returns the original reservation — the double-spend defense). + +## Related packages + +| Package | Purpose | +|---|---| +| [`runcycles`](https://github.com/runcycles/cycles-client-python) (PyPI: [`runcycles`](https://pypi.org/project/runcycles/)) | Underlying Cycles SDK — programmatic client, `@cycles` decorator, streaming context manager | +| [`cycles-protocol`](https://github.com/runcycles/cycles-protocol) | Authoritative YAML API specs | +| [`AP2`](https://github.com/google-agentic-commerce/AP2) | Google's Agent Payments Protocol (upstream) | + +## Development + +```bash +pip install -e ".[dev]" + +# Lint + format +ruff check . +ruff format --check . + +# Type check (strict mode) +mypy runcycles_ap2 + +# Run tests with coverage (95% threshold enforced in CI) +pytest --cov=runcycles_ap2 --cov-fail-under=95 +``` + +CI runs all three checks on Python 3.10 and 3.12 for every push and pull request. See [`AUDIT.md`](AUDIT.md) for the protocol-conformance posture, [`CHANGELOG.md`](CHANGELOG.md) for the release log. + +## Documentation + +- [AP2 Protocol Spec](https://ap2-protocol.org/) — Google's upstream specification +- [AP2 Payment Mandate](https://ap2-protocol.org/ap2/payment_mandate/) — mandate constraints and field reference +- [Cycles Documentation](https://runcycles.io) — Cycles platform docs +- [Cycles Action Kinds Registry](https://github.com/runcycles/cycles-protocol) — authoritative list of built-in action kinds (`payment.charge`, `payment.refund`, etc.) + +## Requirements + +- Python 3.10+ +- `runcycles >= 0.4.1` +- `pydantic >= 2.0` + +## License + +Apache-2.0 — see [LICENSE](LICENSE). diff --git a/examples/README.md b/examples/README.md new file mode 100644 index 0000000..ebb1f8b --- /dev/null +++ b/examples/README.md @@ -0,0 +1,16 @@ +# Examples + +## ap2_human_not_present.py + +End-to-end demo of guarding an AP2 human-not-present payment with a Cycles `reserve / commit / release` lifecycle. + +```bash +pip install -e ".[dev]" +# point at a running Cycles server with `payment.charge` permitted under your tenant: +CYCLES_BASE_URL=http://localhost:7878 \ +CYCLES_API_KEY=test-key \ +CYCLES_TENANT=acme \ +python examples/ap2_human_not_present.py +``` + +Set `DRY_RUN=1` to evaluate the policy decision without creating a reservation. Re-run with the same `transaction_id` to demonstrate that the server returns the original reservation (idempotent replay — the double-spend defense). diff --git a/examples/ap2_human_not_present.py b/examples/ap2_human_not_present.py new file mode 100644 index 0000000..e4f7207 --- /dev/null +++ b/examples/ap2_human_not_present.py @@ -0,0 +1,59 @@ +"""End-to-end example: guard an AP2 human-not-present payment with Cycles. + +Runs against a local Cycles server (see cycles-client-python README). The PSP is a +fake — replace with a real charge call in production. Run: + + CYCLES_BASE_URL=http://localhost:7878 CYCLES_API_KEY=test CYCLES_TENANT=acme \\ + python examples/ap2_human_not_present.py +""" + +from __future__ import annotations + +import json +import os + +from runcycles import CyclesClient, CyclesConfig + +from runcycles_ap2 import AP2Mandate, cycles_guard_payment + + +def fake_psp_charge(mandate: AP2Mandate) -> dict[str, str]: + """Stand-in for a real payment-service-provider call.""" + return {"id": f"psp_{mandate.transaction_id}", "status": "captured"} + + +def main() -> None: + config = CyclesConfig( + base_url=os.environ.get("CYCLES_BASE_URL", "http://localhost:7878"), + api_key=os.environ.get("CYCLES_API_KEY", "test-key"), + tenant=os.environ.get("CYCLES_TENANT", "acme"), + ) + + mandate = AP2Mandate( + transaction_id="ap2-tx-demo-001", + amount_value="199.00", + currency="USD", + payee_website="merchant.example", + checkout_hash="ch_demo_001", + ) + + with CyclesClient(config) as client: + with cycles_guard_payment( + client, + mandate=mandate, + run_id="run_demo_001", + tenant=config.tenant, + agent="checkout-bot", + workflow="ap2-human-not-present", + dry_run=os.environ.get("DRY_RUN") == "1", + ) as guard: + print(f"decision={guard.decision}, reservation_id={guard.reservation_id}") + psp = fake_psp_charge(mandate) + guard.attach_receipt_fields(psp_ref=psp["id"]) + + if guard.receipt is not None: + print(json.dumps(guard.receipt.model_dump(by_alias=True), indent=2)) + + +if __name__ == "__main__": + main() diff --git a/pyproject.toml b/pyproject.toml new file mode 100644 index 0000000..4d94abc --- /dev/null +++ b/pyproject.toml @@ -0,0 +1,116 @@ +[build-system] +requires = ["hatchling"] +build-backend = "hatchling.build" + +[project] +name = "runcycles-ap2" +version = "0.1.0" +description = "Runtime authority guard for AP2 (Agent Payments Protocol) — reserve, commit, release around agent payment mandates to prevent mandate reuse, double-spend, and concurrent checkout attempts. Works with Google's AP2 spec and any AP2-compatible SDK." +readme = "README.md" +license = "Apache-2.0" +requires-python = ">=3.10" +keywords = [ + # Category-search keywords (lead with these — what people actually type) + "ap2", + "agent-payments", + "agent-payments-protocol", + "agentic-commerce", + "agentic-payments", + "ai-payments", + "mandate", + "payment-mandate", + "checkout-mandate", + "payment-guard", + "payment-authorization", + "consume-once", + "double-spend", + "double-spend-prevention", + "idempotent-payments", + "runtime-authority", + "agent-governance", + "agent-budget", + "budget-control", + "spending-limit", + # Framework / ecosystem + "google-agentic-commerce", + "a2a", + "mcp", + "ucp", + "langchain", + "openai-agents", + "crewai", + "autogen", + # Brand + "cycles", + "runcycles", +] +classifiers = [ + "Development Status :: 4 - Beta", + "Intended Audience :: Developers", + "Intended Audience :: Financial and Insurance Industry", + "License :: OSI Approved :: Apache Software License", + "Programming Language :: Python :: 3", + "Programming Language :: Python :: 3.10", + "Programming Language :: Python :: 3.11", + "Programming Language :: Python :: 3.12", + "Topic :: Software Development :: Libraries", + "Topic :: Software Development :: Libraries :: Python Modules", + "Topic :: Office/Business :: Financial", + "Topic :: Scientific/Engineering :: Artificial Intelligence", + "Typing :: Typed", +] +dependencies = [ + "runcycles>=0.4.1", + "pydantic>=2.0", +] + +[project.urls] +Homepage = "https://runcycles.io" +Documentation = "https://runcycles.io" +Repository = "https://github.com/runcycles/cycles-ap2-python" +Changelog = "https://github.com/runcycles/cycles-ap2-python/blob/main/CHANGELOG.md" +"Bug Tracker" = "https://github.com/runcycles/cycles-ap2-python/issues" +"AP2 Protocol" = "https://github.com/google-agentic-commerce/AP2" +"Cycles Protocol" = "https://github.com/runcycles/cycles-protocol" + +[project.optional-dependencies] +dev = [ + "pytest>=7.0", + "pytest-asyncio>=0.21", + "pytest-cov>=4.0", + "pytest-httpx>=0.30", + "mypy>=1.0", + "ruff>=0.1", + "respx>=0.21", +] + +[tool.hatch.build.targets.wheel] +packages = ["runcycles_ap2"] + +[tool.ruff] +target-version = "py310" +line-length = 120 + +[tool.ruff.lint] +select = ["E", "F", "I", "UP"] + +[tool.mypy] +python_version = "3.10" +strict = true +# The PyPI `runcycles` wheel does not ship a py.typed marker, so mypy can't analyze it. +# Silence the resulting `import-untyped` errors without weakening strictness on our own code. +disable_error_code = ["import-untyped"] + +[[tool.mypy.overrides]] +module = ["runcycles", "runcycles.*"] +ignore_missing_imports = true +follow_imports = "skip" + +[tool.pytest.ini_options] +asyncio_mode = "auto" + +[tool.coverage.run] +source = ["runcycles_ap2"] + +[tool.coverage.report] +fail_under = 95 diff --git a/runcycles_ap2/__init__.py b/runcycles_ap2/__init__.py new file mode 100644 index 0000000..274aa4b --- /dev/null +++ b/runcycles_ap2/__init__.py @@ -0,0 +1,21 @@ +"""Cycles runtime authority guard for Google AP2 (Agent Payments Protocol).""" + +from __future__ import annotations + +from runcycles_ap2.exceptions import AP2CurrencyError, AP2GuardDenied, AP2GuardError, AP2MandateError +from runcycles_ap2.guard import GuardedPayment, cycles_guard_payment +from runcycles_ap2.models import AP2Mandate, RuntimeAuthorityReceipt + +__version__ = "0.1.0" + +__all__ = [ + "AP2CurrencyError", + "AP2GuardDenied", + "AP2GuardError", + "AP2Mandate", + "AP2MandateError", + "GuardedPayment", + "RuntimeAuthorityReceipt", + "cycles_guard_payment", + "__version__", +] diff --git a/runcycles_ap2/_constants.py b/runcycles_ap2/_constants.py new file mode 100644 index 0000000..f47fa49 --- /dev/null +++ b/runcycles_ap2/_constants.py @@ -0,0 +1,35 @@ +"""Internal constants for AP2 → Cycles mapping.""" + +from __future__ import annotations + +from typing import Final + +# Default Cycles action kind for an AP2 payment moment. +# `payment.charge` is a built-in high_risk kind in cycles-action-kinds-v0.1.26.yaml +# (no server-side registration required). +DEFAULT_ACTION_KIND: Final[str] = "payment.charge" +DEFAULT_ACTION_NAME: Final[str] = "ap2.payment_mandate.present" + +# Subject.dimensions keys (lower-case, [a-z0-9_.-], <=256 chars per value, max 16 keys). +DIM_RUN_ID: Final[str] = "run_id" +DIM_AP2_TRANSACTION_ID: Final[str] = "ap2_transaction_id" +DIM_CHECKOUT_HASH: Final[str] = "checkout_hash" +DIM_OPEN_MANDATE_HASH: Final[str] = "open_mandate_hash" + +# Action.policy_keys.custom keys. +CUSTOM_PAYMENT_PROTOCOL: Final[str] = "payment_protocol" +CUSTOM_CURRENCY: Final[str] = "currency" +CUSTOM_PAYMENT_PROTOCOL_VALUE: Final[str] = "ap2" + +# Lifecycle defaults — payments must NOT overspend, so DENY (not ALLOW_IF_AVAILABLE). +DEFAULT_TTL_MS: Final[int] = 60_000 +DEFAULT_OVERAGE_POLICY: Final[str] = "REJECT" + +# Receipt schema identifier (client-side; not server-verifiable in protocol v0.1.26). +RECEIPT_SCHEMA: Final[str] = "runtime_authority.ap2.payment.charge.v1" + +# Idempotency key prefix used for all phases. +IDEMPOTENCY_PREFIX: Final[str] = "ap2" + +# USD micro-cents conversion (1 USD == 1e8 micro-cents). +USD_MICROCENTS_PER_DOLLAR: Final[int] = 100_000_000 diff --git a/runcycles_ap2/exceptions.py b/runcycles_ap2/exceptions.py new file mode 100644 index 0000000..62cb008 --- /dev/null +++ b/runcycles_ap2/exceptions.py @@ -0,0 +1,28 @@ +"""Exceptions raised by the AP2 guard.""" + +from __future__ import annotations + + +class AP2GuardError(Exception): + """Base class for all AP2 guard errors.""" + + +class AP2GuardDenied(AP2GuardError): + """Cycles returned Decision.DENY for this AP2 payment attempt. + + Real money has NOT moved. The agent must not retry without resolving the underlying + cause (budget exhaustion, quota, mandate already consumed, etc.). + """ + + def __init__(self, message: str, *, reason_code: str | None = None, request_id: str | None = None) -> None: + super().__init__(message) + self.reason_code = reason_code + self.request_id = request_id + + +class AP2CurrencyError(AP2GuardError, ValueError): + """Currency is unsupported in this version (v0.1 is USD-only).""" + + +class AP2MandateError(AP2GuardError, ValueError): + """AP2 mandate failed client-side validation before being sent to Cycles.""" diff --git a/runcycles_ap2/guard.py b/runcycles_ap2/guard.py new file mode 100644 index 0000000..b26e80a --- /dev/null +++ b/runcycles_ap2/guard.py @@ -0,0 +1,333 @@ +"""Sync context manager wrapping a single AP2 payment moment in a Cycles reservation.""" + +from __future__ import annotations + +import logging +import time +from types import TracebackType +from typing import Any + +from runcycles.client import CyclesClient +from runcycles.models import Decision, ReservationCreateResponse + +from runcycles_ap2._constants import DEFAULT_ACTION_KIND, DEFAULT_OVERAGE_POLICY, DEFAULT_TTL_MS +from runcycles_ap2.exceptions import AP2GuardDenied +from runcycles_ap2.mapping import ( + build_action, + build_commit_body, + build_release_body, + build_reservation_body, +) +from runcycles_ap2.models import AP2Mandate, RuntimeAuthorityReceipt +from runcycles_ap2.receipt import build_runtime_authority_receipt + +logger = logging.getLogger(__name__) + + +class GuardedPayment: + """Sync context manager: reserve on ``__enter__``, commit/release on ``__exit__``. + + Decision rules: + - Clean exit (no exception) → ``commit_reservation`` with ``ap2:{tx}:commit``. + - Exception inside ``with`` block → ``release_reservation`` with reason + ``ap2_guard_failed:{ExcType}`` and key ``ap2:{tx}:release:{ExcType}``. + - Server ``Decision.DENY`` on enter → raises :class:`AP2GuardDenied`; real money + never moves and no commit/release is issued. + - Same ``transaction_id`` on a retry → server returns the original reservation + (idempotent replay), so the second attempt cannot double-spend. + """ + + def __init__( + self, + client: CyclesClient, + *, + mandate: AP2Mandate, + run_id: str, + tenant: str | None = None, + workspace: str | None = None, + app: str | None = None, + workflow: str | None = None, + agent: str | None = None, + toolset: str | None = None, + action_kind: str = DEFAULT_ACTION_KIND, + ttl_ms: int = DEFAULT_TTL_MS, + overage_policy: str = DEFAULT_OVERAGE_POLICY, + dry_run: bool = False, + emit_receipt: bool = True, + metadata: dict[str, Any] | None = None, + extra_dimensions: dict[str, str] | None = None, + ) -> None: + self._client = client + self._mandate = mandate + self._run_id = run_id + self._tenant = tenant + self._workspace = workspace + self._app = app + self._workflow = workflow + self._agent = agent + self._toolset = toolset + self._action_kind = action_kind + self._ttl_ms = ttl_ms + self._overage_policy = overage_policy + self._dry_run = dry_run + self._emit_receipt = emit_receipt + self._metadata = metadata + self._extra_dimensions = extra_dimensions + + self._reservation_id: str | None = None + self._decision: Decision | None = None + self._actual_micros: int | None = None + self._commit_metadata: dict[str, Any] = {} + self._receipt: RuntimeAuthorityReceipt | None = None + self._committed = False + self._aborted_reason: str | None = None + + # -- public properties ------------------------------------------------ + + @property + def reservation_id(self) -> str | None: + return self._reservation_id + + @property + def decision(self) -> Decision | None: + return self._decision + + @property + def receipt(self) -> RuntimeAuthorityReceipt | None: + return self._receipt + + @property + def committed(self) -> bool: + return self._committed + + def set_actual_micros(self, amount: int) -> None: + """Override the committed amount. Defaults to ``mandate.amount_micros()``.""" + if amount < 0: + raise ValueError("actual amount must be non-negative") + self._actual_micros = amount + + def attach_receipt_fields(self, **fields: Any) -> None: + """Attach caller-supplied fields (e.g. PSP reference id) to the commit metadata.""" + self._commit_metadata.update(fields) + + def abort(self, reason: str) -> None: + """Force a release on clean exit (instead of commit). Use for late-discovered failures.""" + self._aborted_reason = reason[:256] + + # -- context manager protocol ---------------------------------------- + + def __enter__(self) -> GuardedPayment: + body = build_reservation_body( + self._mandate, + run_id=self._run_id, + tenant=self._tenant, + workspace=self._workspace, + app=self._app, + workflow=self._workflow, + agent=self._agent, + toolset=self._toolset, + action_kind=self._action_kind, + ttl_ms=self._ttl_ms, + overage_policy=self._overage_policy, + dry_run=self._dry_run, + metadata=self._metadata, + extra_dimensions=self._extra_dimensions, + ) + response = self._client.create_reservation(body) + + if not response.is_success: + error = response.get_error_response() + reason_code = error.error if error else None + request_id = error.request_id if error else None + raise AP2GuardDenied( + f"AP2 reservation failed for transaction {self._mandate.transaction_id}", + reason_code=reason_code, + request_id=request_id, + ) + + result = ReservationCreateResponse.model_validate(response.body) + self._decision = result.decision + + if result.is_denied(): + raise AP2GuardDenied( + f"AP2 reservation denied for transaction {self._mandate.transaction_id}: " + f"{result.reason_code or 'no reason'}", + reason_code=result.reason_code, + ) + + if self._dry_run: + # Dry-run is a "would allow" probe; no reservation_id, no commit/release on exit. + logger.info( + "AP2 dry-run evaluated: decision=%s, tx=%s", + result.decision, + self._mandate.transaction_id, + ) + return self + + if result.reservation_id is None: + raise AP2GuardDenied( + "AP2 reservation allowed but server returned no reservation_id", + reason_code=result.reason_code, + ) + + self._reservation_id = result.reservation_id + logger.info( + "AP2 reservation created: id=%s, tx=%s, decision=%s", + self._reservation_id, + self._mandate.transaction_id, + result.decision, + ) + return self + + def __exit__( + self, + exc_type: type[BaseException] | None, + exc_val: BaseException | None, + exc_tb: TracebackType | None, + ) -> None: + if self._dry_run: + return # nothing to commit or release on dry-run + if self._reservation_id is None: + return # nothing to clean up (denial path already raised) + + if exc_type is not None: + self._handle_release(reason=f"ap2_guard_failed:{exc_type.__name__}", exc_name=exc_type.__name__) + return + + if self._aborted_reason is not None: + self._handle_release(reason=f"ap2_guard_aborted:{self._aborted_reason}", exc_name="Aborted") + return + + self._handle_commit() + + # -- internals ------------------------------------------------------- + + def _handle_commit(self) -> None: + assert self._reservation_id is not None + body = build_commit_body( + self._mandate, + actual_micros=self._actual_micros, + metadata=self._commit_metadata or None, + ) + try: + response = self._client.commit_reservation(self._reservation_id, body) + except Exception: + logger.exception("AP2 commit raised: tx=%s", self._mandate.transaction_id) + raise + + if response.is_success: + self._committed = True + logger.info( + "AP2 commit successful: id=%s, tx=%s", + self._reservation_id, + self._mandate.transaction_id, + ) + if self._emit_receipt: + self._build_receipt() + return + + error = response.get_error_response() + error_code = error.error_code.value if (error and error.error_code) else None + if error_code in ("RESERVATION_FINALIZED", "RESERVATION_EXPIRED", "IDEMPOTENCY_MISMATCH"): + logger.warning( + "AP2 commit returned %s (no release): id=%s, tx=%s", + error_code, + self._reservation_id, + self._mandate.transaction_id, + ) + return + + logger.warning( + "AP2 commit rejected (releasing): id=%s, tx=%s, code=%s", + self._reservation_id, + self._mandate.transaction_id, + error_code, + ) + self._handle_release( + reason=f"ap2_commit_rejected:{error_code or 'UNKNOWN'}", + exc_name="CommitRejected", + ) + + def _handle_release(self, *, reason: str, exc_name: str) -> None: + assert self._reservation_id is not None + body = build_release_body(self._mandate, reason=reason, exception_type=exc_name) + try: + response = self._client.release_reservation(self._reservation_id, body) + if response.is_success: + logger.info( + "AP2 released: id=%s, tx=%s, reason=%s", + self._reservation_id, + self._mandate.transaction_id, + reason, + ) + else: + logger.warning( + "AP2 release returned non-success: id=%s, status=%d", + self._reservation_id, + response.status, + ) + except Exception: + logger.exception("AP2 release raised: id=%s", self._reservation_id) + + def _build_receipt(self) -> None: + assert self._reservation_id is not None + assert self._decision is not None + policy_keys = build_action(self._mandate, action_kind=self._action_kind)["policy_keys"] + committed_micros = self._actual_micros if self._actual_micros is not None else self._mandate.amount_micros() + raw_psp_ref = self._commit_metadata.get("psp_ref") + psp_ref = raw_psp_ref if isinstance(raw_psp_ref, str) else None + extra = {k: v for k, v in self._commit_metadata.items() if k != "psp_ref"} or None + self._receipt = build_runtime_authority_receipt( + self._mandate, + decision=self._decision.value, + reservation_id=self._reservation_id, + tenant=self._tenant, + agent=self._agent, + action_kind=self._action_kind, + policy_keys=policy_keys, + amount_micros=committed_micros, + committed=True, + psp_ref=psp_ref, + extra=extra, + issued_at_ms=int(time.time() * 1000), + ) + + +def cycles_guard_payment( + client: CyclesClient, + *, + mandate: AP2Mandate, + run_id: str, + tenant: str | None = None, + workspace: str | None = None, + app: str | None = None, + workflow: str | None = None, + agent: str | None = None, + toolset: str | None = None, + action_kind: str = DEFAULT_ACTION_KIND, + ttl_ms: int = DEFAULT_TTL_MS, + overage_policy: str = DEFAULT_OVERAGE_POLICY, + dry_run: bool = False, + emit_receipt: bool = True, + metadata: dict[str, Any] | None = None, + extra_dimensions: dict[str, str] | None = None, +) -> GuardedPayment: + """Construct a :class:`GuardedPayment` context manager for a single AP2 payment moment.""" + return GuardedPayment( + client, + mandate=mandate, + run_id=run_id, + tenant=tenant, + workspace=workspace, + app=app, + workflow=workflow, + agent=agent, + toolset=toolset, + action_kind=action_kind, + ttl_ms=ttl_ms, + overage_policy=overage_policy, + dry_run=dry_run, + emit_receipt=emit_receipt, + metadata=metadata, + extra_dimensions=extra_dimensions, + ) diff --git a/runcycles_ap2/mapping.py b/runcycles_ap2/mapping.py new file mode 100644 index 0000000..82bf99b --- /dev/null +++ b/runcycles_ap2/mapping.py @@ -0,0 +1,174 @@ +"""Pure functions that map ``AP2Mandate`` into Cycles wire payloads. + +Kept side-effect-free and network-free so the unit tests in ``tests/test_mapping.py`` +are fully deterministic and run without a server. +""" + +from __future__ import annotations + +from typing import Any + +from runcycles_ap2._constants import ( + CUSTOM_CURRENCY, + CUSTOM_PAYMENT_PROTOCOL, + CUSTOM_PAYMENT_PROTOCOL_VALUE, + DEFAULT_ACTION_NAME, + DIM_AP2_TRANSACTION_ID, + DIM_CHECKOUT_HASH, + DIM_OPEN_MANDATE_HASH, + DIM_RUN_ID, + IDEMPOTENCY_PREFIX, +) +from runcycles_ap2.models import AP2Mandate + + +def idempotency_key(transaction_id: str, phase: str, suffix: str | None = None) -> str: + """Deterministic idempotency key: ``ap2:{tx}:{phase}[:{suffix}]``. + + The same ``transaction_id`` reaching the same phase from any retry, parallel worker, + or process restart will produce the same key — that's the server-side replay defense. + """ + base = f"{IDEMPOTENCY_PREFIX}:{transaction_id}:{phase}" + if suffix: + # Strip characters that would push us over the 256-char limit or break the key shape. + safe_suffix = "".join(c if c.isalnum() or c in ("_", "-", ".") else "_" for c in suffix) + base = f"{base}:{safe_suffix[:64]}" + return base[:256] + + +def build_subject( + mandate: AP2Mandate, + *, + run_id: str, + tenant: str | None, + workspace: str | None, + app: str | None, + workflow: str | None, + agent: str | None, + toolset: str | None, + extra_dimensions: dict[str, str] | None = None, +) -> dict[str, Any]: + """Construct the Subject portion of a reservation create body.""" + subject: dict[str, Any] = {} + if tenant: + subject["tenant"] = tenant + if workspace: + subject["workspace"] = workspace + if app: + subject["app"] = app + if workflow: + subject["workflow"] = workflow + if agent: + subject["agent"] = agent + if toolset: + subject["toolset"] = toolset + + dimensions: dict[str, str] = { + DIM_RUN_ID: run_id, + DIM_AP2_TRANSACTION_ID: mandate.transaction_id, + } + if mandate.checkout_hash: + dimensions[DIM_CHECKOUT_HASH] = mandate.checkout_hash + if mandate.open_mandate_hash: + dimensions[DIM_OPEN_MANDATE_HASH] = mandate.open_mandate_hash + if extra_dimensions: + for key, value in extra_dimensions.items(): + if key in dimensions: + continue # AP2-canonical keys win over caller-supplied + dimensions[key] = value + + subject["dimensions"] = dimensions + return subject + + +def build_action(mandate: AP2Mandate, *, action_kind: str) -> dict[str, Any]: + """Construct the Action portion (including ``policy_keys``) of a reservation create body.""" + return { + "kind": action_kind, + "name": DEFAULT_ACTION_NAME, + "policy_keys": { + "host": mandate.payee_website, + "custom": { + CUSTOM_PAYMENT_PROTOCOL: CUSTOM_PAYMENT_PROTOCOL_VALUE, + CUSTOM_CURRENCY: mandate.currency.upper(), + }, + }, + } + + +def build_estimate(mandate: AP2Mandate) -> dict[str, Any]: + """Construct the Amount portion (USD micro-cents).""" + return {"unit": "USD_MICROCENTS", "amount": mandate.amount_micros()} + + +def build_reservation_body( + mandate: AP2Mandate, + *, + run_id: str, + tenant: str | None, + workspace: str | None, + app: str | None, + workflow: str | None, + agent: str | None, + toolset: str | None, + action_kind: str, + ttl_ms: int, + overage_policy: str, + dry_run: bool, + metadata: dict[str, Any] | None = None, + extra_dimensions: dict[str, str] | None = None, +) -> dict[str, Any]: + """Assemble the full reservation create request body, including deterministic idempotency key.""" + body: dict[str, Any] = { + "idempotency_key": idempotency_key(mandate.transaction_id, "reserve"), + "subject": build_subject( + mandate, + run_id=run_id, + tenant=tenant, + workspace=workspace, + app=app, + workflow=workflow, + agent=agent, + toolset=toolset, + extra_dimensions=extra_dimensions, + ), + "action": build_action(mandate, action_kind=action_kind), + "estimate": build_estimate(mandate), + "ttl_ms": ttl_ms, + "overage_policy": overage_policy, + } + if dry_run: + body["dry_run"] = True + if metadata: + body["metadata"] = metadata + return body + + +def build_commit_body( + mandate: AP2Mandate, + *, + actual_micros: int | None = None, + metadata: dict[str, Any] | None = None, +) -> dict[str, Any]: + """Commit body with deterministic idempotency key derived from ``transaction_id``.""" + amount = actual_micros if actual_micros is not None else mandate.amount_micros() + body: dict[str, Any] = { + "idempotency_key": idempotency_key(mandate.transaction_id, "commit"), + "actual": {"unit": "USD_MICROCENTS", "amount": amount}, + } + if metadata: + body["metadata"] = metadata + return body + + +def build_release_body( + mandate: AP2Mandate, + *, + reason: str, + exception_type: str | None = None, +) -> dict[str, Any]: + """Release body with deterministic idempotency key per exception type.""" + return { + "idempotency_key": idempotency_key(mandate.transaction_id, "release", exception_type), + "reason": reason[:256], + } diff --git a/runcycles_ap2/models.py b/runcycles_ap2/models.py new file mode 100644 index 0000000..c7956a3 --- /dev/null +++ b/runcycles_ap2/models.py @@ -0,0 +1,121 @@ +"""AP2 adapter types and the client-side runtime-authority receipt. + +These models intentionally do NOT depend on Google's AP2 SDK so the wrapper survives +upstream schema renames. Callers construct ``AP2Mandate`` either directly from their +own fields or via :meth:`AP2Mandate.from_ap2` if they hold AP2 SDK objects. +""" + +from __future__ import annotations + +from decimal import Decimal +from typing import Annotated, Any + +from pydantic import BaseModel, ConfigDict, Field + +from runcycles_ap2._constants import USD_MICROCENTS_PER_DOLLAR +from runcycles_ap2.exceptions import AP2CurrencyError, AP2MandateError + +_CONFIG = ConfigDict(populate_by_name=True, extra="forbid", str_strip_whitespace=True) + + +class AP2Mandate(BaseModel): + """Adapter view over an AP2 PaymentMandate (+ optional CheckoutMandate / open mandate). + + Only the fields needed to construct a Cycles reservation are surfaced. Anything else + on the upstream mandate is irrelevant to runtime authority and is ignored on purpose. + """ + + model_config = _CONFIG + + transaction_id: Annotated[str, Field(min_length=1, max_length=256)] + amount_value: Annotated[str, Field(min_length=1, max_length=64)] + currency: Annotated[str, Field(min_length=3, max_length=3)] + payee_website: Annotated[str, Field(min_length=1, max_length=253)] + checkout_hash: Annotated[str, Field(max_length=256)] | None = None + open_mandate_hash: Annotated[str, Field(max_length=256)] | None = None + + def amount_micros(self) -> int: + """Convert ``amount_value`` (decimal string in major units) to USD micro-cents. + + v0.1 enforces USD only. + """ + if self.currency.upper() != "USD": + raise AP2CurrencyError(f"v0.1 supports USD only; got {self.currency!r}. Multi-currency lands in v0.2.") + try: + value = Decimal(self.amount_value) + except (ArithmeticError, ValueError) as exc: + raise AP2MandateError(f"amount_value {self.amount_value!r} is not a valid decimal") from exc + if value < 0: + raise AP2MandateError("amount_value must be non-negative") + micros = int((value * USD_MICROCENTS_PER_DOLLAR).to_integral_value()) + return micros + + @classmethod + def from_ap2( + cls, + payment_mandate: Any, + checkout_mandate: Any | None = None, + *, + open_mandate_hash: str | None = None, + ) -> AP2Mandate: + """Build an ``AP2Mandate`` from upstream AP2 SDK objects (duck-typed). + + Required attributes: + - ``payment_mandate.transaction_id`` + - ``payment_mandate.payment_amount.value`` and ``.currency`` + - ``payment_mandate.payee.website`` (or ``.identifier``) + Optional: + - ``checkout_mandate.hash`` (or ``.checkout_hash``) + """ + try: + transaction_id = str(payment_mandate.transaction_id) + amount = payment_mandate.payment_amount + amount_value = str(amount.value) + currency = str(amount.currency) + payee = payment_mandate.payee + payee_website = getattr(payee, "website", None) or getattr(payee, "identifier", None) + if not payee_website: + raise AP2MandateError("AP2 payee must expose `website` or `identifier`") + except AttributeError as exc: + raise AP2MandateError(f"upstream PaymentMandate is missing a required field: {exc}") from exc + + checkout_hash: str | None = None + if checkout_mandate is not None: + checkout_hash = getattr(checkout_mandate, "hash", None) or getattr(checkout_mandate, "checkout_hash", None) + + return cls( + transaction_id=transaction_id, + amount_value=amount_value, + currency=currency, + payee_website=str(payee_website), + checkout_hash=checkout_hash, + open_mandate_hash=open_mandate_hash, + ) + + +class RuntimeAuthorityReceipt(BaseModel): + """Client-side runtime-authority receipt produced after a successful guarded payment. + + **Important:** This receipt is constructed by the wrapper from the Cycles ALLOW + COMMIT + responses. It is NOT signed by the Cycles server in protocol v0.1.26 and therefore must + not be relied on as cryptographic evidence by third parties. A server-verifiable variant + is planned for v0.3. + """ + + model_config = ConfigDict(populate_by_name=True) + + schema_name: str = Field(alias="schema") + decision: str + reservation_id: str + tenant: str | None = None + agent: str | None = None + ap2_transaction_id: str + checkout_hash: str | None = None + action_kind: str + amount_unit: str + amount_micros: int + policy_keys: dict[str, Any] + issued_at_ms: int + committed: bool = False + psp_ref: str | None = None + extra: dict[str, Any] | None = None diff --git a/runcycles_ap2/py.typed b/runcycles_ap2/py.typed new file mode 100644 index 0000000..e69de29 diff --git a/runcycles_ap2/receipt.py b/runcycles_ap2/receipt.py new file mode 100644 index 0000000..e51d3f8 --- /dev/null +++ b/runcycles_ap2/receipt.py @@ -0,0 +1,44 @@ +"""Construct the client-side runtime-authority receipt.""" + +from __future__ import annotations + +import time +from typing import Any + +from runcycles_ap2._constants import RECEIPT_SCHEMA +from runcycles_ap2.models import AP2Mandate, RuntimeAuthorityReceipt + + +def build_runtime_authority_receipt( + mandate: AP2Mandate, + *, + decision: str, + reservation_id: str, + tenant: str | None, + agent: str | None, + action_kind: str, + policy_keys: dict[str, Any], + amount_micros: int, + committed: bool, + psp_ref: str | None = None, + extra: dict[str, Any] | None = None, + issued_at_ms: int | None = None, +) -> RuntimeAuthorityReceipt: + """Build a receipt object that callers can persist alongside AP2 dispute evidence.""" + return RuntimeAuthorityReceipt( + schema=RECEIPT_SCHEMA, + decision=decision, + reservation_id=reservation_id, + tenant=tenant, + agent=agent, + ap2_transaction_id=mandate.transaction_id, + checkout_hash=mandate.checkout_hash, + action_kind=action_kind, + amount_unit="USD_MICROCENTS", + amount_micros=amount_micros, + policy_keys=policy_keys, + issued_at_ms=issued_at_ms if issued_at_ms is not None else int(time.time() * 1000), + committed=committed, + psp_ref=psp_ref, + extra=extra, + ) diff --git a/tests/__init__.py b/tests/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/conftest.py b/tests/conftest.py new file mode 100644 index 0000000..c0af0ce --- /dev/null +++ b/tests/conftest.py @@ -0,0 +1,93 @@ +"""Shared test fixtures.""" + +from __future__ import annotations + +import time +from typing import Any +from unittest.mock import MagicMock + +import pytest +from runcycles.client import CyclesClient +from runcycles.response import CyclesResponse + +from runcycles_ap2.models import AP2Mandate + + +def make_mandate( + *, + transaction_id: str = "ap2-tx-001", + amount_value: str = "199.00", + currency: str = "USD", + payee_website: str = "merchant.example", + checkout_hash: str | None = "ch_demo", + open_mandate_hash: str | None = None, +) -> AP2Mandate: + return AP2Mandate( + transaction_id=transaction_id, + amount_value=amount_value, + currency=currency, + payee_website=payee_website, + checkout_hash=checkout_hash, + open_mandate_hash=open_mandate_hash, + ) + + +def allow_response(reservation_id: str = "rsv_ap2_001") -> CyclesResponse: + return CyclesResponse.success( + 200, + { + "decision": "ALLOW", + "reservation_id": reservation_id, + "expires_at_ms": int(time.time() * 1000) + 60_000, + "affected_scopes": ["tenant:acme"], + "scope_path": "tenant:acme", + "reserved": {"unit": "USD_MICROCENTS", "amount": 19_900_000_000}, + }, + ) + + +def deny_response(reason_code: str = "BUDGET_EXCEEDED") -> CyclesResponse: + return CyclesResponse.success( + 200, + {"decision": "DENY", "affected_scopes": ["tenant:acme"], "reason_code": reason_code}, + ) + + +def commit_success_response(amount: int = 19_900_000_000) -> CyclesResponse: + return CyclesResponse.success( + 200, + {"status": "COMMITTED", "charged": {"unit": "USD_MICROCENTS", "amount": amount}}, + ) + + +def release_success_response(amount: int = 19_900_000_000) -> CyclesResponse: + return CyclesResponse.success( + 200, + {"status": "RELEASED", "released": {"unit": "USD_MICROCENTS", "amount": amount}}, + ) + + +def commit_error_response(error_code: str, status: int = 409) -> CyclesResponse: + return CyclesResponse.http_error( + status, + error_message=error_code, + body={"error": error_code, "message": error_code, "request_id": "req_test"}, + ) + + +@pytest.fixture +def mock_client() -> MagicMock: + """A MagicMock matching the CyclesClient surface used by GuardedPayment.""" + mock = MagicMock(spec=CyclesClient) + return mock + + +@pytest.fixture +def mandate() -> AP2Mandate: + return make_mandate() + + +@pytest.fixture +def captured() -> dict[str, list[Any]]: + """Generic dict to capture call bodies across helpers in a test.""" + return {"reserve": [], "commit": [], "release": []} diff --git a/tests/test_double_spend_same_transaction.py b/tests/test_double_spend_same_transaction.py new file mode 100644 index 0000000..3052d47 --- /dev/null +++ b/tests/test_double_spend_same_transaction.py @@ -0,0 +1,48 @@ +"""Two parallel guards on the same AP2 transaction send identical idempotency keys. + +The point is not to simulate Cycles' server-side replay logic in-process — that lives +on the server. The point is to prove the wire payload is deterministic, so the server +*can* dedupe. If two workers compute the same reserve key from the same mandate, the +server's `(tenant, endpoint, idempotency_key)` dedup will collapse them onto one +reservation — that is the consume-once defense. +""" + +from __future__ import annotations + +from runcycles_ap2 import cycles_guard_payment +from tests.conftest import allow_response, commit_success_response, make_mandate + + +class TestDoubleSpend: + def test_two_guards_same_transaction_send_same_reserve_key(self, mock_client) -> None: + mock_client.create_reservation.return_value = allow_response("rsv_shared") + mock_client.commit_reservation.return_value = commit_success_response() + + m1 = make_mandate(transaction_id="ap2-tx-shared") + m2 = make_mandate(transaction_id="ap2-tx-shared") + + with cycles_guard_payment(mock_client, mandate=m1, run_id="run_a", tenant="acme"): + pass + with cycles_guard_payment(mock_client, mandate=m2, run_id="run_b", tenant="acme"): + pass + + body1 = mock_client.create_reservation.call_args_list[0][0][0] + body2 = mock_client.create_reservation.call_args_list[1][0][0] + assert body1["idempotency_key"] == body2["idempotency_key"] == "ap2:ap2-tx-shared:reserve" + # Commit keys also collide — the server collapses both onto one reservation. + commit1 = mock_client.commit_reservation.call_args_list[0][0][1] + commit2 = mock_client.commit_reservation.call_args_list[1][0][1] + assert commit1["idempotency_key"] == commit2["idempotency_key"] == "ap2:ap2-tx-shared:commit" + + def test_different_transactions_produce_different_keys(self, mock_client) -> None: + mock_client.create_reservation.return_value = allow_response() + mock_client.commit_reservation.return_value = commit_success_response() + + with cycles_guard_payment(mock_client, mandate=make_mandate(transaction_id="t1"), run_id="r", tenant="acme"): + pass + with cycles_guard_payment(mock_client, mandate=make_mandate(transaction_id="t2"), run_id="r", tenant="acme"): + pass + + body1 = mock_client.create_reservation.call_args_list[0][0][0] + body2 = mock_client.create_reservation.call_args_list[1][0][0] + assert body1["idempotency_key"] != body2["idempotency_key"] diff --git a/tests/test_dry_run.py b/tests/test_dry_run.py new file mode 100644 index 0000000..d34d8f1 --- /dev/null +++ b/tests/test_dry_run.py @@ -0,0 +1,64 @@ +"""Dry-run: reserve request carries dry_run=true; no commit, no release on exit.""" + +from __future__ import annotations + +import pytest +from runcycles.response import CyclesResponse + +from runcycles_ap2 import AP2GuardDenied, cycles_guard_payment +from tests.conftest import allow_response, deny_response + + +class TestDryRun: + def test_dry_run_flag_sent_and_no_commit_called(self, mock_client, mandate) -> None: + # Server can return an ALLOW with no reservation_id for dry-runs. + mock_client.create_reservation.return_value = CyclesResponse.success( + 200, + { + "decision": "ALLOW", + "affected_scopes": ["tenant:acme"], + "scope_path": "tenant:acme", + "reserved": {"unit": "USD_MICROCENTS", "amount": 19_900_000_000}, + }, + ) + + with cycles_guard_payment( + mock_client, + mandate=mandate, + run_id="r", + tenant="acme", + dry_run=True, + ) as guard: + assert guard.decision is not None + assert guard.decision.value == "ALLOW" + assert guard.reservation_id is None + + body = mock_client.create_reservation.call_args[0][0] + assert body["dry_run"] is True + mock_client.commit_reservation.assert_not_called() + mock_client.release_reservation.assert_not_called() + + def test_dry_run_deny_raises_guard_denied(self, mock_client, mandate) -> None: + mock_client.create_reservation.return_value = deny_response("BUDGET_EXCEEDED") + + with pytest.raises(AP2GuardDenied): + with cycles_guard_payment( + mock_client, + mandate=mandate, + run_id="r", + tenant="acme", + dry_run=True, + ): + pass + + def test_non_dry_run_does_not_set_flag(self, mock_client, mandate) -> None: + mock_client.create_reservation.return_value = allow_response() + from tests.conftest import commit_success_response + + mock_client.commit_reservation.return_value = commit_success_response() + + with cycles_guard_payment(mock_client, mandate=mandate, run_id="r", tenant="acme"): + pass + + body = mock_client.create_reservation.call_args[0][0] + assert "dry_run" not in body diff --git a/tests/test_guard_clean_commit.py b/tests/test_guard_clean_commit.py new file mode 100644 index 0000000..7c9b908 --- /dev/null +++ b/tests/test_guard_clean_commit.py @@ -0,0 +1,81 @@ +"""Clean exit → commit_reservation with the deterministic AP2 commit key.""" + +from __future__ import annotations + +from runcycles_ap2 import cycles_guard_payment +from tests.conftest import allow_response, commit_success_response + + +class TestCleanCommit: + def test_commit_called_with_ap2_idempotency_key(self, mock_client, mandate) -> None: + mock_client.create_reservation.return_value = allow_response("rsv_clean_001") + mock_client.commit_reservation.return_value = commit_success_response() + + with cycles_guard_payment(mock_client, mandate=mandate, run_id="run_1", tenant="acme", agent="bot") as guard: + assert guard.reservation_id == "rsv_clean_001" + assert guard.decision is not None + assert guard.decision.value == "ALLOW" + + assert guard.committed is True + mock_client.commit_reservation.assert_called_once() + called_id, called_body = mock_client.commit_reservation.call_args[0] + assert called_id == "rsv_clean_001" + assert called_body["idempotency_key"] == "ap2:ap2-tx-001:commit" + assert called_body["actual"] == {"unit": "USD_MICROCENTS", "amount": 19_900_000_000} + mock_client.release_reservation.assert_not_called() + + def test_actual_micros_override_is_committed(self, mock_client, mandate) -> None: + mock_client.create_reservation.return_value = allow_response() + mock_client.commit_reservation.return_value = commit_success_response() + + with cycles_guard_payment(mock_client, mandate=mandate, run_id="r", tenant="acme") as guard: + guard.set_actual_micros(5_000_000_000) + + body = mock_client.commit_reservation.call_args[0][1] + assert body["actual"]["amount"] == 5_000_000_000 + + def test_attach_receipt_fields_lands_in_commit_metadata_and_receipt(self, mock_client, mandate) -> None: + mock_client.create_reservation.return_value = allow_response() + mock_client.commit_reservation.return_value = commit_success_response() + + with cycles_guard_payment(mock_client, mandate=mandate, run_id="r", tenant="acme", agent="bot") as guard: + guard.attach_receipt_fields(psp_ref="psp_abc", trace_id="t1") + + body = mock_client.commit_reservation.call_args[0][1] + assert body["metadata"]["psp_ref"] == "psp_abc" + assert body["metadata"]["trace_id"] == "t1" + assert guard.receipt is not None + assert guard.receipt.psp_ref == "psp_abc" + assert guard.receipt.extra == {"trace_id": "t1"} + assert guard.receipt.committed is True + assert guard.receipt.action_kind == "payment.charge" + + def test_emit_receipt_false_skips_receipt(self, mock_client, mandate) -> None: + mock_client.create_reservation.return_value = allow_response() + mock_client.commit_reservation.return_value = commit_success_response() + + with cycles_guard_payment( + mock_client, + mandate=mandate, + run_id="r", + tenant="acme", + emit_receipt=False, + ) as guard: + pass + + assert guard.receipt is None + assert guard.committed is True + + def test_reserve_body_carries_policy_keys(self, mock_client, mandate) -> None: + mock_client.create_reservation.return_value = allow_response() + mock_client.commit_reservation.return_value = commit_success_response() + + with cycles_guard_payment(mock_client, mandate=mandate, run_id="r", tenant="acme") as _: + pass + + body = mock_client.create_reservation.call_args[0][0] + assert body["idempotency_key"] == "ap2:ap2-tx-001:reserve" + assert body["action"]["policy_keys"]["host"] == "merchant.example" + assert body["action"]["policy_keys"]["custom"]["payment_protocol"] == "ap2" + assert body["subject"]["dimensions"]["ap2_transaction_id"] == "ap2-tx-001" + assert body["overage_policy"] == "REJECT" diff --git a/tests/test_guard_denial.py b/tests/test_guard_denial.py new file mode 100644 index 0000000..53d303d --- /dev/null +++ b/tests/test_guard_denial.py @@ -0,0 +1,52 @@ +"""Server Decision.DENY on enter → AP2GuardDenied raised; real money never moves.""" + +from __future__ import annotations + +import pytest +from runcycles.response import CyclesResponse + +from runcycles_ap2 import AP2GuardDenied, cycles_guard_payment +from tests.conftest import deny_response + + +class TestDenial: + def test_deny_decision_raises_before_psp_call(self, mock_client, mandate) -> None: + mock_client.create_reservation.return_value = deny_response("BUDGET_EXCEEDED") + sentinel = {"psp_called": False} + + with pytest.raises(AP2GuardDenied) as ei: + with cycles_guard_payment(mock_client, mandate=mandate, run_id="r", tenant="acme") as _: + sentinel["psp_called"] = True # must NOT run + + assert sentinel["psp_called"] is False + assert ei.value.reason_code == "BUDGET_EXCEEDED" + mock_client.commit_reservation.assert_not_called() + mock_client.release_reservation.assert_not_called() + + def test_http_error_response_raises_guard_denied(self, mock_client, mandate) -> None: + mock_client.create_reservation.return_value = CyclesResponse.http_error( + 400, + error_message="bad request", + body={"error": "INVALID_REQUEST", "message": "bad", "request_id": "req_1"}, + ) + + with pytest.raises(AP2GuardDenied) as ei: + with cycles_guard_payment(mock_client, mandate=mandate, run_id="r", tenant="acme") as _: + pass + + assert ei.value.reason_code == "INVALID_REQUEST" + assert ei.value.request_id == "req_1" + + def test_missing_reservation_id_raises_guard_denied(self, mock_client, mandate) -> None: + # ALLOW decision but no reservation_id — protocol violation. + mock_client.create_reservation.return_value = CyclesResponse.success( + 200, + { + "decision": "ALLOW", + "affected_scopes": ["tenant:acme"], + "reserved": {"unit": "USD_MICROCENTS", "amount": 100}, + }, + ) + with pytest.raises(AP2GuardDenied): + with cycles_guard_payment(mock_client, mandate=mandate, run_id="r", tenant="acme") as _: + pass diff --git a/tests/test_guard_release_on_exception.py b/tests/test_guard_release_on_exception.py new file mode 100644 index 0000000..0655cf4 --- /dev/null +++ b/tests/test_guard_release_on_exception.py @@ -0,0 +1,58 @@ +"""Exception inside ``with`` → release_reservation with deterministic key.""" + +from __future__ import annotations + +import pytest + +from runcycles_ap2 import cycles_guard_payment +from tests.conftest import allow_response, release_success_response + + +class TestReleaseOnException: + def test_runtime_error_triggers_release(self, mock_client, mandate) -> None: + mock_client.create_reservation.return_value = allow_response("rsv_rel") + mock_client.release_reservation.return_value = release_success_response() + + with pytest.raises(RuntimeError, match="psp failure"): + with cycles_guard_payment(mock_client, mandate=mandate, run_id="r", tenant="acme") as _: + raise RuntimeError("psp failure") + + mock_client.release_reservation.assert_called_once() + called_id, body = mock_client.release_reservation.call_args[0] + assert called_id == "rsv_rel" + assert body["idempotency_key"] == "ap2:ap2-tx-001:release:RuntimeError" + assert body["reason"].startswith("ap2_guard_failed:RuntimeError") + mock_client.commit_reservation.assert_not_called() + + def test_value_error_uses_exception_type_in_key(self, mock_client, mandate) -> None: + mock_client.create_reservation.return_value = allow_response("rsv_ve") + mock_client.release_reservation.return_value = release_success_response() + + with pytest.raises(ValueError): + with cycles_guard_payment(mock_client, mandate=mandate, run_id="r", tenant="acme") as _: + raise ValueError("nope") + + body = mock_client.release_reservation.call_args[0][1] + assert body["idempotency_key"] == "ap2:ap2-tx-001:release:ValueError" + + def test_abort_releases_on_clean_exit(self, mock_client, mandate) -> None: + mock_client.create_reservation.return_value = allow_response("rsv_abort") + mock_client.release_reservation.return_value = release_success_response() + + with cycles_guard_payment(mock_client, mandate=mandate, run_id="r", tenant="acme") as guard: + guard.abort("psp_returned_failure") + + mock_client.commit_reservation.assert_not_called() + mock_client.release_reservation.assert_called_once() + body = mock_client.release_reservation.call_args[0][1] + assert "psp_returned_failure" in body["reason"] + assert guard.committed is False + + def test_release_failure_is_swallowed(self, mock_client, mandate) -> None: + mock_client.create_reservation.return_value = allow_response() + mock_client.release_reservation.side_effect = ConnectionError("transport down") + + # The original exception must propagate even if release itself fails. + with pytest.raises(RuntimeError, match="boom"): + with cycles_guard_payment(mock_client, mandate=mandate, run_id="r", tenant="acme") as _: + raise RuntimeError("boom") diff --git a/tests/test_idempotency.py b/tests/test_idempotency.py new file mode 100644 index 0000000..28c1283 --- /dev/null +++ b/tests/test_idempotency.py @@ -0,0 +1,54 @@ +"""Idempotency: same mandate sent to server reuses the deterministic AP2 key.""" + +from __future__ import annotations + +from runcycles_ap2 import cycles_guard_payment +from runcycles_ap2.mapping import idempotency_key +from tests.conftest import allow_response, commit_error_response, commit_success_response + + +class TestIdempotency: + def test_reserve_key_is_deterministic_per_transaction(self, mock_client, mandate) -> None: + mock_client.create_reservation.return_value = allow_response() + mock_client.commit_reservation.return_value = commit_success_response() + + with cycles_guard_payment(mock_client, mandate=mandate, run_id="r", tenant="acme") as _: + pass + + body = mock_client.create_reservation.call_args[0][0] + assert body["idempotency_key"] == idempotency_key(mandate.transaction_id, "reserve") + + def test_commit_idempotency_mismatch_does_not_release(self, mock_client, mandate) -> None: + # If a previous commit ran with a different payload under the same key, server returns + # IDEMPOTENCY_MISMATCH. We must not auto-release — that would attempt to undo a real charge. + mock_client.create_reservation.return_value = allow_response() + mock_client.commit_reservation.return_value = commit_error_response("IDEMPOTENCY_MISMATCH", status=409) + + with cycles_guard_payment(mock_client, mandate=mandate, run_id="r", tenant="acme") as guard: + pass + + mock_client.release_reservation.assert_not_called() + assert guard.committed is False + + def test_reservation_finalized_does_not_release(self, mock_client, mandate) -> None: + mock_client.create_reservation.return_value = allow_response() + mock_client.commit_reservation.return_value = commit_error_response("RESERVATION_FINALIZED", status=409) + + with cycles_guard_payment(mock_client, mandate=mandate, run_id="r", tenant="acme") as _: + pass + + mock_client.release_reservation.assert_not_called() + + def test_unrecognized_commit_error_triggers_release(self, mock_client, mandate) -> None: + from tests.conftest import release_success_response + + mock_client.create_reservation.return_value = allow_response() + mock_client.commit_reservation.return_value = commit_error_response("INVALID_REQUEST", status=400) + mock_client.release_reservation.return_value = release_success_response() + + with cycles_guard_payment(mock_client, mandate=mandate, run_id="r", tenant="acme") as _: + pass + + mock_client.release_reservation.assert_called_once() + body = mock_client.release_reservation.call_args[0][1] + assert "INVALID_REQUEST" in body["reason"] diff --git a/tests/test_mapping.py b/tests/test_mapping.py new file mode 100644 index 0000000..bc60f19 --- /dev/null +++ b/tests/test_mapping.py @@ -0,0 +1,320 @@ +"""Pure unit tests for mapping.py — no network, no client.""" + +from __future__ import annotations + +import pytest + +from runcycles_ap2._constants import ( + CUSTOM_CURRENCY, + CUSTOM_PAYMENT_PROTOCOL, + DIM_AP2_TRANSACTION_ID, + DIM_CHECKOUT_HASH, + DIM_OPEN_MANDATE_HASH, + DIM_RUN_ID, +) +from runcycles_ap2.exceptions import AP2CurrencyError, AP2MandateError +from runcycles_ap2.mapping import ( + build_action, + build_commit_body, + build_estimate, + build_release_body, + build_reservation_body, + build_subject, + idempotency_key, +) +from runcycles_ap2.models import AP2Mandate +from tests.conftest import make_mandate + + +class TestIdempotencyKey: + def test_reserve_phase(self) -> None: + assert idempotency_key("ap2-tx-001", "reserve") == "ap2:ap2-tx-001:reserve" + + def test_commit_phase(self) -> None: + assert idempotency_key("ap2-tx-001", "commit") == "ap2:ap2-tx-001:commit" + + def test_release_with_suffix(self) -> None: + assert idempotency_key("ap2-tx-001", "release", "RuntimeError") == "ap2:ap2-tx-001:release:RuntimeError" + + def test_release_sanitizes_unsafe_chars(self) -> None: + # Spaces and slashes must be replaced so the key stays a valid header value. + key = idempotency_key("tx", "release", "some/bad value") + assert key == "ap2:tx:release:some_bad_value" + + def test_total_length_capped(self) -> None: + long_tx = "x" * 400 + key = idempotency_key(long_tx, "commit") + assert len(key) <= 256 + + +class TestBuildSubject: + def test_minimal(self) -> None: + m = make_mandate(checkout_hash=None) + s = build_subject( + m, + run_id="run1", + tenant="acme", + workspace=None, + app=None, + workflow=None, + agent=None, + toolset=None, + ) + assert s["tenant"] == "acme" + assert s["dimensions"][DIM_RUN_ID] == "run1" + assert s["dimensions"][DIM_AP2_TRANSACTION_ID] == "ap2-tx-001" + assert DIM_CHECKOUT_HASH not in s["dimensions"] + + def test_full(self) -> None: + m = make_mandate(checkout_hash="ch_x", open_mandate_hash="omh_y") + s = build_subject( + m, + run_id="run1", + tenant="acme", + workspace="prod", + app="checkout", + workflow="ap2-hnp", + agent="bot", + toolset="psp", + extra_dimensions={"customer_segment": "vip"}, + ) + assert s["agent"] == "bot" + assert s["dimensions"][DIM_CHECKOUT_HASH] == "ch_x" + assert s["dimensions"][DIM_OPEN_MANDATE_HASH] == "omh_y" + assert s["dimensions"]["customer_segment"] == "vip" + + def test_canonical_dimensions_win_over_extras(self) -> None: + m = make_mandate() + s = build_subject( + m, + run_id="r", + tenant="t", + workspace=None, + app=None, + workflow=None, + agent=None, + toolset=None, + extra_dimensions={DIM_RUN_ID: "hijacked"}, + ) + assert s["dimensions"][DIM_RUN_ID] == "r" + + +class TestBuildAction: + def test_default_kind(self) -> None: + a = build_action(make_mandate(), action_kind="payment.charge") + assert a["kind"] == "payment.charge" + assert a["policy_keys"]["host"] == "merchant.example" + assert a["policy_keys"]["custom"][CUSTOM_PAYMENT_PROTOCOL] == "ap2" + assert a["policy_keys"]["custom"][CUSTOM_CURRENCY] == "USD" + + def test_action_kind_override(self) -> None: + a = build_action(make_mandate(), action_kind="payment.refund") + assert a["kind"] == "payment.refund" + + +class TestBuildEstimate: + def test_usd_micros(self) -> None: + e = build_estimate(make_mandate(amount_value="1.50")) + assert e == {"unit": "USD_MICROCENTS", "amount": 150_000_000} + + def test_zero_allowed(self) -> None: + e = build_estimate(make_mandate(amount_value="0")) + assert e["amount"] == 0 + + +class TestAmountMicros: + def test_non_usd_raises(self) -> None: + m = AP2Mandate( + transaction_id="tx", + amount_value="1.00", + currency="EUR", + payee_website="x.example", + ) + with pytest.raises(AP2CurrencyError): + m.amount_micros() + + def test_negative_raises(self) -> None: + m = AP2Mandate( + transaction_id="tx", + amount_value="-1.00", + currency="USD", + payee_website="x.example", + ) + with pytest.raises(AP2MandateError): + m.amount_micros() + + def test_garbage_value_raises(self) -> None: + m = AP2Mandate( + transaction_id="tx", + amount_value="not-a-number", + currency="USD", + payee_website="x.example", + ) + with pytest.raises(AP2MandateError): + m.amount_micros() + + +class TestBuildReservationBody: + def test_full_shape(self) -> None: + body = build_reservation_body( + make_mandate(), + run_id="run1", + tenant="acme", + workspace=None, + app=None, + workflow=None, + agent="bot", + toolset=None, + action_kind="payment.charge", + ttl_ms=60_000, + overage_policy="REJECT", + dry_run=False, + ) + assert body["idempotency_key"] == "ap2:ap2-tx-001:reserve" + assert body["subject"]["tenant"] == "acme" + assert body["action"]["kind"] == "payment.charge" + assert body["estimate"] == {"unit": "USD_MICROCENTS", "amount": 19_900_000_000} + assert body["ttl_ms"] == 60_000 + assert body["overage_policy"] == "REJECT" + assert "dry_run" not in body + + def test_dry_run_flag(self) -> None: + body = build_reservation_body( + make_mandate(), + run_id="run1", + tenant="acme", + workspace=None, + app=None, + workflow=None, + agent=None, + toolset=None, + action_kind="payment.charge", + ttl_ms=60_000, + overage_policy="REJECT", + dry_run=True, + ) + assert body["dry_run"] is True + + def test_metadata_passthrough(self) -> None: + body = build_reservation_body( + make_mandate(), + run_id="run1", + tenant="acme", + workspace=None, + app=None, + workflow=None, + agent=None, + toolset=None, + action_kind="payment.charge", + ttl_ms=60_000, + overage_policy="REJECT", + dry_run=False, + metadata={"trace_id": "abc"}, + ) + assert body["metadata"] == {"trace_id": "abc"} + + +class TestBuildCommitBody: + def test_defaults_to_mandate_amount(self) -> None: + body = build_commit_body(make_mandate()) + assert body["actual"]["amount"] == 19_900_000_000 + assert body["idempotency_key"] == "ap2:ap2-tx-001:commit" + + def test_actual_override(self) -> None: + body = build_commit_body(make_mandate(), actual_micros=5_000_000_000) + assert body["actual"]["amount"] == 5_000_000_000 + + def test_metadata_attached(self) -> None: + body = build_commit_body(make_mandate(), metadata={"psp_ref": "psp_1"}) + assert body["metadata"]["psp_ref"] == "psp_1" + + +class TestBuildReleaseBody: + def test_with_exception_type(self) -> None: + body = build_release_body(make_mandate(), reason="fail", exception_type="RuntimeError") + assert body["idempotency_key"] == "ap2:ap2-tx-001:release:RuntimeError" + assert body["reason"] == "fail" + + def test_reason_truncated(self) -> None: + body = build_release_body(make_mandate(), reason="x" * 500, exception_type=None) + assert len(body["reason"]) == 256 + + +class TestFromAp2: + def test_minimum_fields(self) -> None: + class FakeAmount: + value = "12.34" + currency = "USD" + + class FakePayee: + website = "shop.example" + + class FakePM: + transaction_id = "tx-xyz" + payment_amount = FakeAmount() + payee = FakePayee() + + m = AP2Mandate.from_ap2(FakePM()) + assert m.transaction_id == "tx-xyz" + assert m.amount_value == "12.34" + assert m.payee_website == "shop.example" + + def test_payee_identifier_fallback(self) -> None: + class FakeAmount: + value = "1" + currency = "USD" + + class FakePayee: + website = None + identifier = "merchant_abc" + + class FakePM: + transaction_id = "tx" + payment_amount = FakeAmount() + payee = FakePayee() + + m = AP2Mandate.from_ap2(FakePM()) + assert m.payee_website == "merchant_abc" + + def test_missing_payee_raises(self) -> None: + class FakeAmount: + value = "1" + currency = "USD" + + class FakePayee: + website = None + identifier = None + + class FakePM: + transaction_id = "tx" + payment_amount = FakeAmount() + payee = FakePayee() + + with pytest.raises(AP2MandateError): + AP2Mandate.from_ap2(FakePM()) + + def test_missing_field_raises(self) -> None: + class FakePM: + pass + + with pytest.raises(AP2MandateError): + AP2Mandate.from_ap2(FakePM()) + + def test_checkout_mandate_hash(self) -> None: + class FakeAmount: + value = "1" + currency = "USD" + + class FakePayee: + website = "x.example" + + class FakePM: + transaction_id = "tx" + payment_amount = FakeAmount() + payee = FakePayee() + + class FakeCM: + hash = "ch_test" + + m = AP2Mandate.from_ap2(FakePM(), FakeCM()) + assert m.checkout_hash == "ch_test" diff --git a/tests/test_receipt.py b/tests/test_receipt.py new file mode 100644 index 0000000..2f5f78a --- /dev/null +++ b/tests/test_receipt.py @@ -0,0 +1,82 @@ +"""Pure unit tests for receipt.py.""" + +from __future__ import annotations + +from runcycles_ap2._constants import RECEIPT_SCHEMA +from runcycles_ap2.receipt import build_runtime_authority_receipt +from tests.conftest import make_mandate + + +class TestBuildReceipt: + def test_minimum_fields(self) -> None: + r = build_runtime_authority_receipt( + make_mandate(), + decision="ALLOW", + reservation_id="rsv_1", + tenant="acme", + agent="bot", + action_kind="payment.charge", + policy_keys={"host": "merchant.example"}, + amount_micros=19_900_000_000, + committed=True, + issued_at_ms=1_700_000_000_000, + ) + assert r.schema_name == RECEIPT_SCHEMA + assert r.decision == "ALLOW" + assert r.reservation_id == "rsv_1" + assert r.tenant == "acme" + assert r.agent == "bot" + assert r.ap2_transaction_id == "ap2-tx-001" + assert r.amount_unit == "USD_MICROCENTS" + assert r.amount_micros == 19_900_000_000 + assert r.committed is True + assert r.issued_at_ms == 1_700_000_000_000 + + def test_serialization_uses_schema_alias(self) -> None: + r = build_runtime_authority_receipt( + make_mandate(), + decision="ALLOW", + reservation_id="rsv_1", + tenant="t", + agent=None, + action_kind="payment.charge", + policy_keys={"host": "x"}, + amount_micros=1, + committed=True, + issued_at_ms=1, + ) + dumped = r.model_dump(by_alias=True) + assert dumped["schema"] == RECEIPT_SCHEMA + assert "schema_name" not in dumped + + def test_psp_ref_and_extra(self) -> None: + r = build_runtime_authority_receipt( + make_mandate(), + decision="ALLOW", + reservation_id="rsv_1", + tenant="t", + agent=None, + action_kind="payment.charge", + policy_keys={"host": "x"}, + amount_micros=1, + committed=True, + psp_ref="psp_abc", + extra={"trace": "t1"}, + issued_at_ms=1, + ) + assert r.psp_ref == "psp_abc" + assert r.extra == {"trace": "t1"} + + def test_issued_at_ms_defaults_to_now(self) -> None: + r = build_runtime_authority_receipt( + make_mandate(), + decision="ALLOW", + reservation_id="rsv_1", + tenant="t", + agent=None, + action_kind="payment.charge", + policy_keys={"host": "x"}, + amount_micros=1, + committed=True, + ) + assert r.issued_at_ms > 1_000_000_000_000 # plausibly a real timestamp From 0685a75b0b6f9e11bd61f752cea28dbb923589ec Mon Sep 17 00:00:00 2001 From: Albert Mavashev Date: Wed, 13 May 2026 09:04:02 -0400 Subject: [PATCH 2/7] fix: address P1 idempotency collision + 3 P2 safety findings from review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P1 — idempotency_key() used to slice phase suffix off long transaction_ids and let unsanitized chars reach the Idempotency-Key header, breaking the consume-once defense. Two 256-char tx_ids sharing the first 252 chars collided on reserve. Switch to ap2:{sha256(tx)[:32]}:{phase}[:{suffix}] — fixed-length, header-safe, phase always preserved (128-bit collision resistance). Raw tx_id stays on Subject.dimensions for debug. P2 — dry_run=True returned a guard normally and let the `with` body run; only __exit__ skipped commit/release. A real PSP call inside the block would move money off the books. __enter__ now raises AP2DryRunResult (new exception carrying decision payload) so the body is unreachable. Example reworked to use try/except for dry-run. P2 — amount_micros() accepted NaN, +/-Infinity, and silently rounded sub-micro precision, then later raised raw decimal.InvalidOperation or OverflowError. Add is_finite() check, reject exponent < -8, wrap all decimal failures as AP2MandateError. P2 — unrecognized commit rejection used to release silently and exit normally; caller's only signal was guard.committed == False, easy to miss → unreconciled payment state. New AP2GuardCommitFailed exception is raised after release in that branch. RESERVATION_FINALIZED / RESERVATION_EXPIRED / IDEMPOTENCY_MISMATCH remain silent (benign replays of a prior attempt). Public API additions: - AP2DryRunResult exception - AP2GuardCommitFailed exception Wire shape change: idempotency keys use a SHA-256 prefix of the transaction_id instead of the raw value. Pre-release, so no migration. Test posture: 62 tests (up from 53), 97.45% coverage. ruff + mypy strict clean. `python -m build` produces sdist + wheel cleanly. AUDIT.md and CHANGELOG.md updated. --- AUDIT.md | 25 ++++++ CHANGELOG.md | 8 +- README.md | 17 ++-- examples/ap2_human_not_present.py | 64 ++++++++++----- runcycles_ap2/__init__.py | 11 ++- runcycles_ap2/_constants.py | 6 ++ runcycles_ap2/exceptions.py | 59 ++++++++++++++ runcycles_ap2/guard.py | 33 ++++++-- runcycles_ap2/mapping.py | 35 ++++++-- runcycles_ap2/models.py | 34 ++++++-- tests/test_double_spend_same_transaction.py | 7 +- tests/test_dry_run.py | 70 ++++++++++++---- tests/test_guard_clean_commit.py | 5 +- tests/test_guard_release_on_exception.py | 5 +- tests/test_idempotency.py | 16 +++- tests/test_mapping.py | 88 ++++++++++++++++++--- 16 files changed, 399 insertions(+), 84 deletions(-) diff --git a/AUDIT.md b/AUDIT.md index a6f92e0..ce8c1c3 100644 --- a/AUDIT.md +++ b/AUDIT.md @@ -2,6 +2,31 @@ Per `CLAUDE.md`: this file records material changes to the repo (server, admin, client). For a client package, that means public API, on-the-wire request shape, and protocol-conformance posture. +## 2026-05-13 — pre-release review fixes (P1 + 3×P2) + +**Author:** code-review response on PR #1 +**Scope:** wire shape, public API surface, validation + +Four findings from review addressed before v0.1.0 release: + +1. **[P1] Idempotency key collision** — `idempotency_key()` previously appended the phase suffix to the raw `transaction_id` and sliced the whole string to 256 chars. Two 256-char transaction_ids sharing the first 252 chars produced the same reserve key; a single 256-char id produced identical reserve/commit keys (phase suffix stripped). The raw `transaction_id` could also include whitespace / control bytes that reached the `Idempotency-Key` header. **Fix:** key shape changed to `ap2:{sha256(transaction_id)[:32]}:{phase}[:{suffix}]`. Hash is fixed-length (32 hex chars, 128-bit collision resistance), header-safe (hex only), and phase is always preserved. The raw `transaction_id` is still attached to `Subject.dimensions["ap2_transaction_id"]` for debug. **This is a wire-shape change** — pre-release so no migration cost. + +2. **[P2] Dry-run still executed body** — `__enter__` with `dry_run=True` previously returned a guard normally; only `__exit__` skipped commit/release. If a caller ran a real PSP charge inside the `with` body, money would move with no Cycles record. **Fix:** `__enter__` now raises `AP2DryRunResult` (new exception carrying decision payload) so the `with` body is unreachable in dry-run mode. Public API: added `AP2DryRunResult` to exports. + +3. **[P2] Decimal validation gaps** — `AP2Mandate.amount_micros()` accepted `NaN` and `+/-Infinity` (Pydantic-validated through `Decimal()`), then raised raw `decimal.InvalidOperation` or `OverflowError` later. Sub-micro precision (>8 decimal places) was silently rounded. **Fix:** explicit `is_finite()` check; `as_tuple().exponent < -8` rejection; all decimal failures (`DecimalException`, `OverflowError`) wrapped as `AP2MandateError`. + +4. **[P2] Commit failures invisible to caller** — unrecognized commit rejection (e.g., 400 INVALID_REQUEST after PSP charge) was logged + released and the context manager exited normally. Caller's only signal was `guard.committed == False`, easy to miss → unreconciled payment state. **Fix:** added `AP2GuardCommitFailed` (new exception carrying `error_code`, `request_id`, `reservation_id`); raised from `_handle_commit` after the release in the unrecognized-rejection branch. `RESERVATION_FINALIZED` / `RESERVATION_EXPIRED` / `IDEMPOTENCY_MISMATCH` still return silently — those are benign replays of a prior attempt. + +**Test posture after fixes:** +- 62 tests (up from 53), 97.45% coverage. +- New regression tests cover: long tx_id phase preservation, distinct-but-similar tx_id collision avoidance, header-unsafe char sanitization, NaN / +/-Infinity rejection, sub-micro precision rejection, dry-run body unreachability, commit-failure exception surfacing. + +**Public API additions:** +- `AP2DryRunResult` exception +- `AP2GuardCommitFailed` exception + +No protocol changes required. + ## 2026-05-13 — v0.1.0 initial scaffold **Author:** initial commit diff --git a/CHANGELOG.md b/CHANGELOG.md index 8dc05bc..aa146b0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,9 +11,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `cycles_guard_payment(...)` sync context manager: `reserve` on enter, `commit` on clean exit, `release` on exception. - `AP2Mandate` adapter type insulating the wrapper from upstream AP2 schema churn. - `RuntimeAuthorityReceipt` (`runtime_authority.ap2.payment.charge.v1`) — client-side, informational. -- Deterministic idempotency keys (`ap2:{transaction_id}:reserve|commit|release:{...}`). -- USD-only enforcement; non-USD raises `ValueError`. -- 8 tests, ≥ 95% coverage, ruff + mypy strict. +- Deterministic idempotency keys: `ap2:{sha256(transaction_id)[:32]}:{phase}[:{suffix}]`. Hash is fixed-length (128-bit collision resistance), header-safe, and the phase suffix is always preserved regardless of upstream id length. +- `AP2DryRunResult` exception — raised from `__enter__` when `dry_run=True` so the `with` body cannot execute (prevents real PSP calls under dry-run from moving money off the books). +- `AP2GuardCommitFailed` exception — raised after release when the server rejects a commit with an unrecognized code; surfaces PSP/Cycles reconciliation events that were previously only visible via `guard.committed == False`. +- USD-only enforcement; non-USD raises `AP2CurrencyError`. Rejects NaN, +/-Infinity, and amounts with more than 8 decimal places (sub-micro precision); wraps all `decimal` failures as `AP2MandateError`. +- 62 tests, ≥ 95% coverage, ruff + mypy strict. ### Planned for v0.2 - `AsyncGuardedPayment` (asyncio). diff --git a/README.md b/README.md index 4931115..be59e68 100644 --- a/README.md +++ b/README.md @@ -2,7 +2,7 @@ [![PyPI Downloads](https://img.shields.io/pypi/dm/runcycles-ap2)](https://pypi.org/project/runcycles-ap2/) [![CI](https://github.com/runcycles/cycles-ap2-python/actions/workflows/ci.yml/badge.svg)](https://github.com/runcycles/cycles-ap2-python/actions) [![License](https://img.shields.io/badge/license-Apache%202.0-blue)](LICENSE) -[![Coverage](https://img.shields.io/badge/coverage-98%25-brightgreen)](https://github.com/runcycles/cycles-ap2-python/actions) +[![Coverage](https://img.shields.io/badge/coverage-97%25-brightgreen)](https://github.com/runcycles/cycles-ap2-python/actions) [![OpenSSF Scorecard](https://api.scorecard.dev/projects/github.com/runcycles/cycles-ap2-python/badge)](https://scorecard.dev/viewer/?uri=github.com/runcycles/cycles-ap2-python) # Cycles AP2 Guard — Runtime authority for AP2 agent payments @@ -80,7 +80,8 @@ Required upstream attributes: `payment_mandate.transaction_id`, `payment_mandate | Commit returns `RESERVATION_FINALIZED` / `RESERVATION_EXPIRED` / `IDEMPOTENCY_MISMATCH` | **Neither** | Logged at warning; we never auto-release after these (a previous commit may already have charged) | | Commit returns other 4xx | **Release** | Reason `ap2_commit_rejected:{code}` | | `guard.abort(reason)` called inside `with` | **Release** | Reason `ap2_guard_aborted:{reason}` | -| `dry_run=True` | **Neither** | Returns decision only; no reservation_id, no commit/release on exit | +| `dry_run=True` | **Neither** | `__enter__` raises `AP2DryRunResult` carrying the decision payload — the `with` body never runs, so a real PSP call cannot leak under a dry-run probe | +| Commit rejected with unrecognized code | **Release + raise** | Reservation released; `AP2GuardCommitFailed` raised so the caller cannot miss the reconciliation event | `AP2GuardDenied` carries `reason_code` and `request_id` for upstream logging. @@ -107,11 +108,11 @@ The wrapper computes idempotency keys from `transaction_id`; callers MUST NOT pa | Phase | Key shape | |---|---| -| Reserve | `ap2:{transaction_id}:reserve` | -| Commit | `ap2:{transaction_id}:commit` | -| Release | `ap2:{transaction_id}:release:{ExcType}` | +| Reserve | `ap2:{sha256(transaction_id)[:32]}:reserve` | +| Commit | `ap2:{sha256(transaction_id)[:32]}:commit` | +| Release | `ap2:{sha256(transaction_id)[:32]}:release:{ExcType}` | -Keys are truncated to 256 chars and sanitized to alphanumeric + `_-.` per protocol header rules. +The transaction_id is hashed (SHA-256, first 32 hex chars — 128 bits of collision resistance) so the key is fixed-length, header-safe, and the phase suffix is always preserved regardless of how long the upstream id is. The raw `transaction_id` is still attached to `Subject.dimensions["ap2_transaction_id"]` for debug/audit. ## Runtime authority receipt @@ -164,8 +165,10 @@ Exception hierarchy: |---|---| | `AP2GuardError` | Base for all AP2-guard errors | | `AP2GuardDenied` | Cycles returned `DENY` or the reserve POST failed | +| `AP2DryRunResult` | Raised from `__enter__` when `dry_run=True` — carries the decision payload; the `with` body never executes | +| `AP2GuardCommitFailed` | Commit was rejected with an unrecognized code after the body ran; reservation has been released, PSP state may need reconciliation | | `AP2CurrencyError` | Non-USD mandate in v0.1 (subclass of `ValueError`) | -| `AP2MandateError` | Adapter input is malformed (subclass of `ValueError`) | +| `AP2MandateError` | Adapter input is malformed — NaN, infinity, sub-micro precision, missing payee, etc. (subclass of `ValueError`) | ## Features diff --git a/examples/ap2_human_not_present.py b/examples/ap2_human_not_present.py index e4f7207..da3bbbb 100644 --- a/examples/ap2_human_not_present.py +++ b/examples/ap2_human_not_present.py @@ -1,10 +1,14 @@ """End-to-end example: guard an AP2 human-not-present payment with Cycles. Runs against a local Cycles server (see cycles-client-python README). The PSP is a -fake — replace with a real charge call in production. Run: +fake — replace with a real charge call in production. +Usage: CYCLES_BASE_URL=http://localhost:7878 CYCLES_API_KEY=test CYCLES_TENANT=acme \\ python examples/ap2_human_not_present.py + + # Probe the decision without creating a reservation or moving money: + DRY_RUN=1 python examples/ap2_human_not_present.py """ from __future__ import annotations @@ -14,7 +18,7 @@ from runcycles import CyclesClient, CyclesConfig -from runcycles_ap2 import AP2Mandate, cycles_guard_payment +from runcycles_ap2 import AP2DryRunResult, AP2Mandate, cycles_guard_payment def fake_psp_charge(mandate: AP2Mandate) -> dict[str, str]: @@ -22,13 +26,47 @@ def fake_psp_charge(mandate: AP2Mandate) -> dict[str, str]: return {"id": f"psp_{mandate.transaction_id}", "status": "captured"} +def run_dry_run(client: CyclesClient, mandate: AP2Mandate, tenant: str) -> None: + """Dry-run is a POLICY PROBE — the `with` body never runs. We catch the result.""" + try: + with cycles_guard_payment( + client, + mandate=mandate, + run_id="run_demo_001", + tenant=tenant, + agent="checkout-bot", + workflow="ap2-human-not-present", + dry_run=True, + ): + # This block is unreachable under dry_run=True — AP2DryRunResult was raised. + raise AssertionError("dry-run body must not execute") + except AP2DryRunResult as result: + print(f"dry-run decision={result.decision}, reason_code={result.reason_code}") + + +def run_real(client: CyclesClient, mandate: AP2Mandate, tenant: str) -> None: + with cycles_guard_payment( + client, + mandate=mandate, + run_id="run_demo_001", + tenant=tenant, + agent="checkout-bot", + workflow="ap2-human-not-present", + ) as guard: + print(f"decision={guard.decision}, reservation_id={guard.reservation_id}") + psp = fake_psp_charge(mandate) + guard.attach_receipt_fields(psp_ref=psp["id"]) + + if guard.receipt is not None: + print(json.dumps(guard.receipt.model_dump(by_alias=True), indent=2)) + + def main() -> None: config = CyclesConfig( base_url=os.environ.get("CYCLES_BASE_URL", "http://localhost:7878"), api_key=os.environ.get("CYCLES_API_KEY", "test-key"), tenant=os.environ.get("CYCLES_TENANT", "acme"), ) - mandate = AP2Mandate( transaction_id="ap2-tx-demo-001", amount_value="199.00", @@ -36,23 +74,11 @@ def main() -> None: payee_website="merchant.example", checkout_hash="ch_demo_001", ) - with CyclesClient(config) as client: - with cycles_guard_payment( - client, - mandate=mandate, - run_id="run_demo_001", - tenant=config.tenant, - agent="checkout-bot", - workflow="ap2-human-not-present", - dry_run=os.environ.get("DRY_RUN") == "1", - ) as guard: - print(f"decision={guard.decision}, reservation_id={guard.reservation_id}") - psp = fake_psp_charge(mandate) - guard.attach_receipt_fields(psp_ref=psp["id"]) - - if guard.receipt is not None: - print(json.dumps(guard.receipt.model_dump(by_alias=True), indent=2)) + if os.environ.get("DRY_RUN") == "1": + run_dry_run(client, mandate, config.tenant or "acme") + else: + run_real(client, mandate, config.tenant or "acme") if __name__ == "__main__": diff --git a/runcycles_ap2/__init__.py b/runcycles_ap2/__init__.py index 274aa4b..cc7bf46 100644 --- a/runcycles_ap2/__init__.py +++ b/runcycles_ap2/__init__.py @@ -2,7 +2,14 @@ from __future__ import annotations -from runcycles_ap2.exceptions import AP2CurrencyError, AP2GuardDenied, AP2GuardError, AP2MandateError +from runcycles_ap2.exceptions import ( + AP2CurrencyError, + AP2DryRunResult, + AP2GuardCommitFailed, + AP2GuardDenied, + AP2GuardError, + AP2MandateError, +) from runcycles_ap2.guard import GuardedPayment, cycles_guard_payment from runcycles_ap2.models import AP2Mandate, RuntimeAuthorityReceipt @@ -10,6 +17,8 @@ __all__ = [ "AP2CurrencyError", + "AP2DryRunResult", + "AP2GuardCommitFailed", "AP2GuardDenied", "AP2GuardError", "AP2Mandate", diff --git a/runcycles_ap2/_constants.py b/runcycles_ap2/_constants.py index f47fa49..72e4da0 100644 --- a/runcycles_ap2/_constants.py +++ b/runcycles_ap2/_constants.py @@ -31,5 +31,11 @@ # Idempotency key prefix used for all phases. IDEMPOTENCY_PREFIX: Final[str] = "ap2" +# Length of the hex-encoded SHA-256 prefix used in idempotency keys. +# 32 hex chars == 128 bits of collision resistance. Keys stay well inside the 256-char +# protocol cap, so the phase suffix (`reserve`/`commit`/`release:{ExcType}`) is always +# preserved — which is what protects the consume-once defense. +TRANSACTION_ID_HASH_LEN: Final[int] = 32 + # USD micro-cents conversion (1 USD == 1e8 micro-cents). USD_MICROCENTS_PER_DOLLAR: Final[int] = 100_000_000 diff --git a/runcycles_ap2/exceptions.py b/runcycles_ap2/exceptions.py index 62cb008..fa00fd4 100644 --- a/runcycles_ap2/exceptions.py +++ b/runcycles_ap2/exceptions.py @@ -2,6 +2,8 @@ from __future__ import annotations +from typing import Any + class AP2GuardError(Exception): """Base class for all AP2 guard errors.""" @@ -20,6 +22,63 @@ def __init__(self, message: str, *, reason_code: str | None = None, request_id: self.request_id = request_id +class AP2DryRunResult(AP2GuardError): + """Raised from ``__enter__`` when ``dry_run=True``. + + The body of the ``with`` block does NOT execute — this is deliberate. A dry-run is + a policy *probe*, not a payment attempt; if the body ran and you swapped in a real + PSP, money would move with no Cycles record. Catching this exception is the only + way to read the decision in dry-run mode. + + The exception carries the decision payload that a non-dry-run call would have + produced (``decision``, ``reason_code``, ``caps``, ``balances``). + """ + + def __init__( + self, + message: str, + *, + decision: str, + reason_code: str | None = None, + caps: Any = None, + balances: Any = None, + affected_scopes: list[str] | None = None, + ) -> None: + super().__init__(message) + self.decision = decision + self.reason_code = reason_code + self.caps = caps + self.balances = balances + self.affected_scopes = affected_scopes + + +class AP2GuardCommitFailed(AP2GuardError): + """Cycles rejected the commit AFTER the body ran (PSP may already have charged). + + The reservation has been released to prevent stranding budget, but the caller MUST + treat this as a reconciliation event: payment state on the PSP side may be out of + sync with Cycles' view. We raise instead of returning silently so the caller cannot + miss the condition. + + This exception is NOT raised for ``RESERVATION_FINALIZED`` / ``RESERVATION_EXPIRED`` + / ``IDEMPOTENCY_MISMATCH`` — those indicate a prior attempt already finalized the + reservation and the current call is a benign replay. + """ + + def __init__( + self, + message: str, + *, + error_code: str | None = None, + request_id: str | None = None, + reservation_id: str | None = None, + ) -> None: + super().__init__(message) + self.error_code = error_code + self.request_id = request_id + self.reservation_id = reservation_id + + class AP2CurrencyError(AP2GuardError, ValueError): """Currency is unsupported in this version (v0.1 is USD-only).""" diff --git a/runcycles_ap2/guard.py b/runcycles_ap2/guard.py index b26e80a..a3734f3 100644 --- a/runcycles_ap2/guard.py +++ b/runcycles_ap2/guard.py @@ -11,7 +11,7 @@ from runcycles.models import Decision, ReservationCreateResponse from runcycles_ap2._constants import DEFAULT_ACTION_KIND, DEFAULT_OVERAGE_POLICY, DEFAULT_TTL_MS -from runcycles_ap2.exceptions import AP2GuardDenied +from runcycles_ap2.exceptions import AP2DryRunResult, AP2GuardCommitFailed, AP2GuardDenied from runcycles_ap2.mapping import ( build_action, build_commit_body, @@ -156,13 +156,23 @@ def __enter__(self) -> GuardedPayment: ) if self._dry_run: - # Dry-run is a "would allow" probe; no reservation_id, no commit/release on exit. + # Dry-run is a policy probe — NEVER execute the `with` body. If we returned + # `self` here, callers running a real PSP charge inside the block would move + # money with no Cycles record. Raising blocks the body unconditionally; the + # decision payload rides on the exception. logger.info( "AP2 dry-run evaluated: decision=%s, tx=%s", result.decision, self._mandate.transaction_id, ) - return self + raise AP2DryRunResult( + f"AP2 dry-run decision={result.decision.value} for transaction {self._mandate.transaction_id}", + decision=result.decision.value, + reason_code=result.reason_code, + caps=result.caps, + balances=result.balances, + affected_scopes=result.affected_scopes, + ) if result.reservation_id is None: raise AP2GuardDenied( @@ -185,8 +195,8 @@ def __exit__( exc_val: BaseException | None, exc_tb: TracebackType | None, ) -> None: - if self._dry_run: - return # nothing to commit or release on dry-run + # NOTE: dry-run no longer reaches __exit__ — __enter__ raises AP2DryRunResult + # before returning, so the `with` body never executes. if self._reservation_id is None: return # nothing to clean up (denial path already raised) @@ -228,7 +238,10 @@ def _handle_commit(self) -> None: error = response.get_error_response() error_code = error.error_code.value if (error and error.error_code) else None + request_id = error.request_id if error else None if error_code in ("RESERVATION_FINALIZED", "RESERVATION_EXPIRED", "IDEMPOTENCY_MISMATCH"): + # Benign replay: a previous attempt already finalized the reservation. The + # caller's payment record is correct from that prior attempt; nothing to do. logger.warning( "AP2 commit returned %s (no release): id=%s, tx=%s", error_code, @@ -237,6 +250,9 @@ def _handle_commit(self) -> None: ) return + # Unrecognized commit rejection. The PSP may already have moved money, so we + # release the budget and raise — the caller MUST reconcile, and a silent + # `guard.committed == False` was too easy to miss. logger.warning( "AP2 commit rejected (releasing): id=%s, tx=%s, code=%s", self._reservation_id, @@ -247,6 +263,13 @@ def _handle_commit(self) -> None: reason=f"ap2_commit_rejected:{error_code or 'UNKNOWN'}", exc_name="CommitRejected", ) + raise AP2GuardCommitFailed( + f"AP2 commit rejected for transaction {self._mandate.transaction_id} " + f"(code={error_code}); reservation released. PSP state may need reconciliation.", + error_code=error_code, + request_id=request_id, + reservation_id=self._reservation_id, + ) def _handle_release(self, *, reason: str, exc_name: str) -> None: assert self._reservation_id is not None diff --git a/runcycles_ap2/mapping.py b/runcycles_ap2/mapping.py index 82bf99b..793c481 100644 --- a/runcycles_ap2/mapping.py +++ b/runcycles_ap2/mapping.py @@ -6,6 +6,7 @@ from __future__ import annotations +import hashlib from typing import Any from runcycles_ap2._constants import ( @@ -18,22 +19,44 @@ DIM_OPEN_MANDATE_HASH, DIM_RUN_ID, IDEMPOTENCY_PREFIX, + TRANSACTION_ID_HASH_LEN, ) from runcycles_ap2.models import AP2Mandate +def _hash_transaction_id(transaction_id: str) -> str: + """SHA-256 of the raw ``transaction_id``, hex-encoded and truncated. + + Hashing guarantees: + - fixed-length output regardless of input (avoids the 256-char header truncation + that silently dropped the phase suffix in earlier versions); + - header-safe characters only (lowercase hex) — no whitespace, no control bytes; + - the same input always produces the same key on any platform. + Truncating to 32 hex chars preserves 128 bits of collision resistance — more than + enough for an idempotency-key namespace scoped to a single tenant. + """ + digest = hashlib.sha256(transaction_id.encode("utf-8")).hexdigest() + return digest[:TRANSACTION_ID_HASH_LEN] + + def idempotency_key(transaction_id: str, phase: str, suffix: str | None = None) -> str: - """Deterministic idempotency key: ``ap2:{tx}:{phase}[:{suffix}]``. + """Deterministic idempotency key: ``ap2:{sha256(tx)[:32]}:{phase}[:{suffix}]``. + + The phase suffix is ALWAYS preserved (fixed-length hash + short phase fit comfortably + inside the 256-char protocol cap). The same ``transaction_id`` reaching the same phase + from any retry, parallel worker, or process restart produces the same key — the + consume-once defense. - The same ``transaction_id`` reaching the same phase from any retry, parallel worker, - or process restart will produce the same key — that's the server-side replay defense. + The raw ``transaction_id`` is still attached to ``Subject.dimensions["ap2_transaction_id"]`` + on the Cycles wire payload for debug/audit; only the idempotency key uses the hash. """ - base = f"{IDEMPOTENCY_PREFIX}:{transaction_id}:{phase}" + base = f"{IDEMPOTENCY_PREFIX}:{_hash_transaction_id(transaction_id)}:{phase}" if suffix: - # Strip characters that would push us over the 256-char limit or break the key shape. + # Header-safe charset: alphanumeric, underscore, hyphen, dot. Anything else + # becomes ``_`` so the resulting key is always a valid HTTP header value. safe_suffix = "".join(c if c.isalnum() or c in ("_", "-", ".") else "_" for c in suffix) base = f"{base}:{safe_suffix[:64]}" - return base[:256] + return base def build_subject( diff --git a/runcycles_ap2/models.py b/runcycles_ap2/models.py index c7956a3..083da07 100644 --- a/runcycles_ap2/models.py +++ b/runcycles_ap2/models.py @@ -7,7 +7,7 @@ from __future__ import annotations -from decimal import Decimal +from decimal import Decimal, DecimalException from typing import Annotated, Any from pydantic import BaseModel, ConfigDict, Field @@ -15,6 +15,10 @@ from runcycles_ap2._constants import USD_MICROCENTS_PER_DOLLAR from runcycles_ap2.exceptions import AP2CurrencyError, AP2MandateError +# USD_MICROCENTS is 10^-8 USD; anything finer than 8 decimal places in the source string +# would be silently rounded. We reject it explicitly so callers don't lose precision. +_MAX_DECIMAL_PLACES = 8 + _CONFIG = ConfigDict(populate_by_name=True, extra="forbid", str_strip_whitespace=True) @@ -37,18 +41,38 @@ class AP2Mandate(BaseModel): def amount_micros(self) -> int: """Convert ``amount_value`` (decimal string in major units) to USD micro-cents. - v0.1 enforces USD only. + v0.1 enforces USD only. Rejects NaN, +/-Infinity, negative values, and any input + with more than 8 decimal places (sub-micro precision can't survive the conversion + and we'd rather raise than silently round). All ``decimal`` failure modes — + including ``InvalidOperation`` and ``OverflowError`` from the final ``int()`` + cast — are wrapped as :class:`AP2MandateError`. """ if self.currency.upper() != "USD": raise AP2CurrencyError(f"v0.1 supports USD only; got {self.currency!r}. Multi-currency lands in v0.2.") try: value = Decimal(self.amount_value) - except (ArithmeticError, ValueError) as exc: + except (DecimalException, ValueError) as exc: raise AP2MandateError(f"amount_value {self.amount_value!r} is not a valid decimal") from exc + if not value.is_finite(): + raise AP2MandateError(f"amount_value must be finite; got {self.amount_value!r}") if value < 0: raise AP2MandateError("amount_value must be non-negative") - micros = int((value * USD_MICROCENTS_PER_DOLLAR).to_integral_value()) - return micros + # `as_tuple().exponent` is the negative of the number of decimal places for + # finite values (e.g. "1.234" → exponent -3). It can also be a string sentinel + # ("n", "N", "F") for NaN/Infinity, but is_finite() above rules those out. + exponent = value.as_tuple().exponent + if isinstance(exponent, int) and exponent < -_MAX_DECIMAL_PLACES: + raise AP2MandateError( + f"amount_value {self.amount_value!r} has more than {_MAX_DECIMAL_PLACES} decimal places; " + "sub-micro precision would be lost" + ) + try: + scaled = value * USD_MICROCENTS_PER_DOLLAR + return int(scaled.to_integral_value()) + except (DecimalException, OverflowError, ValueError) as exc: + raise AP2MandateError( + f"amount_value {self.amount_value!r} could not be converted to USD micro-cents" + ) from exc @classmethod def from_ap2( diff --git a/tests/test_double_spend_same_transaction.py b/tests/test_double_spend_same_transaction.py index 3052d47..2978f3e 100644 --- a/tests/test_double_spend_same_transaction.py +++ b/tests/test_double_spend_same_transaction.py @@ -10,6 +10,7 @@ from __future__ import annotations from runcycles_ap2 import cycles_guard_payment +from runcycles_ap2.mapping import idempotency_key from tests.conftest import allow_response, commit_success_response, make_mandate @@ -28,11 +29,13 @@ def test_two_guards_same_transaction_send_same_reserve_key(self, mock_client) -> body1 = mock_client.create_reservation.call_args_list[0][0][0] body2 = mock_client.create_reservation.call_args_list[1][0][0] - assert body1["idempotency_key"] == body2["idempotency_key"] == "ap2:ap2-tx-shared:reserve" + expected_reserve = idempotency_key("ap2-tx-shared", "reserve") + expected_commit = idempotency_key("ap2-tx-shared", "commit") + assert body1["idempotency_key"] == body2["idempotency_key"] == expected_reserve # Commit keys also collide — the server collapses both onto one reservation. commit1 = mock_client.commit_reservation.call_args_list[0][0][1] commit2 = mock_client.commit_reservation.call_args_list[1][0][1] - assert commit1["idempotency_key"] == commit2["idempotency_key"] == "ap2:ap2-tx-shared:commit" + assert commit1["idempotency_key"] == commit2["idempotency_key"] == expected_commit def test_different_transactions_produce_different_keys(self, mock_client) -> None: mock_client.create_reservation.return_value = allow_response() diff --git a/tests/test_dry_run.py b/tests/test_dry_run.py index d34d8f1..a5cf467 100644 --- a/tests/test_dry_run.py +++ b/tests/test_dry_run.py @@ -1,17 +1,22 @@ -"""Dry-run: reserve request carries dry_run=true; no commit, no release on exit.""" +"""Dry-run: ``__enter__`` raises ``AP2DryRunResult`` so the ``with`` body never runs. + +Earlier versions returned a guard and let the body execute, which let real PSP calls move +money with no Cycles record. The new contract: dry-run is a policy probe, period — the +caller MUST catch the exception to read the decision. +""" from __future__ import annotations import pytest from runcycles.response import CyclesResponse -from runcycles_ap2 import AP2GuardDenied, cycles_guard_payment +from runcycles_ap2 import AP2DryRunResult, AP2GuardDenied, cycles_guard_payment from tests.conftest import allow_response, deny_response class TestDryRun: - def test_dry_run_flag_sent_and_no_commit_called(self, mock_client, mandate) -> None: - # Server can return an ALLOW with no reservation_id for dry-runs. + def test_allow_decision_raises_dry_run_result_and_body_does_not_run(self, mock_client, mandate) -> None: + # Server returns ALLOW without a reservation_id (the dry_run signature). mock_client.create_reservation.return_value = CyclesResponse.success( 200, { @@ -21,27 +26,30 @@ def test_dry_run_flag_sent_and_no_commit_called(self, mock_client, mandate) -> N "reserved": {"unit": "USD_MICROCENTS", "amount": 19_900_000_000}, }, ) + body_ran = {"value": False} - with cycles_guard_payment( - mock_client, - mandate=mandate, - run_id="r", - tenant="acme", - dry_run=True, - ) as guard: - assert guard.decision is not None - assert guard.decision.value == "ALLOW" - assert guard.reservation_id is None + with pytest.raises(AP2DryRunResult) as ei: + with cycles_guard_payment( + mock_client, + mandate=mandate, + run_id="r", + tenant="acme", + dry_run=True, + ) as _: + body_ran["value"] = True # MUST NOT execute + assert body_ran["value"] is False + assert ei.value.decision == "ALLOW" body = mock_client.create_reservation.call_args[0][0] assert body["dry_run"] is True mock_client.commit_reservation.assert_not_called() mock_client.release_reservation.assert_not_called() - def test_dry_run_deny_raises_guard_denied(self, mock_client, mandate) -> None: + def test_dry_run_deny_raises_guard_denied_not_dry_run_result(self, mock_client, mandate) -> None: + # DENY is still DENY — that signal pre-empts the dry-run probe path. mock_client.create_reservation.return_value = deny_response("BUDGET_EXCEEDED") - with pytest.raises(AP2GuardDenied): + with pytest.raises(AP2GuardDenied) as ei: with cycles_guard_payment( mock_client, mandate=mandate, @@ -51,6 +59,36 @@ def test_dry_run_deny_raises_guard_denied(self, mock_client, mandate) -> None: ): pass + assert ei.value.reason_code == "BUDGET_EXCEEDED" + + def test_dry_run_result_carries_caps_and_scopes(self, mock_client, mandate) -> None: + mock_client.create_reservation.return_value = CyclesResponse.success( + 200, + { + "decision": "ALLOW_WITH_CAPS", + "affected_scopes": ["tenant:acme", "agent:bot"], + "scope_path": "tenant:acme", + "reserved": {"unit": "USD_MICROCENTS", "amount": 1_000}, + "caps": {"max_tokens": 500}, + "reason_code": "NEAR_LIMIT", + }, + ) + + with pytest.raises(AP2DryRunResult) as ei: + with cycles_guard_payment( + mock_client, + mandate=mandate, + run_id="r", + tenant="acme", + dry_run=True, + ): + pass + + assert ei.value.decision == "ALLOW_WITH_CAPS" + assert ei.value.reason_code == "NEAR_LIMIT" + assert ei.value.affected_scopes == ["tenant:acme", "agent:bot"] + assert ei.value.caps is not None + def test_non_dry_run_does_not_set_flag(self, mock_client, mandate) -> None: mock_client.create_reservation.return_value = allow_response() from tests.conftest import commit_success_response diff --git a/tests/test_guard_clean_commit.py b/tests/test_guard_clean_commit.py index 7c9b908..9563e5d 100644 --- a/tests/test_guard_clean_commit.py +++ b/tests/test_guard_clean_commit.py @@ -3,6 +3,7 @@ from __future__ import annotations from runcycles_ap2 import cycles_guard_payment +from runcycles_ap2.mapping import idempotency_key from tests.conftest import allow_response, commit_success_response @@ -20,7 +21,7 @@ def test_commit_called_with_ap2_idempotency_key(self, mock_client, mandate) -> N mock_client.commit_reservation.assert_called_once() called_id, called_body = mock_client.commit_reservation.call_args[0] assert called_id == "rsv_clean_001" - assert called_body["idempotency_key"] == "ap2:ap2-tx-001:commit" + assert called_body["idempotency_key"] == idempotency_key("ap2-tx-001", "commit") assert called_body["actual"] == {"unit": "USD_MICROCENTS", "amount": 19_900_000_000} mock_client.release_reservation.assert_not_called() @@ -74,7 +75,7 @@ def test_reserve_body_carries_policy_keys(self, mock_client, mandate) -> None: pass body = mock_client.create_reservation.call_args[0][0] - assert body["idempotency_key"] == "ap2:ap2-tx-001:reserve" + assert body["idempotency_key"] == idempotency_key("ap2-tx-001", "reserve") assert body["action"]["policy_keys"]["host"] == "merchant.example" assert body["action"]["policy_keys"]["custom"]["payment_protocol"] == "ap2" assert body["subject"]["dimensions"]["ap2_transaction_id"] == "ap2-tx-001" diff --git a/tests/test_guard_release_on_exception.py b/tests/test_guard_release_on_exception.py index 0655cf4..9958e54 100644 --- a/tests/test_guard_release_on_exception.py +++ b/tests/test_guard_release_on_exception.py @@ -5,6 +5,7 @@ import pytest from runcycles_ap2 import cycles_guard_payment +from runcycles_ap2.mapping import idempotency_key from tests.conftest import allow_response, release_success_response @@ -20,7 +21,7 @@ def test_runtime_error_triggers_release(self, mock_client, mandate) -> None: mock_client.release_reservation.assert_called_once() called_id, body = mock_client.release_reservation.call_args[0] assert called_id == "rsv_rel" - assert body["idempotency_key"] == "ap2:ap2-tx-001:release:RuntimeError" + assert body["idempotency_key"] == idempotency_key("ap2-tx-001", "release", "RuntimeError") assert body["reason"].startswith("ap2_guard_failed:RuntimeError") mock_client.commit_reservation.assert_not_called() @@ -33,7 +34,7 @@ def test_value_error_uses_exception_type_in_key(self, mock_client, mandate) -> N raise ValueError("nope") body = mock_client.release_reservation.call_args[0][1] - assert body["idempotency_key"] == "ap2:ap2-tx-001:release:ValueError" + assert body["idempotency_key"] == idempotency_key("ap2-tx-001", "release", "ValueError") def test_abort_releases_on_clean_exit(self, mock_client, mandate) -> None: mock_client.create_reservation.return_value = allow_response("rsv_abort") diff --git a/tests/test_idempotency.py b/tests/test_idempotency.py index 28c1283..798445c 100644 --- a/tests/test_idempotency.py +++ b/tests/test_idempotency.py @@ -39,16 +39,26 @@ def test_reservation_finalized_does_not_release(self, mock_client, mandate) -> N mock_client.release_reservation.assert_not_called() - def test_unrecognized_commit_error_triggers_release(self, mock_client, mandate) -> None: + def test_unrecognized_commit_error_releases_and_raises(self, mock_client, mandate) -> None: + # An unrecognized commit rejection means the PSP may already have charged. We + # release the reservation AND raise AP2GuardCommitFailed so the caller cannot + # miss the reconciliation event (the silent `guard.committed == False` signal + # from earlier versions was too easy to overlook). + import pytest + + from runcycles_ap2 import AP2GuardCommitFailed from tests.conftest import release_success_response mock_client.create_reservation.return_value = allow_response() mock_client.commit_reservation.return_value = commit_error_response("INVALID_REQUEST", status=400) mock_client.release_reservation.return_value = release_success_response() - with cycles_guard_payment(mock_client, mandate=mandate, run_id="r", tenant="acme") as _: - pass + with pytest.raises(AP2GuardCommitFailed) as ei: + with cycles_guard_payment(mock_client, mandate=mandate, run_id="r", tenant="acme") as _: + pass + assert ei.value.error_code == "INVALID_REQUEST" + assert ei.value.reservation_id == "rsv_ap2_001" mock_client.release_reservation.assert_called_once() body = mock_client.release_reservation.call_args[0][1] assert "INVALID_REQUEST" in body["reason"] diff --git a/tests/test_mapping.py b/tests/test_mapping.py index bc60f19..6e0cfdb 100644 --- a/tests/test_mapping.py +++ b/tests/test_mapping.py @@ -2,6 +2,8 @@ from __future__ import annotations +import hashlib + import pytest from runcycles_ap2._constants import ( @@ -11,6 +13,7 @@ DIM_CHECKOUT_HASH, DIM_OPEN_MANDATE_HASH, DIM_RUN_ID, + TRANSACTION_ID_HASH_LEN, ) from runcycles_ap2.exceptions import AP2CurrencyError, AP2MandateError from runcycles_ap2.mapping import ( @@ -26,25 +29,58 @@ from tests.conftest import make_mandate +def _expected_hash(tx: str) -> str: + return hashlib.sha256(tx.encode("utf-8")).hexdigest()[:TRANSACTION_ID_HASH_LEN] + + class TestIdempotencyKey: def test_reserve_phase(self) -> None: - assert idempotency_key("ap2-tx-001", "reserve") == "ap2:ap2-tx-001:reserve" + h = _expected_hash("ap2-tx-001") + assert idempotency_key("ap2-tx-001", "reserve") == f"ap2:{h}:reserve" def test_commit_phase(self) -> None: - assert idempotency_key("ap2-tx-001", "commit") == "ap2:ap2-tx-001:commit" + h = _expected_hash("ap2-tx-001") + assert idempotency_key("ap2-tx-001", "commit") == f"ap2:{h}:commit" def test_release_with_suffix(self) -> None: - assert idempotency_key("ap2-tx-001", "release", "RuntimeError") == "ap2:ap2-tx-001:release:RuntimeError" + h = _expected_hash("ap2-tx-001") + assert idempotency_key("ap2-tx-001", "release", "RuntimeError") == f"ap2:{h}:release:RuntimeError" def test_release_sanitizes_unsafe_chars(self) -> None: - # Spaces and slashes must be replaced so the key stays a valid header value. + # Whitespace, slashes, and other header-unsafe chars get replaced with `_`. + h = _expected_hash("tx") key = idempotency_key("tx", "release", "some/bad value") - assert key == "ap2:tx:release:some_bad_value" - - def test_total_length_capped(self) -> None: - long_tx = "x" * 400 - key = idempotency_key(long_tx, "commit") - assert len(key) <= 256 + assert key == f"ap2:{h}:release:some_bad_value" + + def test_long_transaction_id_does_not_drop_phase(self) -> None: + # P1 regression: a 256-char tx_id used to overflow the cap and have the phase + # suffix sliced off. Now the hash is fixed-length, so all phases stay distinct. + long_tx = "x" * 256 + reserve = idempotency_key(long_tx, "reserve") + commit = idempotency_key(long_tx, "commit") + release = idempotency_key(long_tx, "release", "RuntimeError") + assert reserve.endswith(":reserve") + assert commit.endswith(":commit") + assert release.endswith(":release:RuntimeError") + assert reserve != commit != release + + def test_distinct_long_tx_ids_yield_distinct_keys(self) -> None: + # P1 regression: two tx_ids sharing the first 252 chars used to collide on + # reserve. SHA-256 distinguishes them. + a = ("y" * 252) + "AAAA" + b = ("y" * 252) + "BBBB" + assert idempotency_key(a, "reserve") != idempotency_key(b, "reserve") + + def test_unsafe_transaction_id_chars_do_not_leak_to_header(self) -> None: + # P1 regression: tx_ids containing whitespace / control bytes used to reach the + # Idempotency-Key header. The hash output is hex only — header-safe by design. + key = idempotency_key("tx with\nnewline\tand\r\x00null", "reserve") + assert "\n" not in key and "\t" not in key and "\r" not in key and "\x00" not in key + assert all(c.isalnum() or c in (":", "_", "-", ".") for c in key) + + def test_key_length_bounded(self) -> None: + # Hash + phase + suffix always fits inside the protocol's 256-char cap. + assert len(idempotency_key("x" * 1024, "release", "x" * 1024)) <= 256 class TestBuildSubject: @@ -153,6 +189,32 @@ def test_garbage_value_raises(self) -> None: with pytest.raises(AP2MandateError): m.amount_micros() + def test_nan_raises(self) -> None: + m = AP2Mandate(transaction_id="tx", amount_value="NaN", currency="USD", payee_website="x.example") + with pytest.raises(AP2MandateError): + m.amount_micros() + + def test_infinity_raises(self) -> None: + m = AP2Mandate(transaction_id="tx", amount_value="Infinity", currency="USD", payee_website="x.example") + with pytest.raises(AP2MandateError): + m.amount_micros() + + def test_negative_infinity_raises(self) -> None: + m = AP2Mandate(transaction_id="tx", amount_value="-Infinity", currency="USD", payee_website="x.example") + with pytest.raises(AP2MandateError): + m.amount_micros() + + def test_sub_micro_precision_raises(self) -> None: + # 9 decimal places — below USD_MICROCENTS resolution. + m = AP2Mandate(transaction_id="tx", amount_value="1.123456789", currency="USD", payee_website="x.example") + with pytest.raises(AP2MandateError): + m.amount_micros() + + def test_exactly_eight_decimals_accepted(self) -> None: + m = AP2Mandate(transaction_id="tx", amount_value="1.12345678", currency="USD", payee_website="x.example") + # 1.12345678 * 1e8 = 112345678 micro-cents — exact, no precision loss. + assert m.amount_micros() == 112_345_678 + class TestBuildReservationBody: def test_full_shape(self) -> None: @@ -170,7 +232,7 @@ def test_full_shape(self) -> None: overage_policy="REJECT", dry_run=False, ) - assert body["idempotency_key"] == "ap2:ap2-tx-001:reserve" + assert body["idempotency_key"] == f"ap2:{_expected_hash('ap2-tx-001')}:reserve" assert body["subject"]["tenant"] == "acme" assert body["action"]["kind"] == "payment.charge" assert body["estimate"] == {"unit": "USD_MICROCENTS", "amount": 19_900_000_000} @@ -218,7 +280,7 @@ class TestBuildCommitBody: def test_defaults_to_mandate_amount(self) -> None: body = build_commit_body(make_mandate()) assert body["actual"]["amount"] == 19_900_000_000 - assert body["idempotency_key"] == "ap2:ap2-tx-001:commit" + assert body["idempotency_key"] == f"ap2:{_expected_hash('ap2-tx-001')}:commit" def test_actual_override(self) -> None: body = build_commit_body(make_mandate(), actual_micros=5_000_000_000) @@ -232,7 +294,7 @@ def test_metadata_attached(self) -> None: class TestBuildReleaseBody: def test_with_exception_type(self) -> None: body = build_release_body(make_mandate(), reason="fail", exception_type="RuntimeError") - assert body["idempotency_key"] == "ap2:ap2-tx-001:release:RuntimeError" + assert body["idempotency_key"] == f"ap2:{_expected_hash('ap2-tx-001')}:release:RuntimeError" assert body["reason"] == "fail" def test_reason_truncated(self) -> None: From 8f0b4ee4bf7bc3102490bd8b20d16d1da0ec3e71 Mon Sep 17 00:00:00 2001 From: Albert Mavashev Date: Wed, 13 May 2026 09:26:58 -0400 Subject: [PATCH 3/7] fix: address round-2 review findings (2xP2 + P3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P2 — amount_micros() used `value * 10**8` as a Decimal multiplication, which uses the default 28-digit decimal context and silently rounded inputs larger than the protocol cap. Rewrote conversion to operate directly on Decimal.as_tuple() digits: exact integer math, no context dependence, no possibility of silent rounding. Removed the now-unused USD_MICROCENTS_PER_DOLLAR constant. New regression test confirms a 29-digit input converts exactly. P2 — AP2GuardCommitFailed previously said "reservation released" regardless of whether the release attempt succeeded; if release transport-failed or returned 5xx, budget was stranded and the caller had no signal. _handle_release() now returns (success, error_detail); AP2GuardCommitFailed gained .released (bool) and .release_error (str | None) attributes; the message reflects the actual outcome ("released" vs "release FAILED ... budget stranded until TTL"). Two regression tests cover the transport-failure and non-success- response paths. P3 — README lifecycle table still referenced raw ap2:{transaction_id}:commit / :release:{ExcType} shapes, contradicting the hashed shape documented elsewhere. Rewrote the table rows to match and to reflect the new release+raise behavior. Test posture: 66 tests (up from 62), 97.87% coverage. ruff + mypy strict clean. `python -m build` produces sdist + wheel cleanly. AUDIT.md and CHANGELOG.md updated. --- AUDIT.md | 20 +++++++++++++ CHANGELOG.md | 5 ++-- README.md | 9 +++--- runcycles_ap2/_constants.py | 3 -- runcycles_ap2/exceptions.py | 18 +++++++++--- runcycles_ap2/guard.py | 57 ++++++++++++++++++++++++++----------- runcycles_ap2/models.py | 35 ++++++++++++++++------- tests/test_idempotency.py | 44 ++++++++++++++++++++++++++++ tests/test_mapping.py | 17 +++++++++++ 9 files changed, 167 insertions(+), 41 deletions(-) diff --git a/AUDIT.md b/AUDIT.md index ce8c1c3..96c889e 100644 --- a/AUDIT.md +++ b/AUDIT.md @@ -2,6 +2,26 @@ Per `CLAUDE.md`: this file records material changes to the repo (server, admin, client). For a client package, that means public API, on-the-wire request shape, and protocol-conformance posture. +## 2026-05-13 — second-round review fixes (2×P2 + P3) + +**Author:** code-review response on PR #1 (round 2) +**Scope:** payment-math correctness, exception fidelity, doc parity + +1. **[P2] Decimal default-context rounding** — `amount_micros()` previously computed `value * 10**8` as a Decimal multiplication, which uses the default 28-digit decimal context and silently rounded inputs larger than the protocol cap (a malformed mandate could carry such a value, e.g. `123456789012345678901.12345678` produced `...680` instead of `...678`). **Fix:** rewrote conversion to operate directly on `Decimal.as_tuple()` digits — exact integer math, no context dependence. Removed the now-unused `USD_MICROCENTS_PER_DOLLAR` constant. New regression test in `TestAmountMicros::test_large_value_converts_exactly`. + +2. **[P2] Release-failure obscured by `AP2GuardCommitFailed` message** — when a commit was rejected with an unrecognized code, the wrapper attempted to release the reservation and raised `AP2GuardCommitFailed` saying *"reservation released"* regardless of whether the release actually succeeded. If release transport-failed or returned 5xx, budget was stranded and the caller had no signal. **Fix:** `_handle_release()` now returns `(success: bool, error_detail: str | None)`. `AP2GuardCommitFailed` gained `.released` and `.release_error` attributes; the exception message says either "reservation released" or "reservation release FAILED ... budget stranded until TTL" based on the actual outcome. Two regression tests cover the transport-failure and non-success-response paths. + +3. **[P3] Stale README lifecycle table rows** — the response-table rows still referenced raw `ap2:{transaction_id}:commit` / `:release:{ExcType}` shapes, contradicting the hashed shape documented in the *Deterministic idempotency keys* section. **Fix:** rewrote the table rows to reference the keys section and to reflect the new "release + raise" semantics for unrecognized commit rejections. + +**Test posture after fixes:** +- 66 tests (up from 62), 97.87% coverage. + +**Public API additions:** +- `AP2GuardCommitFailed.released: bool` +- `AP2GuardCommitFailed.release_error: str | None` + +No protocol changes required. + ## 2026-05-13 — pre-release review fixes (P1 + 3×P2) **Author:** code-review response on PR #1 diff --git a/CHANGELOG.md b/CHANGELOG.md index aa146b0..124d711 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,9 +13,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `RuntimeAuthorityReceipt` (`runtime_authority.ap2.payment.charge.v1`) — client-side, informational. - Deterministic idempotency keys: `ap2:{sha256(transaction_id)[:32]}:{phase}[:{suffix}]`. Hash is fixed-length (128-bit collision resistance), header-safe, and the phase suffix is always preserved regardless of upstream id length. - `AP2DryRunResult` exception — raised from `__enter__` when `dry_run=True` so the `with` body cannot execute (prevents real PSP calls under dry-run from moving money off the books). -- `AP2GuardCommitFailed` exception — raised after release when the server rejects a commit with an unrecognized code; surfaces PSP/Cycles reconciliation events that were previously only visible via `guard.committed == False`. +- `AP2GuardCommitFailed` exception — raised after a release attempt when the server rejects a commit with an unrecognized code; carries `released: bool` and `release_error: str | None` so the caller can distinguish "budget recovered" from "budget stranded until TTL". - USD-only enforcement; non-USD raises `AP2CurrencyError`. Rejects NaN, +/-Infinity, and amounts with more than 8 decimal places (sub-micro precision); wraps all `decimal` failures as `AP2MandateError`. -- 62 tests, ≥ 95% coverage, ruff + mypy strict. +- Exact integer-tuple conversion in `amount_micros()` — does not depend on the default decimal context, so arbitrarily large valid inputs convert exactly instead of being silently rounded. +- 66 tests, ≥ 95% coverage, ruff + mypy strict. ### Planned for v0.2 - `AsyncGuardedPayment` (asyncio). diff --git a/README.md b/README.md index be59e68..95b09a6 100644 --- a/README.md +++ b/README.md @@ -73,15 +73,14 @@ Required upstream attributes: `payment_mandate.transaction_id`, `payment_mandate | Scenario | Outcome | Detail | |---|---|---| -| `Decision.ALLOW`, body completes | **Commit** | Server idempotency key `ap2:{transaction_id}:commit` | -| `Decision.ALLOW`, body raises | **Release** | Reason `ap2_guard_failed:{ExcType}`, key `ap2:{transaction_id}:release:{ExcType}` | +| `Decision.ALLOW`, body completes | **Commit** | Server idempotency key derived from `transaction_id` — see *Deterministic idempotency keys* below | +| `Decision.ALLOW`, body raises | **Release** | Reason `ap2_guard_failed:{ExcType}`, idempotency key includes the exception type | | `Decision.DENY` | **Neither** | `AP2GuardDenied` raised in `__enter__`; real money never moves | | HTTP / transport error on reserve | **Neither** | `AP2GuardDenied` raised; caller can retry — same `transaction_id` ⇒ same reserve key | | Commit returns `RESERVATION_FINALIZED` / `RESERVATION_EXPIRED` / `IDEMPOTENCY_MISMATCH` | **Neither** | Logged at warning; we never auto-release after these (a previous commit may already have charged) | -| Commit returns other 4xx | **Release** | Reason `ap2_commit_rejected:{code}` | +| Commit returns other 4xx | **Release + raise** | Reservation release attempted; `AP2GuardCommitFailed` raised with `released` + `release_error` so the caller cannot miss the reconciliation event | | `guard.abort(reason)` called inside `with` | **Release** | Reason `ap2_guard_aborted:{reason}` | | `dry_run=True` | **Neither** | `__enter__` raises `AP2DryRunResult` carrying the decision payload — the `with` body never runs, so a real PSP call cannot leak under a dry-run probe | -| Commit rejected with unrecognized code | **Release + raise** | Reservation released; `AP2GuardCommitFailed` raised so the caller cannot miss the reconciliation event | `AP2GuardDenied` carries `reason_code` and `request_id` for upstream logging. @@ -166,7 +165,7 @@ Exception hierarchy: | `AP2GuardError` | Base for all AP2-guard errors | | `AP2GuardDenied` | Cycles returned `DENY` or the reserve POST failed | | `AP2DryRunResult` | Raised from `__enter__` when `dry_run=True` — carries the decision payload; the `with` body never executes | -| `AP2GuardCommitFailed` | Commit was rejected with an unrecognized code after the body ran; reservation has been released, PSP state may need reconciliation | +| `AP2GuardCommitFailed` | Commit was rejected with an unrecognized code after the body ran. Check `.released` (bool) and `.release_error` (string \| None) on the exception — `released=False` means budget is stranded until TTL; reconcile with PSP either way | | `AP2CurrencyError` | Non-USD mandate in v0.1 (subclass of `ValueError`) | | `AP2MandateError` | Adapter input is malformed — NaN, infinity, sub-micro precision, missing payee, etc. (subclass of `ValueError`) | diff --git a/runcycles_ap2/_constants.py b/runcycles_ap2/_constants.py index 72e4da0..36caa10 100644 --- a/runcycles_ap2/_constants.py +++ b/runcycles_ap2/_constants.py @@ -36,6 +36,3 @@ # protocol cap, so the phase suffix (`reserve`/`commit`/`release:{ExcType}`) is always # preserved — which is what protects the consume-once defense. TRANSACTION_ID_HASH_LEN: Final[int] = 32 - -# USD micro-cents conversion (1 USD == 1e8 micro-cents). -USD_MICROCENTS_PER_DOLLAR: Final[int] = 100_000_000 diff --git a/runcycles_ap2/exceptions.py b/runcycles_ap2/exceptions.py index fa00fd4..3fc33f6 100644 --- a/runcycles_ap2/exceptions.py +++ b/runcycles_ap2/exceptions.py @@ -55,10 +55,16 @@ def __init__( class AP2GuardCommitFailed(AP2GuardError): """Cycles rejected the commit AFTER the body ran (PSP may already have charged). - The reservation has been released to prevent stranding budget, but the caller MUST - treat this as a reconciliation event: payment state on the PSP side may be out of - sync with Cycles' view. We raise instead of returning silently so the caller cannot - miss the condition. + The wrapper attempts to release the reservation to recover budget; whether that + release succeeded is reported via :attr:`released`. The caller MUST treat this as + a reconciliation event regardless: PSP state and Cycles' view of the budget can + be out of sync. + + - ``released = True``: budget was returned, but PSP-side capture may still need + reconciliation against your books. + - ``released = False``: budget is stranded inside the reservation until TTL expiry + (server-side rollback) AND PSP-side capture may need reconciliation. This is + the worst case — escalate. This exception is NOT raised for ``RESERVATION_FINALIZED`` / ``RESERVATION_EXPIRED`` / ``IDEMPOTENCY_MISMATCH`` — those indicate a prior attempt already finalized the @@ -72,11 +78,15 @@ def __init__( error_code: str | None = None, request_id: str | None = None, reservation_id: str | None = None, + released: bool = False, + release_error: str | None = None, ) -> None: super().__init__(message) self.error_code = error_code self.request_id = request_id self.reservation_id = reservation_id + self.released = released + self.release_error = release_error class AP2CurrencyError(AP2GuardError, ValueError): diff --git a/runcycles_ap2/guard.py b/runcycles_ap2/guard.py index a3734f3..b3c641d 100644 --- a/runcycles_ap2/guard.py +++ b/runcycles_ap2/guard.py @@ -259,38 +259,61 @@ def _handle_commit(self) -> None: self._mandate.transaction_id, error_code, ) - self._handle_release( + released, release_error = self._handle_release( reason=f"ap2_commit_rejected:{error_code or 'UNKNOWN'}", exc_name="CommitRejected", ) + status_phrase = ( + "reservation released" + if released + else f"reservation release FAILED ({release_error or 'unknown'}); budget stranded until TTL" + ) raise AP2GuardCommitFailed( f"AP2 commit rejected for transaction {self._mandate.transaction_id} " - f"(code={error_code}); reservation released. PSP state may need reconciliation.", + f"(code={error_code}); {status_phrase}. PSP state may need reconciliation.", error_code=error_code, request_id=request_id, reservation_id=self._reservation_id, + released=released, + release_error=release_error, ) - def _handle_release(self, *, reason: str, exc_name: str) -> None: + def _handle_release(self, *, reason: str, exc_name: str) -> tuple[bool, str | None]: + """Release the reservation. Returns ``(success, error_description)``. + + ``success`` is True only when the server returned a 2xx for the release. Any + non-success response or raised transport error sets ``success=False`` and + carries a short error description so the caller can surface it to operators. + """ assert self._reservation_id is not None body = build_release_body(self._mandate, reason=reason, exception_type=exc_name) try: response = self._client.release_reservation(self._reservation_id, body) - if response.is_success: - logger.info( - "AP2 released: id=%s, tx=%s, reason=%s", - self._reservation_id, - self._mandate.transaction_id, - reason, - ) - else: - logger.warning( - "AP2 release returned non-success: id=%s, status=%d", - self._reservation_id, - response.status, - ) - except Exception: + except Exception as exc: logger.exception("AP2 release raised: id=%s", self._reservation_id) + return False, f"{type(exc).__name__}: {exc}" + + if response.is_success: + logger.info( + "AP2 released: id=%s, tx=%s, reason=%s", + self._reservation_id, + self._mandate.transaction_id, + reason, + ) + return True, None + + error = response.get_error_response() + error_code = error.error_code.value if (error and error.error_code) else None + logger.warning( + "AP2 release returned non-success: id=%s, status=%d, code=%s", + self._reservation_id, + response.status, + error_code, + ) + detail = f"status={response.status}" + if error_code: + detail = f"{detail}, code={error_code}" + return False, detail def _build_receipt(self) -> None: assert self._reservation_id is not None diff --git a/runcycles_ap2/models.py b/runcycles_ap2/models.py index 083da07..c16d64b 100644 --- a/runcycles_ap2/models.py +++ b/runcycles_ap2/models.py @@ -12,7 +12,6 @@ from pydantic import BaseModel, ConfigDict, Field -from runcycles_ap2._constants import USD_MICROCENTS_PER_DOLLAR from runcycles_ap2.exceptions import AP2CurrencyError, AP2MandateError # USD_MICROCENTS is 10^-8 USD; anything finer than 8 decimal places in the source string @@ -60,19 +59,35 @@ def amount_micros(self) -> int: # `as_tuple().exponent` is the negative of the number of decimal places for # finite values (e.g. "1.234" → exponent -3). It can also be a string sentinel # ("n", "N", "F") for NaN/Infinity, but is_finite() above rules those out. - exponent = value.as_tuple().exponent - if isinstance(exponent, int) and exponent < -_MAX_DECIMAL_PLACES: + sign, digits, exponent = value.as_tuple() + if not isinstance(exponent, int): + # Defensive: is_finite() should have caught this, but the Decimal API permits + # special exponent sentinels on inputs we treat as out-of-domain. + raise AP2MandateError(f"amount_value {self.amount_value!r} is not a finite decimal") + if exponent < -_MAX_DECIMAL_PLACES: raise AP2MandateError( f"amount_value {self.amount_value!r} has more than {_MAX_DECIMAL_PLACES} decimal places; " "sub-micro precision would be lost" ) - try: - scaled = value * USD_MICROCENTS_PER_DOLLAR - return int(scaled.to_integral_value()) - except (DecimalException, OverflowError, ValueError) as exc: - raise AP2MandateError( - f"amount_value {self.amount_value!r} could not be converted to USD micro-cents" - ) from exc + + # Exact integer conversion via `as_tuple()`. We deliberately do NOT compute + # `value * 10**8` as Decimals: that uses the default 28-digit decimal context + # and silently rounds inputs larger than the protocol cap (which a malformed + # mandate could carry). Working off `digits` is unconditional — any input + # that survived the validation above produces an exact int. + if sign: + # Negative was already rejected above; keep the branch for defensive clarity. + raise AP2MandateError("amount_value must be non-negative") + int_value = 0 + for d in digits: + int_value = int_value * 10 + d + # `value` equals int_value * 10**exponent, so micros = int_value * 10**(exponent+8). + # `exponent + _MAX_DECIMAL_PLACES` is always >= 0 here because the validation + # above rejected `exponent < -_MAX_DECIMAL_PLACES`. The cast keeps mypy happy + # (negative-exponent int.__pow__ widens to float). + shift = exponent + _MAX_DECIMAL_PLACES + scale: int = int(10**shift) + return int_value * scale @classmethod def from_ap2( diff --git a/tests/test_idempotency.py b/tests/test_idempotency.py index 798445c..141d335 100644 --- a/tests/test_idempotency.py +++ b/tests/test_idempotency.py @@ -59,6 +59,50 @@ def test_unrecognized_commit_error_releases_and_raises(self, mock_client, mandat assert ei.value.error_code == "INVALID_REQUEST" assert ei.value.reservation_id == "rsv_ap2_001" + assert ei.value.released is True + assert ei.value.release_error is None + assert "released" in str(ei.value) mock_client.release_reservation.assert_called_once() body = mock_client.release_reservation.call_args[0][1] assert "INVALID_REQUEST" in body["reason"] + + def test_commit_failed_records_release_failure(self, mock_client, mandate) -> None: + # P2 regression: when release fails after a commit rejection, the exception + # must report `released=False` and surface the release error in its message. + # Earlier versions said "reservation released" regardless, misleading callers + # about whether budget had actually been recovered. + import pytest + + from runcycles_ap2 import AP2GuardCommitFailed + + mock_client.create_reservation.return_value = allow_response() + mock_client.commit_reservation.return_value = commit_error_response("INVALID_REQUEST", status=400) + mock_client.release_reservation.side_effect = ConnectionError("network down") + + with pytest.raises(AP2GuardCommitFailed) as ei: + with cycles_guard_payment(mock_client, mandate=mandate, run_id="r", tenant="acme") as _: + pass + + assert ei.value.error_code == "INVALID_REQUEST" + assert ei.value.released is False + assert ei.value.release_error is not None + assert "ConnectionError" in ei.value.release_error + assert "FAILED" in str(ei.value) and "stranded" in str(ei.value) + + def test_commit_failed_records_release_non_success(self, mock_client, mandate) -> None: + # Same as above but the release POST returned a 5xx — server didn't release. + import pytest + + from runcycles_ap2 import AP2GuardCommitFailed + + mock_client.create_reservation.return_value = allow_response() + mock_client.commit_reservation.return_value = commit_error_response("INVALID_REQUEST", status=400) + mock_client.release_reservation.return_value = commit_error_response("INTERNAL_ERROR", status=500) + + with pytest.raises(AP2GuardCommitFailed) as ei: + with cycles_guard_payment(mock_client, mandate=mandate, run_id="r", tenant="acme") as _: + pass + + assert ei.value.released is False + assert ei.value.release_error is not None + assert "500" in ei.value.release_error diff --git a/tests/test_mapping.py b/tests/test_mapping.py index 6e0cfdb..7b278f7 100644 --- a/tests/test_mapping.py +++ b/tests/test_mapping.py @@ -215,6 +215,23 @@ def test_exactly_eight_decimals_accepted(self) -> None: # 1.12345678 * 1e8 = 112345678 micro-cents — exact, no precision loss. assert m.amount_micros() == 112_345_678 + def test_large_value_converts_exactly(self) -> None: + # P2 regression: the previous Decimal-multiplication path used the default + # 28-digit context and silently rounded large inputs. The integer-tuple path + # is unconditional — any validated input converts exactly regardless of size. + m = AP2Mandate( + transaction_id="tx", + amount_value="123456789012345678901.12345678", + currency="USD", + payee_website="x.example", + ) + # 21 integer digits + 8 fractional, * 1e8 → exactly 29 digits. + assert m.amount_micros() == 12_345_678_901_234_567_890_112_345_678 + + def test_integer_value_converts_exactly(self) -> None: + m = AP2Mandate(transaction_id="tx", amount_value="100", currency="USD", payee_website="x.example") + assert m.amount_micros() == 10_000_000_000 + class TestBuildReservationBody: def test_full_shape(self) -> None: From c220f256f5b44edf94ebd16fa620d1b8ba1d9c9b Mon Sep 17 00:00:00 2001 From: Albert Mavashev Date: Wed, 13 May 2026 09:34:44 -0400 Subject: [PATCH 4/7] fix: address round-3 review findings (P2 DoS + P3 stale docs) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P2 — amount_micros() had an exponent-notation DoS: a short, finite, positive Decimal like `Decimal("1E+1000000000000")` (16 chars, well inside the 64-char field) passed every validation, then tried to compute `10 ** (10**12 + 8)` and allocate a trillion-digit integer. Even `Decimal("0E+1000000000000")` triggered the same allocation before the multiplication zeroed the result. Add a pre-allocation digit-count cap: `len(digits) + max(0, exponent) <= 19` (digit count of int64.max, the protocol USD_MICROCENTS ceiling). Three regression tests cover the DoS strings and a 20-digit legitimate-shaped overflow. Also updated the prior `test_large_value_converts_exactly` which used a 29-digit value beyond int64 — now uses int64.max exactly, still exercises the no-rounding path. P3 — two doc rows still pointed at the old behavior: - README mapping row claimed `int(round(value * 1e8))`, contradicting the current exact-integer-tuple conversion that rejects sub-micro precision and out-of-range amounts. - `GuardedPayment` class docstring still showed `ap2:{tx}:commit` raw idempotency keys, contradicting the hashed shape that has been in use since round 1. Both updated to match the implementation; the docstring now also mentions the dry-run probe behavior and the `AP2GuardCommitFailed` released/release_error fields so source readers get the same picture as the README readers. Test posture: 69 tests (up from 66), 97.89% coverage. ruff + mypy strict clean. `python -m build` produces sdist + wheel cleanly. No public API additions. AUDIT.md and CHANGELOG.md updated. --- AUDIT.md | 14 +++++++++++ CHANGELOG.md | 5 ++-- README.md | 2 +- runcycles_ap2/guard.py | 13 ++++++++--- runcycles_ap2/models.py | 39 +++++++++++++++++++++++++------ tests/test_mapping.py | 51 +++++++++++++++++++++++++++++++++++------ 6 files changed, 104 insertions(+), 20 deletions(-) diff --git a/AUDIT.md b/AUDIT.md index 96c889e..a02953b 100644 --- a/AUDIT.md +++ b/AUDIT.md @@ -2,6 +2,20 @@ Per `CLAUDE.md`: this file records material changes to the repo (server, admin, client). For a client package, that means public API, on-the-wire request shape, and protocol-conformance posture. +## 2026-05-13 — third-round review fixes (P2 DoS + P3 stale docs) + +**Author:** code-review response on PR #1 (round 3) +**Scope:** denial-of-service vector, internal/external doc parity + +1. **[P2] Exponent-notation DoS in `amount_micros()`** — a short, finite, positive Decimal like `1E+1000000000000` (16 chars, fits the 64-char field) used to pass every validation: `is_finite()` is True, value is positive, and exponent ≥ -8. The code then tried to compute `10 ** (10**12 + 8)`, allocating a trillion-digit integer and hanging the process. Even `0E+1000000000000` triggered the same allocation before the multiplication zeroed the result. **Fix:** pre-allocation digit-count cap. `total_integer_digits = len(digits) + max(0, exponent)` must be ≤ 19 (the digit count of int64.max, which is the protocol's USD_MICROCENTS ceiling). New regression tests cover `1E+1000000000000`, `0E+1000000000000`, and 20-digit "legitimate-shaped but out-of-range" amounts. Also updated the earlier `test_large_value_converts_exactly` test which used a 29-digit value beyond int64 — now uses int64.max (`92233720368.54775807`) which still exercises the no-rounding path. + +2. **[P3] Stale README mapping row + `GuardedPayment` class docstring** — README mapping table still claimed `int(round(value * 1e8))` (which is what the old default-context multiplication did) instead of the current exact-integer-tuple conversion. `GuardedPayment.__doc__` still showed `ap2:{tx}:commit` raw-key shapes. **Fix:** rewrote both to match the implementation; docstring now references `runcycles_ap2.mapping.idempotency_key`, calls out the dry-run probe behavior and `AP2GuardCommitFailed.released`/`release_error` for callers reading source. + +**Test posture after fixes:** +- 69 tests (up from 66), 97.89% coverage. + +No public API additions. + ## 2026-05-13 — second-round review fixes (2×P2 + P3) **Author:** code-review response on PR #1 (round 2) diff --git a/CHANGELOG.md b/CHANGELOG.md index 124d711..8fbee37 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,8 +15,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `AP2DryRunResult` exception — raised from `__enter__` when `dry_run=True` so the `with` body cannot execute (prevents real PSP calls under dry-run from moving money off the books). - `AP2GuardCommitFailed` exception — raised after a release attempt when the server rejects a commit with an unrecognized code; carries `released: bool` and `release_error: str | None` so the caller can distinguish "budget recovered" from "budget stranded until TTL". - USD-only enforcement; non-USD raises `AP2CurrencyError`. Rejects NaN, +/-Infinity, and amounts with more than 8 decimal places (sub-micro precision); wraps all `decimal` failures as `AP2MandateError`. -- Exact integer-tuple conversion in `amount_micros()` — does not depend on the default decimal context, so arbitrarily large valid inputs convert exactly instead of being silently rounded. -- 66 tests, ≥ 95% coverage, ruff + mypy strict. +- Exact integer-tuple conversion in `amount_micros()` — does not depend on the default decimal context, so large valid inputs convert exactly instead of being silently rounded. +- Pre-allocation digit-count cap (≤ 19 integer digits, the int64 USD_MICROCENTS ceiling) blocks exponent-notation DoS like `Decimal("1E+1000000000000")` that would otherwise hang allocating a trillion-digit scaling factor. +- 69 tests, ≥ 95% coverage, ruff + mypy strict. ### Planned for v0.2 - `AsyncGuardedPayment` (asyncio). diff --git a/README.md b/README.md index 95b09a6..07c65f4 100644 --- a/README.md +++ b/README.md @@ -89,7 +89,7 @@ Required upstream attributes: `payment_mandate.transaction_id`, `payment_mandate | AP2 source | Cycles destination | Notes | |---|---|---| | `PaymentMandate.transaction_id` | `Subject.dimensions["ap2_transaction_id"]` | also feeds idempotency keys | -| `PaymentMandate.payment_amount.value` | `Amount.amount` | `int(round(value * 1e8))` USD micro-cents | +| `PaymentMandate.payment_amount.value` | `Amount.amount` | Exact integer conversion to USD micro-cents (10⁻⁸ USD). Rejects NaN, ±Infinity, negative values, more than 8 decimal places, or amounts beyond int64 micro-cents | | `PaymentMandate.payment_amount.currency` | `Action.policy_keys.custom["currency"]` | MVP enforces `"USD"` | | `PaymentMandate.payee.website` | `Action.policy_keys.host` | required for policy routing | | `CheckoutMandate.hash` | `Subject.dimensions["checkout_hash"]` | optional | diff --git a/runcycles_ap2/guard.py b/runcycles_ap2/guard.py index b3c641d..2457d48 100644 --- a/runcycles_ap2/guard.py +++ b/runcycles_ap2/guard.py @@ -28,13 +28,20 @@ class GuardedPayment: """Sync context manager: reserve on ``__enter__``, commit/release on ``__exit__``. Decision rules: - - Clean exit (no exception) → ``commit_reservation`` with ``ap2:{tx}:commit``. + - Clean exit (no exception) → ``commit_reservation`` with the deterministic AP2 + commit idempotency key (see :func:`runcycles_ap2.mapping.idempotency_key`). - Exception inside ``with`` block → ``release_reservation`` with reason - ``ap2_guard_failed:{ExcType}`` and key ``ap2:{tx}:release:{ExcType}``. + ``ap2_guard_failed:{ExcType}`` and the matching release idempotency key. - Server ``Decision.DENY`` on enter → raises :class:`AP2GuardDenied`; real money never moves and no commit/release is issued. + - ``dry_run=True`` → raises :class:`AP2DryRunResult` from ``__enter__`` so the + ``with`` body never executes (a body-level PSP call would otherwise move money + with no Cycles record). + - Commit rejected with unrecognized code → raises :class:`AP2GuardCommitFailed` + after attempting a release; check the exception's ``released`` / + ``release_error`` attributes to know whether budget was recovered. - Same ``transaction_id`` on a retry → server returns the original reservation - (idempotent replay), so the second attempt cannot double-spend. + (idempotent replay) because the idempotency key is deterministic. """ def __init__( diff --git a/runcycles_ap2/models.py b/runcycles_ap2/models.py index c16d64b..a04220e 100644 --- a/runcycles_ap2/models.py +++ b/runcycles_ap2/models.py @@ -18,6 +18,14 @@ # would be silently rounded. We reject it explicitly so callers don't lose precision. _MAX_DECIMAL_PLACES = 8 +# Cycles `Amount.amount` is int64; max is 9_223_372_036_854_775_807 (19 digits ≈ $92.2B). +# We bound the source decimal's significant-plus-integer-position digit count BEFORE doing +# any `10**shift` math — otherwise a short string like "1E+1000000000000" (a finite Decimal +# with a 10^12 exponent) would allocate a trillion-digit integer and hang the process. +# Capping at the int64 digit count lets every legitimate USD amount pass while turning the +# DoS vector into a clean AP2MandateError. +_MAX_INTEGER_DIGITS = 19 + _CONFIG = ConfigDict(populate_by_name=True, extra="forbid", str_strip_whitespace=True) @@ -40,11 +48,14 @@ class AP2Mandate(BaseModel): def amount_micros(self) -> int: """Convert ``amount_value`` (decimal string in major units) to USD micro-cents. - v0.1 enforces USD only. Rejects NaN, +/-Infinity, negative values, and any input - with more than 8 decimal places (sub-micro precision can't survive the conversion - and we'd rather raise than silently round). All ``decimal`` failure modes — - including ``InvalidOperation`` and ``OverflowError`` from the final ``int()`` - cast — are wrapped as :class:`AP2MandateError`. + v0.1 enforces USD only. Rejects: + - non-decimals, NaN, +/-Infinity, negative values; + - inputs with more than 8 decimal places (sub-micro precision would be lost); + - inputs whose total magnitude would exceed the protocol's int64 cap + (this also blocks the ``1E+1000000000000`` exponent-notation DoS vector + that would otherwise allocate a trillion-digit scaling factor). + + All ``decimal`` failure modes are wrapped as :class:`AP2MandateError`. """ if self.currency.upper() != "USD": raise AP2CurrencyError(f"v0.1 supports USD only; got {self.currency!r}. Multi-currency lands in v0.2.") @@ -70,6 +81,19 @@ def amount_micros(self) -> int: "sub-micro precision would be lost" ) + # Bound the eventual integer size BEFORE computing 10**shift. ``len(digits)`` + # counts significant digits; ``max(0, exponent)`` counts the integer-position + # padding zeros a positive exponent implies. Their sum is the digit count of + # the unscaled integer value; capping it at 19 (int64.max digit count) blocks + # exponent-notation DoS like ``Decimal("1E+1000000000000")`` and also rejects + # legitimate-but-too-large amounts (the server would reject them anyway). + total_integer_digits = len(digits) + max(0, exponent) + if total_integer_digits > _MAX_INTEGER_DIGITS: + raise AP2MandateError( + f"amount_value {self.amount_value!r} exceeds int64 USD micro-cent range " + f"({total_integer_digits} integer digits > {_MAX_INTEGER_DIGITS} max)" + ) + # Exact integer conversion via `as_tuple()`. We deliberately do NOT compute # `value * 10**8` as Decimals: that uses the default 28-digit decimal context # and silently rounds inputs larger than the protocol cap (which a malformed @@ -83,8 +107,9 @@ def amount_micros(self) -> int: int_value = int_value * 10 + d # `value` equals int_value * 10**exponent, so micros = int_value * 10**(exponent+8). # `exponent + _MAX_DECIMAL_PLACES` is always >= 0 here because the validation - # above rejected `exponent < -_MAX_DECIMAL_PLACES`. The cast keeps mypy happy - # (negative-exponent int.__pow__ widens to float). + # above rejected `exponent < -_MAX_DECIMAL_PLACES`. The bounded-digits check + # above guarantees `shift <= _MAX_INTEGER_DIGITS + _MAX_DECIMAL_PLACES` so the + # `10**shift` allocation is small. shift = exponent + _MAX_DECIMAL_PLACES scale: int = int(10**shift) return int_value * scale diff --git a/tests/test_mapping.py b/tests/test_mapping.py index 7b278f7..295dec8 100644 --- a/tests/test_mapping.py +++ b/tests/test_mapping.py @@ -215,23 +215,60 @@ def test_exactly_eight_decimals_accepted(self) -> None: # 1.12345678 * 1e8 = 112345678 micro-cents — exact, no precision loss. assert m.amount_micros() == 112_345_678 - def test_large_value_converts_exactly(self) -> None: - # P2 regression: the previous Decimal-multiplication path used the default - # 28-digit context and silently rounded large inputs. The integer-tuple path - # is unconditional — any validated input converts exactly regardless of size. + def test_large_value_converts_exactly_within_int64_cap(self) -> None: + # P2 regression: even at the int64 boundary, the conversion stays exact (no + # silent rounding via the default decimal context that the earlier `value * + # 10**8` path would have introduced for >28-digit results). m = AP2Mandate( transaction_id="tx", - amount_value="123456789012345678901.12345678", + amount_value="92233720368.54775807", currency="USD", payee_website="x.example", ) - # 21 integer digits + 8 fractional, * 1e8 → exactly 29 digits. - assert m.amount_micros() == 12_345_678_901_234_567_890_112_345_678 + # int64.max == 9_223_372_036_854_775_807. Exact, no rounding. + assert m.amount_micros() == 9_223_372_036_854_775_807 def test_integer_value_converts_exactly(self) -> None: m = AP2Mandate(transaction_id="tx", amount_value="100", currency="USD", payee_website="x.example") assert m.amount_micros() == 10_000_000_000 + def test_huge_exponent_notation_rejected_short_input(self) -> None: + # P2 DoS regression: `Decimal("1E+1000000000000")` is short (16 chars), finite, + # and positive — it used to pass every validation and then try to allocate a + # trillion-digit int via 10**shift. The digit-count cap catches it cleanly. + m = AP2Mandate( + transaction_id="tx", + amount_value="1E+1000000000000", + currency="USD", + payee_website="x.example", + ) + with pytest.raises(AP2MandateError, match="int64"): + m.amount_micros() + + def test_zero_with_huge_exponent_rejected(self) -> None: + # P2 DoS regression: even a zero value with a huge exponent would have + # triggered the allocation before the multiplication zeroed the result. + m = AP2Mandate( + transaction_id="tx", + amount_value="0E+1000000000000", + currency="USD", + payee_website="x.example", + ) + with pytest.raises(AP2MandateError, match="int64"): + m.amount_micros() + + def test_too_many_digits_rejected(self) -> None: + # A legitimate-shaped but out-of-range amount: 20 significant digits. The + # 19-digit cap (int64.max digit count) catches it before any allocation. + m = AP2Mandate( + transaction_id="tx", + amount_value="123456789012.12345678", + currency="USD", + payee_website="x.example", + ) + with pytest.raises(AP2MandateError, match="int64"): + m.amount_micros() + class TestBuildReservationBody: def test_full_shape(self) -> None: From deca3660fa0bb790aa19b3625de1cd41ccb6b636 Mon Sep 17 00:00:00 2001 From: Albert Mavashev Date: Wed, 13 May 2026 09:40:01 -0400 Subject: [PATCH 5/7] fix: enforce exact int64 boundary on amount_micros() (P2 round 4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The round-3 fix bounded `len(digits) + max(0, exponent) <= 19` to block the DoS allocation. The cap correctly rejects 20-digit inputs but permits values one over int64.max — e.g. `92233720368.54775808` yields micros == int64.max + 1 (9_223_372_036_854_775_808), and `99999999999.99999999` yields ~1e19 micros, both still within the 19-digit cap. The server would reject them but client-side we'd already have shipped the wrong number. Add a post-conversion check `micros <= _MAX_USD_MICROS` (2**63 - 1) right before the return. The 19-digit cap remains as the pre-allocation DoS guard; this is the exact protocol boundary. Three regression tests: - int64.max exact (92233720368.54775807) — accepted - int64.max + 1 (92233720368.54775808) — rejected - 19 all-9 digits (99999999999.99999999) — rejected Test posture: 72 tests (up from 69), 97.92% coverage. ruff + mypy strict clean. `python -m build` produces sdist + wheel cleanly. No public API additions. AUDIT.md and CHANGELOG.md updated. --- AUDIT.md | 12 ++++++++++++ CHANGELOG.md | 3 ++- runcycles_ap2/models.py | 16 +++++++++++++--- tests/test_mapping.py | 36 ++++++++++++++++++++++++++++++++++++ 4 files changed, 63 insertions(+), 4 deletions(-) diff --git a/AUDIT.md b/AUDIT.md index a02953b..0d33a2f 100644 --- a/AUDIT.md +++ b/AUDIT.md @@ -2,6 +2,18 @@ Per `CLAUDE.md`: this file records material changes to the repo (server, admin, client). For a client package, that means public API, on-the-wire request shape, and protocol-conformance posture. +## 2026-05-13 — fourth-round review fix (P2 exact int64 boundary) + +**Author:** code-review response on PR #1 (round 4) +**Scope:** correctness at the int64 boundary + +**[P2] 19-digit cap permits values one over int64.max** — the round-3 fix bounded `len(digits) + max(0, exponent)` to 19 to block the DoS allocation. That cap correctly rejects 20-digit inputs but lets values like `92233720368.54775808` (int64.max + 1) and `99999999999.99999999` (≈ 10^19 micros) slip through. The server would reject them, but client-side we'd already have shipped the wrong number. **Fix:** add a post-conversion check `micros <= 2**63 - 1` (`_MAX_USD_MICROS = 9_223_372_036_854_775_807`). The 19-digit cap remains as the pre-allocation DoS guard; the post-conversion check is the exact protocol boundary. Three regression tests added: int64.max is accepted, int64.max + 1 is rejected, 19-digit-with-fractional `99999999999.99999999` is rejected. + +**Test posture after fix:** +- 72 tests (up from 69), 97.92% coverage. + +No public API additions. + ## 2026-05-13 — third-round review fixes (P2 DoS + P3 stale docs) **Author:** code-review response on PR #1 (round 3) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8fbee37..f9f8abe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,7 +17,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - USD-only enforcement; non-USD raises `AP2CurrencyError`. Rejects NaN, +/-Infinity, and amounts with more than 8 decimal places (sub-micro precision); wraps all `decimal` failures as `AP2MandateError`. - Exact integer-tuple conversion in `amount_micros()` — does not depend on the default decimal context, so large valid inputs convert exactly instead of being silently rounded. - Pre-allocation digit-count cap (≤ 19 integer digits, the int64 USD_MICROCENTS ceiling) blocks exponent-notation DoS like `Decimal("1E+1000000000000")` that would otherwise hang allocating a trillion-digit scaling factor. -- 69 tests, ≥ 95% coverage, ruff + mypy strict. +- Post-conversion int64 boundary check (`micros <= 2**63 - 1`) rejects amounts one over int64.max client-side instead of relying on the server. +- 72 tests, ≥ 95% coverage, ruff + mypy strict. ### Planned for v0.2 - `AsyncGuardedPayment` (asyncio). diff --git a/runcycles_ap2/models.py b/runcycles_ap2/models.py index a04220e..f29edce 100644 --- a/runcycles_ap2/models.py +++ b/runcycles_ap2/models.py @@ -22,9 +22,10 @@ # We bound the source decimal's significant-plus-integer-position digit count BEFORE doing # any `10**shift` math — otherwise a short string like "1E+1000000000000" (a finite Decimal # with a 10^12 exponent) would allocate a trillion-digit integer and hang the process. -# Capping at the int64 digit count lets every legitimate USD amount pass while turning the -# DoS vector into a clean AP2MandateError. +# The digit cap is a coarse pre-allocation guard; the precise int64 boundary is enforced +# AFTER conversion via `_MAX_USD_MICROS`. _MAX_INTEGER_DIGITS = 19 +_MAX_USD_MICROS = 2**63 - 1 # 9_223_372_036_854_775_807 — Cycles Amount.amount int64 ceiling _CONFIG = ConfigDict(populate_by_name=True, extra="forbid", str_strip_whitespace=True) @@ -112,7 +113,16 @@ def amount_micros(self) -> int: # `10**shift` allocation is small. shift = exponent + _MAX_DECIMAL_PLACES scale: int = int(10**shift) - return int_value * scale + micros = int_value * scale + # The 19-digit cap above is necessary but not sufficient — it permits values + # one over int64.max (e.g. "92233720368.54775808" → 9_223_372_036_854_775_808). + # Enforce the exact protocol ceiling here so we fail-fast client-side rather + # than waiting for the server to reject. + if micros > _MAX_USD_MICROS: + raise AP2MandateError( + f"amount_value {self.amount_value!r} exceeds int64 USD micro-cent max ({micros} > {_MAX_USD_MICROS})" + ) + return micros @classmethod def from_ap2( diff --git a/tests/test_mapping.py b/tests/test_mapping.py index 295dec8..1f0ca2e 100644 --- a/tests/test_mapping.py +++ b/tests/test_mapping.py @@ -269,6 +269,42 @@ def test_too_many_digits_rejected(self) -> None: with pytest.raises(AP2MandateError, match="int64"): m.amount_micros() + def test_int64_max_exact_accepted(self) -> None: + # Exact int64.max in USD_MICROCENTS — must still be accepted. + m = AP2Mandate( + transaction_id="tx", + amount_value="92233720368.54775807", + currency="USD", + payee_website="x.example", + ) + assert m.amount_micros() == 9_223_372_036_854_775_807 + + def test_int64_max_plus_one_rejected(self) -> None: + # P2 regression: the 19-digit cap permits this (also 19 digits), so the + # post-conversion int64 check is what catches it. Previous versions would + # have shipped 9_223_372_036_854_775_808 to the server. + m = AP2Mandate( + transaction_id="tx", + amount_value="92233720368.54775808", + currency="USD", + payee_website="x.example", + ) + with pytest.raises(AP2MandateError, match="int64"): + m.amount_micros() + + def test_19_digit_amount_above_int64_rejected(self) -> None: + # P2 regression: 11 integer 9s + 8 fractional 9s == 19 total digits — within + # the digit cap, but the resulting micros (9_999_999_999_999_999_999 ≈ 1e19) + # exceed int64.max (≈ 9.22e18). The post-conversion check rejects it. + m = AP2Mandate( + transaction_id="tx", + amount_value="99999999999.99999999", + currency="USD", + payee_website="x.example", + ) + with pytest.raises(AP2MandateError, match="int64"): + m.amount_micros() + class TestBuildReservationBody: def test_full_shape(self) -> None: From 05c2aaf2eb0f676d032f5c092e76cbf5a7fccd0e Mon Sep 17 00:00:00 2001 From: Albert Mavashev Date: Wed, 13 May 2026 09:56:20 -0400 Subject: [PATCH 6/7] fix: enforce int64 cap on set_actual_micros() override (P2 round 5) Round 4 added the int64 cap to AP2Mandate.amount_micros(), but the caller-supplied commit override on GuardedPayment.set_actual_micros() only rejected negative values. `guard.set_actual_micros(2**63)` would flow through unchecked and ship `actual.amount = 9223372036854775808` in the commit body. The server would reject it, but client-side we'd already have sent the wrong number. Mirror the same `0 <= amount <= MAX_USD_MICROS` validation as the mandate-derived path. Extracted `MAX_USD_MICROS = 2**63 - 1` to `_constants.py` so models.py and guard.py share the source of truth. Switched set_actual_micros() to raise `AP2MandateError` (a ValueError subclass) for both bounds, so all amount-validation errors are catchable via one exception type. Three regression tests: - int64.max accepted as a legal override - int64.max + 1 rejected; commit not called, release called - -1 rejected; commit not called, release called Test posture: 75 tests (up from 72), 98.23% coverage. ruff + mypy strict clean. `python -m build` produces sdist + wheel cleanly. Minor public API shift: set_actual_micros() now raises AP2MandateError instead of plain ValueError. Code catching ValueError still works. AUDIT.md and CHANGELOG.md updated. --- AUDIT.md | 13 ++++++++++ CHANGELOG.md | 5 ++-- runcycles_ap2/_constants.py | 5 ++++ runcycles_ap2/guard.py | 15 ++++++++--- runcycles_ap2/models.py | 8 +++--- tests/test_guard_clean_commit.py | 43 +++++++++++++++++++++++++++++++- 6 files changed, 78 insertions(+), 11 deletions(-) diff --git a/AUDIT.md b/AUDIT.md index 0d33a2f..7c3dbd2 100644 --- a/AUDIT.md +++ b/AUDIT.md @@ -2,6 +2,19 @@ Per `CLAUDE.md`: this file records material changes to the repo (server, admin, client). For a client package, that means public API, on-the-wire request shape, and protocol-conformance posture. +## 2026-05-13 — fifth-round review fix (P2 bypass via set_actual_micros) + +**Author:** code-review response on PR #1 (round 5) +**Scope:** correctness — int64 cap on the commit-path override + +**[P2] `set_actual_micros()` bypassed the int64 ceiling** — round 4 added the int64 cap to `AP2Mandate.amount_micros()`, but the caller-supplied commit override on `GuardedPayment.set_actual_micros()` only rejected negative values. Passing `2**63` flowed through to `build_commit_body` and into the wire payload as `actual.amount = 9223372036854775808`. **Fix:** mirror the same `0 <= amount <= MAX_USD_MICROS` validation; raise `AP2MandateError` (was plain `ValueError`) so all amount-validation errors are reachable via one exception type. Extracted `MAX_USD_MICROS = 2**63 - 1` to `_constants.py` as the shared source of truth between `models.py` and `guard.py`. Three regression tests cover int64.max acceptance, int64.max + 1 rejection (commit not called, release called), and the existing negative-amount path now raising `AP2MandateError`. + +**Test posture after fix:** +- 75 tests (up from 72), 98.23% coverage. + +**Public API change (very minor):** +- `set_actual_micros(amount)` now raises `AP2MandateError` (a `ValueError` subclass) instead of plain `ValueError`. Code catching `ValueError` still works. + ## 2026-05-13 — fourth-round review fix (P2 exact int64 boundary) **Author:** code-review response on PR #1 (round 4) diff --git a/CHANGELOG.md b/CHANGELOG.md index f9f8abe..6e65ecc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,8 +17,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - USD-only enforcement; non-USD raises `AP2CurrencyError`. Rejects NaN, +/-Infinity, and amounts with more than 8 decimal places (sub-micro precision); wraps all `decimal` failures as `AP2MandateError`. - Exact integer-tuple conversion in `amount_micros()` — does not depend on the default decimal context, so large valid inputs convert exactly instead of being silently rounded. - Pre-allocation digit-count cap (≤ 19 integer digits, the int64 USD_MICROCENTS ceiling) blocks exponent-notation DoS like `Decimal("1E+1000000000000")` that would otherwise hang allocating a trillion-digit scaling factor. -- Post-conversion int64 boundary check (`micros <= 2**63 - 1`) rejects amounts one over int64.max client-side instead of relying on the server. -- 72 tests, ≥ 95% coverage, ruff + mypy strict. +- Post-conversion int64 boundary check (`micros <= 2**63 - 1`) rejects amounts one over int64.max client-side instead of relying on the server. Applied symmetrically to `AP2Mandate.amount_micros()` and `GuardedPayment.set_actual_micros()` so a caller-supplied commit override cannot bypass the cap. +- `set_actual_micros()` raises `AP2MandateError` (a `ValueError` subclass) for both negative and over-int64 inputs. +- 75 tests, ≥ 95% coverage, ruff + mypy strict. ### Planned for v0.2 - `AsyncGuardedPayment` (asyncio). diff --git a/runcycles_ap2/_constants.py b/runcycles_ap2/_constants.py index 36caa10..dbd8cc4 100644 --- a/runcycles_ap2/_constants.py +++ b/runcycles_ap2/_constants.py @@ -36,3 +36,8 @@ # protocol cap, so the phase suffix (`reserve`/`commit`/`release:{ExcType}`) is always # preserved — which is what protects the consume-once defense. TRANSACTION_ID_HASH_LEN: Final[int] = 32 + +# Cycles `Amount.amount` is int64. Both mandate-derived amounts (`AP2Mandate.amount_micros`) +# and caller-supplied overrides (`GuardedPayment.set_actual_micros`) must respect this +# ceiling — otherwise we'd ship an out-of-range value the server is guaranteed to reject. +MAX_USD_MICROS: Final[int] = 2**63 - 1 # 9_223_372_036_854_775_807 diff --git a/runcycles_ap2/guard.py b/runcycles_ap2/guard.py index 2457d48..1119b9d 100644 --- a/runcycles_ap2/guard.py +++ b/runcycles_ap2/guard.py @@ -10,8 +10,8 @@ from runcycles.client import CyclesClient from runcycles.models import Decision, ReservationCreateResponse -from runcycles_ap2._constants import DEFAULT_ACTION_KIND, DEFAULT_OVERAGE_POLICY, DEFAULT_TTL_MS -from runcycles_ap2.exceptions import AP2DryRunResult, AP2GuardCommitFailed, AP2GuardDenied +from runcycles_ap2._constants import DEFAULT_ACTION_KIND, DEFAULT_OVERAGE_POLICY, DEFAULT_TTL_MS, MAX_USD_MICROS +from runcycles_ap2.exceptions import AP2DryRunResult, AP2GuardCommitFailed, AP2GuardDenied, AP2MandateError from runcycles_ap2.mapping import ( build_action, build_commit_body, @@ -108,9 +108,16 @@ def committed(self) -> bool: return self._committed def set_actual_micros(self, amount: int) -> None: - """Override the committed amount. Defaults to ``mandate.amount_micros()``.""" + """Override the committed amount. Defaults to ``mandate.amount_micros()``. + + The override must respect the same int64 ceiling that ``AP2Mandate.amount_micros`` + enforces — otherwise a caller could bypass the cap on the commit path and ship + an out-of-range value the server is guaranteed to reject. + """ if amount < 0: - raise ValueError("actual amount must be non-negative") + raise AP2MandateError("actual amount must be non-negative") + if amount > MAX_USD_MICROS: + raise AP2MandateError(f"actual amount {amount} exceeds int64 USD micro-cent max ({MAX_USD_MICROS})") self._actual_micros = amount def attach_receipt_fields(self, **fields: Any) -> None: diff --git a/runcycles_ap2/models.py b/runcycles_ap2/models.py index f29edce..2267dfd 100644 --- a/runcycles_ap2/models.py +++ b/runcycles_ap2/models.py @@ -12,6 +12,7 @@ from pydantic import BaseModel, ConfigDict, Field +from runcycles_ap2._constants import MAX_USD_MICROS from runcycles_ap2.exceptions import AP2CurrencyError, AP2MandateError # USD_MICROCENTS is 10^-8 USD; anything finer than 8 decimal places in the source string @@ -23,9 +24,8 @@ # any `10**shift` math — otherwise a short string like "1E+1000000000000" (a finite Decimal # with a 10^12 exponent) would allocate a trillion-digit integer and hang the process. # The digit cap is a coarse pre-allocation guard; the precise int64 boundary is enforced -# AFTER conversion via `_MAX_USD_MICROS`. +# AFTER conversion via `MAX_USD_MICROS`. _MAX_INTEGER_DIGITS = 19 -_MAX_USD_MICROS = 2**63 - 1 # 9_223_372_036_854_775_807 — Cycles Amount.amount int64 ceiling _CONFIG = ConfigDict(populate_by_name=True, extra="forbid", str_strip_whitespace=True) @@ -118,9 +118,9 @@ def amount_micros(self) -> int: # one over int64.max (e.g. "92233720368.54775808" → 9_223_372_036_854_775_808). # Enforce the exact protocol ceiling here so we fail-fast client-side rather # than waiting for the server to reject. - if micros > _MAX_USD_MICROS: + if micros > MAX_USD_MICROS: raise AP2MandateError( - f"amount_value {self.amount_value!r} exceeds int64 USD micro-cent max ({micros} > {_MAX_USD_MICROS})" + f"amount_value {self.amount_value!r} exceeds int64 USD micro-cent max ({micros} > {MAX_USD_MICROS})" ) return micros diff --git a/tests/test_guard_clean_commit.py b/tests/test_guard_clean_commit.py index 9563e5d..fd86fa4 100644 --- a/tests/test_guard_clean_commit.py +++ b/tests/test_guard_clean_commit.py @@ -2,7 +2,10 @@ from __future__ import annotations -from runcycles_ap2 import cycles_guard_payment +import pytest + +from runcycles_ap2 import AP2MandateError, cycles_guard_payment +from runcycles_ap2._constants import MAX_USD_MICROS from runcycles_ap2.mapping import idempotency_key from tests.conftest import allow_response, commit_success_response @@ -67,6 +70,44 @@ def test_emit_receipt_false_skips_receipt(self, mock_client, mandate) -> None: assert guard.receipt is None assert guard.committed is True + def test_set_actual_micros_at_int64_max_accepted(self, mock_client, mandate) -> None: + # Boundary: int64.max is a legal commit amount. + mock_client.create_reservation.return_value = allow_response() + mock_client.commit_reservation.return_value = commit_success_response() + + with cycles_guard_payment(mock_client, mandate=mandate, run_id="r", tenant="acme") as guard: + guard.set_actual_micros(MAX_USD_MICROS) + + body = mock_client.commit_reservation.call_args[0][1] + assert body["actual"]["amount"] == MAX_USD_MICROS + + def test_set_actual_micros_above_int64_rejected(self, mock_client, mandate) -> None: + # P2 regression: AP2Mandate.amount_micros enforces the int64 cap on + # mandate-derived amounts, but set_actual_micros() used to only reject + # negatives. A caller could pass 2**63 and we'd ship it. + from tests.conftest import release_success_response + + mock_client.create_reservation.return_value = allow_response() + mock_client.release_reservation.return_value = release_success_response() + + with pytest.raises(AP2MandateError, match="int64"): + with cycles_guard_payment(mock_client, mandate=mandate, run_id="r", tenant="acme") as guard: + guard.set_actual_micros(MAX_USD_MICROS + 1) + + # Raise inside the body → release on exit, no commit. + mock_client.commit_reservation.assert_not_called() + mock_client.release_reservation.assert_called_once() + + def test_set_actual_micros_negative_rejected(self, mock_client, mandate) -> None: + from tests.conftest import release_success_response + + mock_client.create_reservation.return_value = allow_response() + mock_client.release_reservation.return_value = release_success_response() + + with pytest.raises(AP2MandateError, match="non-negative"): + with cycles_guard_payment(mock_client, mandate=mandate, run_id="r", tenant="acme") as guard: + guard.set_actual_micros(-1) + def test_reserve_body_carries_policy_keys(self, mock_client, mandate) -> None: mock_client.create_reservation.return_value = allow_response() mock_client.commit_reservation.return_value = commit_success_response() From d04888f7f94db4bd89974da6994dfb21c3fd7de8 Mon Sep 17 00:00:00 2001 From: Albert Mavashev Date: Wed, 13 May 2026 10:02:12 -0400 Subject: [PATCH 7/7] fix: reject non-int amount inputs in set_actual_micros + build_commit_body (P2 round 6) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit bool is a subclass of int in Python (isinstance(True, int) is True), and float < int compares numerically. So set_actual_micros(True) and set_actual_micros(1.5) both slipped past the round-5 numerical bounds and would ship `true` / `1.5` as `actual.amount` on the wire. The 1.5 case only surfaced as a pydantic error during receipt construction — *after* the commit POST had already gone out, which is a much worse failure mode than a clean client-side reject. Add a private `runcycles_ap2._validation.validate_micros` helper that uses `type(amount) is int` (intentionally NOT isinstance) to reject bool and everything else non-int. Apply it in two places that share the failure mode: - GuardedPayment.set_actual_micros() — caller-facing override - mapping.build_commit_body(actual_micros=...) — direct-builder path 14 new tests cover True / False / 1.5 / 1.0 / "100" / None / [100] across both entry points, plus the existing negative + over-int64 paths via the shared helper. Test posture: 90 tests (up from 75), 98.29% coverage. ruff + mypy strict clean. `python -m build` produces sdist + wheel cleanly. New internal module: runcycles_ap2/_validation.py (private, not exported). No public API additions. AUDIT.md and CHANGELOG.md updated. --- AUDIT.md | 13 +++++++++++++ CHANGELOG.md | 5 +++-- runcycles_ap2/_validation.py | 30 ++++++++++++++++++++++++++++++ runcycles_ap2/guard.py | 18 ++++++++---------- runcycles_ap2/mapping.py | 13 +++++++++++-- tests/test_guard_clean_commit.py | 16 ++++++++++++++++ tests/test_mapping.py | 16 ++++++++++++++++ 7 files changed, 97 insertions(+), 14 deletions(-) create mode 100644 runcycles_ap2/_validation.py diff --git a/AUDIT.md b/AUDIT.md index 7c3dbd2..7d6cc18 100644 --- a/AUDIT.md +++ b/AUDIT.md @@ -2,6 +2,19 @@ Per `CLAUDE.md`: this file records material changes to the repo (server, admin, client). For a client package, that means public API, on-the-wire request shape, and protocol-conformance posture. +## 2026-05-13 — sixth-round review fix (P2 non-int amount types) + +**Author:** code-review response on PR #1 (round 6) +**Scope:** correctness — exact-type validation on commit-amount inputs + +**[P2] `set_actual_micros` and `build_commit_body` accepted non-int types** — the round-5 fix bounded numerical comparisons, but `bool` is an `int` subclass (`isinstance(True, int) is True`) and `float < int` compares numerically. `set_actual_micros(True)` would ship `true` as `actual.amount`; `set_actual_micros(1.5)` would ship `1.5` and only surface as a pydantic error during receipt construction — *after* the commit POST had already gone out. **Fix:** new private `runcycles_ap2._validation.validate_micros(amount, *, field)` helper using `type(amount) is int` to reject `bool` (and everything else non-int). Wired into both `GuardedPayment.set_actual_micros` AND `mapping.build_commit_body` so direct callers of the builder get the same protection. 14 new tests cover `True`/`False`/`1.5`/`1.0`/`"100"`/`None`/`[100]` across both entry points. + +**Test posture after fix:** +- 90 tests (up from 75), 98.29% coverage. + +**New internal module:** +- `runcycles_ap2/_validation.py` (private; not exported) + ## 2026-05-13 — fifth-round review fix (P2 bypass via set_actual_micros) **Author:** code-review response on PR #1 (round 5) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e65ecc..a2f074a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,8 +18,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Exact integer-tuple conversion in `amount_micros()` — does not depend on the default decimal context, so large valid inputs convert exactly instead of being silently rounded. - Pre-allocation digit-count cap (≤ 19 integer digits, the int64 USD_MICROCENTS ceiling) blocks exponent-notation DoS like `Decimal("1E+1000000000000")` that would otherwise hang allocating a trillion-digit scaling factor. - Post-conversion int64 boundary check (`micros <= 2**63 - 1`) rejects amounts one over int64.max client-side instead of relying on the server. Applied symmetrically to `AP2Mandate.amount_micros()` and `GuardedPayment.set_actual_micros()` so a caller-supplied commit override cannot bypass the cap. -- `set_actual_micros()` raises `AP2MandateError` (a `ValueError` subclass) for both negative and over-int64 inputs. -- 75 tests, ≥ 95% coverage, ruff + mypy strict. +- `set_actual_micros()` raises `AP2MandateError` (a `ValueError` subclass) for both negative and over-int64 inputs, plus any non-int type (notably `bool` and `float`, which numerical comparisons would otherwise let slip through). +- Direct callers of `mapping.build_commit_body(actual_micros=...)` get the same input validation via a shared private validator. +- 90 tests, ≥ 95% coverage, ruff + mypy strict. ### Planned for v0.2 - `AsyncGuardedPayment` (asyncio). diff --git a/runcycles_ap2/_validation.py b/runcycles_ap2/_validation.py new file mode 100644 index 0000000..7b17630 --- /dev/null +++ b/runcycles_ap2/_validation.py @@ -0,0 +1,30 @@ +"""Internal shared validators. + +Kept private so the public API stays minimal. These are imported from ``guard.py`` +and ``mapping.py`` to enforce the same input rules on both the wrapper-level and +builder-level commit-amount paths. +""" + +from __future__ import annotations + +from runcycles_ap2._constants import MAX_USD_MICROS +from runcycles_ap2.exceptions import AP2MandateError + + +def validate_micros(amount: object, *, field: str) -> int: + """Validate that ``amount`` is a non-negative ``int`` within the int64 ceiling. + + Uses ``type(amount) is int`` rather than ``isinstance(amount, int)`` deliberately: + ``bool`` is a subclass of ``int``, and we do not want ``set_actual_micros(True)`` + to slip through and ship ``true`` over the wire. ``float`` is also rejected — + pydantic would later raise during receipt construction (after the commit POST + has already gone out), and that post-hoc failure mode is much harder to + reconcile than a clean client-side reject. + """ + if type(amount) is not int: + raise AP2MandateError(f"{field} must be an int, got {type(amount).__name__}: {amount!r}") + if amount < 0: + raise AP2MandateError(f"{field} must be non-negative, got {amount}") + if amount > MAX_USD_MICROS: + raise AP2MandateError(f"{field} {amount} exceeds int64 USD micro-cent max ({MAX_USD_MICROS})") + return amount diff --git a/runcycles_ap2/guard.py b/runcycles_ap2/guard.py index 1119b9d..f43b051 100644 --- a/runcycles_ap2/guard.py +++ b/runcycles_ap2/guard.py @@ -10,8 +10,9 @@ from runcycles.client import CyclesClient from runcycles.models import Decision, ReservationCreateResponse -from runcycles_ap2._constants import DEFAULT_ACTION_KIND, DEFAULT_OVERAGE_POLICY, DEFAULT_TTL_MS, MAX_USD_MICROS -from runcycles_ap2.exceptions import AP2DryRunResult, AP2GuardCommitFailed, AP2GuardDenied, AP2MandateError +from runcycles_ap2._constants import DEFAULT_ACTION_KIND, DEFAULT_OVERAGE_POLICY, DEFAULT_TTL_MS +from runcycles_ap2._validation import validate_micros +from runcycles_ap2.exceptions import AP2DryRunResult, AP2GuardCommitFailed, AP2GuardDenied from runcycles_ap2.mapping import ( build_action, build_commit_body, @@ -110,15 +111,12 @@ def committed(self) -> bool: def set_actual_micros(self, amount: int) -> None: """Override the committed amount. Defaults to ``mandate.amount_micros()``. - The override must respect the same int64 ceiling that ``AP2Mandate.amount_micros`` - enforces — otherwise a caller could bypass the cap on the commit path and ship - an out-of-range value the server is guaranteed to reject. + Raises :class:`AP2MandateError` if ``amount`` is not a plain ``int`` (``bool`` + and ``float`` are explicitly rejected — they would otherwise reach the wire + payload and only surface as a post-commit pydantic failure during receipt + construction), is negative, or exceeds the int64 USD_MICROCENTS ceiling. """ - if amount < 0: - raise AP2MandateError("actual amount must be non-negative") - if amount > MAX_USD_MICROS: - raise AP2MandateError(f"actual amount {amount} exceeds int64 USD micro-cent max ({MAX_USD_MICROS})") - self._actual_micros = amount + self._actual_micros = validate_micros(amount, field="actual amount") def attach_receipt_fields(self, **fields: Any) -> None: """Attach caller-supplied fields (e.g. PSP reference id) to the commit metadata.""" diff --git a/runcycles_ap2/mapping.py b/runcycles_ap2/mapping.py index 793c481..c361d36 100644 --- a/runcycles_ap2/mapping.py +++ b/runcycles_ap2/mapping.py @@ -21,6 +21,7 @@ IDEMPOTENCY_PREFIX, TRANSACTION_ID_HASH_LEN, ) +from runcycles_ap2._validation import validate_micros from runcycles_ap2.models import AP2Mandate @@ -173,8 +174,16 @@ def build_commit_body( actual_micros: int | None = None, metadata: dict[str, Any] | None = None, ) -> dict[str, Any]: - """Commit body with deterministic idempotency key derived from ``transaction_id``.""" - amount = actual_micros if actual_micros is not None else mandate.amount_micros() + """Commit body with deterministic idempotency key derived from ``transaction_id``. + + ``actual_micros`` is validated when supplied (rejects ``bool``, ``float``, and + out-of-range ints) so direct callers of this builder get the same protection as + :meth:`GuardedPayment.set_actual_micros`. + """ + if actual_micros is None: + amount = mandate.amount_micros() + else: + amount = validate_micros(actual_micros, field="actual_micros") body: dict[str, Any] = { "idempotency_key": idempotency_key(mandate.transaction_id, "commit"), "actual": {"unit": "USD_MICROCENTS", "amount": amount}, diff --git a/tests/test_guard_clean_commit.py b/tests/test_guard_clean_commit.py index fd86fa4..49458c9 100644 --- a/tests/test_guard_clean_commit.py +++ b/tests/test_guard_clean_commit.py @@ -108,6 +108,22 @@ def test_set_actual_micros_negative_rejected(self, mock_client, mandate) -> None with cycles_guard_payment(mock_client, mandate=mandate, run_id="r", tenant="acme") as guard: guard.set_actual_micros(-1) + @pytest.mark.parametrize("bad", [True, False, 1.5, 1.0, "100", None, [100]]) + def test_set_actual_micros_rejects_non_int_types(self, mock_client, mandate, bad) -> None: + # P2 regression: bool is a subclass of int and float compares numerically, so + # the bound check passes for True/1.5 and the value would flow to the wire. + # set_actual_micros must reject anything that isn't a plain int up front. + from tests.conftest import release_success_response + + mock_client.create_reservation.return_value = allow_response() + mock_client.release_reservation.return_value = release_success_response() + + with pytest.raises(AP2MandateError, match="must be an int"): + with cycles_guard_payment(mock_client, mandate=mandate, run_id="r", tenant="acme") as guard: + guard.set_actual_micros(bad) # type: ignore[arg-type] + + mock_client.commit_reservation.assert_not_called() + def test_reserve_body_carries_policy_keys(self, mock_client, mandate) -> None: mock_client.create_reservation.return_value = allow_response() mock_client.commit_reservation.return_value = commit_success_response() diff --git a/tests/test_mapping.py b/tests/test_mapping.py index 1f0ca2e..4b6a688 100644 --- a/tests/test_mapping.py +++ b/tests/test_mapping.py @@ -380,6 +380,22 @@ def test_metadata_attached(self) -> None: body = build_commit_body(make_mandate(), metadata={"psp_ref": "psp_1"}) assert body["metadata"]["psp_ref"] == "psp_1" + @pytest.mark.parametrize("bad", [True, False, 1.5, 1.0, "100", [100]]) + def test_actual_micros_rejects_non_int_types(self, bad) -> None: + # P2 regression: build_commit_body is called directly by some callers (and by + # the guard); it must reject bool/float/str up front rather than letting the + # bad value reach the wire and only fail at receipt-construction time. + with pytest.raises(AP2MandateError, match="must be an int"): + build_commit_body(make_mandate(), actual_micros=bad) + + def test_actual_micros_negative_rejected(self) -> None: + with pytest.raises(AP2MandateError, match="non-negative"): + build_commit_body(make_mandate(), actual_micros=-1) + + def test_actual_micros_above_int64_rejected(self) -> None: + with pytest.raises(AP2MandateError, match="int64"): + build_commit_body(make_mandate(), actual_micros=2**63) + class TestBuildReleaseBody: def test_with_exception_type(self) -> None: