Uh oh!
There was an error while loading. Please reload this page.
feat: support imperative eval recommendation command - #2111
Conversation
| description: "get a recommendation by id", | ||
| flags: [flag("id", "the ID of the recommendation", z.string().optional())], | ||
| handle: async (ctx, flags) => { | ||
| if (!flags["id"]) throw new InputValidationError("required option '--id <id>' not specified"); |
There was a problem hiding this comment.
we can definitely abstracted this so we have a common error message for this type of thing. OBO
There was a problem hiding this comment.
yeah I think marking flags as required and supporting it directly in the framework would be ideal, but likely OOS here.
| } | ||
| async startRecommendation( | ||
| request: StartRecommendationRequest, |
There was a problem hiding this comment.
We create our own types here because handlers define the interface for the core client. Even if it's a one-to-one mapping, we should still define our own type.
| @@ -0,0 +1,119 @@ | |||
| import { describe, expect, test } from "bun:test"; | |||
There was a problem hiding this comment.
We don't need this. Use Golden tests please
There was a problem hiding this comment.
are the handler tests sufficient? I think these are testing the same things at a lower level.
| flag("tags", "tags as key=value (repeatable) or JSON object", z.array(z.string()).optional()), | ||
| ], | ||
| handle: async (ctx, flags) => { | ||
| if (!flags["name"]) { |
There was a problem hiding this comment.
const requiredFlags: Array<[string, string]> = [
["name", "--name <name>"],
["type", "--type <type>"],
["recommendation-config", "--recommendation-config <recommendation-config>"],
];
for (const [key, usage] of requiredFlags) {
if (!flags[key]) {
throw new InputValidationError(`required option '${usage}' not specified`);
}
}
There was a problem hiding this comment.
nit: we should use something like this.
There was a problem hiding this comment.
Sure will add something like this
| return call.args; | ||
| } | ||
| describe("eval recommendation command hierarchy", () => { |
There was a problem hiding this comment.
+1, I've seen these tests in other places, but they feel pretty low value. The tests on the handlers would fail if the hierarchy is incorrect. Also, what coverage do we get on the tests below that we don't get on the fixture based tests?
There was a problem hiding this comment.
I added these tests to cover behavior the fixtures don't exercise. E.g. malformed json, exercising source resolver, etc. But consensus is against unit test so I've removed this + the other one entirely
| description: "get a recommendation by id", | ||
| flags: [flag("id", "the ID of the recommendation", z.string().optional())], | ||
| handle: async (ctx, flags) => { | ||
| if (!flags["id"]) throw new InputValidationError("required option '--id <id>' not specified"); |
There was a problem hiding this comment.
yeah I think marking flags as required and supporting it directly in the framework would be ideal, but likely OOS here.
| return (error as Error).name === "ResourceNotFoundException"; | ||
| } | ||
| async function waitForTerminal( |
There was a problem hiding this comment.
is there a way to leverage
agentcore-cli/src/testing/timing.tsx
Line 16 in 01c9317
There was a problem hiding this comment.
Good call out I'll switch to use this util
| return call.args; | ||
| } | ||
| describe("eval recommendation command hierarchy", () => { |
There was a problem hiding this comment.
+1, I've seen these tests in other places, but they feel pretty low value. The tests on the handlers would fail if the hierarchy is incorrect. Also, what coverage do we get on the tests below that we don't get on the fixture based tests?
| status: "PENDING", | ||
| createdAt: new Date("2026-08-26T12:00:00.000Z"), | ||
| updatedAt: new Date("2026-08-26T12:00:00.000Z"), | ||
| } as StartRecommendationResponse; |
There was a problem hiding this comment.
why do we need to cast here?
There was a problem hiding this comment.
N/A since test file was removed altogether
| @@ -0,0 +1,119 @@ | |||
| import { describe, expect, test } from "bun:test"; | |||
There was a problem hiding this comment.
are the handler tests sufficient? I think these are testing the same things at a lower level.
295482e to
2783292CompareClaude Security Review: no high-confidence findings. (run) |
codecov-commenter
commented
Aug 27, 2026
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## refactor #2111 +/- ##
==========================================
Coverage 97.41% 97.41% ==========================================
Files 453 458 +5 Lines 27637 27810 +173 ==========================================
+ Hits 26922 27091 +169 - Misses 715 719 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Uh oh!
There was an error while loading. Please reload this page.
Description
Adds imperative command-line support for AgentCore evaluation recommendations:
eval recommendation starteval recommendation geteval recommendation listeval recommendation deleteSummary of Changes
SYSTEM_PROMPT_RECOMMENDATIONandTOOL_DESCRIPTION_RECOMMENDATIONrecommendation typesstartaccepts recommendation configuration as inline JSON, afile://path, or stdin (-) throughSourceResolverclassEvalClient, the shared handler types, andTestCoreClientType of Change
Testing
How have you tested the change?
Added unit tests and golden fixture tests exercising recommendation logic.
bun run test(2052 pass, 0 fail)npm run test:unitandnpm run test:integnpm run typechecknpm run lintsrc/assets/, I rannpm run test:update-snapshotsand committed the updated snapshotsChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.