fix(cli): Primary (matches DoD 'verify the created payments credent... (#1559)#55
Draft
aidandaly24 wants to merge 1 commit into
Draft
fix(cli): Primary (matches DoD 'verify the created payments credent... (#1559)#55aidandaly24 wants to merge 1 commit into
aidandaly24 wants to merge 1 commit into
Conversation
The flaky payments e2e test (e2e-tests/payment-strands-bedrock.test.ts) hardcoded managerName='E2ePayMgr' and connectorName='E2ePayConn', making the derived payment credential-provider name the static account-global value 'E2ePayMgr-E2ePayConn-cdp', which collides with orphaned providers from crashed prior runs or concurrent CI pipelines. Randomize those two names per run via new src/test-utils/run-naming.ts helpers, mirroring the file's existing agentName (Date.now slice) and testDir (randomUUID) randomization. PaymentConnectorPrimitive.ts:88 (the user-visible name builder) was deliberately NOT touched. Refs aws#1559
Coverage Report
|
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.
Refs aws#1559
Issues
Root cause
The payment credential-provider name is deterministic and the e2e test feeds it constant inputs, while the provider name is unique only per account+region (no project scoping). (1) src/cli/primitives/PaymentConnectorPrimitive.ts:87-88 builds credentialName =
${options.manager}-${options.name}-${credentialSuffix}(suffix 'cdp'/'stripe-privy'), no random component; unit tests at src/cli/primitives/tests/PaymentConnectorPrimitive.test.ts:103,172,174 lock 'mgr1-conn1-cdp'. (2) e2e-tests/payment-strands-bedrock.test.ts:30-31 hardcodes managerName/connectorName even though the SAME file randomizes testDir (:39 randomUUID) and agentName (:42 Date.now()), so the provider name is always exactly 'E2ePayMgr-E2ePayConn-cdp'. (3) At deploy, src/cli/operations/deploy/pre-deploy-identity.ts:670-676 (createOrUpdatePaymentCredentialProvider) does getPaymentCredentialProvider then createPaymentCredentialProvider — a TOCTOU: a concurrent run that creates the provider between the GET (returns null at agentcore-payments.ts:271 on 404) and the POST makes the POST return the 400 'already exists' wrapped at agentcore-payments.ts:222-224. The credential provider is created imperatively in pre-deploy BEFORE the CFN stack, and is deleted only by a successful teardown deploy via cleanupPaymentCredentialProviders (src/cli/commands/deploy/actions.ts:512), so a failed run leaves the static-named provider orphaned and the next run collides. The get-then-update-or-create + static name was added in PR aws#1261 (commit 4469333) and is unchanged at HEAD; the 'Credential provider for ...' wrapper in the issue body was removed in aws#1525 (1f5d794), confirming the report was filed against aws#1261 and the mechanism persists at v0.20.2.The fix
Primary (matches DoD 'verify the created payments credentials have a unique suffix'): randomize managerName/connectorName per run in e2e-tests/payment-strands-bedrock.test.ts:30-31 exactly as the same file already randomizes agentName/testDir — e.g. const suffix = String(Date.now()).slice(-8) (or randomUUID().slice(0,8)); managerName =
E2ePayMgr${suffix}; connectorName =E2ePayConn${suffix}. This makes the derived provider name unique per run and closes both the orphaned-prior-run and concurrent-pipeline collision windows. Optional CLI hardening for real users sharing an account: in createOrUpdatePaymentCredentialProvider (pre-deploy-identity.ts:670-676) catch the 400/'already exists' from createPaymentCredentialProvider and fall back to updatePaymentCredentialProvider — turning the racy get-then-create into create-or-recover. Do NOT add a random suffix inside PaymentConnectorPrimitive.ts:88: that name is user-visible, persisted in agentcore.json and env-var names, and asserted by unit tests, so randomizing it would be a breaking behavior change. Test-side fix is preferred.Files touched: e2e-tests/payment-strands-bedrock.test.ts:30-31 (randomize managerName/connectorName per run, mirroring agentName at :42 and testDir at :39). Optional hardening: src/cli/operations/deploy/pre-deploy-identity.ts:670-676 (createOrUpdatePaymentCredentialProvider get-then-create -> create-or-recover on 'already exists'). Root of the deterministic name is src/cli/primitives/PaymentConnectorPrimitive.ts:88 — do NOT change it.
Validation evidence
The fix was verified by reproducing the original symptom and re-running after the change:
BEFORE (pre-fix static names
managerName='E2ePayMgr',connectorName='E2ePayConn'): run1 = 'E2ePayMgr-E2ePayConn-cdp', run2 = 'E2ePayMgr-E2ePayConn-cdp', equal = TRUE. This is the constant account-global name with no per-run scoping -> collides with orphaned providers from a crashed prior run or a concurrent CI pipeline -> intermittent 400 'Credential provider with name E2ePayMgr-E2ePayConn-cdp already exists'.AFTER (fix on branch fix/1559, using shipped src/test-utils/run-naming.ts helpers): run1 = 'E2ePayMgr33445566-E2ePayConn33445566-cdp', run2 = 'E2ePayMgr33449999-E2ePayConn33449999-cdp'. Asserted (a) matches /^E2ePayMgr\d{8}-E2ePayConn\d{8}-cdp$/ (carries per-run 8-digit suffix), (b) two independent evaluations differ (run1 !== run2), and (c) neither equals the static 'E2ePayMgr-E2ePayConn-cdp'. Symptom no longer reproduces. This mirrors the existing agentName/testDir randomization in the same file.
The shipped regression test src/test-utils/run-naming.test.ts encodes exactly this (passes). PaymentConnectorPrimitive.ts:88 is untouched and PaymentConnectorPrimitive.test.ts still asserts the static 'mgr1-conn1-cdp' (lines 103,172,174) and passes — fix is test-side only as required.
Test suite: green.
Staged on the fork as a draft for human review. Promote to aws/agentcore-cli after vetting.