Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change masks (rather than fixes) a real user-facing bug: any user who runs
add harness --no-memoryand theninvokewill hit the sameAccessDeniedExceptiononListEventsat runtime, because the harness still callsListEventson a service-managed memory even when the CLI/CDK has skipped the IAM grant.The PR description acknowledges this ("the harness still calls
ListEventson a service-managed memory at invoke time, so it 403s"), but #1626 is being closed by this fix and there's no follow-up tracking the actual product bug. After this merges, the only E2E coverage we had for the--no-memoryinvoke path is gone, and the next time someone reports this from the field there'll be no signal in CI.A few ways to handle this — please pick one before merging:
--no-memoryfails at invoke with AccessDenied on ListEvents" (CDK should grant the action even when memory is disabled, or the harness runtime should not callListEventswhen memory is disabled — that's for the harness/CDK owners to decide), reference it from CI Failure: harness-custom-jwt bearer-token invoke fails with AccessDenied on ListEvents #1626 so CI Failure: harness-custom-jwt bearer-token invoke fails with AccessDenied on ListEvents #1626 isn't closed without the underlying issue being tracked, and link it from the inline comment here (lines 138–140) so future readers see it.--no-memory— e.g., split the suite so the deploy/auth-config assertions still run against a--no-memoryharness (those don't invoke and won't 403), and only the invoke-based assertions use managed memory. That preserves coverage of the--no-memoryadd/deploy path.ListEventsunder--no-memoryis considered expected and the user-facing remediation is "don't use--no-memorywith a harness" (or--no-memoryis being deprecated for harnesses), document that in the--no-memoryflag help text onadd harnessso users discover it at CLI time rather than at AccessDenied time.The cheapest thing is probably (1) — it costs nothing in this PR and keeps the bug visible.
Tangential nit on the PR description (not a blocker): the quoted CDK snippet (
const managedMemory = props.spec ? props.spec.memory?.mode !== 'disabled' : harness.managedMemory;) doesn't exist inAgentCoreHarnessEnvironment.tsinagentcore-l3-cdk-constructsonmain. The actualListEventsgrant is inAgentCoreApplication.wireMemoriesToHarnesses(src/cdk/constructs/l3/AgentCoreApplication.ts:235-262), gated onharness.memoryNamebeing set — which--no-memorydoesn't set, so the effective behavior matches your description, just citing the wrong source location.