feat: add MCP server for ACK-ID and ACK-Pay operations#125
Open
ak68a wants to merge 16 commits into
Open
Conversation
Expose 9 tools via stdio MCP transport: identity (create/sign/verify credentials, resolve DIDs), payments (create/verify payment requests and receipts), and keypair generation. Signing tools accept JWK to prevent curve mismatches. MCP SDK pinned to 1.21.2 for zod 3 compatibility.
- Change shebang from node to npx tsx so TypeScript source runs directly - Fix expiresInSeconds check: use !== undefined instead of truthy check so that 0 correctly creates an immediately-expiring request
- Use instanceof Error check instead of unsafe (e as Error).message cast in all 3 verification catch blocks (identity, payment-requests, payment-receipts) - Remove .default(3600) from expiresInSeconds schema so omitting the field leaves expiresAt unset rather than always setting it - Add .int().nonnegative() validation to expiresInSeconds
- Remove raw privateKey/publicKey hex from ack_generate_keypair output (JWK is sufficient) - Change expiresInSeconds from .nonnegative() to .positive() to prevent zero-expiry requests - Add JSON object validation in keypairFromJwk before passing to jwkToKeypair - Add payment-receipts.test.ts (happy path, untrusted issuer, wrong payment request issuer) - Add keypairFromJwk tests for array, primitive, and null input - Add expiresInSeconds conversion and empty paymentOptions rejection tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Spawns the server over stdio, performs the real MCP handshake, and exercises tools through the protocol — the same path a real MCP client takes. Covers: handshake, tool listing, keypair generation, DID resolution, and a full credential sign+verify round-trip. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Rename subject/controller to subjectDid/controllerDid and jwk/did to signerJwk/signerDid. During manual testing, the agent guessed wrong parameter names on first attempts — these names match the mental model agents use when reasoning about the tools. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…xfmt Move the empty-array check from a runtime guard to Zod `.min(1)` on the paymentOptions schema, so the MCP SDK rejects invalid input at the validation layer. Apply oxfmt formatting to align with the repo's switch from Prettier. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…roved descriptions - Wire up getControllerClaimVerifier in ack_verify_credential (opt-in via verifyControllerClaims param — requires did:web, not did:key) - Add ack_create_did_web and ack_create_did_pkh tools so agents can represent web services and blockchain wallets as DIDs - Add ack_create_jwt and ack_verify_jwt tools for challenge-response auth and custom signed payloads - Update ack_create_payment_receipt description to clarify the output is unsigned and must be signed via ack_sign_credential - Improve all tool descriptions with data flow guidance and allow agent-provided JWKs (not just ack_generate_keypair output) - Add tests for all new tools and verification paths (39 tests total) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 7 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
WalkthroughThis PR adds a new ChangesMCP Server Tooling
Review Records, Protocol, and Vector Scripts
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (11)
tools/mcp-server/src/util.ts (1)
66-72: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueGuard against
validkey collision in spread.
{ valid, ...data }lets avalidkey insidedatasilently override the boolean argument. No current call site does this, but it's a latent footgun for future callers.♻️ Suggested guard
export function verification( valid: boolean, data: Record<string, unknown>, ): CallToolResult { + if ("valid" in data) { + throw new Error("verification() data must not contain a 'valid' key") + } return ok({ valid, ...data }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/mcp-server/src/util.ts` around lines 66 - 72, The verification helper in verification() currently spreads data after the valid flag, so a valid property inside data can override the boolean argument. Update the merge order or otherwise sanitize the input so the explicit valid parameter always wins, and keep the fix localized to verification() in util.ts.packages/jwt/scripts/generate-vectors.ts (1)
21-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate claims payload between the two JWT vectors.
The claim set is repeated identically for secp256k1 and Ed25519. Extracting it to a shared constant would reduce duplication.
♻️ Proposed fix
+const claims = { + iss: "did:key:test-issuer", + sub: "did:key:test-subject", + aud: "https://example.com", + iat: 1700000000, + exp: 1800000000, +} + const secp256k1Jwt = await createJwt( - { - iss: "did:key:test-issuer", - sub: "did:key:test-subject", - aud: "https://example.com", - iat: 1700000000, - exp: 1800000000, - }, + claims, { issuer: "did:key:test-issuer", signer: secp256k1Signer }, { alg: "ES256K" }, ) ... const ed25519Jwt = await createJwt( - { - iss: "did:key:test-issuer", - sub: "did:key:test-subject", - aud: "https://example.com", - iat: 1700000000, - exp: 1800000000, - }, + claims, { issuer: "did:key:test-issuer", signer: ed25519Signer }, { alg: "EdDSA" }, )Also applies to: 44-54
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/jwt/scripts/generate-vectors.ts` around lines 21 - 31, The JWT vector claim payload is duplicated between the secp256k1 and Ed25519 examples in generate-vectors.ts, so extract the repeated claims object into a shared constant and reuse it in both createJwt calls. Update the createJwt usages for the secp256k1Jwt and Ed25519Jwt vectors to reference that shared payload while keeping the issuer, signer, and alg-specific options separate.packages/did/scripts/generate-vectors.ts (2)
18-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer a static import over dynamic
import().
getPublicKeyBytesis used unconditionally at module top-level logic; a dynamic import adds no value here versus a static import alongside line 1.♻️ Proposed fix
import { generateKeypair } from "`@agentcommercekit/keys`" import { hexStringToBytes, bytesToHexString } from "`@agentcommercekit/keys/encoding`" +import { getPublicKeyBytes } from "`@agentcommercekit/keys/secp256k1`" import { createDidKeyUri } from "../src/methods/did-key.ts" ... - const { getPublicKeyBytes } = await import("`@agentcommercekit/keys/secp256k1`") const compressed = getPublicKeyBytes(privBytes, true)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/did/scripts/generate-vectors.ts` around lines 18 - 20, The module-level use of getPublicKeyBytes in generate-vectors.ts does not need a dynamic import, so replace the awaited import() with a normal static import at the top of the file alongside the existing imports. Keep the rest of the top-level logic unchanged, and ensure the generate-vectors flow still calls getPublicKeyBytes, bytesToHexString, and logs the compressed secp256k1 pubkey as before.
6-6: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winHardcoded private key is duplicated verbatim in
packages/jwt/scripts/generate-vectors.ts(line 14).Both scripts must stay byte-for-byte in sync to produce interoperable golden vectors (the jwt script's header notes it's consumed by
ack-swift). A future edit to one copy without the other would silently desynchronize cross-language test vectors with no build-time signal. Consider extracting the shared constant to a small common fixture module, or at minimum add a comment in both files cross-referencing the other's location.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/did/scripts/generate-vectors.ts` at line 6, The hardcoded private key in generate-vectors is duplicated in the parallel JWT vectors script, so keep the two sources synchronized. Update packages/did/scripts/generate-vectors.ts and the matching generate-vectors logic in the jwt script to share a single common fixture constant or helper module, using the existing privHex symbol as the reference point. If extracting is too large, add explicit cross-referencing comments in both scripts so future edits must be made in both places together. Ensure the shared value remains byte-for-byte identical for the golden vector generation path.tools/mcp-server/src/tools/identity.test.ts (1)
48-70: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDynamic import bypasses the actual utility under test.
The comment says "Simulate what the MCP tool does: reconstruct from JWK" (Line 53), but this test dynamically imports
jwkToKeypairdirectly fromagentcommercekitrather than exercisingkeypairFromJwkfrom../util— the actual helper the MCP tool calls (seejwt.test.tsLines 12, 40, which does this correctly). As written, this test doesn't cover the JSON-parsing/type-guard logic inkeypairFromJwk, so a bug there wouldn't be caught here.♻️ Suggested fix
-import { - createControllerCredential, - createDidKeyUri, - createJwtSigner, - generateKeypair, - keypairToJwk, - signCredential, - type DidUri, -} from "agentcommercekit" +import { + createControllerCredential, + createDidKeyUri, + createJwtSigner, + generateKeypair, + keypairToJwk, + signCredential, + type DidUri, +} from "agentcommercekit" import { describe, expect, it } from "vitest" -import { curveToAlg } from "../util" +import { curveToAlg, keypairFromJwk } from "../util" @@ it("round-trips a keypair through JWK for signing", async () => { const keypair = await generateKeypair("secp256k1") const did = createDidKeyUri(keypair) - const jwk = keypairToJwk(keypair) - - // Simulate what the MCP tool does: reconstruct from JWK - const { jwkToKeypair } = await import("agentcommercekit") - const restored = jwkToKeypair(jwk) + const jwkJson = JSON.stringify(keypairToJwk(keypair)) + + // Simulate what the MCP tool does: reconstruct from JWK + const restored = keypairFromJwk(jwkJson) const signer = createJwtSigner(restored)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/mcp-server/src/tools/identity.test.ts` around lines 48 - 70, The identity round-trip test is importing jwkToKeypair directly instead of exercising the local key conversion helper used by the MCP tool. Update the test in identity.test.ts to use keypairFromJwk from ../util (as done in jwt.test.ts) so it covers the JSON parsing and type-guard logic the tool actually relies on, while keeping the rest of the signing flow with createJwtSigner and signCredential unchanged.tools/mcp-server/src/tools/identity.ts (1)
33-53: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winValidate the DID inputs before casting.
subjectDid,controllerDid,issuerDid, andsignerDidare only cast toDidUrihere, andcreateControllerCredential/signCredentialdon’t re-check them. Use the existing DID schema helper at the tool boundary so malformed agent input fails fast.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/mcp-server/src/tools/identity.ts` around lines 33 - 53, Validate the DID inputs at the tool boundary before any casting: in the identity tool handler around createControllerCredential and signCredential, ensure subjectDid, controllerDid, issuerDid, and signerDid are parsed with the existing DID schema helper instead of only being cast to DidUri. Update the zod inputs and/or add a schema-based parse step in the handler so malformed agent input is rejected early, then pass the validated values into createControllerCredential and signCredential.tools/mcp-server/src/tools/payment-receipts.ts (1)
62-65: 🔒 Security & Privacy | 🔵 TrivialOptional
trustedReceiptIssuersmeans any issuer is accepted by default.If
trustedReceiptIssuersisn't supplied,verifyPaymentReceiptvalidates signature/schema but doesn't restrict who signed the receipt. Downstream integrators should always passtrustedReceiptIssuersin production flows to avoid accepting receipts from arbitrary issuers.Also applies to: 71-77
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/mcp-server/src/tools/payment-receipts.ts` around lines 62 - 65, The optional trustedReceiptIssuers parameter in payment-receipts setup and verifyPaymentReceipt currently allows any issuer when omitted, so tighten the default handling in those paths. Update the validation flow in trustedReceiptIssuers and verifyPaymentReceipt to require or explicitly enforce a trusted issuer list for production use, and make sure the acceptance logic only succeeds when the receipt signer matches one of the configured issuer DIDs.tools/mcp-server/src/tools/payment-requests.test.ts (1)
1-142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTests exercise the library, not the MCP tool wrapper.
None of these tests import/invoke
registerPaymentRequestTools; they callcreateSignedPaymentRequest/verifyPaymentRequestTokendirectly. The wrapper logic unique topayment-requests.ts—expiresInSeconds→expiresAtconversion (lines 84-88 there), the Zod.min(1)guard, UUID generation, and error wrapping — is untested here.Please confirm this is covered by
server.test.ts/workflow.test.ts(not included in this batch); if not, consider adding a test that calls the registered tool handler directly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/mcp-server/src/tools/payment-requests.test.ts` around lines 1 - 142, The current tests target the underlying payment request library helpers instead of the MCP wrapper logic, so they do not cover the behavior in registerPaymentRequestTools or the payment-requests tool handler. Update or add tests to invoke the registered tool handler directly (or confirm coverage already exists in server.test.ts/workflow.test.ts) so the wrapper-specific paths are exercised: expiresInSeconds to expiresAt conversion, the Zod min(1) validation, UUID generation, and error wrapping in payment-requests.ts.tools/mcp-server/src/tools/payment-requests.ts (1)
108-114: 🔒 Security & Privacy | 🔵 TrivialOptional
issuermeans no issuer restriction by default.When
issueris omitted, verification only checks signature/schema/expiry, not who issued the token. Fine as a caller-controlled option, but worth calling out for downstream integrators relying on this tool for trust decisions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/mcp-server/src/tools/payment-requests.ts` around lines 108 - 114, Clarify the optional issuer behavior in the Payment Request tool schema: the `issuer` field in `payment-requests.ts` is caller-controlled and, when omitted, token verification should not enforce any issuer restriction. Update the `z.string().optional().describe(...)` text (and any related tool docs/comments near the `issuer` definition) to explicitly state that verification then only checks signature, schema, and expiry, so downstream integrators understand the trust boundary.tools/mcp-server/src/tools/utility.ts (2)
21-42: 🔒 Security & Privacy | 🔵 TrivialPrivate key exposure via tool output.
ack_generate_keypairreturns the raw private-key JWK in plaintext tool output, which is required for downstream tools that needsignerJwk. Since MCP tool results often flow through host application logs/transcripts, ensure any orchestrator wrapping this server treats tool outputs as sensitive and avoids persisting/logging them.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/mcp-server/src/tools/utility.ts` around lines 21 - 42, The `ack_generate_keypair` tool in `utility.ts` intentionally returns a private-key JWK for downstream `signerJwk` use, so the fix is to treat this result as sensitive end-to-end. Update the MCP orchestrator or host integration that calls `server.tool("ack_generate_keypair", ...)` to suppress logging, transcript persistence, and analytics for the `jwk` field while still passing it through to dependent tools in memory. Keep the tool contract unchanged, but make sure any wrapper around `generateKeypair`, `keypairToJwk`, or `createDidKeyUri` handles the returned payload as secret material.
17-17: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
did:pkhchainId restricted to only 8 hardcoded chains.Enum values come from
caip2ChainIds, limitingack_create_did_pkhto Ethereum/Base/Arbitrum and Solana mainnet/devnet only. Any other CAIP-2 chain (Polygon, other L2s, other Solana clusters, etc.) is rejected even thoughdid:pkhsupports arbitrary chains.Consider validating the general CAIP-2 format instead of a fixed enum, keeping the known IDs as description examples.
♻️ Proposed relaxed validation
- chainId: z - .enum(chainIdValues) - .describe( - "CAIP-2 chain ID (e.g. 'eip155:1' for Ethereum mainnet, 'eip155:8453' for Base, 'solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp' for Solana)", - ), + chainId: z + .string() + .regex(/^[-a-z0-9]{3,8}:[-a-zA-Z0-9]{1,32}$/) + .describe( + "CAIP-2 chain ID (e.g. 'eip155:1' for Ethereum mainnet, 'eip155:8453' for Base, 'solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp' for Solana)", + ),Also applies to: 64-73
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/mcp-server/src/tools/utility.ts` at line 17, The `did:pkh` chainId check is too restrictive because `chainIdValues` is built from `caip2ChainIds`, limiting `ack_create_did_pkh` to a small hardcoded set. Update the validation in `utility.ts` (and the related `ack_create_did_pkh` schema/handler logic) to accept any valid CAIP-2 chain identifier format instead of enumerating known IDs. Keep the existing `caip2ChainIds` entries only as examples in the description or docs, not as the validator source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.mcp.json:
- Line 6: The `cwd` value in the `.mcp.json` server config is hardcoded to a
personal absolute path, which breaks portability across machines and CI. Update
the `cwd` entry to use a relative or repo-root-relative path that works
regardless of the contributor’s local filesystem, keeping the fix localized to
the JSON config and the server declaration it feeds.
In `@packages/did/scripts/generate-vectors.ts`:
- Around line 5-22: `main()` in generate-vectors is currently invoked without
rejection handling, so failures from `generateKeypair`, `createDidKeyUri`, or
the dynamic import can surface as unhandled promise rejections. Update the
script’s `main()` entrypoint to mirror the sibling generate-vectors script by
attaching a `.catch(...)` handler to the `main` call and logging the error
cleanly. Keep the change localized to the `main` function and its invocation so
any async failure is reported consistently.
In `@packages/jwt/scripts/generate-vectors.ts`:
- Around line 1-6: The header run command in generate-vectors.ts points to the
wrong script name, so update the usage text to reference the actual file name
shown by this module. Fix the Run line in the top comment so it matches
generate-vectors.ts exactly, keeping the rest of the JWT vector-generation
description unchanged.
In `@tools/mcp-server/package.json`:
- Line 23: Remove the per-package typecheck script from the tools/mcp-server
package.json; the offending entry is the check:types script that runs tsc
--noEmit. Update the package.json scripts section to delete this script entirely
and keep type checking in the workspace build/check flow as required by the repo
policy.
In `@tools/mcp-server/src/server.test.ts`:
- Around line 32-46: The test setup in beforeAll for the
Client/StdioClientTransport leaves stderr piped but unread, so add a consumer or
otherwise drain the transport stderr stream to avoid blocking on noisy failures.
Also make the tsx executable an explicit local dependency for the
tools/mcp-server package so the test does not rely on a global PATH; update the
package manifest and keep the transport invocation in server.test.ts aligned
with that local binary.
In `@tools/mcp-server/src/tools/identity.ts`:
- Around line 142-155: Add an abortable timeout to DID resolution so a slow
did:web lookup cannot hang the tool call. Update the shared resolver creation in
util.ts or wrap the calls at the tool boundary in ack_resolve_did and
ack_verify_credential so resolveDid/credential verification is cancelled after a
deadline and returns a timeout error through ok/err rather than waiting
indefinitely.
---
Nitpick comments:
In `@packages/did/scripts/generate-vectors.ts`:
- Around line 18-20: The module-level use of getPublicKeyBytes in
generate-vectors.ts does not need a dynamic import, so replace the awaited
import() with a normal static import at the top of the file alongside the
existing imports. Keep the rest of the top-level logic unchanged, and ensure the
generate-vectors flow still calls getPublicKeyBytes, bytesToHexString, and logs
the compressed secp256k1 pubkey as before.
- Line 6: The hardcoded private key in generate-vectors is duplicated in the
parallel JWT vectors script, so keep the two sources synchronized. Update
packages/did/scripts/generate-vectors.ts and the matching generate-vectors logic
in the jwt script to share a single common fixture constant or helper module,
using the existing privHex symbol as the reference point. If extracting is too
large, add explicit cross-referencing comments in both scripts so future edits
must be made in both places together. Ensure the shared value remains
byte-for-byte identical for the golden vector generation path.
In `@packages/jwt/scripts/generate-vectors.ts`:
- Around line 21-31: The JWT vector claim payload is duplicated between the
secp256k1 and Ed25519 examples in generate-vectors.ts, so extract the repeated
claims object into a shared constant and reuse it in both createJwt calls.
Update the createJwt usages for the secp256k1Jwt and Ed25519Jwt vectors to
reference that shared payload while keeping the issuer, signer, and alg-specific
options separate.
In `@tools/mcp-server/src/tools/identity.test.ts`:
- Around line 48-70: The identity round-trip test is importing jwkToKeypair
directly instead of exercising the local key conversion helper used by the MCP
tool. Update the test in identity.test.ts to use keypairFromJwk from ../util (as
done in jwt.test.ts) so it covers the JSON parsing and type-guard logic the tool
actually relies on, while keeping the rest of the signing flow with
createJwtSigner and signCredential unchanged.
In `@tools/mcp-server/src/tools/identity.ts`:
- Around line 33-53: Validate the DID inputs at the tool boundary before any
casting: in the identity tool handler around createControllerCredential and
signCredential, ensure subjectDid, controllerDid, issuerDid, and signerDid are
parsed with the existing DID schema helper instead of only being cast to DidUri.
Update the zod inputs and/or add a schema-based parse step in the handler so
malformed agent input is rejected early, then pass the validated values into
createControllerCredential and signCredential.
In `@tools/mcp-server/src/tools/payment-receipts.ts`:
- Around line 62-65: The optional trustedReceiptIssuers parameter in
payment-receipts setup and verifyPaymentReceipt currently allows any issuer when
omitted, so tighten the default handling in those paths. Update the validation
flow in trustedReceiptIssuers and verifyPaymentReceipt to require or explicitly
enforce a trusted issuer list for production use, and make sure the acceptance
logic only succeeds when the receipt signer matches one of the configured issuer
DIDs.
In `@tools/mcp-server/src/tools/payment-requests.test.ts`:
- Around line 1-142: The current tests target the underlying payment request
library helpers instead of the MCP wrapper logic, so they do not cover the
behavior in registerPaymentRequestTools or the payment-requests tool handler.
Update or add tests to invoke the registered tool handler directly (or confirm
coverage already exists in server.test.ts/workflow.test.ts) so the
wrapper-specific paths are exercised: expiresInSeconds to expiresAt conversion,
the Zod min(1) validation, UUID generation, and error wrapping in
payment-requests.ts.
In `@tools/mcp-server/src/tools/payment-requests.ts`:
- Around line 108-114: Clarify the optional issuer behavior in the Payment
Request tool schema: the `issuer` field in `payment-requests.ts` is
caller-controlled and, when omitted, token verification should not enforce any
issuer restriction. Update the `z.string().optional().describe(...)` text (and
any related tool docs/comments near the `issuer` definition) to explicitly state
that verification then only checks signature, schema, and expiry, so downstream
integrators understand the trust boundary.
In `@tools/mcp-server/src/tools/utility.ts`:
- Around line 21-42: The `ack_generate_keypair` tool in `utility.ts`
intentionally returns a private-key JWK for downstream `signerJwk` use, so the
fix is to treat this result as sensitive end-to-end. Update the MCP orchestrator
or host integration that calls `server.tool("ack_generate_keypair", ...)` to
suppress logging, transcript persistence, and analytics for the `jwk` field
while still passing it through to dependent tools in memory. Keep the tool
contract unchanged, but make sure any wrapper around `generateKeypair`,
`keypairToJwk`, or `createDidKeyUri` handles the returned payload as secret
material.
- Line 17: The `did:pkh` chainId check is too restrictive because
`chainIdValues` is built from `caip2ChainIds`, limiting `ack_create_did_pkh` to
a small hardcoded set. Update the validation in `utility.ts` (and the related
`ack_create_did_pkh` schema/handler logic) to accept any valid CAIP-2 chain
identifier format instead of enumerating known IDs. Keep the existing
`caip2ChainIds` entries only as examples in the description or docs, not as the
validator source of truth.
In `@tools/mcp-server/src/util.ts`:
- Around line 66-72: The verification helper in verification() currently spreads
data after the valid flag, so a valid property inside data can override the
boolean argument. Update the merge order or otherwise sanitize the input so the
explicit valid parameter always wins, and keep the fix localized to
verification() in util.ts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b80431f9-7f2a-49df-8757-8ae383972eb8
⛔ Files ignored due to path filters (2)
demos/delegation-poc/package-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (26)
.claude/review-harness.md.mcp.json.reviews/REVIEW-2026-03-27-caip10-validation.md.reviews/REVIEW-2026-03-27-mcp-server-v2.md.reviews/REVIEW-2026-03-27-mcp-server.md.reviews/REVIEW-2026-03-27-payment-request-error-cause.mdpackages/did/scripts/generate-vectors.tspackages/jwt/scripts/generate-vectors.tstools/mcp-server/package.jsontools/mcp-server/src/index.tstools/mcp-server/src/server.test.tstools/mcp-server/src/tools/identity.test.tstools/mcp-server/src/tools/identity.tstools/mcp-server/src/tools/jwt.test.tstools/mcp-server/src/tools/jwt.tstools/mcp-server/src/tools/payment-receipts.test.tstools/mcp-server/src/tools/payment-receipts.tstools/mcp-server/src/tools/payment-requests.test.tstools/mcp-server/src/tools/payment-requests.tstools/mcp-server/src/tools/utility.tstools/mcp-server/src/tools/workflow.test.tstools/mcp-server/src/util.test.tstools/mcp-server/src/util.tstools/mcp-server/tsconfig.jsontools/mcp-server/vitest.config.tstsconfig.tsbuildinfo
- Remove hardcoded absolute path from .mcp.json for portability - Add private key (d field) validation in keypairFromJwk for clear errors - Add top-level error handler on MCP server connect - Add missing .catch() on DID vector script main() - Fix wrong filename in JWT vector script comment - Apply oxfmt formatting to review docs and scripts Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.claude/review-harness.md (2)
91-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDon't freeze the consumer list.
This checklist hardcodes a narrow set of downstream packages, so a new workspace consumer can be missed as the repo grows. Please derive the package-boundary check from the workspace/import graph instead of enumerating
agent-core,mcp-banking,x402-gateway,a2a,demo, andevalhere.♻️ Proposed wording
- Check which other packages in the monorepo import from it (`agent-core`, `mcp-banking`, `x402-gateway`, `a2a`, `demo`, `eval`). + Check which workspace packages import from it (derive this from the import graph).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/review-harness.md around lines 91 - 95, The package-boundary checklist is hardcoding a fixed downstream consumer list, which can miss new workspace packages as the repo grows. Update the review-harness guidance to derive consumer checks from the workspace/import graph instead of naming agent-core, mcp-banking, x402-gateway, a2a, demo, and eval directly; keep the check focused on identifying all packages that import the changed code and verifying they still compile.
181-181: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMake log redaction explicit.
Pass 6 asks for observability and auditability, but it never requires payment or credential logs to be redacted/minimized. In a fintech review harness, that gap can normalize sensitive payload logging as “good observability.”
🛡️ Proposed wording
- | Observability | Can you tell this code ran? Is there logging for failures? Are audit trails maintained for payments and credential operations? | + | Observability | Can you tell this code ran? Are failures logged without sensitive payloads? Are audit trails maintained and redacted for payments and credential operations? |🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/review-harness.md at line 181, The observability checklist currently asks about logging and audit trails but does not explicitly require redaction/minimization of payment or credential data, which can normalize sensitive payload logging. Update the wording in the review harness guidance to make redaction explicit, so any observability/auditability check also asks whether logs from payment and credential operations are minimized and scrubbed of sensitive fields. Keep the change anchored in the observability/auditability prompt text itself so reviewers are prompted to verify safe logging practices.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.claude/review-harness.md:
- Around line 91-95: The package-boundary checklist is hardcoding a fixed
downstream consumer list, which can miss new workspace packages as the repo
grows. Update the review-harness guidance to derive consumer checks from the
workspace/import graph instead of naming agent-core, mcp-banking, x402-gateway,
a2a, demo, and eval directly; keep the check focused on identifying all packages
that import the changed code and verifying they still compile.
- Line 181: The observability checklist currently asks about logging and audit
trails but does not explicitly require redaction/minimization of payment or
credential data, which can normalize sensitive payload logging. Update the
wording in the review harness guidance to make redaction explicit, so any
observability/auditability check also asks whether logs from payment and
credential operations are minimized and scrubbed of sensitive fields. Keep the
change anchored in the observability/auditability prompt text itself so
reviewers are prompted to verify safe logging practices.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8c5cd1fc-72c7-459a-a8dd-f228ef3ef5cf
📒 Files selected for processing (10)
.claude/review-harness.md.mcp.json.reviews/REVIEW-2026-03-27-caip10-validation.md.reviews/REVIEW-2026-03-27-mcp-server-v2.md.reviews/REVIEW-2026-03-27-mcp-server.md.reviews/REVIEW-2026-03-27-payment-request-error-cause.mdpackages/did/scripts/generate-vectors.tspackages/jwt/scripts/generate-vectors.tstools/mcp-server/src/index.tstools/mcp-server/src/util.ts
✅ Files skipped from review due to trivial changes (4)
- .reviews/REVIEW-2026-03-27-mcp-server.md
- .reviews/REVIEW-2026-03-27-caip10-validation.md
- .reviews/REVIEW-2026-03-27-mcp-server-v2.md
- .reviews/REVIEW-2026-03-27-payment-request-error-cause.md
🚧 Files skipped from review as they are similar to previous changes (4)
- tools/mcp-server/src/index.ts
- packages/did/scripts/generate-vectors.ts
- tools/mcp-server/src/util.ts
- packages/jwt/scripts/generate-vectors.ts
# Conflicts: # pnpm-lock.yaml
- Remove check:types script (removed repo-wide per new policy) - Remove @repo/typescript-config devDep (package deleted upstream) - Extend root tsconfig like other tools/ packages - Upgrade @modelcontextprotocol/sdk 1.21.2 → 1.29.0 (Zod v4 compat) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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
Adds an MCP server that exposes ACK identity and payment operations as tools. Any MCP-compatible agent can generate keypairs, create and verify credentials, issue payment requests, and verify receipts.
13 tools provided:
Identity (ACK-ID):
ack_create_controller_credential— create ownership proof (unsigned VC)ack_sign_credential— sign a credential, returns JWTack_verify_credential— verify signature, expiry, trusted issuers, and optionally controller claimsack_resolve_did— resolve did:key, did:web, did:pkhPayments (ACK-Pay):
ack_create_payment_request— create signed payment request token (JWT)ack_verify_payment_request— verify and parse a payment request JWTack_create_payment_receipt— issue an unsigned receipt as a Verifiable Credentialack_verify_payment_receipt— verify a signed payment receiptJWT:
ack_create_jwt— create a signed JWT with arbitrary payload (challenge-response, signed messages)ack_verify_jwt— verify a signed JWT and return decoded payloadUtility:
ack_generate_keypair— generate secp256k1/secp256r1/Ed25519 keypair with did:key DIDack_create_did_web— create a did:web DID URI from a URLack_create_did_pkh— create a did:pkh DID URI from chain ID + wallet addressUses stdio transport. MCP SDK pinned to 1.21.2 for zod 3 compatibility with the workspace.
Supersedes #72 (closed as stale draft). All CodeRabbit review feedback from #72 has been addressed.
Test plan
tsc --noEmit)oxfmt)AI Disclosure
Developed with assistance from Claude Code (Claude Opus).
🤖 Generated with Claude Code
Summary by CodeRabbit