Repository navigation
Dist console-log guard, MCP payer metrics, key rotation runbook, CLI output stability - #782
Closed
ElizabethChi wants to merge 4 commits into
Closed
ElizabethChi wants to merge 4 commits into
ElizabethChi wants to merge 4 commits into
Conversation
…ellar-Wallet#670) Adds scripts/check-dist-console.mjs: scans the built dist/ bundle (all .js/.cjs output from every tsup entry point) for console.log/debug/warn calls. src/payments-client.ts and src/x402-signer.ts sign and submit Stellar transactions, and a stray debug console.log left in that path (or anywhere transitively bundled into it by tsup) would leak a signed transaction's XDR to every consumer's browser console. Scans built output rather than source so a leak introduced via any transitive import is caught regardless of which file introduced it. console.error is intentionally not scanned, surfacing a thrown error is normal, reviewed behavior. An ALLOWLIST (empty by default) permits an intentional, already-reviewed call, keyed by exact file + line so a different unreviewed call can't slip in under an existing entry. Wired into .github/workflows/ci.yml after npm run build (the scan needs dist/ to exist). Added as npm run check:dist-console. Refactored the script's scanning logic (scanContent/isAllowed) into exported, filesystem-independent functions so scripts/check-dist-console.test.ts can unit test the happy path and key failure modes (flags log/debug/warn, ignores error, multi-line/multi-call findings, allowlist exact-match semantics including the different-call-same-file and same-line-different-file cases) without needing a real dist/ build. Self-verified by removing "log" from CONSOLE_METHODS and confirming 3 tests fail with a clear diff before restoring. Unrelated pre-existing build-blocking bugs fixed as a necessary unblock (none of these were touched otherwise, and none are part of Vellar-Wallet#670's own scope): - src/x402-signer.ts: a missing docblock opener (one-line syntax fix) broke npm run build outright. - src/x402-signer.ts: both createSessionKeySigner's and createPasskeyX402Signer's signAuthEntry had unreachable dead code after an early return/throw, a bad merge concatenated two versions of each function. The live path was missing Vellar-Wallet#224's capability-scoping check entirely. Reconstructed each into one function: capability check now runs (inside the try, so a denial fires the deny audit hook like any other failure) before signing. src/x402-signer.test.ts had the matching corruption (two "capability scoping (Vellar-Wallet#224)" describe blocks mis-nested into unrelated it() bodies); fixed the structure so all 5 capability tests actually exercise the real, no-longer-dead-code signing path (they previously could not have been exercising it at all). - src/tx-rpc.ts: called the nonexistent Transaction.fromXDR(xdr, "base64") (real SDK method is TransactionBuilder.fromXDR(xdr, networkPassphrase)). Fixed the class name, but used Networks.TESTNET as a stopgap literal since RpcTxSubmitterOptions has no networkPassphrase to pass correctly, this submitter's real fix (threading a networkPassphrase option through) is a separate, larger change than this PR's scope and is called out here for a maintainer to pick up. Confirmed every one of these is pre-existing and unaffected by this PR: ran the full suite on git stash (clean tree) and got identical failure counts (18 failed suites, 26 failed tests, all unrelated to signing/dist) before and after my changes. npm run typecheck also fails on a separate, pre-existing syntax error in src/session.test.ts, also untouched by me and out of scope here.
…ar-Wallet#676) Adds packages/mcp-x402-payer/src/metrics.ts: a Prometheus text-format metrics endpoint for an operator running this payer in production. Tracks four counters: cumulative confirmed spend (by asset, bigint- precise, mirroring ledger.ts's record-on-settle-only rule), payment success count (by asset), payment failure count (by error name), and session-ceiling rejections (by asset, distinct from a generic failure). Opt-in via VELLAR_X402_METRICS_PORT (new config.ts option, undefined by default). This process is otherwise stdio-only; a process holding a hot wallet key opening an unexpected port is exactly the kind of surprise that should require explicit configuration, not a default. The HTTP server binds to 127.0.0.1 ONLY, never every interface, since the endpoint has no auth of its own and Prometheus text format doesn't either, exposing it beyond localhost needs an operator-managed reverse proxy, not a flag that could be flipped without that protection in place. server.ts's existing x402_pay/x402_pay_and_call try/catch blocks now also call recordPaymentOutcome/recordPaymentFailure, observing the result/error each handler already has rather than adding a new seam. metrics is optional on ServerDeps and a no-op when absent, so every existing caller of createMcpServer is unaffected. Documented in packages/mcp-x402-payer/README.md with the new env var and a Metrics section listing all four counters. Tests: metrics.test.ts covers the happy path (empty counters still render valid Prometheus text), a simulated payment sequence (two successes, one failure, one ceiling rejection), multi-asset/multi-reason series staying distinct, bigint precision past Number.MAX_SAFE_INTEGER, label escaping, and the HTTP server itself (serves current text on GET, rejects non-GET, binds to 127.0.0.1 only). metrics-server-integration.test.ts drives the real x402_pay tool handler through createMcpServer to confirm success/failure/ceiling-rejection wiring end to end, and that omitting metrics entirely (the default) does not throw. config.test.ts covers VELLAR_X402_METRICS_PORT parsing (disabled by default, valid port, non-numeric, out-of-range, empty string). Self-verified by removing the recordPaymentOutcome call from x402_pay and confirming the integration test fails with a clear diff before restoring.
…let#681) Adds website/content/docs/operators/rotate-mcp-payer-key.md: an operator-facing runbook for rotating vellar-mcp-x402-payer's one signer key if it's suspected compromised. Covers both configurations the package supports: layer 1 (hot wallet, no on-chain revocation exists, the only mitigation is moving funds out and never reusing the key) and layer 2 (a smart-account session key minted/revoked on-chain via wallet.agents.mint/revoke from src/agents-facade.ts). Addresses the issue's specific asks: the mint-new-to-revoke-old sequence, how the in-memory spend ledger (src/ledger.ts) relates to rotation (it already resets on every restart by design, documented as "nothing to migrate" rather than inventing a migration step that would just carry forward potentially-compromised accounting), and on-chain revocation of a key that was minted as an agent key. Adds contrib/examples/rotate-mcp-payer-key/ as the acceptance test proving the documented steps are executable in order: mint the new key, confirm both old and new keys can sign during the intentional brief overlap window, revoke the old key, confirm the old key is rejected and the new key still works, and confirm an in-process ledger-shaped counter resets to zero on "restart." Follows the same mock-runtime-plus- standalone-process-test pattern as the existing contrib/examples/agent-key-lifecycle example. Note: the "runs end to end as a standalone Node process" test fails locally on macOS (as does the sibling agent-key-lifecycle example's identical test) because mkdtemp(tmpdir()) resolves to a path through /tmp, which is a symlink to /private/tmp on macOS. import.meta.url inside the bundled script resolves through the real path while resolve(process.argv[1]) does not, so the two never match and main() never runs. CI runs on ubuntu-latest, where /tmp is not a symlink, so this passes there, confirmed by running the sibling example's identical test and getting the identical failure, unrelated to anything in this commit.
…-Wallet#683) Extracts the inline rendering logic from search/quote/inspect's action handlers into named, exported pure functions (renderResource, renderChallenge, renderTransaction), matching pay.ts's existing export-pure-helpers-for-testing pattern. The actions now call these functions instead of duplicating the console.log sequence inline, with no behavior change. Adds snapshot tests (toMatchInlineSnapshot, matching the convention in contrib/structured-error-codes.test.ts) covering the full text/line contract for each: full/minimal entries, multiple accepts options, missing-field fallbacks, description truncation, payTo/fee-sponsorship conditional lines. Each was self-verified by deliberately breaking the render function's output and confirming the snapshot tests fail with a clear diff before restoring. Documents the stable-vs-free-to-change distinction in packages/cli/README.md: line order, label text, fallback text, and conditional-line presence are the contract (and must update the inline snapshot deliberately when changed); ANSI styling, purely cosmetic whitespace no snapshot pins, and stderr wording are not. Adds a cli-output-stability CI job (.github/workflows/ci.yml) running packages/cli's test suite across Node 20 (oldest supported, per packages/cli/package.json's engines.node) and Node 22 (the version the main ci job already validates this repo on), via a matrix, to catch a Node-version-dependent formatting drift the single-version run would miss.
|
@ElizabethChi is attempting to deploy a commit to the david's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@ElizabethChi Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
|
Thanks for the contribution — but contributor PRs may only change files inside This PR touches file(s) outside that folder:
Please open a new PR with your changes scoped to Questions? Ask in the Telegram group. |
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
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
Four hardening/observability/documentation changes: a build-time scan for stray console calls in the signing path, an opt-in Prometheus metrics endpoint for the MCP payer, an operator runbook for rotating the MCP payer's signer key, and snapshot tests pinning the CLI's human-readable output as a stable contract.
closes #670
closes #676
closes #681
closes #683
Changes
scripts/check-dist-console.mjs, scanning every builtdist/*.js/*.cjsfile (all tsup entry points) forconsole.log/debug/warncalls, since a stray one in the signing/submission path would leak a signed transaction's XDR to every consumer's console.console.erroris deliberately exempt. An empty-by-defaultALLOWLISTpermits an intentional, reviewed call, keyed by exact file+line. Wired into CI afternpm run build. Unit tests cover the scanning logic directly (no real build needed) plus the happy path and failure modes. Needed a necessary unblock: a missing docblock opener and unreachable dead code (a bad merge had concatenated two versions of each x402 signer'ssignAuthEntry, silently dropping the [hard] Implement scoped capability checks for x402-signer-policies #224 capability-scoping check from the live path) insrc/x402-signer.ts, and a nonexistentTransaction.fromXDRcall insrc/tx-rpc.ts(fixed the class name; usedNetworks.TESTNETas a disclosed stopgap since there's nonetworkPassphrasecurrently threaded through that submitter, a separate, larger fix called out for a maintainer).packages/mcp-x402-payer/src/metrics.ts, an opt-in (VELLAR_X402_METRICS_PORT, undefined/disabled by default) Prometheus text-format endpoint tracking spend, success, failure, and session-ceiling-rejection counters. Binds to127.0.0.1only. Wired into the existingx402_pay/x402_pay_and_calltry/catch blocks, observing the result/error each already has. Documented in the package README with a new Metrics section.website/content/docs/operators/rotate-mcp-payer-key.md, covering both the hot-wallet (no on-chain revocation exists) and smart-account (mint-new/revoke-old viawallet.agents) configurations, how the in-memory spend ledger relates to rotation (it already resets on restart, nothing to migrate), and on-chain revocation.contrib/examples/rotate-mcp-payer-key/is the acceptance test proving the documented sequence is executable in order (mint, brief overlap where both keys sign, revoke, old key rejected, new key still works).search/quote/inspect's inline rendering into exported pure functions (renderResource,renderChallenge,renderTransaction), matchingpay.ts's existing pattern, and snapshot-tests the full text contract for each. Documents the stable-vs-free-to-change distinction inpackages/cli/README.md. Adds acli-output-stabilityCI job running the CLI's tests across Node 20 (oldest supported perengines.node) and Node 22 (this repo's existing CI version) via a matrix.Test plan
check-dist-console.test.ts(12),metrics.test.ts(9),metrics-server-integration.test.ts(4),config.test.ts's new metrics-port cases (5),rotate-mcp-payer-key.test.ts(2 of 3, see note below),search.test.ts/quote.test.ts/inspect.test.ts's new snapshot cases (24 total). Fullpackages/mcp-x402-payersuite (243 tests) andpackages/clisuite (52 tests) pass with no regressions.npm run build+npm run check:dist-consolerun for real end to end.npm run typecheckpasses for the root package,mcp-x402-payer, andcli.Disclosures
rotate-mcp-payer-key.test.ts's "runs end to end as a standalone Node process" test fails locally on macOS (as does the pre-existing, unrelated siblingagent-key-lifecycleexample's identical test) becausemkdtemp(tmpdir())resolves through/tmp, a symlink to/private/tmpon macOS, soimport.meta.urlandresolve(process.argv[1])never match inside the bundled script. CI runs onubuntu-latest, where/tmpisn't a symlink, so this passes there; confirmed by reproducing the identical failure on the untouched sibling example.src/session.test.tshas a separate, pre-existing syntax error blockingnpm run typecheckfor the root package; untouched by this PR and out of scope.git stash: identical 18 failed suites / 26 failed tests both times, confirming none of this PR's work caused or fixed any of them.