Skip to content

feat(online-eval): add --output-config and --tags, --description on update - #2266

Open
jariy17 wants to merge 2 commits into
refactorfrom
feat/online-eval-output-config
Open

jariy17 wants to merge 2 commits into
refactorfrom
feat/online-eval-output-config

Conversation

@jariy17

@jariy17 jariy17 commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Fourth and last of the stack. Now based directly on refactor (#2265 and #2294 have merged).

--output-config on create and update

Same 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 OutputConfig is a plain object with no logStreamName; 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.

--tags on create, --description on update

--tags reuses TagsSchema + parseJsonFlagWithSchema, matching project add memory, so {"team":42} is rejected by the CLI rather than by the API. --description on update was simply missing — create had it.

The execution role, which is what makes the destination usable

At create, executionPolicy granted 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 create-time policy now also grants:

  • the customer-named group, for DEDICATED_LOG_GROUP
  • the sampled source groups, for SOURCE_LOG_GROUP

update never provisions or re-scopes the execution role — it forwards a caller's --role-arn as-is and leaves the policy to them, matching harness update and the pattern #2294 established for online-eval. So the destination-aware widening applies at create only.

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.ts now 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 stored outputConfig is the service-managed default the API echoes back — otherwise the same orphaning problem would hit every config ever created through the CLI.

One shared SourceResolver per command

create and update each build a single SourceResolver and thread it through every stdin-capable flag (--tags, --filters, --data-source-config, --output-config). The resolver's one-stdin guard then fires across options: --filters - --output-config - is reported as a conflict instead of the second flag silently reading an empty string after the first drains stdin.

Live verification — an exploratory account, 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:123456789012:log-group:/aws/bedrock-agentcore/evaluations/*",
    "arn:aws:logs:us-west-2:123456789012:log-group:/company/pr4-online-eval-results*"
  ]
}

Tags landed: {"purpose": "pr4-verify", "team": "agentcore-cli"}.

Update with a new --description and --output-config → both reached the service and a follow-up get confirmed them; the execution role was left untouched, as intended.

Cleanup: the config, the provisioned IAM role and its inline policy, and the /company/pr4-online-eval-results log group were all deleted (verified absent).

Tests

  • online-eval.flags.test.tsx (new, 13 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 staying undefined; malformed --output-config or --tags failing with an empty Core call log, which is the assertion that no IAM role was provisioned for a config that was never created; --description reaching Core; and, on each command, a second option reading from stdin rejected before any SDK call.
  • onlineEvalExecutionRole.test.ts — the bare-string invariant, the custom dedicated group, SOURCE_LOG_GROUP scoping to the source prefix, a reserved-namespace destination adding nothing, and a destination change producing a different policy name.

Verification

  • bun test — 3242 pass, 0 fail (228 files)
  • bun run typecheck, bun run lint:check, bun run format:check — clean
  • Existing online-eval fixture suite passes unchanged, which is the backward-compatibility evidence for the policy document

@github-actions github-actions Bot added the size/l PR size: L label Sep 9, 2026
@agentcore-devx-automation agentcore-devx-automation Bot added the agentcore-harness-reviewing AgentCore Harness review in progress label Sep 9, 2026

@agentcore-devx-automation agentcore-devx-automation Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 outputMovedrefreshManagedRole), stale-scope is reachable for an output-only move. In that case:

  • oldLogGroups === newLogGroups (data source unchanged), so logGroupNames: oldLogGroups in 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:

  1. Have the handler branch on scope for stale-scope too, mirroring the non-stale wording (e.g. `the ${MOVED[scope]} moved but the superseded inline policy could not be detached`), and populate the warning's logGroupNames in eval.tsx with the groups that were actually stale — for output-only moves that's destinationLogGroupNames(current.outputConfig, current.dataSourceConfig); for input-only it's oldLogGroups; for input-and-output it's the union.
  2. 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.

@agentcore-devx-automation agentcore-devx-automation Bot removed the agentcore-harness-reviewing AgentCore Harness review in progress label Sep 9, 2026
@jariy17
jariy17 added this pull request to stack #2270 September 10, 2026 15:43
@jariy17
jariy17 force-pushed the feat/online-eval-output-config branch from e2ebc30 to 6192ad6 Compare September 10, 2026 15:53
@github-actions github-actions Bot added size/l PR size: L and removed size/l PR size: L labels Sep 10, 2026
@codecov-commenter

codecov-commenter commented Sep 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.99%. Comparing base (25a7a82) to head (b324c4e).
⚠️ Report is 2 commits behind head on refactor.

Additional details and impacted files
@@            Coverage Diff            @@
##           refactor    #2266   +/-   ##
=========================================
  Coverage     96.98%   96.99%           
=========================================
  Files           579      580    +1     
  Lines         39442    39515   +73     
=========================================
+ Hits          38253    38326   +73     
  Misses         1189     1189           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jariy17
jariy17 force-pushed the feat/online-eval-output-config branch from 6192ad6 to 704e402 Compare September 10, 2026 17:07
@github-actions github-actions Bot added size/l PR size: L and removed size/l PR size: L labels Sep 10, 2026
@jariy17
jariy17 force-pushed the feat/online-eval-output-config branch from 704e402 to 6654d10 Compare September 10, 2026 23:48
@github-actions github-actions Bot added size/l PR size: L and removed size/l PR size: L labels Sep 10, 2026
@jariy17
jariy17 force-pushed the feat/online-eval-output-config branch from 6654d10 to 50a6753 Compare September 11, 2026 17:19
@github-actions github-actions Bot added size/m PR size: M and removed size/l PR size: L labels Sep 11, 2026
arns.push(...sampledArns);
} else if (
cloudWatch?.logGroupName &&
!cloudWatch.logGroupName.startsWith(SERVICE_RESULT_PREFIX)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to add permissions to write back in a service log?

@github-actions github-actions Bot added size/m PR size: M and removed size/m PR size: M labels Sep 14, 2026
@jariy17
jariy17 force-pushed the feat/online-eval-output-config branch from c385d0e to 552ab8c Compare September 14, 2026 19:36
@github-actions github-actions Bot added size/m PR size: M and removed size/m PR size: M labels Sep 14, 2026
Base automatically changed from feat/batch-eval-output-config to refactor September 14, 2026 21:13
@jariy17
jariy17 force-pushed the feat/online-eval-output-config branch from 552ab8c to e6752f0 Compare September 14, 2026 21:13
@github-actions github-actions Bot added size/m PR size: M and removed size/m PR size: M labels Sep 14, 2026
@agentcore-devx-automation agentcore-devx-automation Bot added the claude-security-reviewing Claude Code /security-review in progress label Sep 14, 2026
@agentcore-devx-automation

Copy link
Copy Markdown
Contributor

Claude Security Review: no high-confidence findings. (run)

@agentcore-devx-automation agentcore-devx-automation Bot removed the claude-security-reviewing Claude Code /security-review in progress label Sep 14, 2026
…pdate

Adds --output-config and --tags to online-eval create, --description and
--output-config to update. Create-time execution-role provisioning widens the
write scope to the chosen destination.

Update never provisions or re-scopes the execution role, matching #2294: a role
passed via --role-arn is forwarded as-is and is the caller's to manage.

Share one SourceResolver across each command's stdin-capable flags (--tags,
--filters, --data-source-config, --output-config) so a second `-` is rejected
rather than reading an empty string after the first drains stdin.
@jariy17
jariy17 force-pushed the feat/online-eval-output-config branch from e6752f0 to c3ee6a7 Compare September 14, 2026 22:39
@github-actions github-actions Bot added size/m PR size: M and removed size/m PR size: M labels Sep 14, 2026
@agentcore-devx-automation agentcore-devx-automation Bot added the claude-security-reviewing Claude Code /security-review in progress label Sep 14, 2026
@agentcore-devx-automation

Copy link
Copy Markdown
Contributor

Claude Security Review: no high-confidence findings. (run)

@agentcore-devx-automation agentcore-devx-automation Bot removed the claude-security-reviewing Claude Code /security-review in progress label Sep 14, 2026
@jariy17 jariy17 changed the title feat(online-eval): add --output-config and --tags, --description on update, and widen the managed role feat(online-eval): add --output-config and --tags, --description on update Sep 15, 2026
@github-actions github-actions Bot added size/m PR size: M and removed size/m PR size: M labels Sep 15, 2026
…xtures

Move the positive assertions into the recorded golden suite: the create golden
now carries --output-config and --tags and the update golden --output-config and
--description, so each command request fixture is the assertion those fields
reach the SDK unchanged. Keep only the genuinely fixture-less negatives (malformed
--output-config/--tags, non-string tag value) as flag-validation assertions in
online-eval.test.tsx, and drop online-eval.flags.test.tsx. Re-recorded against the
exploratory account; SOURCE_LOG_GROUP keeps the recording residue-free.
@github-actions github-actions Bot added size/l PR size: L and removed size/m PR size: M labels Sep 15, 2026
@agentcore-devx-automation agentcore-devx-automation Bot added the claude-security-reviewing Claude Code /security-review in progress label Sep 15, 2026
@agentcore-devx-automation

Copy link
Copy Markdown
Contributor

Claude Security Review: no high-confidence findings. (run)

@agentcore-devx-automation agentcore-devx-automation Bot removed the claude-security-reviewing Claude Code /security-review in progress label Sep 15, 2026
@jariy17
jariy17 marked this pull request as ready for review September 15, 2026 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/l PR size: L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants