[nborges-aws]

Description

Follow up based on harness reviewer findings in PR #2761.

#2761 introduced connector-compatible names for new payment providers. For projects converted by agentcore migrate, it chose a new hashed name when the current target had no recorded provider, while its lost-state guard still looked for the old bare name.

This change makes migrated projects keep their v0 bare payment provider names, matching their API key and OAuth behavior. It also checks the recorded provider type before parsing an ARN as a payment provider ARN, so a stale API key or OAuth entry does not fail payment provisioning.

Type of Change

• Bug fix
• New feature
• Breaking change
• Documentation update
• Other (please describe):

Testing

How have you tested the change?

• I ran bun test
• I ran the relevant end-to-end tests with bun run test:e2e, or explained why they are not applicable
  (CI does not run them for you)
• I ran bun run typecheck
• I ran bun run lint:check
• I ran bun run format:check
• I ran bun run audit
• I ran bun run secrets:check
• I ran bun run build
• If I modified src/assets/, I updated affected snapshots with bun test <test-file> --update-snapshots and committed them
• If I re-recorded fixtures or golden files, every change beyond account identifiers is explained
  by this PR
• This PR does not change command.md (the release workflow regenerates it)

Checklist

• I have read the CONTRIBUTING document
• I have added any necessary tests that prove my fix is effective or my feature works
• I have updated the documentation accordingly
• I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
• My changes generate no new warnings
• Any dependent changes have been merged and published

---

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.

[github-actions]

Package Tarball

aws-agentcore-1.0.1.tgz

How to install

gh release download pr-2771-tarball --repo aws/agentcore-cli --pattern "*.tgz" --dir /tmp/pr-tarball
npm install -g /tmp/pr-tarball/aws-agentcore-1.0.1.tgz

[agentcore-devx-automation]

Claude Security Review: no high-confidence findings. (run)

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.55%. Comparing base (950a0f4) to head (8a350a3).
⚠️ Report is 1 commits behind head on v1.x.

Additional details and impacted files

@@           Coverage Diff           @@
##             v1.x    #2771   +/-   ##
=======================================
  Coverage   96.55%   96.55%           
=======================================
  Files         707      707           
  Lines       58769    58849   +80     
=======================================
+ Hits        56743    56823   +80     
  Misses       2026     2026           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[agentcore-devx-automation]

AgentCore Harness Review

Verdict: Changes requested

The core fix is right: option (a) from the #2761 follow-up is implemented cleanly (keepsV0CredentialNames in the payment branch of providerName, L78), the guard at L351–353 can no longer fire for migrated projects because previousName === provider, and recordedPaymentCredential (L91–100) correctly stops a stale API-key/OAuth entry from being parsed as a payment ARN in all three call sites. The new tests use the in-memory Identity account and real temp dirs — no excessive mocking — and the two migrated-project cases are exactly the ones that were missing.

Two things worth resolving before merge.

1. New payment credentials in migrated projects can get a connector-incompatible name, with no preflight (src/core/project/backends/cdk/credentials.ts L78, L114–134)

CredentialNameSchema allows _, and #2761 added the hashed replaceAll("_", "-") name precisely because CreatePaymentConnector rejects underscored provider names — the test "uses a connector-compatible name for a new credential containing underscores" (credentials.test.ts L723) pins that.

With this change, a project that agentcore migrate converted and that then runs agentcore credential add --type payment --name wallet_us plus a connector referencing it gets the bare name wallet_us. Nothing catches that before the write: assertPaymentConnectorCredentials only inspects recorded entries, and there is nothing recorded on a first deploy. So the provider is created, state is recorded, synth succeeds, and the deploy fails inside CloudFormation on CreatePaymentConnector; only the next deploy produces the good ProjectStateError with recovery steps. On today's v1.x this same project deploys successfully with the hashed name, so this is a reachable regression rather than pure v0 parity — a credential added after migration has no v0 provider or agent code to preserve.

Options:

