fix(project): deploy payments through the published AgentCorePayments construct - #2186
fix(project): deploy payments through the published AgentCorePayments construct#2186aidandaly24 wants to merge 7 commits into
Conversation
agentcore-devx-automation
Bot
added
agentcore-harness-reviewing
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-devx-automation
Bot
removed
the
claude-security-reviewing
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## refactor #2186 +/- ##
=========================================
Coverage 97.13% 97.14%
=========================================
Files 541 542 +1
Lines 37618 37700 +82
=========================================
+ Hits 36542 36624 +82
Misses 1076 1076 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
agentcore-devx-automation
Bot
removed
the
agentcore-harness-reviewing
![agentcore-devx-automation[bot]](https://avatars.githubusercontent.com/u/2232217?s=60&v=4){kind=small}
There was a problem hiding this comment.
AgentCore Harness Review
Verdict: Looks good
Deleting the vendored payment orchestration in favor of the published AgentCorePayments construct is the right call — it fixes the Quick Create synth throw in bin/cdk.ts (which required every connector to resolve a credentialName) and the id-collision bug in cdk-stack.ts, and it moves future Payment changes onto a dependency bump rather than a scaffold regeneration.
Things I checked and liked:
- The new
paymentConnectors.tsreporter never fails the deploy:try/catcharoundlistStackManagerIdsand around each per-managerreportManagercall downgrade errors to a "could not be retrieved" step. The tests cover both branches (top-level and per-manager failure). PaymentConnectorCallsis a clean SDK-boundary seam. The unit tests use an in-memoryfakeAccountrather than mocking the AWS SDK, which matches the project's testing guidance.paymentManagerIdcorrectly strips the manager id out of the CFN-reported ARN and passes a bare id through, so it stays working if CFN ever changes what it reports.- The reporter is invoked only after
updateTargetStateon the successful-deploy path (teardown returns earlier), and skips all AWS calls when the spec declares no Quick Create connectors, so projects without Payments pay nothing. AgentCorePayments@0.1.0-alpha.51's exportedAgentCorePaymentsPropsmatches whatcdk-stack.tspasses (spec,credentials,agentCoreApplication).- Restored tests cover the
credentialName-less Quick Create synth path, the manual/Quick Create mix, and the id-collision cases (foo_bar/foobar,A/BCvsAB/C) that motivated the fix.
No changes requested.
… construct The generated CDK app has been carrying its own Payment orchestration since #2146 restored a pre-Quick-Create adapter to unblock scaffolding while AgentCorePayments was unpublished. That construct now ships in @aws/agentcore-cdk 0.1.0-alpha.51, so the vendored loops are obsolete. Quick Create is broken on refactor today: bin/cdk.ts requires every connector to resolve a credentialName to a deployed ARN, and a Quick Create connector deliberately has neither, so it throws before synthesis. The restored adapter also concatenates underscore-stripped manager and connector names into construct IDs, so foo_bar/foobar and A/BC versus AB/C collide, and the tests covering both behaviors were deleted alongside it. bin/cdk.ts now passes the raw spec and the target's deployed credentials straight through, and cdk-stack.ts hands them to AgentCorePayments, which owns connector variant mapping, credential resolution, runtime wiring and outputs inside the upgradeable package. Payment orchestration leaves the frozen scaffold, so future Payment changes reach existing projects through a dependency bump rather than a regenerated app. The generated-CDK tests for manual and Quick Create synthesis and for collision-safe identities are restored.
A Quick Create connector is deployed but unusable until someone follows its authorization link, and deploy said nothing about it. The link had to be dug out of the stack's CloudFormation outputs to be found at all, and it expires about ten minutes after the connector is created, so by the time anyone thought to look the window was usually gone. The project reported a successful deploy either way. Deploy now names each Quick Create connector the spec declares, with its live status and, while one exists, the link and how long it lasts. An expired connector says how to get a new link, which is to recreate it: neither UpdatePaymentConnector nor redeploying an unchanged connector mints another one. The status and the link are read from the Payments service rather than the outputs of the same name. Those outputs are Fn::GetAtt values resolved when the connector was created, so they keep serving a dead link and a stale PENDING_AUTHENTICATION long after the service has moved on to AUTHENTICATION_EXPIRED. Managers are found by their CloudFormation resource type in the project's own stack. Manager names are account-scoped, so matching the spec against the account would confuse two projects that both declare `payments`, and reading the L3's output keys would tie the CLI to names the L3 owns. Connectors then come from the manager, so connector names cannot collide across managers. Reads run after the stack is up and never fail the deploy: a status that cannot be retrieved is reported as such. A project without Quick Create connectors makes no calls at all.
The authorization report asked the Payments service about a manager using the physical resource id CloudFormation gives for it, which is the manager's ARN. `paymentManagerId` accepts only the bare identifier and rejects an ARN against its own pattern, so every deploy of a project with a Quick Create connector reported that the connector's status could not be retrieved instead of handing over its authorization link. Only reachable against CloudFormation, so the unit tests could not see it: they fake the service boundary, and the boundary was the defect.
…utput The generated app does not read this output, and neither does the CLI: the authorization URL is read from the Payments service because it expires. The L3 is removing the output for that reason, since an output holding an attribute that disappears fails every later stack update. Asserting it here only pins a value nothing consumes to whichever L3 version is pinned, so the pin bump can stand alone.
921232d to
94ea166
Compare
|
Rebased onto The
That is the only typecheck error in the tree. Everything else on this branch is green: Verified end to end against a real account since the last push, including the paths that only real AWS can exercise:
|
agentcore-devx-automation
Bot
added
the
claude-security-reviewing
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-devx-automation
Bot
removed
the
claude-security-reviewing
The generated app hands the whole spec to AgentCorePayments, which derives every connector's construct identity. Feeding it colliding names asserted that derivation, not anything this package decides, and the L3 covers the same foo_bar/foobar case in its own suite. Seventy lines of fixture for one resource count, over code owned elsewhere. The manual and Quick Create synthesis test stays: it is the only check that the generated app passes the spec and the target's deployed credentials to the construct at all, which is exactly what regressed when the vendored Payment loops were restored.
agentcore-devx-automation
Bot
added
the
claude-security-reviewing
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-devx-automation
Bot
removed
the
claude-security-reviewing
agentcore-devx-automation
Bot
added
the
claude-security-reviewing
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-devx-automation
Bot
removed
the
claude-security-reviewing
| const ids = paymentConnectorIds(resource.PhysicalResourceId); | ||
| if (!ids) continue; | ||
|
|
||
| const connector = await paymentsClient.send( |
There was a problem hiding this comment.
Could we catch GetPaymentConnector failures per resource and continue processing?
Currently, one stale or throttled connector lookup stops the entire loop, preventing authorization URLs from being reported for later Quick Create connectors. This can also happen if an unrelated manual connector lookup fails.
Please add a test where the first lookup fails and a subsequent connector still reports its authorization URL.
There was a problem hiding this comment.
Updated in 1bf77c64. The catch now wraps only the individual GetPaymentConnector SDK request, emits a connector-scoped warning on failure, and continues processing later resources. I added the requested regression where the first lookup throws and the second still reports its authorization URL. Focused backend tests pass 43/43; lint, format, and build also pass.
agentcore-devx-automation
Bot
added
the
claude-security-reviewing
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-devx-automation
Bot
removed
the
claude-security-reviewing
| }, | ||
| "dependencies": { | ||
| "@aws/agentcore-cdk": "0.1.0-alpha.45", | ||
| "@aws/agentcore-cdk": "0.1.0-alpha.51", |
There was a problem hiding this comment.
I presume this is the crucial bit. Do we know how these changes will impact existing projects? There's a lot of changes here.
4 participants
What
Retires the Payment orchestration the generated CDK app has been carrying, and makes
project deployprint a Quick Create connector's authorization URL while the service makes one available.Why
#2146restored a pre-Quick-Create adapter into the generated app to unblock scaffolding whileAgentCorePaymentswas unpublished. That construct now ships in@aws/agentcore-cdk@0.1.0-alpha.51, so the vendored loops are obsolete. While they remain:bin/cdk.tsrequires every connector to resolve acredentialNameto a deployed ARN. A Quick Create connector deliberately has neither, so synthesis throws before constructing the stack.cdk-stack.tsconcatenates underscore-stripped manager and connector names into construct IDs, allowing distinct names to collide.Quick Create is also a two-phase operation: deployment creates the connector, then someone must follow its short-lived authorization URL. A successful deploy that does not print that URL leaves the connector unusable.
How
bin/cdk.tspasses the raw spec and target credentials through toAgentCorePayments. The published construct owns connector variant mapping, credential resolution, runtime wiring, IAM, and outputs instead of the frozen generated scaffold.After a successful deploy, the CLI:
GetPaymentConnector.authorizationUrlwhen the response contains one.That is the complete post-deploy behavior. It does not implement connector status reporting or recovery guidance. A project without Quick Create connectors performs no additional reads, and a read failure never turns a completed deployment into a failure.
Verification
Verified the service contract directly against account
603141041947inus-east-1:CreatePaymentConnectorreturnedPENDING_AUTHENTICATIONwith an authorization URL.GetPaymentConnectorreturned the same status and URL.READYconnector returned no authorization URL.Code validation:
bun run lintbun run format:checkbun run buildThe earlier full lifecycle verification still covers:
READYREADYremove alldeleting the stack, manager, connector, and credential providerFollow-ups
aws/agentcore-l3-cdk-constructs#348is merged but unreleased. It removes ephemeralAuthorizationUrlandConnectorStatusstack outputs that can break later stack updates. This PR still pins alpha.51; the pin moves to alpha.52 once published. The CLI requires no behavioral change because it reads the URL fromGetPaymentConnector.