Skip to content

fix(payments): preserve existing provider names and use connector-compatible names for new providers - #2761

Merged
jariy17 merged 2 commits into
v1.xfrom
fix/payment-arn-mismatch
Nov 5, 2026
Merged

jariy17 merged 2 commits into
v1.xfrom
fix/payment-arn-mismatch

Conversation

@nborges-aws

@nborges-aws nborges-aws commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

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:

  • Give new payment credential providers hyphenated physical names, normalizing underscores
  • Reuse the physical name in each target’s recorded provider ARN for existing credentials. Upgrading the CLI does not rename or replace them
  • Fail before provisioning when a manual connector references an incompatible recorded ARN, with steps to create a new credential and repoint the connector
  • Use recorded physical names when removing payment providers

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.

@nborges-aws
nborges-aws requested a review from a team November 4, 2026 19:00
@github-actions github-actions Bot added the size/m PR size: M label Nov 4, 2026
@agentcore-devx-automation agentcore-devx-automation Bot added claude-security-reviewing Claude Code /security-review in progress agentcore-harness-reviewing AgentCore Harness review in progress and removed agentcore-harness-reviewing AgentCore Harness review in progress labels Nov 4, 2026
@github-actions

github-actions Bot commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

Package Tarball

aws-agentcore-1.0.1.tgz

How to install

gh 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

@agentcore-devx-automation

Copy link
Copy Markdown
Contributor

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

@agentcore-devx-automation agentcore-devx-automation Bot removed the claude-security-reviewing Claude Code /security-review in progress label Nov 4, 2026
@jariy17 jariy17 changed the title fix(payments): use connector-compatible names for new providers fix(payments): preserve existing provider names and use connector-compatible names for new providers Nov 5, 2026
@github-actions github-actions Bot added size/m PR size: M and removed size/m PR size: M labels Nov 5, 2026
@jariy17
jariy17 force-pushed the fix/payment-arn-mismatch branch from 7ee2b4d to e7dd33c Compare November 5, 2026 19:17
@github-actions github-actions Bot added size/m PR size: M and removed size/m PR size: M labels Nov 5, 2026
@agentcore-devx-automation agentcore-devx-automation Bot added the claude-security-reviewing Claude Code /security-review in progress label Nov 5, 2026
@agentcore-devx-automation

Copy link
Copy Markdown
Contributor

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

@agentcore-devx-automation agentcore-devx-automation Bot removed the claude-security-reviewing Claude Code /security-review in progress label Nov 5, 2026
@codecov-commenter

codecov-commenter commented Nov 5, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.11538% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.55%. Comparing base (272585d) to head (12e7b6c).

Files with missing lines Patch % Lines
src/core/project/backends/cdk/credentials.ts 96.80% 3 Missing ⚠️
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.
📢 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.

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}`;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@nborges-aws
nborges-aws force-pushed the fix/payment-arn-mismatch branch from e7dd33c to 12e7b6c Compare November 5, 2026 20:26
@github-actions github-actions Bot added size/m PR size: M and removed size/m PR size: M labels Nov 5, 2026
@agentcore-devx-automation agentcore-devx-automation Bot added the claude-security-reviewing Claude Code /security-review in progress label Nov 5, 2026
@agentcore-devx-automation

Copy link
Copy Markdown
Contributor

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

@agentcore-devx-automation agentcore-devx-automation Bot removed the claude-security-reviewing Claude Code /security-review in progress label Nov 5, 2026
@agentcore-devx-automation agentcore-devx-automation Bot added the agentcore-harness-reviewing AgentCore Harness review in progress label Nov 5, 2026
@jariy17
jariy17 merged commit 950a0f4 into v1.x Nov 5, 2026
27 of 28 checks passed
@jariy17
jariy17 deleted the fix/payment-arn-mismatch branch November 5, 2026 21:16

@agentcore-devx-automation agentcore-devx-automation Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 because previousName === 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.

@agentcore-devx-automation agentcore-devx-automation Bot removed the agentcore-harness-reviewing AgentCore Harness review in progress label Nov 5, 2026
@nborges-aws

Copy link
Copy Markdown
Contributor Author

I'm implementing option A from harness reviewer finding as a follow up. Will publish PR shortly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/m PR size: M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Refactor] project add credentials payment does not match Connector regex

4 participants