Skip to content

fix(cli): Primary (matches DoD 'verify the created payments credent... (#1559)#55

Draft
aidandaly24 wants to merge 1 commit into
mainfrom
fix/1559
Draft

fix(cli): Primary (matches DoD 'verify the created payments credent... (#1559)#55
aidandaly24 wants to merge 1 commit into
mainfrom
fix/1559

Conversation

@aidandaly24

Copy link
Copy Markdown
Owner

Refs aws#1559

Issues

  • bug(flaky-test): payments credential provider setup aws/agentcore-cli#1559 — The payments e2e test (payment-strands-bedrock.test.ts) hardcodes managerName='E2ePayMgr' and connectorName='E2ePayConn', so the derived payment credential-provider name is always the static, account-global value 'E2ePayMgr-E2ePayConn-cdp'. When a prior run died before teardown left the provider orphaned, or when a concurrent CI pipeline shares the account, deploy fails with a 400 'Credential provider with name ... already exists', making the test flaky. No shipped user functionality breaks; this is CI reliability.

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:

Reproduced the symptom by deriving the payment credential-provider name the way PaymentConnectorPrimitive.ts:88 does (${manager}-${connector}-cdp) from the e2e test's chosen names, evaluated twice.

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.

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
@github-actions github-actions Bot added the size/s PR size: S label Jun 25, 2026
@github-actions

Copy link
Copy Markdown

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 37.16% 13593 / 36577
🔵 Statements 36.43% 14452 / 39667
🔵 Functions 31.8% 2333 / 7336
🔵 Branches 31.1% 9000 / 28932
Generated in workflow #109 for commit 18128b7 by the Vitest Coverage Report Action

@github-actions github-actions Bot added agentcore-harness-reviewing AgentCore Harness review in progress and removed agentcore-harness-reviewing AgentCore Harness review in progress labels Jun 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/s PR size: S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant