feat(eval): add eval evaluator CLI commands (CRUD + LLaJ/code-based) - #1822
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## refactor #1822 +/- ##
============================================
+ Coverage 94.92% 95.21% +0.29%
============================================
Files 155 170 +15
Lines 7565 8026 +461
============================================
+ Hits 7181 7642 +461
Misses 384 384 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
AlexanderRichey
left a comment
There was a problem hiding this comment.
Really good work here. Left a few comments. Main blocking things are using types vs interfaces appropriately and handling duplication of source reading.
6db704d to
0a2afdb
Compare
Add the imperative `agentcore eval evaluator` command surface: llm-as-a-judge create/update, code-based create/update, and type-agnostic get/list/delete. CLI-only for now; the TUI follows later. - New CoreEvalClient (consumer-owned interface) + EvalClient impl over the Bedrock AgentCore control plane. Update paths do get-then-merge since UpdateEvaluator replaces the whole evaluatorConfig union. - Rating scale accepts a preset (--rating-scale) or raw JSON (--rating-scale-json). - Source-aware field values: inline, file://<path>, or - for stdin. - --timeout has no CLI default; the service applies its own. - list --type filters the returned page client-side (the API paginates only): Builtin | code-based | llm-as-a-judge. - delete requires --yes (headless-safe confirmation). Tests: handler behavior via TestCoreClient, EvalClient get-then-merge unit tests, and source resolver tests.
A single --rating-scale flag now takes either a preset id or a source-aware custom RatingScale (JSON inline, file://<path>, or -). A value matching a known preset id expands to that preset; anything else is parsed as a RatingScale JSON value.
…rop --yes - Split llm-as-a-judge and code-based create/update into their own directories per the documented handler convention; shared flags/helpers live in a sibling utils.tsx. - LlmAsAJudgeUpdate / CodeBasedUpdate are now type aliases (data-carrying), leaving CoreEvalClient as the implemented interface. - Drop --yes from evaluator delete to stay consistent with the other CRUDL commands.
- Reject a type mismatch in both update paths before merging: UpdateEvaluator replaces the whole evaluatorConfig union, so merging into the wrong arm silently converted an evaluator to the other type. - Preserve the existing bedrockEvaluatorModelConfig (inferenceConfig, additionalModelRequestFields) and override only modelId; same for lambdaConfig. - Throw the modeled InputValidationError instead of TypeError in the handlers and core, per the errors module. - Drop the client-side `--type` filter on list: ListEvaluators has no server-side type filter, so filtering a page could return empty results while later pages held matches. - Move the handler tests to the real CoreClient with fixture-backed SDK clients and golden output, recorded against the shared test account, matching the Harness/Runtime/Identity pattern. Removes src/core/eval.test.ts, which tested the core implementation directly. - Rename llm-as-a-judge/utils.tsx to sharedFlags.tsx and hoist the duplicated LEVELS into evaluator/levels.tsx. - Fix the stale README entries for `--yes` and the `--type` filter.
The update path used to rebuild bedrockEvaluatorModelConfig from modelId alone, dropping inferenceConfig and additionalModelRequestFields. The existing tests only proved instructions and ratingScale survived, because no CLI flag can set inferenceConfig: `--model` carries a model id. Seed an evaluator carrying an inferenceConfig through the SDK during recording, then assert an instructions-only update leaves the tuning intact. Fixtures are keyed by request input, so the recorded UpdateEvaluator fixture also pins the exact request: reintroducing the bug changes the hash and fails the test.
0a2afdb to
122b13f
Compare
- `--timeout` documented a 1-300 range but validated nothing: 0, 301, and 1.5 all reached the Core client. Declare z.number().int().min(1).max(300) on create and update, matching harness/exec. - The not-found test used an id that fails the service's evaluator-id pattern, so it recorded a ValidationException while claiming to cover ResourceNotFoundException, and a bare rejects.toThrow() let the mismatch pass. Use a well-formed absent id and assert the error name, as Identity does. - Fix the README llm-as-a-judge example: SESSION instructions require an allowed placeholder, and the model id was truncated. Fixtures re-recorded, so the evaluator ids in them change.
|
one thing I noticed testing e2e is that the help pages no longer tell us whats required since we aren't using commander's required option: I think this is an issue with a lot of the work we've done though, not specific to here. the apis are definitely a little awkward, but the commands themselves work great! |
Addressed his comments and two other reviewers reviewed it
Adds `agentcore eval online-eval create/get/list/update/pause/resume/delete`, following the conventions established in the eval evaluator review (#1822). - `--agent <id>` accepts a plain AgentCore Runtime ID or a Harness ID and derives the CloudWatch data source from it. A harness is itself backed by a runtime, so resolution tries GetAgentRuntime and falls back to GetHarness, reading the underlying runtime out of the harness environment. The log group is keyed by runtime *id*, the service name by runtime *name* — verified against live resources, the two are not interchangeable. - `--log-group-name`/`--service-name` are the escape hatch for agents emitting traces under a custom OTel service name rather than AgentCore's default path. - No execution role required at create time: Core auto-provisions and reuses a default one, mirroring the harness execution-role pattern. The policy grants Logs Insights query access over `aws/spans` plus the runtime's log-group prefix, since the service validates query access at the runtime level and rejects a role scoped to a single endpoint. - `update` merges over the current config because UpdateOnlineEvaluationConfig replaces the whole `rule`, so unset fields are preserved. - `list` exposes only the API's server-side pagination; no client-side filters.
Adds the imperative `agentcore eval online-eval` surface (CLI only), following the conventions established in the eval evaluator review (#1822). - `--agent <id>` accepts a plain AgentCore Runtime ID or a Harness ID and derives the CloudWatch data source from it. A harness is itself backed by a runtime, so resolution tries GetAgentRuntime and falls back to GetHarness, reading the underlying runtime out of the harness environment. The log group is keyed by runtime *id*, the service name by runtime *name* — verified against live resources, the two are not interchangeable. - `--log-group-name`/`--service-name` are the escape hatch for agents emitting traces under a custom OTel service name rather than AgentCore's default path. - No execution role required at create time: Core auto-provisions and reuses a default one, mirroring the harness execution-role pattern. The policy grants Logs Insights query access over `aws/spans` plus the runtime's log-group prefix, since the service validates query access at the runtime level and rejects a role scoped to a single endpoint. - `update` merges over the current config because UpdateOnlineEvaluationConfig replaces the whole `rule`, so unset fields are preserved. - `list` exposes only the API's server-side pagination; no client-side filters. - Lives on EvalClient / CoreEvalClient rather than a sibling core sub-client: Core clients are scoped to a CLI namespace, as HarnessClient spans harness, versions, endpoints, invoke, and exec.
Adds the imperative `agentcore eval online-eval` surface (CLI only), following the conventions established in the eval evaluator review (#1822). agentcore eval online-eval ├── create ├── get ├── list ├── update ├── pause # executionStatus -> DISABLED ├── resume # executionStatus -> ENABLED └── delete Data source. `--agent` accepts a plain AgentCore Runtime ID or a Harness ID and derives the CloudWatch source from it: a harness is itself backed by a runtime, so resolution tries GetAgentRuntime and falls back to GetHarness, reading the underlying runtime out of the harness environment. The log group is keyed by the runtime *id* and the service name by the runtime *name* — verified against live resources; the two are not interchangeable. `--data-source-config` takes the API's DataSourceConfig as JSON for agents that emit traces under a custom OTel service name, which no derivation can guess. update takes the same two entry points plus an endpoint re-scope, in precedence order: --data-source-config replaces the source outright, --agent re-derives it, and --endpoint/--clear-endpoint re-scope the agent the config was already built from (recovering its runtime id from the stored log group). Merge semantics. UpdateOnlineEvaluationConfig does partial updates at the top level — an omitted field is untouched — but sending `rule` replaces the whole object, so the client merges the untouched sub-fields over the current config. sessionConfig is optional on Rule and the service does not backfill it, so it is omitted when unset rather than materializing the service's own default. Flags are validated against the service model's documented ranges (--sampling-rate 0.01-100, --session-timeout-minutes an int in 1-1440). --data-source-config and --filters are source-aware (inline, file://<path>, or - for stdin). --role-arn is required: the CLI does not provision an execution role. Testing. Handler tests use the fixture/golden pattern (real CoreClient behind a recorded SDK seam). The recording creates one config, exercises get/list/update/pause/resume, then deletes it, so it leaves no residue. Beyond the recorded flow, the following were exercised against the live API: file:// and stdin sources, the one-stdin-per-command guard, all three update data-source paths applied in sequence to one config, and the client-side rejection of --endpoint on a config built from custom log groups.
* feat(eval): add eval online-eval CLI commands Adds the imperative `agentcore eval online-eval` surface (CLI only), following the conventions established in the eval evaluator review (#1822). agentcore eval online-eval ├── create ├── get ├── list ├── update ├── pause # executionStatus -> DISABLED ├── resume # executionStatus -> ENABLED └── delete Data source. `--agent` accepts a plain AgentCore Runtime ID or a Harness ID and derives the CloudWatch source from it: a harness is itself backed by a runtime, so resolution tries GetAgentRuntime and falls back to GetHarness, reading the underlying runtime out of the harness environment. The log group is keyed by the runtime *id* and the service name by the runtime *name* — verified against live resources; the two are not interchangeable. `--data-source-config` takes the API's DataSourceConfig as JSON for agents that emit traces under a custom OTel service name, which no derivation can guess. update takes the same two entry points plus an endpoint re-scope, in precedence order: --data-source-config replaces the source outright, --agent re-derives it, and --endpoint/--clear-endpoint re-scope the agent the config was already built from (recovering its runtime id from the stored log group). Merge semantics. UpdateOnlineEvaluationConfig does partial updates at the top level — an omitted field is untouched — but sending `rule` replaces the whole object, so the client merges the untouched sub-fields over the current config. sessionConfig is optional on Rule and the service does not backfill it, so it is omitted when unset rather than materializing the service's own default. Flags are validated against the service model's documented ranges (--sampling-rate 0.01-100, --session-timeout-minutes an int in 1-1440). --data-source-config and --filters are source-aware (inline, file://<path>, or - for stdin). --role-arn is required: the CLI does not provision an execution role. Testing. Handler tests use the fixture/golden pattern (real CoreClient behind a recorded SDK seam). The recording creates one config, exercises get/list/update/pause/resume, then deletes it, so it leaves no residue. Beyond the recorded flow, the following were exercised against the live API: file:// and stdin sources, the one-stdin-per-command guard, all three update data-source paths applied in sequence to one config, and the client-side rejection of --endpoint on a config built from custom log groups. * feat(eval): auto-provision the online eval execution role CreateOnlineEvaluationConfig requires a role the service can assume, and the policy it validates is not obvious: spans live in `aws/spans` rather than the runtime's own log group, and query access must be scoped to the runtime log-group *prefix* — a policy pinned to a single endpoint is rejected. Both took live failures to pin down, surfacing only as "the provided execution role does not have permissions to access the specified log groups", so leaving it to the caller made `create` hard to use. Core now provisions a default role scoped to the resolved log groups unless --role-arn is passed, mirroring the harness execution-role pattern and the design doc's auto-provisioning philosophy. A freshly created role is not immediately assumable (IAM is eventually consistent) and the service rejects the create rather than retrying, so the create is retried on that specific error with a bounded backoff — only when we provisioned the role, since a caller-supplied one that cannot be assumed is a real misconfiguration and should fail fast. Fixtures re-recorded: the create flow now captures the GetRole/CreateRole/ PutRolePolicy calls, and the recorded policy scopes to the runtime prefix. * fix(eval): address review on the online-eval execution role Four fixes from @notgitika's review: Role name (onlineEvalExecutionRole.tsx). `AgentCoreOnlineEval-` is 20 chars, so slicing to IAM's 64-char cap kept only the first 44 characters of a config name that can run to 100. Two configs sharing that prefix mapped to one role, and because provisioning is idempotent by name the second create silently re-scoped the first's policy to a different runtime. Long names are now truncated with a hash suffix. Role scope on update (eval.tsx). Moving the data source left the role scoped to the old runtime, so sampling stopped with no error — the service validates the role's log-group access on create, not update. A CLI-provisioned role (identified by its derived name) is now re-scoped when the source moves; --update-role false opts out. A role named via --role-arn is never edited, and update accepts --role-arn so a caller can replace it. Both skip paths return a warning that the handler prints to stderr, naming the role and the log groups it needs access to. IAM endpoint leak (eval.tsx). The IAM client was built with toClientConfig, which forwards --endpoint-url; IAM is global and must not receive the agentcore override. Now passes { region } only, matching harness.tsx. KMS (kms:Decrypt for encrypted evaluators) is deliberately not addressed here. Verified against the live API: the policy re-scopes from one runtime to another on --agent, --update-role false skips it and warns, and --role-arn leaves a custom role's own policy untouched. The role-warning test lives in its own describe with its own config, since adding a call to the CRUDL sequence shifts which recording each of its calls keys to. * fix(eval): suppress the role-scope warning under --json runtime/invoke's advisory summary is gated on `!output.json` (response.ts:180), with tests asserting an empty stderr in JSON mode; only its failure-path summaries write regardless. The role-scope warning follows the advisory precedent, so a scripted caller now gets machine-readable stdout and nothing else. * feat(eval): grant kms:Decrypt for encrypted evaluators CreateOnlineEvaluationConfig documents that the execution role must hold kms:Decrypt on the key of any evaluator encrypted with a customer managed key, and that it validates this when the config is created. The provisioned role had no KMS permissions, so such a config could not be created without --role-arn. Core now resolves each referenced evaluator via GetEvaluator, collects any kmsKeyArn, and adds a DecryptEvaluatorKeys statement scoped to exactly those keys. The statement is omitted when nothing is encrypted, so the common builtin case does not widen the role. Applied on update too, when the evaluator list changes alongside the data source. Resolution depends on GetEvaluator reporting kmsKeyArn, which the service currently only does for ~2 minutes after an evaluator is created (P484740478). The encrypted-evaluator fixture is therefore hand-authored to represent the documented behavior; a RECORD run overwrites it with the live response, so it must be restored afterwards. Policy assertions live in a unit test rather than the fixture layer: recorded IAM responses are empty, so the policy body is not observable through a golden, and an earlier attempt to pin it via fixture hashing did not fail when the statement was removed. The new test catches removal of the KMS statement, scoping to an endpoint log group instead of the runtime prefix, and blind role-name truncation. * fix(eval): let GetEvaluator failures propagate during KMS resolution Wrapping the failure in InputValidationError mislabelled it: that type sets source: USER, but a GetEvaluator call failing is not the caller's input at fault — it is a service or permissions condition. It also replaced the SDK's error, which already names the operation and the evaluator, with a less precise message. The try/catch is removed so the original error surfaces unchanged. * fix(eval): re-scope the execution role transactionally on update Nico's review: re-scoping the managed role before UpdateOnlineEvaluationConfig left the config broken if the update then failed — the role granted query access to the new runtime's logs while the config still pointed at the old one, so it silently stopped sampling. The role is now widened to the union of old and new log groups before the update and narrowed to the new set only after it succeeds. A superset role is valid for either config state, so a failed update never strands a live config without query access; a later successful update narrows it back. If the narrow itself fails, a "narrow-failed" warning reports the role is broader than the current data source (a real reason the handler now renders distinctly, rather than mislabelling it as a declined re-scope). Verified against the live API: a forced update failure leaves the config on the old runtime with the role still covering it, and a successful --agent repoint narrows the role to the new runtime only. Fixtures re-recorded for the two-step PutRolePolicy sequence. * refactor(eval): scope the execution role per-policy instead of rewriting The transactional re-scope on update rewrote the role's single inline policy in place (widen to the union, then narrow), so the pre-update scope existed nowhere once the widen ran — an interrupted update left the role permanently over-scoped. Each scope is now its own inline policy, named after a fingerprint of the scope: - grantOnlineEvalScope attaches the new scope's policy (create path uses this too) - revokeOnlineEvalScope detaches the superseded one update grants the new scope, runs UpdateOnlineEvaluationConfig, then revokes the old scope only on success. Because granting never overwrites another scope's policy and IAM unions Allows across a role's inline policies, both scopes are granted in between, and a failed update leaves the policy backing the current data source byte-identical. Verified live: a forced update failure left the old policy's md5 unchanged with the config still pointing at (and granted) the old runtime; a successful --agent repoint revoked the old scope. Also widens the create-time propagation retry: a freshly written policy under a new name is not immediately visible, surfacing as "does not have permissions to create log group" / "access the specified log groups" in addition to the existing "role cannot be assumed". Renamed to retryWhileRolePropagates. The stale-scope warning replaces narrow-failed: on update the concern is now a superseded scope policy left attached, not a policy that could not be narrowed. * refactor(eval): fingerprint the scope policy on its rendered document scopePolicyName hashed the (logGroups, kmsKeys) inputs, which meant the name tracked only those two arguments — any other change to what executionPolicy renders (a new statement, a changed resource shape) would reuse the same name and overwrite the prior policy. Hashing the rendered policy document instead ties the name to the exact contents, so any change to the granted permissions yields a new name and the create-then-revoke sequence stays collision-free. grant renders the document once, names it from that, and puts it; the update path re-renders the old document to derive the name to revoke. accountIdFromRoleArn is exported for that re-render. Verified live: a successful --agent repoint revoked the old policy and left exactly the new one, confirming the re-derived old name matches what create wrote. --------- Co-authored-by: jariy17 <tjariy+jariy17@users.noreply.github.com>
What
Adds the imperative
agentcore eval evaluatorcommand surface (CLI only). Covers LLM-as-a-Judge and code-based evaluator create/update plus type-agnostic get/list/delete, following the DevX Evaluations/Optimization refactor doc and the existingidentityhandler conventions.Command structure
Flags
eval evaluator llm-as-a-judge create--name--levelSESSION | TRACE | TOOL_CALL--model--instructionsfile://<path>, or-(stdin)--rating-scale1-5-quality | 1-3-simple | pass-fail | good-neutral-bad--rating-scale-jsonRatingScaleJSON (source-aware). Mutually exclusive with--rating-scale; exactly one required--kms-key-arn--tags--client-tokeneval evaluator llm-as-a-judge update--id--instructions--model--rating-scale/--rating-scale-json--kms-key-arn--client-tokenFields left unset are preserved:
UpdateEvaluatorreplaces the wholeevaluatorConfigunion, so the client does get-then-merge over the current definition.eval evaluator code-based create--name--levelSESSION | TRACE | TOOL_CALL--lambda-arn--timeout--kms-key-arn--tags--client-tokeneval evaluator code-based update--id--lambda-arn--timeout--kms-key-arn--client-tokeneval evaluator get--ideval evaluator list--next-token--max-results--typeBuiltin | code-based | llm-as-a-judge. The ListEvaluators API paginates only, so the filter is page-localeval evaluator delete--id--yesNotes / decisions
--instructions,--rating-scale-json,--tags): inline,file://<path>, or-for stdin, following the AWS CLIfile://convention. One flag reads stdin per command.--timeouthas no CLI default; the service default applies when omitted.CoreEvalClientis declared next to the handlers (src/handlers/eval/types.tsx) and implemented insrc/core/eval.tsx.Testing
bun test— full suite green (356 tests).bun run typecheck,bun run lint:check,bun run format:check— clean.bun run build+ bundle smoke (node dist/index.js eval evaluator --help) — works.TestCoreClient,EvalClientget-then-merge unit tests, and source-resolver tests.TUI flows for these commands will come in a later PR.