feat(eval): grouped help and command examples for the eval commands - #2262
Conversation
|
Claude Security Review: no high-confidence findings. (run) |
There was a problem hiding this comment.
AgentCore Harness Review
Verdict: Looks good
Reviewed the router mechanism (flags.tsx, handler.tsx, router.tsx, index.tsx), the --evaluator → --evaluators rename across the 8 handlers, and the SDK bump.
- The
Flag.groupplumbing is minimal and correctly optional; ungrouped flags stay in Commander's default section. Handler.examplesis optional and read off the authored node incompile(), so middleware wrappers (which only need to forwardhandle) don't have to carry it.withEffectiveTuiSupportalso forwards it viahandler.examples?.()so nothing gets dropped through TUI wrapping.- The
-h, --helprelocation toOther options:correctly triggers only whenownFlags.some(f => f.group), keeping unchanged behavior for commands that don't opt in. - New router tests cover: heading ordering, ungrouped flags remaining in
Options:,-hplacement in both cases, examples rendering exactly once afterParameter details:, and the no-examples case. - The rename is complete: no leftover
"--evaluator"argv,flag("evaluator", …), orflags["evaluator"]reads insrc/, and the singularevaluatorresource-type enum inproject removeis correctly left alone (it's a resource selector, not a flag list). Error messages updated to'--evaluators <evaluators...>'. All corresponding fixture/unit tests updated in the same PR. - SDK bump to
3.1129.0on both agentcore clients is scoped and justified for the follow-up PRs.
No changes requested.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## refactor #2262 +/- ##
==========================================
Coverage 97.05% 97.06%
==========================================
Files 566 568 +2
Lines 39228 39346 +118
==========================================
+ Hits 38073 38191 +118
Misses 1155 1155 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
662bc9b to
e6da03d
Compare
|
Claude Security Review: no high-confidence findings. (run) |
560bd65 to
b14c0dc
Compare
|
Claude Security Review: no high-confidence findings. (run) |
A const earns its place when the string appears more than once; naming a single use only adds a hop. simulate's Evaluation, online-eval create's Execution, and update's Target headings are each used by one flag, so they go back to literals. The rest stay named.
|
Claude Security Review: no high-confidence findings. (run) |
notgitika
left a comment
There was a problem hiding this comment.
added comments for 4 bugs my agent found otherwise LGTM
| { | ||
| "sessionId": "string", // [required] the session the reference applies to | ||
| "testScenarioId": "string", // groups sessions replaying the same scenario | ||
| "groundTruth": { |
There was a problem hiding this comment.
this documented shape doesn’t match SessionMetadataShape. groundTruth.inline must be an InlineGroundTruth object, not a string. The current example will produce an invalid API request. Could we update it to use a valid shape such as {"inline":{"turns":[...]}}?
| "serviceNames": ["string", ...], // e.g. "my_agent.DEFAULT" | ||
| "filterConfig": { | ||
| "sessionIds": ["string", ...], | ||
| "sessionFilterConfig": { |
There was a problem hiding this comment.
this property should be timeRange, not sessionFilterConfig. the SDK serializer drops the unknown property, so these dates are silently omitted and the evaluation may include sessions outside the intended window
There was a problem hiding this comment.
The dependenxy will be updated in the next pr
| { | ||
| "cloudWatchLogs": { | ||
| "serviceNames": ["string", ...], // [required] e.g. "my_agent.DEFAULT" | ||
| "logGroupNames": ["string", ...], // exact group names |
There was a problem hiding this comment.
The locked control-plane SDK does not support logGroupNamePrefixes; CloudWatchLogsInputConfig requires both logGroupNames and serviceNames. Following the documented prefix alternative sends no logGroupNames and fails service validation.
Could we remove that alternative?
There was a problem hiding this comment.
This grouping is lost in the interactive TUI. These eval commands render through CliOnlyScreen, which flattens visibleOptions() into one “options” table without consulting each option’s help group. Could we preserve the groups there as well so the CLI and TUI help remain consistent?
There was a problem hiding this comment.
Sure ill look into this
There was a problem hiding this comment.
Fixed in 77bbb9f6 — CliOnlyScreen now reads each option's helpGroupHeading and renders one section per heading, in the same first-seen order Commander uses, so the CLI and TUI agree. Commands whose flags carry no group keep the single options section unchanged.
configuration
--name <name> batch evaluation name (must be unique in the account)
--description <description> optional description
--kms-key-arn <kms-key-arn> KMS key to encrypt evaluation data at rest
session source (choose exactly one)
--agent <agent> harness ID or Runtime ID whose sessions to use
--online-eval <online-eval> use sessions an online-eval config already sampled
--data-source-config <data-source-config> the traces to read (JSON DataSourceConfig); escape
hatch
source filters
--endpoint <endpoint> Runtime endpoint qualifier (default DEFAULT; only with --agent)
--start-time <start-time> window start (ISO-8601, with --end-time)
--end-time <end-time> window end (ISO-8601, with --start-time)
--session-ids <session-ids...> specific session IDs (only with --agent)
evaluation
--evaluators <evaluators...> evaluator ID(s) to apply
--ground-truth <ground-truth> expected answers for the sessions (JSON SessionMetadataShape[])
Headings are lowercased and lose the trailing colon so they sit alongside the screen's own arguments heading.
Why not just render Commander's own help text
The obvious simplification is to drop the hand-built table entirely and print command.helpInformation(), which already renders the groups — one source of truth, no duplicated layout. I prototyped it and it does not work here, for a reason worth recording rather than rediscovering:
Help.formatItem skips wrapping when the description column would be too narrow (commander/lib/help.js:663):
const remainingWidth = helpWidth - termWidth - spacerWidth - itemIndent;
if (remainingWidth < this.minWidthToWrap /* 40 */) {
formattedDescription = description; // one long unwrapped line
}termWidth is the width of the command's longest flag term, so the threshold is per-command. At the TUI's usable width of 96:
| Command | Term column | Needs helpWidth ≥ |
At 96 |
|---|---|---|---|
eval batch-evaluation evaluate |
41 | 85 | wraps |
eval online-eval create |
51 | 95 | wraps |
harness create |
53 | 97 | skipped |
gateway create |
57 | 101 | skipped |
When wrapping is skipped, Commander emits the whole description on one line and Ink re-wraps it with no hanging indent, so continuations land at column 1:
--protocol <protocol> restrict Target protocols to MCP;
omitted allows every Target protocol
Raising helpWidth cannot fix gateway create — it needs 101 columns inside a 100-column terminal. Rendering per-option and letting Ink measure each column sidesteps the bailout entirely, and gives tighter columns per section as a side effect.
Worth revisiting if Commander ever exposes minWidthToWrap per call, or if the long-flag commands get shorter terms.
Three review findings, each verified against the generated SDK model: --ground-truth documented `groundTruth.inline` as a string. It is an InlineGroundTruth object — turns, assertions, expectedTrajectory — so the previous example would have produced an invalid request. The block now spells out the nested shape, including that groundTruth is a union with inline as its only arm today. --data-source-config named the time filter `sessionFilterConfig`, which is the type's name rather than the property's. The property is `timeRange`. This was the worst of the three: the serializer drops an unknown key, so a caller's window was silently ignored and sessions outside it were evaluated. The online-evaluation source block documents logGroupNamePrefixes, which the previously pinned control-plane model did not expose — following it would have sent no logGroupNames and failed service validation. Both AgentCore clients move to 3.1129.0, where CloudWatchLogsInputConfig accepts either name list.
|
Claude Security Review: no high-confidence findings. (run) |
CliOnlyScreen rebuilt the option list as one flat table, so a command whose flags carry help groups showed five headings under --help and a single undifferentiated block on screen. It now reads each option's helpGroupHeading and renders a section per heading, in first-seen order, matching how Commander orders them. Commands whose flags carry no group are unchanged: they keep the single "options" section. Headings are lowercased and lose their trailing colon so they sit alongside the screen's own "arguments" heading. Commander's generated --help option is still filtered out, which also keeps the "Other options:" heading off a screen that is itself the help.
|
Claude Security Review: no high-confidence findings. (run) |
|
Claude Security Review: no high-confidence findings. (run) |
notgitika
left a comment
There was a problem hiding this comment.
One remaining blocker: logGroupNamePrefixes is now documented and supported by the upgraded SDK, but the CLI-managed execution role still scopes permissions using only logGroupNames.
For a prefix-only data source, logGroupNamesOf() returns an empty list, so create/update provisions no query access for the matching log groups and will likely fail service validation unless the user supplies --role-arn.
Could we make role scoping prefix-aware and add coverage for prefix-only create/update without a custom role?
Otherwise, my other comments look addressed. TY!
I'm adding this support in |
I have decided to implement help groups using the Commander helpGroup feature. As shown below, organizing flags into groups makes it easier for users to understand how each flag functions within the command.
agentcore eval batch-evaluation evaluateBefore (
refactor):After (this PR):
agentcore eval batch-evaluation simulateBefore (
refactor):After (this PR):
agentcore eval batch-insights runBefore (
refactor):After (this PR):
agentcore eval online-eval createBefore (
refactor):After (this PR):
agentcore eval online-eval updateBefore (
refactor):After (this PR):
Tests
Router (6 new): heading names and ordering, ungrouped flags staying in
Options:,-hplacement with and without groups, examples rendering exactly once afterParameter details:, backslash continuation for array commands vs single-line for strings, and the no-examples case.Handlers (14 new,
src/handlers/eval/helpGroups.test.ts): asserts what the router's rendering tests cannot — that every flag on these five commands carries a group, that the group comes from the shared vocabulary, that a heading's flags are declared contiguously (a stray one would reorder the headings), and that no example names a flag its command doesn't declare.Mutation-checked: ungrouping a flag, hand-typing a heading that drifts from the vocabulary, and misspelling a flag inside an example each fail a test.
Verification
bun test— 3199 pass, 0 fail (225 files)bun run typecheck,bun run lint:check,bun run format:check— clean*.screen.test.tsxsuites don't assert on these commands' option layoutFollow-ups
#2265 adds
--output-configto batch-evaluation, renames--qualifier→--endpoint, and makes--start-time/--end-timerequire a timezone. #2266 does the online-eval equivalents plus the managed-role widening.