• (a) In providerName, keep the bare name for keepsV0CredentialNames projects only when it is connector-compatible: if (keepsV0CredentialNames(project) && CONNECTOR_PAYMENT_NAME_PATTERN.test(credential.name)) return credential.name; and otherwise fall through to the hashed name. Preserves every v0 name that could ever have worked with a connector, and new underscored names stay usable.
• (b) Extend the preflight to the name the credential will get: have assertPaymentConnectorCredentials compute providerName(...) when nothing is recorded and raise the existing ProjectStateError ("create a payment credential under a different name…") before any provider is written. Keeps strict v0 naming but fails fast with guidance instead of mid-stack.
• (c) Keep the bare name only for credentials that actually exist in v0 — the guard already does a getPaymentCredentialProvider(previousName) lookup, so the bare name can be adopted when that provider exists and the hashed name used when it does not.

Whichever you pick, please add a test for a migrated project with an underscored new payment credential; that is the case this PR changes and nothing covers it.

2. Removal paths can now delete a bare-named payment provider another target shares (L231–238, L262, and cdk.ts L406–411 / L477–487)

The new test "a new target reuses the bare payment provider recorded by another target" establishes that for migrated projects a single account-global wallet provider backs every target. The removal side has no corresponding guard: the new test "removes a migrated payment provider by its bare name without recorded state" asserts the remover deletes wallet outright. So dropping the credential from the spec on target prod, or tearing prod down, deletes the provider default is still using and still records — the comment at L231–239 ("A provider's name is scoped to this project and target, so nothing outside this target can be using it") no longer holds for payment providers of v0/migrated projects.

