Skip to content

feat(evaluator): add kmsKeyArn support for custom evaluator#994

Draft
aws-aditya21 wants to merge 1 commit intomainfrom
feat/evaluator-kms-key-arn
Draft

feat(evaluator): add kmsKeyArn support for custom evaluator#994
aws-aditya21 wants to merge 1 commit intomainfrom
feat/evaluator-kms-key-arn

Conversation

@aws-aditya21
Copy link
Copy Markdown

@aws-aditya21 aws-aditya21 commented Apr 27, 2026

Description

Add optional kmsKeyArn support for custom evaluators, enabling customers to specify a KMS key for evaluator encryption at rest.

Changes (9 files)

  • src/schema/schemas/agentcore-project.ts — added kmsKeyArn to EvaluatorSchema
  • src/cli/primitives/EvaluatorPrimitive.ts — added --kms-key-arn CLI flag, updated options, action handler, and createEvaluator
  • src/cli/tui/screens/evaluator/types.ts — added kms-key-arn step and kmsKeyArn to AddEvaluatorConfig
  • src/cli/tui/screens/evaluator/useAddEvaluatorWizard.ts — added KMS state, callback, wired into all wizard flows
  • src/cli/tui/screens/evaluator/AddEvaluatorScreen.tsx — added KMS key ARN text input and confirm field
  • src/cli/tui/hooks/useCreateEvaluator.ts — passes kmsKeyArn through to primitive
  • src/cli/commands/import/import-evaluator.tstoEvaluatorSpec forwards kmsKeyArn from API response
  • src/cli/aws/agentcore-control.ts — added kmsKeyArn to GetEvaluatorResult
  • src/cli/commands/import/__tests__/import-evaluator.test.ts — added tests for kmsKeyArn forwarding

Companion PR

CDK constructs: https://github.com/aws/agentcore-l3-cdk-constructs/pull/184

CFN support for EncryptionKeyArn on AWS::BedrockAgentCore::Evaluator is assumed ready per service team confirmation.

Closes #

Type of Change

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

Testing

How have you tested the change?

  • I ran npm run test:unit and npm run test:integ
  • I ran npm run typecheck
  • I ran npm run lint
  • If I modified src/assets/, I ran npm run test:update-snapshots and committed the updated snapshots

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 github-actions Bot added size/s PR size: S agentcore-harness-reviewing AgentCore Harness review in progress labels Apr 27, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 27, 2026

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines Unknown% 0 / 0
🔵 Statements Unknown% 0 / 0
🔵 Functions Unknown% 0 / 0
🔵 Branches Unknown% 0 / 0
Generated in workflow #2040 for commit fe5cd4b by the Vitest Coverage Report Action

@aws-aditya21 aws-aditya21 changed the title feat(evaluator): Add kmsKeyArn support for custom evaluator feat(evaluator): add kmsKeyArn support for custom evaluator Apr 27, 2026
@github-actions github-actions Bot removed the agentcore-harness-reviewing AgentCore Harness review in progress label Apr 27, 2026
@aws-aditya21 aws-aditya21 force-pushed the feat/evaluator-kms-key-arn branch from ba4c55c to fe5cd4b Compare April 27, 2026 23:46
@github-actions github-actions Bot added size/m PR size: M and removed size/s PR size: S labels Apr 27, 2026
@aws aws deleted a comment from agentcore-cli-automation Apr 28, 2026
@aws-aditya21 aws-aditya21 reopened this Apr 28, 2026
@github-actions github-actions Bot added size/m PR size: M agentcore-harness-reviewing AgentCore Harness review in progress and removed size/m PR size: M labels Apr 28, 2026
@aws aws deleted a comment from agentcore-cli-automation Apr 28, 2026
@agentcore-cli-automation
Copy link
Copy Markdown

Field naming inconsistency: kmsKeyArn vs encryptionKeyArn

Other primitives in both agentcore-cli and agentcore-l3-cdk-constructs schemas use encryptionKeyArn for KMS key fields — see MemorySchema (src/schema/schemas/agentcore-project.ts:142) and PolicyEngineSchema (src/schema/schemas/primitives/policy.ts:71). This PR introduces kmsKeyArn only for the Evaluator, which is inconsistent and will be confusing for users editing agentcore.json.

