docs(agentic-payments): production patterns for x402 + MPP - #97
Conversation
|
🤖 Automated message from Kaan's Automated Triage Bot. 👀 Picked this up — a review will follow shortly. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds practical documentation for configuring paid routes with x402 and for running MPP Charge/Session together in production without crashing on missing/mis-set secrets.
Changes:
- Document
paymentMiddlewareFromConfigfor pricing multiple x402 routes with per-route prices. - Add “Production patterns” guidance for MPP (fail-open recipient resolution, optional dual-intent setup, and an
/infodiscovery endpoint).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| skills/agentic-payments/x402.md | Adds multi-route pricing example using paymentMiddlewareFromConfig and guidance to fail open when recipient config is missing. |
| skills/agentic-payments/mpp.md | Adds production-ready patterns and example code for resilient configuration and runtime discovery. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| ## Pricing multiple routes with `paymentMiddlewareFromConfig` | ||
|
|
||
| The seller example above prices a single route through `paymentMiddleware` + |
| **Env vars (server):** `CHANNEL_CONTRACT`, `COMMITMENT_PUBKEY`, `MPP_SECRET_KEY`, `FEE_PAYER_SECRET` | ||
| **Env vars (client):** `COMMITMENT_SECRET` |
| process.env.MPP_CHANNEL_CONTRACT && | ||
| process.env.MPP_COMMITMENT_KEY && | ||
| RECIPIENT && | ||
| process.env.MPP_SECRET_KEY | ||
| ) | ||
| ? Mppx.create({ methods: [stellarChannel.channel({ channel: process.env.MPP_CHANNEL_CONTRACT, /* ... */ })] }) |
| const chargeMppx = (RECIPIENT && process.env.MPP_SECRET_KEY) | ||
| ? Mppx.create({ methods: [stellar.charge({ recipient: RECIPIENT, /* ... */ })] }) | ||
| : null; | ||
|
|
||
| const sessionMppx = ( | ||
| process.env.MPP_CHANNEL_CONTRACT && | ||
| process.env.MPP_COMMITMENT_KEY && | ||
| RECIPIENT && | ||
| process.env.MPP_SECRET_KEY | ||
| ) | ||
| ? Mppx.create({ methods: [stellarChannel.channel({ channel: process.env.MPP_CHANNEL_CONTRACT, /* ... */ })] }) | ||
| : null; |
- x402.md: drop a stray "+" line-join artifact in the multi-route pricing intro. - mpp.md: align the pre-existing server env var names to what the service actually reads (MPP_CHANNEL_CONTRACT / MPP_COMMITMENT_KEY, verified against packages/agent/src/middleware/mpp.ts) instead of weakening the new examples to match the wrong CHANNEL_CONTRACT / COMMITMENT_PUBKEY names already in the doc. - mpp.md: add the missing imports to the dual-intent snippet, and note why Channel's server adapter needs an alias (`stellarChannel`) — both it and Charge's export their namespace as `stellar`. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Addressed Copilot's 4 review comments:
|
Adds three patterns verified against a service that has been billing real USDC over MPP Charge and x402 in production: - Multi-route pricing with paymentMiddlewareFromConfig (x402.md) - Recipient resolution that fails open instead of crashing on missing or misconfigured STELLAR_RECIPIENT, including recovery when a secret key lands in the public-key env var (mpp.md) - Optional dual-intent server: Charge and Session gated independently by their own env vars, each middleware no-op'ing rather than throwing when its intent isn't configured (mpp.md) - A runtime-accurate /info discovery endpoint reporting which intents are actually live, not a static capability list (mpp.md) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- x402.md: drop a stray "+" line-join artifact in the multi-route pricing intro. - mpp.md: align the pre-existing server env var names to what the service actually reads (MPP_CHANNEL_CONTRACT / MPP_COMMITMENT_KEY, verified against packages/agent/src/middleware/mpp.ts) instead of weakening the new examples to match the wrong CHANNEL_CONTRACT / COMMITMENT_PUBKEY names already in the doc. - mpp.md: add the missing imports to the dual-intent snippet, and note why Channel's server adapter needs an alias (`stellarChannel`) — both it and Charge's export their namespace as `stellar`. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
cdf7caa to
16c2d92
Compare
|
@kaankacar the merge conflict on this one is resolved — rebased onto current Separately, #103 is small and unrelated — a real bug, not a docs addition: 27 of 28 Whenever you get to either, no rush on my end. |
|
🤖 Automated message from Kaan's Automated Triage Bot. Thanks for this — the fail-open recipient pattern is genuinely good, and the I checked the new API claims against the published packages and they hold up: the Two things before this can land: 1. 2. The Nirium mention needs a maintainer. The x402 section points at "Nirium's own mainnet endpoint", and you own Small suggestion: Copilot's four earlier points all look correctly addressed. |
…oute example Real bug in the "Pricing multiple routes with paymentMiddlewareFromConfig" example: description was nested inside accepts for all three routes (GET /signals, GET /market, POST /execute), contradicting the single-route GET /weather example just above it in the same file, where description is already a sibling of accepts. Verified against the real installed @x402/core@2.22.0 types before fixing, not assumed from the existing example alone: PaymentOption (what accepts holds) declares scheme/payTo/price/network/ maxTimeoutSeconds/extra only, no description field at all. RouteConfig itself declares description as a top-level, sibling field. A description nested inside accepts is silently never read by the SDK. Moved all three description fields one level out to match the real type and the existing correct example. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
🤖 Automated message from Kaan's Automated Triage Bot. Thanks for the quick turnaround — the fix is exactly right. I checked This push changed only those three lines, so the earlier verification still stands. One item is left, and it is not yours to fix. The x402 section credits Nirium's mainnet endpoint, and you own |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
Suppressed comments (3)
skills/agentic-payments/x402.md:168
- Failing open here turns a missing production secret into a payment bypass: every configured paid route can reach its handler without an x402 challenge. Keep process startup resilient, but install a guard that returns 503 for paid routes until the recipient is configured instead of calling
next().
**Fail open, not crash, when the recipient isn't configured.** A server
that throws at boot because `STELLAR_RECIPIENT` is unset breaks CI and
any environment that hasn't provisioned secrets yet. See
[Recipient resolution](mpp.md#recipient-resolution-fail-open-not-crash)
in mpp.md for the fuller pattern — it applies here too: wrap middleware
initialization in a check, and fall through to `next()` when the
recipient is missing instead of throwing.
skills/agentic-payments/mpp.md:254
- Non-
S...input is returned without validation, so a typo, aC...address, or any arbitrary nonempty value still reaches the SDK and can throw—the failure mode this helper claims to prevent. Validate the public key in this branch and disable MPP on parse failure too.
return raw;
skills/agentic-payments/mpp.md:288
- The dual-intent guidance repeats the same fail-open payment bypass: a null instance must not let a paid handler run unpriced. Route middleware should return 503 for that intent while leaving unrelated public routes available.
level. Give each mode its own `Mppx` instance, initialize it only when
its full config is present, and let route middleware no-op — not
throw — when the instance for that intent is `null`. Charge's and
|
|
||
| **`payTo` is the recipient's classic Stellar account (`G...`), not the USDC SAC contract address.** Sending USDC lands in the classic balance of the `payTo` account, which is why that account also needs a USDC trustline. The SAC contract address is what the protocol invokes `transfer` on; see [Two USDC addresses](SKILL.md#two-usdc-addresses-dont-confuse-them) in the router. | ||
|
|
||
| ## Pricing multiple routes with `paymentMiddlewareFromConfig` |
| channel: process.env.CHANNEL_CONTRACT, // C... contract address | ||
| commitmentKey: process.env.COMMITMENT_PUBKEY, // 64-char hex ed25519 public key | ||
| channel: process.env.MPP_CHANNEL_CONTRACT, // C... contract address | ||
| commitmentKey: process.env.MPP_COMMITMENT_KEY, // 64-char hex ed25519 public key |
| if (!chargeMppx) { | ||
| res.setHeader("X-MPP-Warning", "MPP not configured on this server"); | ||
| return next(); // route still responds — unpriced, not broken | ||
| } |
| const sessionMppx = ( | ||
| process.env.MPP_CHANNEL_CONTRACT && | ||
| process.env.MPP_COMMITMENT_KEY && | ||
| RECIPIENT && | ||
| process.env.MPP_SECRET_KEY | ||
| ) |
| **Env vars (server):** `MPP_CHANNEL_CONTRACT`, `MPP_COMMITMENT_KEY`, `MPP_SECRET_KEY`, `FEE_PAYER_SECRET` | ||
| **Env vars (client):** `COMMITMENT_SECRET` | ||
|
|
||
| ## Production patterns |
| Not the OpenAPI discovery document below — this is a lighter, unauthenticated | ||
| health check specific to this server's own deployment. A client (human or | ||
| agent) shouldn't have to guess which intents are live. Report the true | ||
| runtime state, not a static capability list — `enabled` reflects whether | ||
| the instance actually initialized, which is also a live health check: |
…atterns Copilot's review on stellar#97 found what turned out to be a real bug, not a docs nit: the production-pattern examples in mpp.md taught fail-OPEN per-request behavior (next() when the payment instance is null), which lets a misconfigured or later-rotated deployment serve a paid route's content for free with only a warning header most clients never read. Verified against packages/agent/src/middleware/mpp.ts (the real service this doc says it's drawn from) before touching anything here: the real code had the identical bug, now fixed there to fail closed (503). This commit brings the doc back in sync with that fix, and separates two failure surfaces the old single heading conflated: - Booting: never throw at import time on a missing/misconfigured STELLAR_RECIPIENT — unchanged, this part was already correct. - Billing: once the server is up, a null payment instance must return a non-200 (503) per request, never fall through to the route's normal, unpriced 200. Renamed the two affected headings accordingly and updated the fail-open prose in both mpp.md sections plus the /500/503 reference in the /info paragraph. Fixed the now-stale anchor link from x402.md's own 'Fail open, not crash' cross-reference, which still correctly describes x402.ts's actual current behavior (unchanged in this PR — flagging that gap for a separate fix, out of scope here) and only needed its link target updated. Also renamed /info from 'health check' to 'configuration-status endpoint' — Copilot correctly pointed out it only reports whether each Mppx instance was constructed at startup, not whether the RPC, facilitator, or store backend it depends on are actually reachable right now. Calling that a health check overpromises. Adds the two evals/ scenarios this change was missing per README.md's own contribution rule ('update or add the matching scenario under evals/ in the same PR'): 04-multi-route-pricing.json for paymentMiddlewareFromConfig, and 05-production-hardening.json for the three mpp.md patterns, both asserting the fail-closed behavior above. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
Suppressed comments (4)
skills/agentic-payments/mpp.md:147
stellar.channelexpectscommitmentKeyto be a StellarG...public key (or aKeypair), but this example documents and passes raw 64-character hex. A user following it will reach runtime signature-verification failures. Require the environment variable to contain the encodedG...key, or explicitly convert the hex withStrKey.encodeEd25519PublicKey.
commitmentKey: process.env.MPP_COMMITMENT_KEY, // 64-char hex ed25519 public key
skills/agentic-payments/mpp.md:312
- The initializer checks
MPP_SECRET_KEYbut never passes it toMppx.create, so the configured verification key is ignored. This also contradicts both complete server examples above, which setsecretKeyat theMppx.createlevel.
const chargeMppx = (RECIPIENT && process.env.MPP_SECRET_KEY)
? Mppx.create({ methods: [stellar.charge({ recipient: RECIPIENT, /* ... */ })] })
: null;
skills/agentic-payments/mpp.md:320
- Session is not independently gated here: it unnecessarily requires
RECIPIENT, even though the standalone Session configuration andstellarChannel.channeldo not use that value. It also checksMPP_SECRET_KEYwithout supplying it toMppx.create. A valid Session-only deployment would therefore remain disabled, while its configured verification key would be ignored.
RECIPIENT &&
process.env.MPP_SECRET_KEY
)
? Mppx.create({ methods: [stellarChannel.channel({ channel: process.env.MPP_CHANNEL_CONTRACT, /* ... */ })] })
skills/agentic-payments/x402.md:168
- Falling through to
next()when a priced route's recipient is missing serves the protected response without payment. That is a billing bypass caused by configuration failure; keep startup resilient, but fail closed with a non-200 response for matched paid routes.
initialization in a check, and fall through to `next()` when the
recipient is missing instead of throwing.
| } | ||
| return raw; | ||
| } |
| res.status(503).json({ error: "MPP charge unavailable — payment middleware not initialized" }); | ||
| return; | ||
| } | ||
| // ... normal charge flow |
| by choice — it requires deploying and funding a channel contract per | ||
| deployment, a step that carries custody implications worth a compliance | ||
| pass before turning on for a given business. Session works the same way | ||
| Charge does once its four env vars are set; nothing in the server code |
| app.use( | ||
| paymentMiddlewareFromConfig( | ||
| PAID_ROUTES, | ||
| facilitator, | ||
| [{ network: NETWORK, server: new ExactStellarScheme() }], | ||
| { appName: "My API", testnet: NETWORK === "stellar:testnet" }, | ||
| ), | ||
| ); |
| Not the OpenAPI discovery document below, and not a health check either — | ||
| call it that and a caller will expect it to confirm the Soroban RPC, the | ||
| facilitator, and the store backend it depends on are actually reachable | ||
| right now. It doesn't: it only reports whether each intent's `Mppx` | ||
| instance was constructed at startup, which is configuration state, not |
| verified against a service that's been billing real USDC over MPP Charge | ||
| in production since before this skill existed. | ||
|
|
||
| ### Recipient resolution (recover at boot, fail closed per request) |
|
🤖 Automated message from Kaan's Automated Triage Bot. Thanks for this — flipping to fail closed is the right call, and the reasoning you wrote into the code comment is clearer than most production code gets. I checked Three things I would like you to look at. 1. 2. Session is gated on a variable the file never lists. In the dual-intent snippet, Copilot asked you to drop the check. I disagree. 3. Smaller point, your call: The Nirium item is unchanged. The x402 section credits Nirium's mainnet endpoint, and you own |
Four gaps the bot's review of the previous commit (23c7988) found, all real, all checked against the actual service and library types before touching anything. 1. x402.md's own "Fail open, not crash" callout still taught the old pattern after mpp.md's matching section was already renamed and rewritten to fail-closed in the previous commit — the anchor link was fixed then, but the prose it points at wasn't, so the link and the text it introduced contradicted each other. Rewritten to describe the real pattern: never throw at boot, but return 503 (not next()) per request once the payment instance is null. This now matches packages/agent/src/middleware/x402.ts on the private side, fixed in the same session this doc change responds to. 2. STELLAR_RECIPIENT was missing from Session mode's own env var list, even though the Session server example requires it as a precondition. Added it, and wired an explicit recipient (plus currency) into both the Session server example and the dual-intent snippet's channel() call — checked against @stellar/mpp/dist/channel/server/Channel.d.ts first: both fields are optional but the library's own doc comment calls them "strongly recommended," specifically because the channel contract is deployed out-of-band and these are what let the library reject a channel whose payout address or token don't match what was expected. 3. The pre-existing Session-mode walkthrough server example (not the newer production-patterns section) had the identical commitmentKey bug already fixed there: MPP_COMMITMENT_KEY is stored as hex, but stellar.channel()'s commitmentKey expects a Stellar G... address (or a Keypair), never raw bytes. Fixed with the same StrKey.encodeEd25519PublicKey() re-encoding, and applied the same fix to the dual-intent snippet's previously-elided commitmentKey. 4. The testnet runbook generated the client's seed (COMMITMENT_SECRET) but never showed how the server's MPP_COMMITMENT_KEY — the public key derived from that same seed — actually gets produced. Added the derivation command and labeled which env var belongs to which side (client holds the seed, server holds the derived public key), since items 2-3 above depend on readers understanding that distinction. evals/05-production-hardening.json gained two more expected_behavior assertions covering the commitmentKey conversion and the recipient/currency wiring, since both are now part of what this scenario's pattern actually teaches. Everything here is a doc-only change; no code in this repo runs, so nothing to re-test beyond re-reading the diff against the library types and the private service's real source, which is what was done before each edit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
skills/agentic-payments/mpp.md:273
- Non-
S...values are returned without validation.stellar.charge()accepts the string during construction, so a malformed recipient leaveschargeMppxtruthy and/inforeports Charge enabled instead of disabling it and serving the intended 503. Validate the requiredG...public key before returning it.
return raw;
skills/agentic-payments/mpp.md:297
- When
chargeMppxexists, this branch neither invokes the payment handler nor callsnext()or sends a response, so every correctly configured paid request hangs. Delegate to the instance's Express handler here.
// ... normal charge flow
skills/agentic-payments/mpp.md:336
- Checking only that
MPP_COMMITMENT_KEYis nonempty does not make optional Session initialization fail-safe: malformed/non-64-character hex throws during key encoding or channel construction, crashing the whole Charge server beforesessionMppxcan becomenull. Catch initialization failures and keep only Session disabled as the section promises.
const sessionMppx = (
process.env.MPP_CHANNEL_CONTRACT &&
process.env.MPP_COMMITMENT_KEY &&
RECIPIENT &&
process.env.MPP_SECRET_KEY
| `MPP_COMMITMENT_KEY`, what the **server** holds, stored as hex the same | ||
| way (the server code above re-encodes it to a G... address at startup): | ||
| ```bash | ||
| node -e "const {Keypair}=require('@stellar/stellar-sdk');const seed=Buffer.from(process.argv[1],'hex');console.log(Keypair.fromRawEd25519Seed(seed).rawPublicKey().toString('hex'))" <COMMITMENT_SECRET> |
…ecipient() Small follow-up flagged as 'your call' in the bot's review: a typo'd G... (wrong length, bad checksum) reached the SDK unvalidated and threw deep inside Mppx.create(), away from the env var that caused it. Added a StrKey.isValidEd25519PublicKey() check as the last step, matching the validation just added to the real service's resolveRecipient() in this session's private-repo commit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@kaankacar all four points addressed, across two commits (23b0968, then 2bde1e9 for the smaller one):
All four are also fixed in the real service this doc is drawn from, not just here — Assistance disclosure: Claude Sonnet 5 was used for the review, the fixes, and this comment. Verified independently before each one landed — the real |
|
🤖 Automated message from Kaan's Automated Triage Bot. Thanks — all three items are fixed, and the runbook derivation step you added on your own was a good call. I checked
One small item is left. Line 366 still says Session works "once its four env vars are set". The Session env list on line 234 now names five. Copilot's new The Nirium item is unchanged. The x402 section credits Nirium's mainnet endpoint, and you own |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Suppressed comments (4)
skills/agentic-payments/mpp.md:306
- On the configured path this middleware neither invokes
chargeMppx.charge(...)nor callsnext(), so every request hangs. Delegate to the per-route MPP handler here.
// ... normal charge flow
skills/agentic-payments/mpp.md:356
- A non-empty but malformed
MPP_COMMITMENT_KEYpasses the enablement guard, thenBuffer.from/encodeEd25519PublicKeycan throw during module initialization. That defeats this section's recover-at-boot pattern. Validate exactly 64 hex characters and catch conversion/adapter initialization failures, leavingsessionMppxnull so its middleware can return 503.
commitmentKey: StrKey.encodeEd25519PublicKey(
Buffer.from(process.env.MPP_COMMITMENT_KEY, "hex")
),
skills/agentic-payments/mpp.md:367
- This section promises fail-closed middleware for each intent, but only exposes Session's status; it never demonstrates a Session route wrapper. Add a
mppSessionMiddlewarefactory that returns 503 whensessionMppxis null and otherwise returnssessionMppx.channel({ amount, description }), so readers do not accidentally mount an unprotected fallback route.
export const isSessionEnabled = () => !!sessionMppx;
skills/agentic-payments/x402.md:152
- The example still constructs the middleware unconditionally with an undefined
payTo, despite the added guidance and scenario requiring initialization to be gated. Add a guard around middleware creation and a fallback that returns 503 only for requests matchingPAID_ROUTES; unmatched routes must still callnext().
paymentMiddlewareFromConfig(
PAID_ROUTES,
facilitator,
[{ network: NETWORK, server: new ExactStellarScheme() }],
{ appName: "My API", testnet: NETWORK === "stellar:testnet" },
| import { ExactStellarScheme } from "@x402/stellar/exact/server"; | ||
|
|
||
| const facilitator = new HTTPFacilitatorClient({ | ||
| url: process.env.FACILITATOR_URL, |
| recipient: RECIPIENT, | ||
| /* ... */ |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
skills/agentic-payments/x402.md:212
- The fallback is also selected when
STELLAR_RECIPIENTis present but malformed, so both messages incorrectly say it is “not set.” Use “missing or invalid” for the server warning and a configuration-neutral 503 response for callers.
console.warn("STELLAR_RECIPIENT not set — paid routes return 503 instead of pricing");
x402Middleware = (req, res) => {
res.status(503).json({ error: "x402 payment middleware unavailable — STELLAR_RECIPIENT not set" });
skills/agentic-payments/x402.md:140
- This normalization removes quotes anywhere in the value, not just surrounding quotes. A malformed key such as
GBBD...WK"7P...is therefore converted into a valid, different input beforeStrKeyvalidates it, contradicting the fail-closed behavior described below. Strip only matching outer quotes so embedded or unmatched quotes remain invalid.
let raw = (process.env.STELLAR_RECIPIENT || "").trim().replace(/['"]/g, "");
| import { Keypair, StrKey } from "@stellar/stellar-sdk"; | ||
|
|
||
| function resolveRecipient() { | ||
| let raw = (process.env.STELLAR_RECIPIENT || "").trim().replace(/['"]/g, ""); |
| } | ||
|
|
||
| const sessionMppx = ( | ||
| process.env.MPP_CHANNEL_CONTRACT && |
|
🤖 Automated message from Kaan's Automated Triage Bot. Thanks — the store comment points at the right place now. I followed it: mpp.md:51 and mpp.md:174 both show Copilot reviewed this head. One of its points is real. The Session gate accepts any So a typo'd
Small: two warnings say "not set" when the value can also be invalid.
"missing or invalid" fixes both. I did not pass on Copilot's quote-stripping point (mpp.md:267, x402.md:140). It says |
…s sessionMppx's gate Copilot's latest round on stellar#97: the Session gate at mpp.md checks process.env.MPP_CHANNEL_CONTRACT for presence only, unlike commitmentKey and feePayerSigner right beside it, which are both validated before use. channel() in @stellar/mpp@0.7.1 only validates `store` at construction time — a typo'd C... value still builds sessionMppx, /info still reports Session as enabled, and the bad address only reaches `new Contract(...)` deep inside the SDK on the first paid request, where it throws "Invalid contract ID" instead of failing at boot the way this whole section exists to prevent. StrKey.isValidContract() returns a boolean and never throws, so it fits the same validate-before-use pattern already used for commitmentKey (hex regex) and feePayerSigner (isValidEd25519SecretSeed) two blocks above it. Added a channelAddress variable following that pattern, wired it into the gate and into stellarChannel.channel()'s config, and added the matching assertion to evals/05-production-hardening.json. Also fixed the two small wording items from the same review round: x402.md's and mpp.md's "not set" warnings both cover a value that can also be present-but-invalid (resolveRecipient() returns "" for a malformed STELLAR_RECIPIENT too, and already logs its own specific reason one line earlier) — "not set" contradicted that. Both now say "missing or invalid". Assistance disclosure: Claude Sonnet 5 was used for this round. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (8)
Previously missed (3) — in code that hasn't changed since the last review.
skills/agentic-payments/mpp.md:607
- This new client/server distinction conflicts with the later “Session: wrong commitment key format” guidance, which still says without qualification that the commitment key is the raw seed. A server operator following that later fix could put
COMMITMENT_SECRETintoMPP_COMMITMENT_KEY; the server would encode the seed bytes as a different public key, causing signature failures and exposing the client secret. Update that pitfall to distinguish both values.
6. Derive the corresponding public key from that same seed — this is
`MPP_COMMITMENT_KEY`, what the **server** holds, stored as hex the same
way (the server code above re-encodes it to a G... address at startup).
Export the seed from step 5 first, then read it from the environment
evals/scenarios/agentic-payments/05-production-hardening.json:18
- This assertion's rationale does not match the documented implementation:
Keypair.fromSecret()is called during the standalone validation block before theMppx.create()branch, not inside that call. Keep the eval focused on the observable requirement so the judge does not require an incorrect code structure.
"Validates FEE_PAYER_SECRET with StrKey.isValidEd25519SecretSeed() before calling Keypair.fromSecret() on it, same pattern as commitmentKey's hex regex — Keypair.fromSecret() throws synchronously on a malformed secret, and since that call sits inside the Mppx.create() branch, an unvalidated bad key throws at import time and crashes chargeMppx along with it, not just Session",
skills/agentic-payments/x402.md:140
- This removes quote characters anywhere in the credential rather than only stripping surrounding quotes. An otherwise malformed value containing an embedded quote can therefore be transformed into a valid public or secret key and bypass the intended validation. Strip only a matching outer quote pair so unexpected internal characters fail closed.
let raw = (process.env.STELLAR_RECIPIENT || "").trim().replace(/['"]/g, "");
skills/agentic-payments/x402.md:53
- The
||change avoids the library's generic fallback, but this hard-coded testnet URL still breaks the documented mainnet configuration whenSTELLAR_NETWORK=stellar:pubnetandFACILITATOR_URLis empty: pubnet requirements are sent to the OZ testnet facilitator. Derive the OZ default fromNETWORKso both supported networks have a valid fallback.
url: process.env.FACILITATOR_URL || "https://channels.openzeppelin.com/x402/testnet",
skills/agentic-payments/x402.md:126
- This production pattern has the same mainnet failure as the seller example: with
NETWORKset tostellar:pubnetand an empty/unset URL, it silently chooses the OZ testnet endpoint. Make the explicit fallback network-aware; otherwise the change only replaces one unsupported facilitator with another for mainnet.
url: process.env.FACILITATOR_URL || "https://channels.openzeppelin.com/x402/testnet",
skills/agentic-payments/mpp.md:267
- This removes every quote character, although the pattern is described as stripping surrounding quotes. That can turn an invalid value with an embedded quote into a valid key before
StrKeysees it. Limit normalization to a matching outer quote pair so malformed credentials are rejected.
let raw = (process.env.STELLAR_RECIPIENT || "").trim().replace(/['"]/g, "");
skills/agentic-payments/mpp.md:298
- Checking only for presence does not satisfy the promised boot-safe behavior.
Mppx.create()callsSecretKey.assert(), which throws synchronously whenMPP_SECRET_KEYis under 32 bytes, so a malformed value still crashes the process instead of leavingchargeMppxnull and returning 503. Validate the byte length before construction (or catch construction errors).
if (RECIPIENT && process.env.MPP_SECRET_KEY) {
chargeMppx = Mppx.create({ /* ... */ });
skills/agentic-payments/mpp.md:355
- The dual-intent example also gates on non-emptiness only. A short
MPP_SECRET_KEYmakes the firstMppx.create()throw synchronously, so neither intent reaches the fail-closed state and the process exits. Validate the key's minimum 32-byte length once and use that result in both the Charge and Session gates, or catch each construction independently.
const chargeMppx = (RECIPIENT && process.env.MPP_SECRET_KEY)
? Mppx.create({ methods: [stellar.charge({ recipient: RECIPIENT, /* ... */ })] })
|
🤖 Automated message from Kaan's Automated Triage Bot. Thanks — the Copilot generated no new comments. Its suppressed block carries two real points. 1. The commitment-key pitfall still tells the server to hold the seed. Line 627 says "the commitment key is a raw ed25519 seed as a 64-char hex string". Your new runbook splits that into two values: 2. The install line does not pin
So "no intent's setup may throw at import time" holds on 0.6.x and breaks on 0.9.x. Pinning Small: eval I did not pass on two of Copilot's points:
|
…ale eval line Three points from Copilot's suppressed review, surfaced via the triage bot: - The commitment-key pitfall still told readers to hold one seed. Split it into COMMITMENT_SECRET (client, the seed) and MPP_COMMITMENT_KEY (server, the derived public key) -- this was the only place in skills/ still using the old single-value wording; the runbook and server code already had it right. - The install line didn't pin mppx, which is what actually keeps "no intent's setup may throw at import time" true: @stellar/mpp@0.7.1 declares peerDependencies.mppx: ^0.6.29, but an unpinned install resolves the latest 0.9.x, which adds a SecretKey.assert() guard mppx@0.6.x doesn't have. Pinned mppx@^0.6.31 on the install line and named it again in the ERESOLVE pitfall. - Eval 05's expected_behavior still described Keypair.fromSecret() as "sitting inside the Mppx.create() branch" -- d79ebb5 already moved that call into the validation block above. Updated the eval text to match. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Fixed all three, pushed at b998c8e.
Agreed on both points you didn't pass on — no change needed there. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
skills/agentic-payments/mpp.md:300
- This gate accepts any nonempty
MPP_SECRET_KEY, including a trivially guessable value such asx. In this pinned mppx release there is no minimum-length assertion; the newer guard requires at least 32 bytes because this key authenticates HMAC-bound challenge IDs. Validate the byte length here and leavechargeMppxnull on failure so startup remains recoverable without weakening payment verification.
if (RECIPIENT && process.env.MPP_SECRET_KEY) {
chargeMppx = Mppx.create({ /* ... */ });
} else {
console.warn("MPP_SECRET_KEY missing, or STELLAR_RECIPIENT missing or invalid — MPP charge middleware disabled");
skills/agentic-payments/mpp.md:356
- The dual-intent Charge gate also treats a one-character
MPP_SECRET_KEYas valid, bypassing the 32-byte minimum added by newer mppx releases for HMAC-bound challenge IDs. Apply the same explicit byte-length validation here before constructing the instance.
const chargeMppx = (RECIPIENT && process.env.MPP_SECRET_KEY)
? Mppx.create({ methods: [stellar.charge({ recipient: RECIPIENT, /* ... */ })] })
: null;
skills/agentic-payments/mpp.md:419
- The Session gate repeats the weak-secret issue: any nonempty
MPP_SECRET_KEYenablessessionMppxunder mppx 0.6.31. Require at least 32 UTF-8 bytes here as well; otherwise a misconfigured short HMAC key is reported as an enabled production intent.
process.env.MPP_SECRET_KEY &&
feePayerSigner
| ``` | ||
|
|
||
| > **Version alignment matters:** `@stellar/mpp@0.7.x` pins `@stellar/stellar-sdk@^15.1.0` (installing alongside SDK 13/14 fails with `ERESOLVE`), and `mppx` expects `express@>=5`. | ||
| > **Version alignment matters:** `@stellar/mpp@0.7.x` pins `@stellar/stellar-sdk@^15.1.0` (installing alongside SDK 13/14 fails with `ERESOLVE`) and `peerDependencies.mppx: ^0.6.29` — pin `mppx@^0.6.31` explicitly, since an unpinned install resolves the latest `mppx` (0.9.x), which falls outside that peer range and also adds a `SecretKey.assert()` guard this doc's "no intent's setup may throw at import time" guarantee assumes isn't there yet. `mppx` also expects `express@>=5`. |
|
🤖 Automated message from Kaan's Automated Triage Bot. Thanks — all three items from the last round are fixed, and I checked each one.
Two items are left. Copilot raised both, and both are real. 1. 2. The doc pins below the guard, and never names the rule. Line 29 says const minimumBytes = 32;
// "Secret key must be at least 32 bytes. Generate one with `openssl rand -base64 32`"
Small: Copilot's other two comments repeat the |
…EY's minimum, fix /info Two more from Copilot's suppressed review, plus one small: - The "Packages and subpath imports" install line (skills/agentic-payments/mpp.md) was still unpinned. @stellar/mpp@0.7.1 declares peerDependencies.mppx: ^0.6.29 and @stellar/stellar-sdk: ^15.1.0; unpinned resolves each package's current latest (mppx@0.9.2, @stellar/stellar-sdk@17.0.1), both outside those ranges, reproducing the exact ERESOLVE the pitfall two sections up already describes. Pinned the same way the Charge mode install does. - MPP_SECRET_KEY's 32-byte minimum was never named anywhere the env var appears. mppx@0.6.31's Mppx.create() only checks presence, not length, so a one-character value is accepted and signs every HMAC challenge. Named the minimum and `openssl rand -base64 32` at Charge mode's first appearance, cross-referenced at Session's. - Small: /info's session block reported process.env.MPP_CHANNEL_CONTRACT directly instead of the validated channelAddress, so a contract ID that failed StrKey.isValidContract() still showed as if valid, right beside enabled: false. Now reports channelAddress (undefined -> null on reject). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Both fixed, plus the small one, pushed at 3d464a6.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (2)
skills/agentic-payments/mpp.md:107
- The 32-byte requirement is only documented, not enforced by the examples: the production gates below merely test
process.env.MPP_SECRET_KEYfor truthiness, and the pinnedmppx@0.6.31only rejects a missing key. Consequently, a one-byte value still initializes both intents and weakens HMAC challenge authentication. Add one shared byte-length validator and pass its validated value to everyMppx.create()/initialization gate.
> **`MPP_SECRET_KEY` must be at least 32 bytes.** `mppx@0.6.31`'s `Mppx.create()` only checks it's present, not its length — a one-character value is accepted and then signs every HMAC challenge. Generate one with `openssl rand -base64 32`. (`mppx@0.9.x` adds this check itself via `SecretKey.assert()`; pinned to `0.6.31` per the install note above, this doc's own examples don't get it for free.)
skills/agentic-payments/mpp.md:520
- This overstates what
/infoguarantees. The preceding paragraph explicitly says it does not check RPC or store reachability, soenabled: trueidentifies an initialized intent, not necessarily a working one. Reword this to avoid encouraging clients to treat configuration status as health.
An agent that reads this before its first request can pick a working
intent instead of finding out from a 503 that Session was never
configured.
|
🤖 Automated message from Kaan's Automated Triage Bot. Thanks — all three items are fixed. I checked each one against the packages.
I also swept every One small item is left, then I can merge.
Copilot asked again for a byte-length check inside the gates. I still read that as optional, and your note now names the rule, so it does not block. |
…lly promises The sentence right above already says /info reports construction state, not live health -- "pick a working intent" contradicted that in the very next line. Matches what the paragraph actually says. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Fixed, pushed at 1cbcf79 — "working" → "configured", matching the sentence right above it. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
skills/agentic-payments/mpp.md:107
- The examples below still gate
Mppx.create()only on whetherMPP_SECRET_KEYis present. Because the pinnedmppx@0.6.31performs no length validation, a one-character or whitespace-only value initializes both intents and/inforeports them enabled despite this stated minimum. Please validate the UTF-8 byte length once and use that validated result in every Charge/Session construction gate (with an explicit warning for an undersized key).
> **`MPP_SECRET_KEY` must be at least 32 bytes.** `mppx@0.6.31`'s `Mppx.create()` only checks it's present, not its length — a one-character value is accepted and then signs every HMAC challenge. Generate one with `openssl rand -base64 32`. (`mppx@0.9.x` adds this check itself via `SecretKey.assert()`; pinned to `0.6.31` per the install note above, this doc's own examples don't get it for free.)
|
🤖 Automated message from Kaan's Automated Triage Bot. Thanks — that was the last item, and it clears the PR. I checked At this head I also confirmed:
Copilot reviewed this head and generated no new comments. Its one suppressed point asks again for a byte-length gate on Thank you for the many careful rounds on this one. Merging. |
|
Thanks for the thorough review across all these rounds, @kaankacar — genuinely made the doc better each time. Appreciate the patience. |
Fresh proposal per Kaan's note closing #14: that PR targeted files removed in the #17 restructure, so it couldn't be rebased. He confirmed the current
skills/agentic-payments/SKILL.mddoesn't yet cover four of the patterns from the old PR, and invited a fresh PR proposing them individually. This is that — re-verified against a service that's been running x402 and MPP Charge in production, not copied from the old diff.Landing against
x402.mdandmpp.md(the detail filesSKILL.mdroutes to), notSKILL.mditself, since it's a decision table and these are implementation patterns.What changed and why each one still holds up (re-checked today, not assumed from the old PR):
Multi-route pricing with
paymentMiddlewareFromConfig(x402.md) — the seller example in the skill prices a single route. This documents the config-object form for pricing several routes behind one middleware call, with the route-keyed shape and scheme/facilitator wiring spelled out — including an explicitFACILITATOR_URLfallback, since@x402/core'sHTTPFacilitatorClientsilently falls back to its own genericx402.orgfacilitator (no Stellar scheme support) when that env var is unset. Matches what's running behind a production endpoint pricing three routes at three different amounts, cited with its first on-chain settlement.Recipient resolution: recover at boot, fail closed per request (
mpp.md) — two separate failure surfaces. Booting: a server that throws at import time becauseSTELLAR_RECIPIENTis unset breaks CI and any environment without secrets provisioned yet, so that case is recovered instead — including a secret key (S...) landing in the public-key env var by accident, recovered with a loud warning instead of a cryptic downstream throw, and a malformedG...rejected withStrKey.isValidEd25519PublicKey()instead of throwing deep in the SDK. Billing: once the server is up, a paid route with no payment instance behind it now returns503, never a silent200— an earlier draft of this PR had that backwards, caught in review and fixed (also fixed in the real service this doc reflects).Optional dual-intent server (
mpp.md) — reframed from the old PR, not copied. The old version implied running Charge and Session together in production; that's not accurate today. What is true and worth documenting: each intent gets its own SDK instance, gated independently by its own env vars — includingrecipient/currencyverification, strongly recommended by the library's own types and easy to leave out since both are optional — with route middleware failing closed, never serving unpriced content, when its instance isnull./inforuntime configuration-status endpoint (mpp.md) — reports which intents were actually initialized (!!chargeInstance), not a static claim that can drift from reality. Deliberately not called a health check: it reports whether eachMppxinstance was constructed at startup, not whether the RPC, facilitator, or store backend it depends on are reachable right now.Verified:
pnpm lint:ts,pnpm lint,pnpm sync:skills, andpnpm generate:llms-txtall pass clean against the new content.Happy to split this into separate PRs per pattern if that's easier to review — bundled them here since the recipient-resolution section is referenced from the pricing section via anchor link, but they don't otherwise depend on each other.
Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com