Repository navigation
fix(payments): preserve existing provider names and use connector-compatible names for new providers - #2761
Conversation
Package TarballHow to installgh release download pr-2761-tarball --repo aws/agentcore-cli --pattern "*.tgz" --dir /tmp/pr-tarball
npm install -g /tmp/pr-tarball/aws-agentcore-1.0.1.tgz |
|
Claude Security Review: no high-confidence findings. (run) |
7ee2b4d to
e7dd33c
Compare
|
Claude Security Review: no high-confidence findings. (run) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v1.x #2761 +/- ##
=======================================
Coverage 96.54% 96.55%
=======================================
Files 706 706
Lines 58626 58720 +94
=======================================
+ Hits 56603 56695 +92
- Misses 2023 2025 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| const parts = [project.name, targetName, credential.name]; | ||
| const suffix = createHash("sha256").update(JSON.stringify(parts)).digest("hex").slice(0, 16); | ||
| const prefix = parts.join("-").replaceAll("_", "-"); | ||
| return `${prefix.slice(0, PROVIDER_NAME_MAX_LENGTH - suffix.length - 1)}-${suffix}`; |
There was a problem hiding this comment.
When a payment credential has nothing recorded, this always takes the hashed name. If the account already has the old ${project}_${target}_${cred} provider, the deploy creates a second one next to it, and the original (which holds the real vendor secrets) is never recorded and never deleted. Base reused it by name. The remover has the same path at L240: with nothing recorded it deletes the hashed name, which doesn't exist, so it reports success while the old provider stays.
The trigger is narrow. deployed-state.json became a committed file on 2026-08-27 (#2105) and payments landed 2026-08-28 (#2120), so it needs a project created before that gitignore change which added payments later, or someone ignoring the state file.
I don't think adopting the legacy name is the fix: it's underscored, so a connector still can't use it, and trying both names on delete could remove an account-global provider we don't own (the delete path has no vendor check, unlike requireVendorMatch).
Two cheaper options. Which do you prefer?
(a) When nothing is recorded and a provider exists under the legacy name, fail with "a provider named 'example_dev_wallet' exists but is not recorded for this target. Restore deployed-state.json, or delete that provider if nothing uses it."
(b) Same lookup, but warn and carry on creating the hashed provider.
(a) is safer, (b) doesn't block CI.
There was a problem hiding this comment.
Agree with this finding. I've updated this PR with option A: the code now fails with an error message explaining the lost state and outlining recovery paths.
…patible names for new providers
e7dd33c to
12e7b6c
Compare
|
Claude Security Review: no high-confidence findings. (run) |
There was a problem hiding this comment.
AgentCore Harness Review
Verdict: Changes requested
The new naming + guard are right for v1-native and raw-v0 projects, but projects converted by agentcore migrate (migratedFrom: "v1", project.legacy === undefined) fall between the two cases and can get blocked on a path that works today.
1. migratedFrom: "v1" projects can be hard-blocked by the new guard (src/core/project/backends/cdk/credentials.ts)
previousProviderName (L49–56) treats both project.legacy and migratedFrom: "v1" as keeping v0's bare credential name, but the payment branch of providerName (L73–84) only special-cases project.legacy (L77). For a migrated project the "previous" payment provider name is therefore the account-global bare name (wallet), which every target of that project shares, while the computed name is the new hashed one.
The guard at L339–353 then fires for any migrated project where that bare provider exists in AWS but is not recorded for the target being deployed. The clearest reachable case: a migrated project that deployed its payment credential on target default (so wallet exists and is recorded under default) and now adds target prod. Before this PR, prod reused the bare provider and deployed; now the deploy fails with
Payment credential 'wallet' has no provider recorded for target 'prod' … but provider 'wallet' exists in AWS. Restore the target's deployed-state.json entry, or delete that provider if unused, then re-deploy.
and both suggestions are wrong there: there is no prod entry to restore, and deleting wallet would break default, which does use it. The user has no way forward short of hand-editing deployed-state.json.
The remover has the mirrored problem: at L248–249 a migrated project's payment credential with no recorded physicalName now resolves to the hashed name, so the delete targets a provider that does not exist, swallows ResourceNotFoundException, and reports success while the bare-named provider stays — the same silent-success shape already raised for L240.
Options:
- (a) Treat migrated projects like raw v0 ones in the payment branch:
if (keepsV0CredentialNames(project)) return credential.name;at L77. Restores base behavior for migrated projects, and the guard can no longer fire for them becausepreviousName === provider. Their payment names stay underscored, which the connector check at L102–126 already reports with recovery steps. - (b) Keep the new hashed naming for migrated projects but scope the guard to real state loss: pass the whole deployed state (or the set of payment provider names recorded across all targets) into the provisioner and only fail when the previous name is recorded for no target, so another target's shared bare-named provider is not mistaken for lost state.
- (c) Minimum: skip the guard when
keepsV0CredentialNames(project)is true and the previous name is a bare credential name, since its existence says nothing about this target.
Whichever you pick, the describe("projects migrated from v0") block (credentials.test.ts L1143–1170) only exercises API_KEY; please add a payment case for a migrated project — both the "bare provider exists for another target" path and the teardown/removal path — since that is the combination this PR changes and nothing currently covers.
2. Minor: recorded ARNs are parsed without checking what kind of entry they are
providerName L74–76 and assertPaymentConnectorCredentials L110–115 call paymentProviderNameFromArn on whatever is recorded under the credential name, ignoring existing.authorizerType. If that entry is an API-key/OAuth record for the same name (a credential whose authorizerType was edited in agentcore.json, or stale state), the deploy now dies with "has an invalid recorded payment provider ARN … in deployed-state.json" instead of just creating the payment provider and overwriting the stale entry. Either gate the reuse on existing.authorizerType being PaymentCredentialProvider (or absent, for v0-written entries), or treat an ARN that is not a payment-provider ARN as "nothing recorded" rather than a fatal state error.
Everything else checks out: the hash inputs are deterministic (I verified example-dev-wallet-02961cdcdac06c7f and example-dev-pay-a5e6e3eef03187be against sha256(JSON.stringify(parts))), truncation keeps names ≤128 and collision-free, the connector ARN comes from deployed-state.json rather than being recomposed by the CDK app, the connector check correctly skips QUICK_CREATE connectors (no credentialName), and the cdk.ts test proves nothing is provisioned and state is untouched when the check fails.
|
I'm implementing option A from harness reviewer finding as a follow up. Will publish PR shortly |
Fixes #2533
Description
Fix incompatibility between payment provider and provider ARN accepted by payment connectors. Provider API allows underscores in the ARN, but connectors do not accept underscored ARNs. This means that a provider assigned a underscored ARN can never be attached to a payment connector.
Fixed by:
Type of Change
Testing
How have you tested the change?
bun testbun run test:e2e, or explained why they are not applicable(CI does not run them for you)
bun run typecheckbun run lint:checkbun run format:checkbun run auditbun run secrets:checkbun run buildsrc/assets/, I updated affected snapshots withbun test <test-file> --update-snapshotsand committed themby this PR
command.md(the release workflow regenerates it)Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.