Repository navigation
Conversation
There was a problem hiding this comment.
AgentCore Harness Review
Verdict: Looks good
Thorough review of the source changes — nothing serious to block merging. The feature is cleanly decomposed (core/dev/harness/{hosts,endpoint,image,request}.ts), tests favor real temp directories and real Bun.serve fakes over mocks, and the loopback HTTP endpoint has good defense in depth (loopback-only Host, Origin rejection, required X-Agentcore-Local header).
A few things I checked and am comfortable with:
containerCredentialscredential_process flow (src/core/dev/container.ts) — the0600+ bind-mountedcredential_processfile is a clean replacement for the~/.awsmount, and the Linux uid-mismatch limitation is already called out in the PR description. The atomicwriteFile→renamefor the credentials JSON and thelongLived/forceRefreshlogic look correct.HarnessDevHostslifecycle (src/core/dev/harness/hosts.ts) —busy/removed/startinginterleaving looked suspicious at first (concurrentinvoke, mid-settle removal), butassertIdleplus therelease→retirein thecatchandstreampaths handles it correctly, andpumpis tracked inrunningso the abort-on-shutdown wait reliably awaits container cleanup.token()retry — swallowing all errors fromworkloadAccessToken()and retrying once is slightly broad (not just 404), but the retry is bounded and errors from the second attempt surface normally. Minor, not worth changing.- AWS env forwarding change — intentionally dropping
AWS_PROFILEmatches the PR description. If a user setsAWS_PROFILEvia.env.localor runtimeenvVars, it still flows into the container but will fail against the[default]-only config; that's an acceptable edge case to leave as-is since the error is self-explanatory. - Telemetry — the existing pattern only instruments
cli.command_runat the router level, soagentcore dev --harnessandagentcore invoke --localare automatically covered. No per-feature instrumentation is required by current conventions.
Lockfile churn is large but is just the knock-on from adding @aws-sdk/credential-provider-node and transitive dedup.
|
Claude Security Review: no high-confidence findings. (run) |
8f743f8 to
2776775
Compare
|
Claude Security Review: no high-confidence findings. (run) |
2776775 to
61f2f72
Compare
|
Claude Security Review: no high-confidence findings. (run) |
|
rft says no. |
What
agentcore devruns a project's harnesses locally in the public harness image, one container per harness, behind an endpoint on port 8090.--harness <name>selects one.agentcore invoke --harness <name> --localsends a turn to that endpoint and prints the same transcript as the deployed path.apiKeyArn) work through a dev workload identity,agentcore-dev-<project>-<target>, created on first use and deleted on target teardown.credential_processfile the CLI refreshes. The host~/.awsmount andAWS_PROFILEforwarding are removed.Why
Harness changes to
harness.yamlorsystem-prompt.mdneeded a deploy to test. The credentials change fixes dev containers for profiles that usecredential_process(for example ada), which failed because the host program does not exist in the container.How tested
bun run typecheck,bun run lint,bun run format:check,bun test(4099 pass).bun run test:e2escoped toe2eTest/project/harness.test.tsadd tests and the new local invocation test (deploy cases skipped).0600credentials file (rootless Podman, host uid other than 1000).