Part of this is pre-existing (v0 and pre-#2761 state behave the same way), but this PR re-establishes sharing as a supported state and adds a test that pins the unconditional delete, so it should be a deliberate decision. Options: (a) in cdk.ts, where the whole deployed state is already read, skip deleting a payment provider whose physical name is recorded by another target (and say so in a step message); (b) emit a warning for the keepsV0CredentialNames payment case so the user knows a shared provider is going away; (c) if the behavior is intentional v0 parity, state that in the comment at L229–239 rather than leaving the "scoped to this target" claim, which is now false for this case.

Minor

recordedPaymentCredential treats a recorded entry with no authorizerType as a payment entry. That matches what was suggested in the #2761 review for v0-written entries, but it leaves the stale-entry hole half-open: a pre-authorizerType entry holding an API-key ARN under the same credential name still dies in paymentProviderNameFromArn. Either additionally treat an ARN that does not match /paymentcredentialprovider/ as "nothing recorded", or add a one-line comment on L95–98 recording why the absent case is assumed to be payment — the rule is not self-evident from the code.

[agentcore-devx-automation]

Claude Security Review: no high-confidence findings. (run)

[agentcore-devx-automation]

AgentCore Harness Review

Verdict: Looks good

This revision addresses all three points from the previous pass, and I could not find a new blocking issue.

What I verified:

• New payment credentials in migrated projects (credentials.ts L73–85): providerName now only keeps the bare name when it is connector-compatible (CONNECTOR_PAYMENT_NAME_PATTERN). Since CredentialNameSchema allows exactly [a-zA-Z0-9-_], that pattern test is precisely "has no underscore," so the connector-incompatible set is fully covered — an underscored new credential in a migrated project gets the hashed name and deploys ("a new underscored payment credential deploys with a connector-compatible name").
• The underscored-but-already-exists case is handled explicitly in the new preflight loop (L363–400): the v0 bare provider is adopted when nothing references it from a connector, and when a connector does reference it the deploy fails with a ProjectStateError naming the connector, manager, credential, target, and provider — before any write, and before secret resolution. The old lost-state error is preserved verbatim for non-migrated projects, and migrated/legacy projects short-circuit on previousName === provider, so the guard that misfired in #2761 can no longer fire. Tests cover adopt, reject-with-connector, and recorded-bare-provider.
• Shared-provider removal (cdk.ts L116–140, L527–551): both removal call sites (deploy orphans and teardown) now go through removeUnsharedCredentials, and createCredentialRemover has no other caller in the tree, so there is no second surface that still deletes unconditionally. The ARN match correctly pins service/region/account against the current target and compares the full /paymentcredentialprovider/<name> segment (so wallet does not match my-wallet), and the last target torn down still deletes the provider because removeTargetState has already dropped the other entries. The doc comment on createCredentialRemover was updated rather than left asserting the now-false "scoped to this target" claim.
• recordedPaymentCredential (L97–114) closes the stale-entry hole in all three call sites, including the untyped-ARN case the previous review asked about, and the comment on L105 says why.

Non-blocking observations, take or leave:

• The physical name is derived twice: the guard uses provider.physicalName ?? provider.name (cdk.ts L123) while the remover uses physicalName ?? providerName(project, targetName, …, {}) (credentials.ts L278–280). These agree in every case where the guard can actually match (a bare-named match implies a v0/migrated project, where providerName also returns the bare name), so I could not construct a realistic divergence — but a single exported resolver used by both would remove the need to re-derive that argument on the next change. Relatedly, targetsRecordingPaymentProvider is the only place outside credentials.ts that parses a payment provider ARN; it would sit naturally next to paymentProviderNameInArn.
• Raw v0 (project.legacy) projects still get the bare name unconditionally, so an underscored v0 payment credential referenced by a connector fails inside CreatePaymentConnector with no preflight. That is pre-existing and arguably unreachable (v0 itself could not have created such a connector), but it is now the one asymmetry with the migrated path.

The verify / Windows failure is in src/io/source.test.ts:112, unrelated to this change.

[agentcore-devx-automation]

Claude Security Review: no high-confidence findings. (run)

[agentcore-devx-automation]

AgentCore Harness Review

Verdict: Changes requested

The core of this revision still holds up: providerName keeps the bare v0 name only when it is connector-compatible (credentials.ts L72–85), recordedPaymentCredential (L95–112) stops a stale API-key/OAuth/untyped entry from being parsed as a payment ARN in all three call sites, and both removal call sites now go through removeUnsharedCredentials (cdk.ts L530–552), which is the only caller of removeCredentials. Tests use the in-memory Identity account and real temp dirs — no excessive mocking.

One thing from the latest push needs resolving.

targetsRecordingPaymentProvider's service allowlist (src/core/project/backends/cdk.ts L131–136)

const parts = arn.split(":");
return (
  (parts[2] === "bedrock-agentcore" || parts[2] === "acps") &&
  ...

acps appears nowhere else in either repo, in any branch other than this commit, and every recorded payment-provider ARN in the tree (src/handlers/identity/payment-credential-provider/__fixtures__/*, credentialSecrets.test.ts, credentials.test.ts's paymentArn) uses bedrock-agentcore. Only the teardown test in this PR uses it, and it was switched to acps in the last push — so the literal is an unexplained magic string whose only evidence is the test that was edited to match it.

The failure mode is asymmetric and silent: an ARN whose service token is not in the list is treated as "not shared," and the provider another target still records is deleted with no warning. That is exactly the destruction this guard exists to prevent, and nothing catches it — there is no test for a non-matching service id, so dropping the check entirely would not fail the suite either.

It is also inconsistent with the rest of the payment-ARN handling: paymentProviderNameInArn and providerRefFromRecorded in credentials.ts recognize a payment provider purely by the /paymentcredentialprovider/<name> segment, with no service check. So the remover will happily derive a physical name from an ARN that this guard refuses to recognize as the same resource.

Options:

• (a) Drop the service comparison. Partition/region/account plus the trailing /paymentcredentialprovider/<name> segment already identify the resource, and this matches how credentials.ts parses the same ARNs.
• (b) Keep it, but add a comment saying where acps comes from (an actual service response, presumably) and add a test for a third/unknown service token so the negative case is pinned rather than incidental.

While you are in there: src/core/arn.ts already exports parseArn, which handles a resource part containing colons; arn.split(":") with positional indices re-implements it.

Non-blocking

• The last push replaced the adopt path with the original lost-state error (credentials.ts L362–380), pinned by "does not adopt an unrecorded underscored provider". For the reachable case — migrated project, underscored payment credential, new target — this means the deploy dead-ends: the suggested recovery ("Restore the target's deployed-state.json entry, or delete that provider if unused") requires hand-editing state for a target that was never deployed, or deleting a provider the other target is using. This matches v1.x today so it is not a regression, but if the error is now the intended terminal behavior for that case, it would help for the message to name it (e.g. that a migrated project's v0 provider can be adopted by recording it for this target).
• A target dropped from the spec without a teardown leaves its state entry behind, and that entry now pins the shared payment provider alive indefinitely. Minor leak, no action needed unless you want to filter state.targets to targets the spec still declares.