Repository navigation
feat(export): preserve Harness dependencies and captured IAM - #2561
aidandaly24 wants to merge 12 commits into
Conversation
There was a problem hiding this comment.
AgentCore Harness Review
Verdict: Looks good
I reviewed the full diff end-to-end — IAM capture, template changes (main.py / memory/session.py / mcp_client/client.py / pyproject.toml), the new connection/discovery wiring, the companion-CDK fail-closed check, and the schema additions. The design is consistently fail-closed: missing actorId raises at invoke time with a pointer to EXPORT_NOTES.md, missing session_id/MEMORY_ID/gateway URL raise RuntimeError, unsupported ${arn} header templates throw InputValidationError, and requireRuntimeCapabilities throws if the installed CDK silently strips bindingMode / executionRoleConfig / connections. IAM capture validates Role.Arn matches the requested ARN, filters reserved aws: tags, decodes URL-encoded policy documents, verifies pagination makes progress, and refuses to proceed on malformed responses — and the handler aborts before scaffolding when capture fails (covered by the "does not scaffold a project when source IAM capture fails" test).
A couple of minor behavioral notes worth being aware of (not blockers):
addConnectionsilently upgradesexisting.accessto"readwrite"when a pre-existing user connection matched a memory target with a different access level. For memory this is required for the session manager, so it's defensible, but it does override user intent without a note.- In
src/core/executionRoleSource.ts, theMarkervariable is reused across three sequential pagination loops. It happens to work because each loop exits withMarker === undefined, but it would be a touch more robust to scopeMarkerper loop.
Neither needs to change before merging. Test coverage for the new paths (gateway IAM/no-auth, memory ARN with retrieval config, provider-ARN placeholder preservation, source-role capture + collision checks) looks thorough and uses real temp directories + stubbed SDK boundaries, which is the pattern we want.
|
Claude Security Review: no high-confidence findings. (run) |
d1d7a2e to
527a32a
Compare
|
Claude Security Review: no high-confidence findings. (run) |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## refactor #2561 +/- ##
============================================
- Coverage 97.50% 97.48% -0.02%
============================================
Files 656 657 +1
Lines 48540 48918 +378
============================================
+ Hits 47329 47688 +359
- Misses 1211 1230 +19 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Claude Security Review: no high-confidence findings. (run) |
Description
Phase 1 keeps Harness export on one JSON-driven path for existing and new projects, without editing customer CDK stacks.
GetHarnessGateway/Memory dependencies into existingconnections; generated Python clients/session manager preserve source region and configured static actor.additionalPolicies; create a new Runtime-owned role.bindingMode: "explicit"separate from capturedexecutionRoleConfig.policyMode: "explicit"; reject incompatible companion CDK packages.Companion CDK draft: https://github.com/aws/agentcore-l3-cdk-constructs/pull/417. Bootstrap #2542 is already in the tested base.
Limits: V1 hooks, custom images, OAuth Gateway auth and dynamic/absent actors require manual follow-up. IAM copying does not infer application/resource-policy grants or clone external infrastructure.
Review follow-up: reuse project Memory namespaces and consolidate pre-scaffold failure tests. The companion CDK preserves credential discovery independently of automatic permissions and compares captured-policy documents directly. No new test files or dependencies.
Related Issue
None.
Documentation PR
Generated
EXPORT_NOTES.mdcovers unsupported features and source Memory ownership/retention; no separate documentation PR or snapshot update.Type of Change
Testing
Current independently tested review-fix heads: CLI
dcb6acb8aa1932fa78fdf327106b64f178b75f89, companion CDK6aaaf69205fd6193b03849efae3fa7ba1736722c.Independent review-fix QA: CLI 110 scoped tests passed across 3 existing files; CDK 119 scoped tests passed across 4 existing files. Both passed typecheck, lint, format, build, secrets and diff checks; CDK retains 5 preexisting lint warnings. These are scoped reruns, not all PR-feature tests or 229 new tests. Credential discovery was checked through local CDK synthesis using supplied deployed-state ARNs, not new live provider discovery. Neither final head had a new AWS deployment or invocation.
Historical CLI verification at OLD CLI
527a32aa533b142c9ed52efaa41b62136dbf5e18, rebased onto38d955e35f449b57545448c41577552c706926a6, with OLD CDK5342daa81ec2cb8951f44959504745a695143f26: 230 scoped tests passed; audit, types, build, lint, format, secrets and conflict/diff checks passed. Fresh generated output passed paired-CDK synthesis and matched prior Python output byte-for-byte. Upstream #2560 supplies Handlebars 4.7.10 and fixes the Linux audit failure; credential modes were covered by a focused regression test.Historical read-only ARN export/synth smoke applies only to OLD CLI
527a32aa533b142c9ed52efaa41b62136dbf5e18with OLD CDK5342daa81ec2cb8951f44959504745a695143f26: real Harness ARN export and actual generated-app CDK synthesis passed, including source IAM fidelity, Memory discovery, explicit modes and a new Runtime-owned role. No cloud resources changed; no deployment or model invocation. Gateway was absent from this smoke source.Historical functional proof applies only to OLD CLI
d1d7a2e0c905f9e5ba9285b3a2e501473ec5fe5awith OLD CDK5342daa81ec2cb8951f44959504745a695143f26: both export/deploy flows, four model turns, IAM Gateway/Lambda results, automatic Memory writes and cross-Runtime cold reads passed. Independent cleanup verified all 55 ledger entries absent. The owned Nova fixture usediamEcho; hyphenated tool names were not validated.GitHub CI is reported separately in Checks; pending checks are not treated as passed. These scoped tests and historical manual AWS checks are not
bun run test:e2e; asset behavior is covered by existing tests without snapshot updates.bun test(scoped coverage above)bun run test:e2e, or explained why they are not applicablebun run typecheckbun run lint:checkbun run format:checkbun run buildsrc/assets/, I updated affected snapshots withbun test <test-file> --update-snapshotsand committed themChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.