feat(x402): adopt settle-first ordering (stacks on #563) - #564
Closed
raahulrahl wants to merge 5 commits into
Closed
feat(x402): adopt settle-first ordering (stacks on #563)#564raahulrahl wants to merge 5 commits into
raahulrahl wants to merge 5 commits into
Conversation
PR #563 closed the artifact-leak half of issue #562 (failed settle no longer delivered the artifact) but agents still ate the LLM cost on every failed settle — a payer who drained their wallet after verify, or who lost a parallel-nonce settle race, still triggered an LLM call. This PR moves _settle_payment to BEFORE manifest.run, so a failed settlement costs the agent zero LLM tokens: - run_task now calls _settle_payment immediately after extracting payment_context and before transitioning the task to "working". Settle-fail → _handle_settlement_failure with full recovery metadata → return. Manifest never runs. - _handle_terminal_state takes settlement_metadata (pre-settled receipts) instead of payment_context; the redundant post-execute gate from #563 is removed. - _handle_task_failure accepts the settlement_metadata and tags the payment as "payment-orphaned" when work raises after a successful settle. x402 has no refund primitive, so the EIP-3009 fields are persisted for the operator to issue a manual transfer back. Wall-clock latency on the happy path is unchanged — settle (2-5s on Base) just moves from after the LLM call to before it. This is the same choice Google's A2A x402 extension makes for the same reason; Coinbase's x402-express middleware uses the opposite ordering, but that's tuned for sub-second API endpoints, not for agent workloads where the verify-vs-settle gap can span minutes. Three new tests cover the new contract: - settle-fail must not invoke manifest.run - settle must complete before manifest.run starts (call-order assertion) - work failure after successful settle persists payment-orphaned metadata Two tests from #563 (test_settle_failure_does_not_deliver_artifact, test_settle_success_unchanged) are removed — they exercised the post-execute gate that no longer exists. The new end-to-end settle-first tests subsume them. Stacks on #563. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
A single integration test file that drives the real worker pipeline (ManifestWorker + InMemoryStorage + InMemoryNonceStore) against a mocked facilitator for each scenario from the #562 discussion: 1. Front-run drain — settle returns failure → manifest.run not called 2. Settle timeout — settle raises → recovery metadata persisted 3. Parallel-nonce double-spend — first task settles & runs, second fails settle and burns zero LLM tokens 4. Replay — middleware rejects identical nonce on the second request Scenarios 1-3 hit the worker directly with a realistic payment_context dict (the same shape the middleware produces). Scenario 4 goes through Starlette TestClient and the real X402Middleware to verify the middleware-level defense is intact. Each test prints a narrative trace so `pytest -s` reads like a walkthrough — useful when explaining the fix to reviewers or auditing post-merge behavior. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…2E demo
Two changes:
1. Fix latent loguru misuse in _settle_payment.
``logger.error(f"Error settling payment: {e}", exc_info=True)`` eagerly
interpolates the exception into the message string, then passes that
string to loguru — which re-parses it for ``{placeholder}`` templates.
When the exception text contains JSON (every x402 SDK error does),
loguru raises ``KeyError`` trying to resolve ``{success}`` / ``{error}``,
masking the original error and skipping the recovery-metadata return.
Switched both error logs to loguru's native templating:
``logger.error("Payment settlement failed: {}", error_reason)``
``logger.opt(exception=True).error("Error settling payment: {}", e)``
Positional values are inserted literally — braces inside them are
never re-parsed.
This was already in the codebase before settle-first; the E2E demo
below is what surfaced it.
2. Add a runnable subprocess E2E demo for all four #562 scenarios.
``tests/e2e/x402_scenarios/`` contains:
- ``mock_facilitator.py`` — programmable fake that keys failure modes
off the EIP-3009 nonce prefix (0xfa11 = settle-reverted,
0xcdcd = facilitator timeout)
- ``agent.py`` — minimal Bindu echo agent behind an x402 paywall,
pointed at the mock facilitator via ``X402__FACILITATOR_URL``
- ``run_e2e.py`` — driver that spawns both subprocesses, fires real
HTTP requests for each scenario, and prints the observed task
state, metadata, and recovery fields
Run with: ``uv run python tests/e2e/x402_scenarios/run_e2e.py``
Unlike the existing unit/integration tests (which mock at the worker
boundary), this demo drives the whole stack — HTTP → X402Middleware
→ scheduler → ManifestWorker → real facilitator client — exactly as
a real deployment would.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PAYMENT.md already had a "Watching it work" section with an inline fake facilitator, but it only covered the happy path. The new tests/e2e/x402_scenarios/ harness covers all four #562 failure modes end-to-end with a single command — surface it from the docs so operators don't have to discover it from the PR history. Also adds manifest_worker.py and the E2E directory to the "Where to look in the code" pointer list. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…re message Per CodeRabbit review on #563: the failure message included the raw ``error_text`` from the facilitator (e.g. internal HTTP 500 body, JSON payloads). That can disclose facilitator topology or internal state to the caller — and the caller already gets a clear "task failed" signal plus the structured metadata fields they need. The full reason still lives in ``task.metadata["x402.payment.error"]`` for operator audit; it's just no longer echoed in the agent message that goes back over the wire. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
_settle_paymentto beforemanifest.run.Why this ordering
Researched x402 reference implementations before picking the order. Two camps:
Bindu's workload is agent tasks. Following A2A's lead. Coinbase's buffer-then-settle pattern works for 200ms endpoints but leaves a verify-vs-settle window of seconds-to-minutes wide open for agent workloads — exactly the gap exploited in #562.
Latency on the happy path is unchanged: settle (2-5s on Base) just moves from after the LLM call to before it. Total wall-clock is identical.
Behavior change
payment-orphanedwith full EIP-3009 fields for manual refund.Orphan payment — the new failure mode
Settle-first introduces a symmetric risk: settle succeeds, then
manifest.runraises (LLM provider outage, agent code bug). The payer paid, got nothing. x402 has no refund primitive, so this PR does NOT auto-refund — it persists the EIP-3009 fields and tags the paymentpayment-orphaned. Operator responsibility.Documented in docs/PAYMENT.md. The expectation is orphan payments are rare; each one is a real bug to investigate.
Test plan
uv run pytest tests/unit/server/workers/test_manifest_worker.py— 32 passed (3 new + updated existing).uv run pytest tests/unit/server/ tests/unit/extensions/x402/— 427 passed.uv run ruff check— clean.Three new tests are the contract:
test_settle_first_failure_does_not_invoke_manifest— failed settle →manifest.runwas not called.test_settle_first_runs_manifest_only_after_settle— strict call-order assertion: settle first, then manifest.test_work_failure_after_settle_persists_orphan_payment— work raises after successful settle → task carriespayment-orphanedmetadata.Removed two #563 tests (
test_settle_failure_does_not_deliver_artifact,test_settle_success_unchanged) — they exercised the post-execute gate that no longer exists. Replaced with stronger end-to-end coverage.What this still does NOT fix
🤖 Generated with Claude Code