Uh oh!
There was an error while loading. Please reload this page.
feat: create credential providers before synthesizing a deploy - #2123
feat: create credential providers before synthesizing a deploy#2123notgitika wants to merge 9 commits into
Conversation
There was a problem hiding this comment.
AgentCore Harness Review
Verdict: Looks good
Nice PR. The design of running credential provisioning before cdk synth and threading the ARNs through deployed-state.json is well-motivated and the comments do a great job of capturing why. The seam boundary (IdentityProviderClient) is drawn at the SDK client rather than at fs/process boundaries, so the tests avoid excessive mocking while still exercising the real spec + .env.local parsing paths. Sharing credentialEnvVarName/CLIENT_SECRET_SUFFIX between add and deploy via envLocal.ts (with a re-export from shared.ts) removes a latent format-drift bug.
A couple of small things that aren't blockers but worth confirming intentional:
Stale credential entries when the spec goes to zero credentials. In
src/core/project/backends/cdk.ts(~L115)updateTargetStateis only called whenObject.keys(provisioned).length > 0. If a user deletes their last credential fromagentcore.jsonand re-deploys,provisionedis{}, the state write is skipped, and the previousresources.credentialsmap is left on disk. The doc-comment onupdateTargetStatepromises "A resource map provided in the patch replaces the previous map for that kind wholesale, so a credential dropped from the spec stops being advertised" — that guarantee is only actually delivered when at least one credential remains. Since the synthesized CDK app looks up credentials by name, this is likely inert in practice, but if you want the drop-to-zero case to behave the same as drop-one-of-many, you'd either always callupdateTargetState({ resources: { credentials: provisioned } })or explicitly write{}whendeclaredis non-empty on the spec side but you provisioned nothing.parseEnvcast inEnvLocalFile.read(src/core/project/envLocal.tsL90):parseEnv's declared return type isRecord<string, string | undefined>(last-write-wins across duplicate keys), but you cast toRecord<string, string>. All callers happen to useif (!value)so undefined is handled safely today; just be aware the type is a small lie and a future caller doingenv[k].trim()would compile but crash.
Neither of these needs to block the merge.
4b787a8 to
0db4266Compare
The synthesized CDK app reads credential provider ARNs out of deployed-state.json and fails to synth a project that declares credentials until they exist. Provision them between the account preflight and the build, then record their ARNs via updateTargetState so the assembly is synthesized against a state file that already describes them. Providers are created when absent and reused when present, never updated, so a redeploy neither mints a new secret version nor overwrites one rotated outside the CLI. Payment credentials are rejected up front (agentcore.json can't express the vendor config they need). Secrets come from the same place 'project add credentials' writes them, so the env-var name is now derived from one function in envLocal.ts that both sides share.
0db4266 to
3c09c23CompareCodecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## refactor #2123 +/- ##
==========================================
Coverage 97.29% 97.29% ==========================================
Files 478 479 +1 Lines 29642 29880 +238 ==========================================
+ Hits 28840 29073 +233 - Misses 802 807 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Claude Security Review: no high-confidence findings. (run) |
…v type - Add SDK-mocked coverage for createIdentityProviderClient (the real Identity factory the provisioner tests bypass): ~55% -> ~95% on credentials.ts. - Always record the provisioned credential set, so removing the last credential from the spec clears the stale entry instead of leaving it advertised. - EnvLocalFile.read returns Record<string, string | undefined> (parseEnv's real type) rather than casting it away. - Tighten a few verbose comments.
Claude Security Review: no high-confidence findings. (run) |
Claude Security Review: no high-confidence findings. (run) |
… lint - Keep the 'same environment variable' wording so the existing add-credentials collision test still asserts it, and add an integration test for the cross-type case (oauth 'foo' vs api-key 'foo_client_secret'). - Silence require-yield on a deploy-prereq spy generator that never runs.
Claude Security Review: no high-confidence findings. (run) |
| return { | ||
| customOauth2ProviderConfig: { | ||
| oauthDiscovery: { discoveryUrl: credential.discoveryUrl }, | ||
| ...(credential.clientId !== undefined && { clientId: credential.clientId }), |
There was a problem hiding this comment.
I think older OAuth projects need a compatibility fallback here. v0.28.0 stored the client ID only in AGENTCORE_CREDENTIAL_<NAME>_CLIENT_ID, not in agentcore.json. If the provider needs to be recreated in another target/account or after deletion, this sends the discovery URL and secret without the client ID. Would it make sense to prefer credential.clientId and fall back to the legacy environment variable, with an upgrade test?
Resolve conflicts in cdk.ts (combine credential provisioning + prereq/validate with refactor's teardown/stack-probe deploy path) and cdk.test.ts (keep both provisionCredentials and stackExists harness options; use the deployInput helper).
Claude Security Review: no high-confidence findings. (run) |
Older CLIs stored an OAuth credential's client id in AGENTCORE_CREDENTIAL_<NAME>_CLIENT_ID rather than agentcore.json. When recreating such a provider, prefer credential.clientId and fall back to that legacy variable so an upgraded project keeps its client id. Adds an upgrade test.
Claude Security Review: no high-confidence findings. (run) |
Refactor independently added the credential env-var collision check (credentialEnvironmentVariableNames) and moved credentialEnvVarName into projectSchemas/credential. Adopt those: drop the duplicate collision helper and naming from shared.ts/envLocal.ts, source credentialEnvVarName from the schema in the provisioner, and inline the _CLIENT_SECRET/_CLIENT_ID suffixes. Keep the provisioner, EnvLocalFile.read(), and the legacy _CLIENT_ID clientId fallback.
Claude Security Review: no high-confidence findings. (run) |
| ? { name: credential.name, secretRef: credential.secretRef } | ||
| : { | ||
| name: credential.name, | ||
| apiKey: requireEnvSecret(credential.name, env, rootPath, "secretRef"), |
There was a problem hiding this comment.
I think we already have all the code we need to do this in the existing core package. Let's reuse that. We can discuss strategies for making that code reusable if it's not clear.
Creates a project's credential providers before synthesis, so the synthesized CDK app can read their ARNs out of
deployed-state.jsonand wire them into the stack. Without this, deploying a project that declares any credential fails insidecdk synth.What it does
CdkBackend.deployprovisions each declared credential provider: reuse-if-present, create-if-absent, never update (so a redeploy neither mints a new secret version nor overwrites one rotated outside the CLI), then records the ARNs viaupdateTargetState.project add credentialswrites them —.env.local(AGENTCORE_CREDENTIAL_<NAME>) or a Secrets ManagersecretRef. The env-var name is now derived from one shared function inenvLocal.tssoaddanddeployagree.Note
Tested e2e
Ran the full flow against a real account (us-west-2):
project add credentials api-key→project deploy. Confirmed deploy creates the API-key credential provider imperatively before synth, writes it toagentcore/.cli/deployed-state.json, then deploys the stack and merges thestackArninto the same target entry (no clobbering). Resulting file held bothresources.credentials.e2ekeyandstackArn.