feat(online-eval): add --output-config and --tags, --description on update, and widen the managed role - #2266
Conversation
There was a problem hiding this comment.
AgentCore Harness Review
Verdict: Changes requested
Nice, careful change — the invariants around the hashed policy name (bare-string vs. one-element array, reserved-namespace destinations, output-included policy fingerprint) are well thought out and well tested. One user-facing correctness issue in the update path warrants a change before merge.
stale-scope warning is wrong when only the output destination moved
src/handlers/eval/online-eval/update/index.tsx (lines ~200–207) still hard-codes the old wording:
if (reason === "stale-scope") {
io.stderr.write(
`warning: the execution role still grants access to the previous data source.\n` +
` role: ${roleArn}\n` +
` detach the inline policy covering: ${logGroupNames.join(", ")}\n`,
);
}Now that a --output-config change alone can reach the refresh path in src/core/eval.tsx (via outputMoved → refreshManagedRole), stale-scope is reachable for an output-only move. In that case:
oldLogGroups === newLogGroups(data source unchanged), sologGroupNames: oldLogGroupsin the warning payload lists the current data-source groups — which are not stale at all.- The message tells the caller the role "still grants access to the previous data source," but the actually-stale portion of the old inline policy is the previous output destination.
So a customer who moves only their result destination will see a warning that names log groups that are still in use and blames the wrong half.
A few ways to fix:
- Have the handler branch on
scopeforstale-scopetoo, mirroring the non-stale wording (e.g.`the ${MOVED[scope]} moved but the superseded inline policy could not be detached`), and populate the warning'slogGroupNamesineval.tsxwith the groups that were actually stale — for output-only moves that'sdestinationLogGroupNames(current.outputConfig, current.dataSourceConfig); for input-only it'soldLogGroups; for input-and-output it's the union. - Or, at minimum, keep the payload but broaden the message to not claim "data source" — something like "the execution role still grants a superseded scope" and let the caller detach the old inline policy by hash.
Option 1 is the more consistent fix given how the custom-role / update-declined branches already carry scope and format accordingly.
Non-blocking observation (won't block merge)
outputMoved = update.outputConfig !== undefined treats any --output-config as a move, even when it matches the stored config. That's fine for the managed-role refresh (idempotent by hash), but for the custom-role / update-declined branches it can emit a warning telling the customer the "output destination moved" when it didn't. The comment acknowledges the trade-off; not a change request, just worth knowing that the two warning branches also inherit it.
Everything else — the bare-string invariant, reserved-namespace suppression, effectiveOutputConfig for source-only moves, the pre-Core resolution of --output-config and --tags so a malformed JSON can't leave an IAM role behind, and the TestCoreClient.setOnlineEvalRoleScopeWarning seam — looks good.
e2ebc30 to
6192ad6
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/batch-eval-output-config #2266 +/- ##
=================================================================
- Coverage 97.06% 97.05% -0.02%
=================================================================
Files 569 570 +1
Lines 39374 39488 +114
=================================================================
+ Hits 38219 38324 +105
- Misses 1155 1164 +9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
6192ad6 to
704e402
Compare
…pdate --output-config lets a customer say where results and metrics are written. Same passthrough contract as batch evaluation's flag of the same name, but a separate module and a separate generated type: the online-evaluation OutputConfig is a plain object with no logStreamName, while the batch one is a tagged union that has one. --tags on create reuses TagsSchema and parseJsonFlagWithSchema, matching `project add memory`, so a non-string tag value is rejected here rather than by the API. --description on update was simply missing. The execution role is what makes the destination usable. Its policy granted result writes only to /aws/bedrock-agentcore/evaluations/*, so a CLI-managed role plus a custom log group would have produced a config whose results could not be written anywhere. executionPolicy now also grants the customer-named group, or the sampled source groups for SOURCE_LOG_GROUP, and an output change joins a data-source change as a trigger for re-scoping. A single ARN stays a bare string rather than a one-element array. IAM treats those identically, but the document's exact text is hashed to name the inline policy, so wrapping it would rename the policy attached to every config that has no custom destination. A destination already inside the reserved /aws/bedrock-agentcore/evaluations/ namespace adds nothing, keeping the document stable for a config whose stored outputConfig is the service-managed default the API echoes back. RoleScopeWarning carries which half of the scope moved, because the permissions the caller has to add differ: querying traces needs logs:StartQuery and logs:GetQueryResults on the sampled groups, while writing results needs logs:PutLogEvents and, for a group that does not exist yet, logs:CreateLogGroup.
704e402 to
6654d10
Compare
Fourth and last of the stack. Based on #2265 (batch-evaluation flags) — GitHub retargets to
refactoras the stack merges.--output-configoncreateandupdateSame passthrough contract as batch evaluation's flag of the same name: inline JSON,
file://<path>, or-, parsed and forwarded with field names and values untouched, rejected before any side effect if malformed.A separate module and a separate generated type, deliberately. The online-evaluation
OutputConfigis a plain object with nologStreamName; the batch one is a tagged union that has one. An online evaluation runs continuously rather than as one job with one stream, so sharing the type would be wrong even though the flag name matches.--tagson create,--descriptionon update--tagsreusesTagsSchema+parseJsonFlagWithSchema, matchingproject add memory, so{"team":42}is rejected by the CLI rather than by the API.--descriptiononupdatewas simply missing —createhad it.The execution role, which is what makes the destination usable
executionPolicygranted result writes only to/aws/bedrock-agentcore/evaluations/*. A CLI-managed role plus a custom log group would therefore have created a config whose results could not be written anywhere — the flag would have looked like it worked. So the policy now also grants:DEDICATED_LOG_GROUPSOURCE_LOG_GROUPand an output-destination change now joins a data-source change as a trigger for re-scoping a managed role.
Two invariants worth reviewing closely
A single ARN stays a bare string, not a one-element array. IAM treats those identically, but this document's exact text is hashed to name the inline policy (
scopePolicyName). Wrapping it would rename the policy attached to every existing config with no custom destination, orphaning the grant it is currently running on. My first attempt did exactly that, and the fixture suite caught it —onlineEvalExecutionRole.test.tsnow pins it.A destination already inside the reserved
/aws/bedrock-agentcore/evaluations/namespace contributes nothing, since the wildcard covers it. That keeps the document stable for a config whose storedoutputConfigis the service-managed default the API echoes back — otherwise the same orphaning problem would hit every config ever created through the CLI.Already correct, no work needed
scopePolicyNamealready hashed the whole document, so the fingerprint picks up a destination change for free. And the update path already granted the new scope before the service call and revoked the superseded one only after success. Two of the four IAM items in the original plan turned out to be no-ops.RoleScopeWarningnow says which half movedThe permissions a caller has to add differ by scope: querying traces needs
logs:StartQuery/logs:GetQueryResultson the sampled groups; writing results needslogs:PutLogEvents, pluslogs:CreateLogGroupfor a group that doesn't exist yet. The warning carriesscope: "input" | "output" | "input-and-output"and names the matching actions.For
input-and-outputit lists both action sets over the union of moved groups rather than tracking which action belongs to which group — for a role the caller manages themselves, naming a superset is more useful than naming half of it.Live verification — account 725476964917 (EXPLORE_PROFILE), us-west-2
Create with a custom dedicated group and tags → the config came back with
logGroupName: /company/pr4-online-eval-results, and the CLI-provisioned role's policy granted exactly the intended scope:{ "Sid": "WriteEvaluationResults", "Action": ["logs:CreateLogGroup", "logs:CreateLogStream", "logs:DescribeLogStreams", "logs:PutLogEvents"], "Resource": [ "arn:aws:logs:us-west-2:725476964917:log-group:/aws/bedrock-agentcore/evaluations/*", "arn:aws:logs:us-west-2:725476964917:log-group:/company/pr4-online-eval-results*" ] }Tags landed:
{"purpose": "pr4-verify", "team": "agentcore-cli"}.Update to
SOURCE_LOG_GROUPplus a new--description→ the role was re-scoped in place. The policy went from...-ded5f54ato...-226a20fc, exactly one remained (so the superseded one was revoked only after the update landed), and the write scope moved to the source runtime group:A follow-up
getconfirmed both the new description andoutputConfigwithresultDestination: SOURCE_LOG_GROUPand nologGroupName.Cleanup: the config, the provisioned IAM role and its inline policy, and the
/company/pr4-online-eval-resultslog group were all deleted (verified absent).Tests
online-eval.flags.test.tsx(new, 15 tests) — output config from inline / stdin reaching Core unchanged on both commands; tags parsed to a map; a non-string tag value rejected; omitted flags stayingundefined; malformed--output-configor--tagsfailing with an empty Core call log, which is the assertion that no IAM role was provisioned for a config that was never created;--descriptionreaching Core; the three warning scopes naming what moved and what to grant; the warning suppressed under--json.onlineEvalExecutionRole.test.ts(+5) — the bare-string invariant, the custom dedicated group,SOURCE_LOG_GROUPscoping to the source prefix, a reserved-namespace destination adding nothing, and a destination change producing a different policy name.TestCoreClientgainedsetOnlineEvalRoleScopeWarning, since the real Core decides this from IAM state a double can't see.Verification
bun test— 3237 pass, 0 fail (229 files)bun run typecheck,bun run lint:check,bun run format:check— clean