Additionally, CFN uses EncryptionKeyArn on AWS::BedrockAgentCore::Evaluator (per this PR's description), which maps to encryptionKeyArn in camelCase. The companion CDK PR (aws/agentcore-l3-cdk-constructs#184) will almost certainly use encryptionKeyArn to stay consistent with the other primitives — if so, the kmsKeyArn field in the CLI's agentcore.json won't flow through to the CDK construct.

Options:

  1. Rename the project-schema field to encryptionKeyArn to match existing primitives (recommended). The CLI flag --kms-key-arn can stay as-is or be renamed to --encryption-key-arn for consistency.
  2. If there's a deliberate reason to diverge (e.g., the service team named the API field kmsKeyArn), call it out and consider renaming the Memory/Policy fields instead — but that would be a breaking change.

Please confirm the field name with the companion CDK PR before merging, since whatever is in agentcore.json must match what the CDK schema expects.

@agentcore-cli-automation
Copy link
Copy Markdown

Unsafe cast in getEvaluator likely masks a wrong SDK field name

src/cli/aws/agentcore-control.ts:545:

kmsKeyArn: (response as unknown as Record<string, unknown>).kmsKeyArn as string | undefined,

This double-cast bypasses TypeScript's checking of the SDK response type. Compare with how getMemoryDetail reads the analogous field directly as memory.encryptionKeyArn (line 415) — typed and safe.

Given that CFN exposes this as EncryptionKeyArn (per the PR description) and the service's SDK convention matches CFN PascalCase → camelCase (e.g. memory.encryptionKeyArn), the SDK field on GetEvaluatorResponse is almost certainly encryptionKeyArn, not kmsKeyArn. If that's correct, this line will silently always return undefined at runtime, so agentcore import evaluator will never preserve the KMS key on imported evaluators — and the existing unit tests won't catch it because they construct GetEvaluatorResult objects directly instead of exercising the SDK → result mapping.

Options:

  1. Use the real SDK field name directly (e.g. kmsKeyArn: response.encryptionKeyArn) once the SDK typings are updated. Drop the as unknown cast so TypeScript verifies the field exists.
  2. If the SDK types aren't updated yet, at minimum add a comment explaining why the cast is needed and add an integration/mapping test that stubs the SDK response and verifies the field is forwarded — so this regression is caught when the SDK types land.

Either way, please verify by inspecting the actual GetEvaluatorResponse shape (or the service's API model) that kmsKeyArn is the correct field name before merging.

@agentcore-cli-automation
Copy link
Copy Markdown

KMS key ARN validation regex is too restrictive

The regex used in both EvaluatorPrimitive.ts (non-interactive flag validation) and AddEvaluatorScreen.tsx (TUI input validation):

/^arn:aws(?:|-cn|-us-gov):kms:[a-zA-Z0-9-]*:[0-9]{12}:key\/[a-zA-Z0-9-]{36}$/

will reject several valid KMS key ARN formats:

  1. Alias ARNsarn:aws:kms:us-east-1:123456789012:alias/my-key. KMS alias ARNs are commonly used and accepted by most AWS services for encryption configuration.
  2. Other AWS partitionsaws-iso, aws-iso-b, aws-iso-e, aws-iso-f are not matched. Less critical but worth noting.
  3. Custom key store keys — may have IDs that don't fit the 36-char UUID pattern.

Additionally, duplicating the same regex in two places risks them drifting; consider extracting to a shared helper (e.g. isValidKmsKeyArn) alongside the other schema helpers.

Options:

  1. Loosen the regex to accept both key/... and alias/... resource parts, e.g. ^arn:aws[a-z0-9-]*:kms:[a-z0-9-]+:[0-9]{12}:(key|alias)\/.+$, and extract it to a shared helper.
  2. Skip client-side ARN format validation entirely (let the AWS control-plane API reject bad ARNs on deploy). Other primitives in this repo don't appear to do client-side KMS ARN regex validation.
  3. Keep the strict validation but at least extract the regex to a single constant so both call sites stay in sync.

@github-actions github-actions Bot removed the agentcore-harness-reviewing AgentCore Harness review in progress label Apr 28, 2026
@aws-aditya21
Copy link
Copy Markdown
Author

KMS key ARN validation regex is too restrictive

The regex used in both EvaluatorPrimitive.ts (non-interactive flag validation) and AddEvaluatorScreen.tsx (TUI input validation):

/^arn:aws(?:|-cn|-us-gov):kms:[a-zA-Z0-9-]*:[0-9]{12}:key\/[a-zA-Z0-9-]{36}$/

will reject several valid KMS key ARN formats:

  1. Alias ARNsarn:aws:kms:us-east-1:123456789012:alias/my-key. KMS alias ARNs are commonly used and accepted by most AWS services for encryption configuration.
  2. Other AWS partitionsaws-iso, aws-iso-b, aws-iso-e, aws-iso-f are not matched. Less critical but worth noting.
  3. Custom key store keys — may have IDs that don't fit the 36-char UUID pattern.

Additionally, duplicating the same regex in two places risks them drifting; consider extracting to a shared helper (e.g. isValidKmsKeyArn) alongside the other schema helpers.

Options:

  1. Loosen the regex to accept both key/... and alias/... resource parts, e.g. ^arn:aws[a-z0-9-]*:kms:[a-z0-9-]+:[0-9]{12}:(key|alias)\/.+$, and extract it to a shared helper.
  2. Skip client-side ARN format validation entirely (let the AWS control-plane API reject bad ARNs on deploy). Other primitives in this repo don't appear to do client-side KMS ARN regex validation.
  3. Keep the strict validation but at least extract the regex to a single constant so both call sites stay in sync.

@aws-aditya21 aws-aditya21 reopened this Apr 28, 2026
@github-actions github-actions Bot added size/m PR size: M agentcore-harness-reviewing AgentCore Harness review in progress and removed size/m PR size: M labels Apr 28, 2026
@agentcore-cli-automation
Copy link
Copy Markdown

Reviewed the PR. The three existing automation comments (field naming kmsKeyArn vs encryptionKeyArn, unsafe SDK cast in getEvaluator, and overly-restrictive KMS ARN regex) cover the serious issues I found — no additional blockers from me. The naming issue in particular should be resolved against the companion CDK PR before merging, since the existing MemorySchema in agentcore-l3-cdk-constructs uses encryptionKeyArn and a mismatch would cause the field to be silently stripped at deploy time.

@github-actions github-actions Bot removed the agentcore-harness-reviewing AgentCore Harness review in progress label Apr 28, 2026
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.

2 participants