Skip to content

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

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

feat(evaluator): add kmsKeyArn support for custom evaluator#993
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 field to the evaluator primitive to support customer-managed KMS encryption.

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

Type of Change

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

Testing

How have you tested the change?

Testing

  • 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.

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

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 #2035 for commit ba4c55c by the Vitest Coverage Report Action

@agentcore-cli-automation
Copy link
Copy Markdown

The kmsKeyArn field added here is persisted into agentcore.json but is never actually applied to the deployed evaluator, so the feature has no effect at runtime. Specifically:

  1. CDK L3 construct does not read it. AgentCoreEvaluator in aws/agentcore-l3-cdk-constructs (src/cdk/constructs/components/primitives/evaluator/AgentCoreEvaluator.ts) never references evaluator.kmsKeyArn, and the EvaluatorSchema in that repo does not include a kmsKeyArn field. When deploy synthesizes CFN, the value is silently dropped. There is also no companion PR in agentcore-l3-cdk-constructs to add it.

  2. CloudFormation resource does not support it. AWS::BedrockAgentCore::Evaluator only accepts Description, EvaluatorConfig, EvaluatorName, Level, and Tags (confirmed via CFN docs and aws-cdk-lib's generated CfnEvaluator type — which exposes kmsKeyArn only on the Gateway resource, not Evaluator). Even if the construct were updated to forward it, CFN would reject the template.

  3. Service API does not support it either. CreateEvaluatorRequest and GetEvaluatorResponse in @aws-sdk/client-bedrock-agentcore-control (latest) do not contain a kmsKeyArn field. That's why the new getEvaluator code has to cast with (response as unknown as Record<string, unknown>).kmsKeyArn — there is no typed field to read, and in practice the service will never return one. The new toEvaluatorSpec test passes only because the test manually constructs a GetEvaluatorResult with kmsKeyArn set; round-tripping via the actual SDK will always yield undefined.

Net effect: a user who runs agentcore evaluator add --kms-key-arn … (or enters a KMS ARN in the TUI wizard) will see the ARN written to agentcore.json, but the deployed evaluator will not be encrypted with that key. That's worse than not having the option, because it silently implies encryption is configured.

Options to resolve:

  1. Hold this PR until the backend supports it. Ship kmsKeyArn on the evaluator once the service API, the CFN resource, and the L3 construct all accept it. At that point, update this PR together with a matching change in agentcore-l3-cdk-constructs so the field is actually forwarded to CfnEvaluator/CfnResource props.

  2. Drop the evaluator kmsKeyArn entirely for now. If the intent was to mirror what already exists for the identity token vault (identityKmsKeyArn), note that those are separate code paths and don't feed evaluator encryption.

  3. If there is a documented non-CFN path (e.g., a post-deploy SDK call that applies KMS to an evaluator), wire it up in the deploy pipeline — right now nothing reads evaluator.kmsKeyArn from the spec at deploy time (verified with grep -rn 'evaluator.*kmsKeyArn' src/ — only hit is the wizard's own memo).

Happy to re-review once the plumbing is end-to-end.

@github-actions github-actions Bot removed the agentcore-harness-reviewing AgentCore Harness review in progress label Apr 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/s PR size: S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants