Uh oh!
There was an error while loading. Please reload this page.
feat(project): add 'project add credentials' (AgentCore Identity api-key + oauth) - #2046
Conversation
OAuth credentials now model the two authoring paths: guided custom OAuth (clientId, discoveryUrl, scopes) and vendored providers (free-form vendor + complete providerConfig). providerConfig rejects nested secret material so secrets can only travel via .env.local or Secrets Manager references, which both credential types record as secretRef/clientSecretRef. The unused usage field is dropped. This shape is the shared contract with @aws/agentcore-cdk (mirror change lands on that repo's refactor branch).
Credentials are the first spec-only resource in FsProjectManager.addResource:
no scaffolding, the spec entry lands in agentcore.json and secret values or
commented placeholders land in agentcore/.env.local (existing keys are never
overwritten, and a missing .gitignore guard for .env.local is restored).
Secret flags accept only stdin ('-') or file:// sources; inline values are
rejected so keys stay out of shell history. External Secrets Manager
references skip .env.local entirely and are recorded in the spec.…path The standalone newline test duplicated the file-secret pipe end to end; the combined test feeds an echo-style newline-terminated file and asserts the stored value, covering both behaviors in one test.
Inline the single-caller placeholder notice, extract the duplicated secret-reference exclusivity parse shared by both leaves, and simplify the envEntries guard.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## refactor #2046 +/- ##
============================================
+ Coverage 97.19% 97.21% +0.02%
============================================
Files 389 394 +5 Lines 23415 23757 +342 ============================================
+ Hits 22758 23096 +338 - Misses 657 661 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
E2E verification of 'project build' against the published @aws/agentcore-cdk found a contract drift: its ConfigIO requires credential names of 3-255 characters while the CLI allowed 1-128, so a 2-character name passed 'add' and then broke synth. Tighten the CLI to the intersection (3-128, service character set) until the L3 schema aligns with the service's 1-128 rule.
tejaskash
commented
Aug 19, 2026
Additional E2E verification: built the artifact and ran 'project build' (real npm install + CDK synth via published @aws/agentcore-cdk 0.1.0-alpha.45) with all three credential shapes in agentcore.json. Synth passes; the published ConfigIO strips the new fields harmlessly since nothing consumes them before deploy work lands. One live contract drift found and contained in 1cd5d14: ConfigIO requires credential names of 3-255 chars while the CLI allowed 1-128, so a 2-char name passed 'add' and then broke 'build'. The CLI now enforces the intersection (3-128). The L3 refactor-branch follow-up should align its name rule with the service's 1-128 and add the new fields (providerConfig, clientId, secretRef, clientSecretRef). |
| throw err; | ||
| } | ||
| if (input.resourceType === "credential" && input.envEntries?.length) { |
There was a problem hiding this comment.
Is there a way we can handle this in the same pattern as scaffolding such that it:
- rolls back entire process on failure.
- avoids a second conditional branch in this function.
One potential way I see to do this would be to implement some interface:
interface LocalEnvAccessor {
upsert(entries: LocalEnvEntries[]): Promise<LocalEnvAccessor>;
rollback(silent?: boolean): Promise<void>
}
Then here, we instantiate it with the project, call upsert in the credential scaffold case, then call rollback where we rollback the scaffolding.
This would also move the envLocal.ts to a class implementation, which feels consistent with how we've done things like SourceResolver.
There was a problem hiding this comment.
I think there is still one rollback case here. .env.local is updated before the complete spec is validated, but rollback only runs when json.write fails. I tried the invalid name ab: the command failed validation and left agentcore.json unchanged, but the environment entry remained. Could validation happen before the env write, or be included in the same rollback boundary?
There was a problem hiding this comment.
Fixed. The whole-spec safeParse now runs inside the same try as the write, so a rejected spec runs the .env.local rollback together with the scaffolded-file cleanup rather than leaving the staged secret behind.
| import type { AddProjectResourceConfig } from "../types"; | ||
| import type { AddResourceInput, EnvLocalEntry } from "../../types"; | ||
| export function createAddCredentialsHandler(config: AddProjectResourceConfig): Router { |
There was a problem hiding this comment.
should we continue to match the command tree with the file tree + 1:1 of file to handler pattern?
| @@ -1,8 +1,11 @@ | |||
| import z from "zod"; | |||
| import { PaymentProviderSchema } from "./payment"; | |||
| // Min 3 keeps names inside what @aws/agentcore-cdk's ConfigIO accepts (3-255) | |||
There was a problem hiding this comment.
i feel like this comment is at risk of becoming stale if the referenced implementation changes.
| for (const key of skipped) { | ||
| yield { message: `'${key}' already exists in ${ENV_LOCAL_RELATIVE_PATH}; left unchanged` }; | ||
| } | ||
| if (await ensureGitignoreCoversEnvLocal(project.rootPath)) { |
There was a problem hiding this comment.
don't we already scaffold the project with it in the .gitignore here
? If the user removes it later, that feels like their own (wrong) choice.| async function run(args: string[], opts?: { core?: TestCoreClient; stdin?: string }) { | ||
| const io = testIO(); | ||
| // testIO's stdin is a PassThrough; pre-filling and ending it simulates piped input. | ||
| if (opts?.stdin !== undefined) (io.io.stdin as unknown as PassThrough).end(opts.stdin); |
There was a problem hiding this comment.
whats the difference between this pattern and
?Maybe OOS here, but is there a way we can standardize on something in
agentcore-cli/src/testing/testIO.tsx
Line 36 in 33e2d2f
envLocal read/write now uses readTextFile and atomicWrite from src/io instead of raw node:fs calls, matching how the rest of core does file IO and making the secrets-file write atomic. Adds coverage for the create-when-missing .env.local branch, which the scaffolded template previously masked. Also drops a review-marker comment.
…drop redundant gitignore write, standardize testIO stdin
…credentials # Conflicts: # src/core/project/manager.tsx # src/handlers/project/add/index.ts # src/handlers/project/types.ts
| /** Reads a file, returning null when it does not exist so callers can tell empty from absent. */ | ||
| async function readOrNull(path: string): Promise<string | null> { | ||
| try { | ||
| return await readTextFile(path); |
There was a problem hiding this comment.
I tried this through the built CLI with node dist/index.js, and it fails here with Bun is not defined because readTextFile() uses Bun.file(). I think this needs a Node-compatible read path, otherwise the Bun tests pass but the published artifact cannot add a credential that writes to .env.local.
There was a problem hiding this comment.
Good catch. The root cause was in readTextFile itself (src/io/fileRead.ts), which called Bun.file, a global that exists only under the Bun runtime. The published bundle targets node, so this broke every caller of that function, not only this path (eval.tsx reads files the same way). Fixed it there by reading through node:fs/promises, which keeps the AbortSignal support. Verified the built artifact with node dist/index.js.
| continue; | ||
| } | ||
| const separator = content === "" || content.endsWith("\n") ? "" : "\n"; | ||
| content += `${separator}# ${entry.comment}\n${entry.key}=${entry.value ?? ""}\n`; |
There was a problem hiding this comment.
I think we need to quote or escape these values before writing them as dotenv. For example, left#right is written as-is, but parseEnv() reads it back as only left. Surrounding spaces are changed too. Silently changing credential material will probably make the provider fail later and be difficult to diagnose.
There was a problem hiding this comment.
Fixed. Values are single-quoted before they are written. node:utilparseEnv treats everything inside single quotes as literal, so left#right, leading and trailing spaces, $, and backslashes all read back byte for byte. The one value single quotes cannot represent is a value that itself contains a single quote, so that case is rejected with a message that points the caller at a Secrets Manager reference instead. A parseEnv round-trip test covers it.
| /** Derives the .env.local variable name a credential's secret is stored under. */ | ||
| export function credentialEnvVarName(credentialName: string, suffix = ""): string { | ||
| return `AGENTCORE_CREDENTIAL_${credentialName.replace(/-/g, "_").toUpperCase()}${suffix}`; |
There was a problem hiding this comment.
One edge case here: foo-bar and foo_bar are both valid credential names, but they both become AGENTCORE_CREDENTIAL_FOO_BAR. I tried adding both and the second command succeeds, but it reuses the first secret because the existing key is skipped. Maybe normalized-name collisions should be rejected, or the mapping should remain one-to-one? What do you think?
There was a problem hiding this comment.
Fixed. addCredentialToProject now rejects a new name that derives the same environment variable as an existing credential, and the error names both. Test added in project.test.ts.
| throw err; | ||
| } | ||
| if (input.resourceType === "credential" && input.envEntries?.length) { |
There was a problem hiding this comment.
I think there is still one rollback case here. .env.local is updated before the complete spec is validated, but rollback only runs when json.write fails. I tried the invalid name ab: the command failed validation and left agentcore.json unchanged, but the environment entry remained. Could validation happen before the env write, or be included in the same rollback boundary?
jariy17
left a comment
There was a problem hiding this comment.
Pretty good beside the upsert Name function
| skipped.push(entry.key); | ||
| continue; | ||
| } | ||
| const separator = content === "" || content.endsWith("\n") ? "" : "\n"; |
There was a problem hiding this comment.
nit: Leave a code comment show the format if you can.
// # <entry.comment>
<entry.key>=<entry.value>
| } | ||
| /** Reads a file, returning null when it does not exist so callers can tell empty from absent. */ | ||
| async function readOrNull(path: string): Promise<string | null> { |
There was a problem hiding this comment.
private this function if no class uses.
There was a problem hiding this comment.
Fixed. readOrNull is now a private method on EnvLocalFile.
| * are left unchanged so user-managed values survive re-runs. Returns the keys | ||
| * written and those skipped. | ||
| */ | ||
| async upsert(entries: EnvLocalEntry[]): Promise<{ written: string[]; skipped: string[] }> { |
There was a problem hiding this comment.
This doesn't update existing keys, it just skips. So the word upsert doesn't work here. Maybe insertIfNew...
There was a problem hiding this comment.
Fixed. Renamed to insertIfNew.
| if (!flags.name) | ||
| throw new InputValidationError("required option '--name <name>' not specified"); | ||
| const secretRef = parseExclusiveSecretRef( |
There was a problem hiding this comment.
Should we combine these flags because they do the same thing and only one of them can be used?
There was a problem hiding this comment.
The two flags carry different meanings, so I kept them apart. --api-key takes the secret value itself (from stdin or a file) and the CLI stores it in .env.local. --api-key-secret-reference names a secret the caller already keeps in Secrets Manager and stores no value. parseExclusiveSecretRef makes sure only one is given. Folding them into one flag would force the CLI to guess whether the argument is a reference or a raw secret, and that ambiguity is how a real secret ends up read as a reference or the reverse.
| @@ -0,0 +1,115 @@ | |||
| import z from "zod"; | |||
There was a problem hiding this comment.
This would be a good example of using templates. We can provide default templates for Google Oauth, Okta and so on.
There was a problem hiding this comment.
Agreed that vendor templates for Google, Okta, and the rest would be a good addition. It is out of scope for this change, which lands the guided-custom and vendored paths. Noted as a follow-up.
| return "harnesses"; | ||
| case "runtime": | ||
| return "runtimes"; | ||
| case "credential": |
There was a problem hiding this comment.
Side Note: I really want to abstract this.
There was a problem hiding this comment.
Leaving it for now. The switch is exhaustive over ProjectResource, and the inferred return type is what keeps unrelated spec keys (name, managedBy) from leaking in, which the doc comment above it calls out. An abstraction here would need to preserve that per-case return typing to earn its place, so I would rather keep the direct mapping until a second use appears.
| * into shell history and process listings, so only stdin and files are allowed. | ||
| * A single trailing newline is stripped (echo and editors add one). | ||
| */ | ||
| export async function resolveSecretFlag( |
There was a problem hiding this comment.
I feel like this should be part of the SourceResolver named resolveSecretValueFlags because it could be use in other locations
There was a problem hiding this comment.
Leaving resolveSecretFlag local for now. Its only callers today are the two credential handlers, which share it through shared.ts. Moving it onto SourceResolver would widen that class for a single use. When a second, unrelated caller needs the same stdin-or-file-only rule, that is the point to lift it.
There was a problem hiding this comment.
It's should be used in runtime too now: https://github.com/aws/agentcore-cli/pull/2035/changes#r3833097791. Shift it into the SourceResolver when you rebase :)
There was a problem hiding this comment.
Done. Moved it onto SourceResolver as resolveSecret (commit a37b46d). It throws SourceResolutionError, which is already a subtype of InputValidationError, so the io package stays free of handler-level error types. Both credential handlers now call resolver.resolveSecret(...), and the runtime handler in #2035 can do the same.
- readTextFile: read via node:fs so the node-target bundle works (Bun.file is a Bun-runtime global, absent under node); options.signal passes through - .env.local values are single-quoted so node:util parseEnv round-trips them byte-for-byte; reject values containing a single quote with a clear message - move spec validation inside the rollback try so a rejected spec reverses the staged .env.local write instead of leaving it behind - reject two credential names that derive the same environment variable - rename EnvLocalFile.upsert to insertIfNew (never overwrites)
| * into shell history and process listings, so only stdin and files are allowed. | ||
| * A single trailing newline is stripped (echo and editors add one). | ||
| */ | ||
| export async function resolveSecretFlag( |
There was a problem hiding this comment.
It's should be used in runtime too now: https://github.com/aws/agentcore-cli/pull/2035/changes#r3833097791. Shift it into the SourceResolver when you rebase :)
The stdin-or-file-only rule now has a second caller (runtime, PR 2035), so resolveSecretFlag moves off the credentials shared module and onto SourceResolver as resolveSecret. It throws SourceResolutionError (already a subtype of InputValidationError), which keeps the io package from depending on handler-level error types. Also bump the generated CDK app template's aws-cdk-lib to ~2.266.0 so it satisfies the @aws/agentcore-cdk peer floor once the credential constructs release.
…credentials # Conflicts: # src/core/project/manager.tsx # src/handlers/project/add/index.ts
What
Adds the project-spec authoring path for AgentCore Identity credential providers:
--vendor(a free string, so new vendors need no CLI release) plus a complete--provider-configurationJSON.-) orfile://sources. Inline values are rejected so keys never reach shell history. A single trailing newline is stripped. Multi-line values are rejected.agentcore/.env.local(AGENTCORE_CREDENTIAL_<NAME>[_CLIENT_SECRET]) and prints a fill-before-deploy notice. Existing keys are never overwritten.{"secretId","jsonKey"}) are recorded in the spec (secretRef/clientSecretRef) and skip.env.local.projectSchemas/credential.ts:providerConfig(rejects nestedclientSecret/apiKey),clientId,secretRef/clientSecretRef. Theusagefield is dropped. Deep validation lives in the schema so every writer path enforces it.Structure
add/credentials/api-key,add/credentials/oauth), matching the command tree. Shared helpers live inadd/credentials/shared.ts..env.localwrites go through anEnvLocalFileclass withupsertandrollback, so a failed spec write reverses the secrets file in the same cleanup path as scaffolded files.Reuse
SourceResolver(stdin and file sources),parseSecretReference, and the identity oauth2config.tshelpers (parseProviderConfigFlags,validateProviderConfigMode).buildProviderConfigInputis not used here, because SDK input construction happens at deploy time.Not in this PR
project deployalready throwsNotImplementedError. The lifecycle design (CFN-owned providers via the newCfnApiKeyCredentialProvider/CfnOAuth2CredentialProviderL1s, convergent pre and post-deploy secret sync from.env.local, hash tracking in deployed state) lands with deploy.@aws/agentcore-cdkcredential construct and the schema mirror in that repo (L3 PR 333).projectcommand tree is undocumented there today, so no partial documentation was added.Testing
FsProjectManager, and filesystem across the three credential shapes and the failure paths. Schema tests cover the reshape.EnvLocalFilehas unit tests for rollback (delete when created, restore prior content, no-op when nothing written). Full suite: 1605 pass.bun run build,node dist/index.js): a real project, all three credential shapes, and the inline-secret and duplicate-name failures exit non-zero with no partial writes.