feat(payment): add read-only imperative commands - #2276
Draft
aidandaly24 wants to merge 6 commits into
Draft
Conversation
Contributor
|
Claude Security Review: no high-confidence findings. (run) |
Contributor
There was a problem hiding this comment.
AgentCore Harness Review
Verdict: Looks good
This is a well-scoped read-only surface for AgentCore Payments. I reviewed the diff end-to-end and did not find issues that need to change before merging.
Highlights that made this easy to sign off on:
- Boundaries.
PaymentClientsits cleanly on top of the shared control/data client factories. Data-plane methods route through a singlesendDatahelper that (a) rejects ARN-shapedmanagerIdbefore any client is configured, (b) fetches the manager fresh on every call, (c) refusesCUSTOM_JWTmanagers with aUSER-sourcedInputValidationErrorthat explains the bearer-token limitation, and (d) surfaces aMalformedServiceResponseErrorwhen the service omits the ARN. Each of these branches is asserted inpayment.read.test.ts. - Handlers. Required-flag validation happens before Core is touched, and the tests confirm no SDK client is ever constructed on the failure path (both
omittedandemptyvariants). ARN selectors and the removed--manager-arnflag are explicitly rejected. - Big-number fidelity.
GetPaymentInstrumentBalanceresponses are handed straight to the JSON renderer, andbalance.test.tsxpins down that the atomicamountstring beyondMAX_SAFE_INTEGERand itsdecimalssurvive round-tripping, including the"0"case. The handler correctly does not convert service errors to a zero balance. - Region/endpoint propagation. Both control and data client factories receive the same
{ region, endpoint }derived from context; verified for theeu-west-1+ custom endpoint case. - Test style. Mocks live at the SDK
sendboundary and everything else — router, CoreClient, JSON rendering, IO — runs for real. Fixture replay viafixtureFactoriesgives realistic wire shapes without over-mocking. - Command tree.
payment.read.test.tsxasserts the exact nine-leaf shape (manager get/list,connector get/list,session get/list,instrument get/list/balance) and that no leaf accepts--wait/--browseror exposes TUI, matching the "CLI only for now" stance in the README. - Identity surface. Only
getandlistare wired forpayment-credential-provider, consistent with the read-only scope; the create/update/delete methods added toCoreIdentityClientare unexposed but ready for a follow-up. - Connector hints.
connector getemits theAUTHENTICATION_EXPIRED/AUTHENTICATION_FAILEDwarning only in non-JSON mode and never suggests write commands that don't exist yet — tested for both--jsonand bare invocations.
Nothing blocking. Ship it.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## refactor #2276 +/- ##
============================================
+ Coverage 97.04% 97.06% +0.02%
============================================
Files 566 584 +18
Lines 39353 39918 +565
============================================
+ Hits 38190 38747 +557
- Misses 1163 1171 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
|
Claude Security Review: no high-confidence findings. (run) |
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.
Description
First layer of the AgentCore Payments command stack, targeting
refactor.The dependent CUD layer is #2277.
Adds 11 project-free, headless read commands:
payment manager get|listpayment connector get|listpayment session get|listpayment instrument get|list|balanceidentity payment-credential-provider get|listPrimary selectors use IDs. For session/instrument reads, Core resolves the manager with
GetPaymentManagerin the configured region and supplies the returned ARN to the data plane.Callers therefore need the manager-read permission as well as the requested data-plane action.
JWT-only managers are rejected before data-plane access.
Balance requires an explicit chain, defaults to USDC, preserves atomic amount strings/decimals,
and propagates service errors rather than converting them to zero.
The layer includes read-side Core contracts, injected client wiring, read-only tests, and recorded
Get/List fixtures. It adds no payment mutation handlers, IAM provisioning, credential-write input
handling, or dedicated TUI screens. The CUD layer builds on this branch; read-only TUI is a later PR.
Related Issue
Part of #2272. The dependent CUD PR completes the headless scope.
Documentation PR
N/A. README command tree, read examples, and permission requirements are updated here.
Type of Change
Testing
RECORD=0 bun test --coverage --coverage-reporter=lcov: 3,240 passed, 0 failed across 228 files.bun run typecheckbun run lint:check,bun run format:check,bun run secrets:checkbun run build975d8192: https://github.com/aws/agentcore-cli/actions/runs/34532197010Test source was consolidated from 1,475 to 561 added lines; the PR is now 2,233 additions
instead of 3,448. Production code is unchanged by this cleanup.
Each read command retains fixture-backed root/Core coverage, list pagination is checked at the
SDK boundary, shared manager-resolution failures are tested once in Core, and balance keeps its
chain/default-token/precision/zero/error checks. Unused test-client configuration and write-only
fixtures were removed from this layer.
Uses the
refactorBun scripts rather than the npm scripts from the repository'smaintemplate.No AWS resources were changed while preparing this split.
Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.