feat(batch-evaluation): add --output-config and rename --qualifier to --endpoint - #2265
Conversation
There was a problem hiding this comment.
AgentCore Harness Review
Verdict: Looks good
Nice, well-scoped PR. The three changes are cleanly separated, the tests cover each layer where it can fail (handler, SessionSource timestamp parsing, and — deliberately — the Core→SDK seam that the TestCoreClient and fixture suites can't cover), and the "resolve --output-config before any Runtime invocation" ordering in simulate is exactly the right call for a command that bills per example. The BatchOutputConfig module and long-form parameter help follow existing conventions and are appropriately API-shaped.
No blocking issues. A couple of small notes, take or leave:
eval ondemand simulatestill uses--qualifier(src/handlers/eval/ondemand/simulate/index.tsx:19,69,92). The PR body justifies the rename by aligningsimulatewith its own command family (eval), but this sibling command in the same family is left inconsistent. If it's intentionally deferred, a follow-up TODO/issue reference would help; otherwise consider renaming it in the same breaking change so users only see one flag flip.- Timestamp regex accepts
HH:MMwith no seconds (sessionSource.tsx:171) — that's fine and matches ISO-8601, but the error message example (2026-09-01T00:00:00Z) and all tests use the seconds form; worth confirming2026-09-01T00:00Zis intentionally allowed (looks like it is, and it's a superset of what customers will type).
Live verification, mutation-testing notes, and the explanation for the new src/core/eval.test.ts file (the gap between handler-level TestCoreClient assertions and re-record-only fixture assertions) are all appreciated.
73e7c29 to
2a11a5b
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## refactor #2265 +/- ##
=========================================
Coverage 97.07% 97.07%
=========================================
Files 571 572 +1
Lines 39442 39478 +36
=========================================
+ Hits 38287 38323 +36
Misses 1155 1155 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2a11a5b to
ccd369d
Compare
| import { createSilentLogger } from "../testing"; | ||
| import type { OutputConfig } from "@aws-sdk/client-bedrock-agentcore"; | ||
|
|
||
| // The TestCoreClient suites assert what a handler hands to Core; the fixture |
ccd369d to
290780f
Compare
| @@ -97,6 +99,8 @@ export const createSimulateBatchEvaluationHandler = (core: Core, _io: AppIO) => | |||
|
|
|||
| // Ctrl-C aborts the run (invokes, the ingestion wait, the dataset download). | |||
| // TODO(#1986): swap for the shared SIGINT/abort helper once it merges. | |||
There was a problem hiding this comment.
I'll fix this TODO later in a follow up PR.
| Example: | ||
| --output-config '{"cloudWatchConfig":{"logGroupName":"/company/agent-evaluations","metricsNamespace":"Company/AgentEvaluations","resultDestination":"DEDICATED_LOG_GROUP"}}'`; | ||
|
|
||
| const RESULT_OUTPUT = "Result output:"; |
| // Already-parsed --ground-truth (SessionMetadataShape[]) → evaluationMetadata. | ||
| groundTruth?: SessionMetadataShape[]; | ||
| kmsKeyArn?: string; | ||
| // Already-parsed --output-config, forwarded to the request untouched. Left |
There was a problem hiding this comment.
remove this code comment
290780f to
7a95dc7
Compare
notgitika
left a comment
There was a problem hiding this comment.
Looks solid! my agent found some bugs that I added in the comments.
apart from this, the PR title still mentions requiring timestamp timezones, but the current diff and commit msg say that change was dropped. lets update the PR title
| ] as const; | ||
|
|
||
| static async resolve(value: string | undefined, io: AppIO): Promise<OutputConfig | undefined> { | ||
| const resolver = new SourceResolver({ stdin: io.stdin }); |
There was a problem hiding this comment.
SourceResolver owns the one-stdin-option guard, but this creates a new instance separate from the resolvers used by SessionSource and --ground-truth in evaluate. With --ground-truth - --output-config - (or --data-source-config - --output-config -), the first option drains stdin and this one receives an empty string instead of reporting the conflict. Could we share one resolver or prevalidate that only one option uses stdin, and add a regression test?
There was a problem hiding this comment.
We will share one resolved then and I'll add a regression test!
| "@aws-sdk/client-bedrock-agent": ["@aws-sdk/client-bedrock-agent@3.1121.0", "", { "dependencies": { "@aws-sdk/core": "^3.977.9", "@aws-sdk/credential-provider-node": "^3.972.81", "@aws-sdk/types": "^3.974.5", "@smithy/core": "^3.33.3", "@smithy/fetch-http-handler": "^5.7.2", "@smithy/node-http-handler": "^4.11.3", "@smithy/types": "^4.17.2", "tslib": "^2.6.2" } }, "sha512-RAjn1g6X+u4WwnuwxYU8sKna5q1waTBCM+kzXWAvxSaNbgRsXFL6ZylPjOl0qCDZ7x6aVTj7Jb7Q0z6DZifP3w=="], | ||
|
|
||
| "@aws-sdk/client-bedrock-agentcore": ["@aws-sdk/client-bedrock-agentcore@3.1131.0", "", { "dependencies": { "@aws-sdk/core": "^3.978.0", "@aws-sdk/credential-provider-node": "^3.972.83", "@aws-sdk/types": "^3.974.5", "@smithy/core": "^3.33.3", "@smithy/fetch-http-handler": "^5.7.2", "@smithy/node-http-handler": "^4.11.3", "@smithy/types": "^4.17.2", "tslib": "^2.6.2" } }, "sha512-xyPxp33VylkesNriC2WOJMkYmj0BX7borAUMojWg/sFcoTyEU0KVs0U6bElZkq7X46/1w6w3hhSGrMVEti7RcA=="], | ||
| "@aws-sdk/client-bedrock-agentcore": ["@aws-sdk/client-bedrock-agentcore@3.1130.0", "", { "dependencies": { "@aws-sdk/core": "^3.978.0", "@aws-sdk/credential-provider-node": "^3.972.83", "@aws-sdk/types": "^3.974.5", "@smithy/core": "^3.33.3", "@smithy/fetch-http-handler": "^5.7.2", "@smithy/node-http-handler": "^4.11.3", "@smithy/types": "^4.17.2", "tslib": "^2.6.2" } }, "sha512-HgHR24kDJwRN9NNrtWNeuyrJZgl0nXOikeeD2HBAprsRXFl3tvZQw6hHOADHR41sjT4z3+SmqIqN5U7JiaF40Q=="], |
There was a problem hiding this comment.
This downgrades @aws-sdk/client-bedrock-agentcore from 3.1131.0 to 3.1130.0, and the control client is downgraded on the next entry as well, while package.json is unchanged. Could we regenerate the lockfile or otherwise preserve the parent branch's 3.1131.0 resolutions to avoid an unrelated dependency regression?
| flags: [ | ||
| flag("runtime-id", "Runtime ID to invoke per scenario", z.string().optional()), | ||
| flag("qualifier", "Runtime endpoint qualifier (default DEFAULT)", z.string().optional()), | ||
| flag("endpoint", "Runtime endpoint qualifier (default DEFAULT)", z.string().optional()), |
There was a problem hiding this comment.
This also makes the breaking --qualifier → --endpoint rename for on-demand simulate, while the PR description only calls out batch-evaluation simulate. Please document both affected commands. Should we retain --qualifier as a deprecated alias to avoid breaking existing scripts?
There was a problem hiding this comment.
I'll update the PR description and we haven't released the CLI to the public so it's okay to make breaking changes :)
There was a problem hiding this comment.
Yeah makes sense to me I think we wanted to use --endpoint anyway AFAIK but I wanted to just point it out
… --endpoint --output-config lets a customer say where results and metrics are written instead of taking the service-managed default. An API-shaped passthrough: BatchOutputConfig resolves inline JSON, file://, or stdin and hands the parsed object to StartBatchEvaluation with field names and nested values untouched. Field documentation goes in the flag's `help:` block, rendered under "Parameter details". `simulate --qualifier` becomes `--endpoint`, matching SessionSource, which has always called the same concept --endpoint. The InvokeDatasetInput field stays `qualifier` — that is the Runtime API's name, not ours. simulate resolves --output-config before invoking the Runtime, since the replay bills the customer per dataset example and malformed JSON must not surface only after the whole run. Dependencies move to @aws-sdk/client-bedrock-agentcore 3.1129.0, the first release exposing request-side outputConfig on StartBatchEvaluationRequest. The generated type is used directly; no cast papers over an older model. src/core/eval.test.ts covers the seam the other suites miss: the TestCoreClient suites assert what a handler hands to Core, and the fixture suites need an account to re-record. Deleting Core's outputConfig forwarding passed every test until this one existed. Timestamp handling is unchanged. An earlier revision required an explicit timezone on --start-time/--end-time; that is a breaking change and has been dropped.
The aws/spans log group holding trace data has 30-day retention, so the previous window (2026-08-12/13) aged out and StartQuery failed with MalformedQueryException on replay. Re-recorded against two sessions invoked within the retention window and bumped FIXTURE_SESSION_IDS/WINDOW together.
599ca0f to
03e5003
Compare
|
Claude Security Review: no high-confidence findings. (run) |
…lags Each stdin-capable flag built its own SourceResolver, so the resolver's one-stdin guard never fired across options. With --ground-truth - --output-config - the first drained stdin and the second read an empty string instead of reporting the conflict. Thread one resolver through SessionSource.resolve and BatchOutputConfig.resolve on evaluate/simulate/batch-insights so the guard covers every flag. Adds a regression test.
A rebase re-resolved @aws-sdk/client-bedrock-agentcore and its control client down to 3.1130.0 while package.json stayed at ^3.1129.0, downgrading against the base branch. Restore the 3.1131.0 resolutions refactor already uses.
|
Claude Security Review: no high-confidence findings. (run) |
notgitika
left a comment
There was a problem hiding this comment.
Thanks for addressing the comments LGTM!
nborges-aws
left a comment
There was a problem hiding this comment.
PR LGTM after fixes. Thanks!
Renamed
--qualifierto--endpointin bothagentcore eval batch-evaluation simulateandagentcore eval ondemand simulateto match the other commands' runtime-endpoint flag. This is a breaking rename on both commands. We also introduce--output-configtoagentcore eval batch-evaluation simulateandagentcore eval batch-evaluation evaluateso customers can control where their results go (docs).--helpagentcore eval batch-evaluation evaluateagentcore eval batch-evaluation simulate