feat: add invoke functionality to web-ui#1326
Conversation
Package TarballHow to installgh release download pr-1326-tarball --repo aws/agentcore-cli --pattern "*.tgz" --dir /tmp/pr-tarball
npm install -g /tmp/pr-tarball/aws-agentcore-0.14.1.tgz |
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-cli-automation
left a comment
There was a problem hiding this comment.
Nice refactor extracting resolveInvokeTarget into a reusable module — the unit tests in resolve.test.ts are well-structured and only mock at the I/O boundary (token fetch). The new web-UI deployed-invocation path is a useful addition, but I have a few concerns that should be addressed before merging.
Summary of issues:
- Mid-stream errors in the new
handleDeployed*Invocationhelpers leave the client hanging (headers sent, but nores.end()and no error frame). - ~250 lines of new handler code in
invocations.ts(the entire deployed-invocation path) has zero test coverage. handleDeployedInvocationdoesn't accepttargetName, so multi-target projects can only invoke against the first deployed target.
Details inline.
agentcore-cli-automation
left a comment
There was a problem hiding this comment.
LGTM. Verified the three issues raised in earlier automated review comments have been addressed in the latest commit:
- Mid-stream errors: each
handleDeployed*Invocationhelper now wraps thefor awaitin try/catch/finally that emits an SSE error frame and ends the response. targetNameplumbing: the request body now includestargetNameand it's forwarded toresolveInvokeTarget.- Test coverage:
deployed-invocations.test.tscovers the validation paths, protocol routing (HTTP/A2A/AGUI), the resolve-error path, the mid-stream failure path, and the config-load failure path.
The resolveInvokeTarget extraction looks clean, the existing handleInvoke is faithfully refactored onto it, and resolve.test.ts gives good coverage of the auth/session/baggage edge cases.
No new blocking concerns from me.
|
Claude Security Review: no high-confidence findings. (run) |
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-cli-automation
left a comment
There was a problem hiding this comment.
One follow-up issue from my own review: the new handleDeployedMcpProxy was added in the fix: support mcp commit (after the earlier review pass on the deployed invocation path) and has no test coverage. This mirrors exactly the gap the prior automated review flagged for handleDeployedInvocation, which got addressed. Please extend mcp-proxy.test.ts with ?target=deployed cases.
| }) | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
handleDeployedMcpProxy (~130 LOC) has no test coverage.
The deployed MCP proxy was added in the fix: support mcp commit, after the previous round of review. It now mirrors handleDeployedInvocation in size and complexity but mcp-proxy.test.ts only covers the local proxy path (no ?target=deployed cases). Please add tests parallel to those in deployed-invocations.test.ts, covering at minimum:
- 404 when
configRootis unset - 400 on invalid JSON body
- 400 when
bodyis missing - 400 when
resolveInvokeTargetfails (forwardsresolved.error.message) - 500 when config loading throws
- happy path for each JSON-RPC method:
initialize(returnssessionIdfrommcpInitSession),tools/list(returns wrapped JSON-RPC envelope),tools/call(validatesparams.name, wraps response as{ content: [{ type: 'text', text }] }) - 400 for an unsupported method
- 502 when an AWS SDK call throws
Mock at the AWS SDK boundary (mcpInitSession, mcpListTools, mcpCallTool) like deployed-invocations.test.ts does for invokeAgentRuntimeStreaming etc.
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-cli-automation
left a comment
There was a problem hiding this comment.
Earlier review concerns (mid-stream errors, targetName plumbing, handleDeployedInvocation test coverage, handleDeployedMcpProxy test coverage) have all been addressed in the latest commits — nice work. I do see two new shape/inconsistency issues in the deployed MCP proxy that I think should be sorted out before merging, plus an FYI on telemetry. Details inline.
| }) | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
FYI / non-blocking: deployed invocation paths have no telemetry.
The equivalent agentcore invoke command emits cli.command_run with protocol/streaming attrs (src/cli/commands/dev/command.tsx:257), and the telemetry README explicitly calls out instrumenting new features. The new deployed-invocation paths in the web UI (and the deployed MCP proxy in mcp-proxy.ts) don't emit anything.
I realize the local web-ui invocation paths above also lack telemetry, so this is partly a pre-existing gap rather than a regression — flagging in case you want to address both in this PR or a follow-up. Up to you whether to block on this; if you're going to defer it, an issue/TODO would be helpful so we don't lose track.
|
Claude Security Review: no high-confidence findings. (run) |
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-cli-automation
left a comment
There was a problem hiding this comment.
All serious issues from the prior review rounds have been addressed:
- Mid-stream errors silently dropped — fixed: each deployed streaming helper now wraps the
for awaitin try/catch/finally, emits an SSE error frame, and always callsres.end()(invocations.ts:515-523, 553-561, 593-601). targetNamenot plumbed through — fixed: parsed from request body and forwarded toresolveInvokeTargetin bothinvocations.ts:373-379, 422andmcp-proxy.ts:107-122, 156.- Test coverage for deployed-invocation path — added:
__tests__/deployed-invocations.test.tscovers validation, protocol routing (HTTP/A2A/AGUI), mid-stream failure, resolve errors, and config load failure. - Test coverage for
handleDeployedMcpProxy— added:__tests__/mcp-proxy.test.ts?target=deployeddescribe block covers all 8 listed cases. mcpListToolsleakingmcpSessionId— fixed:const { tools } = await mcpListTools(...)(mcp-proxy.ts:184), and the test fixture at line 322-350 explicitly returnsmcpSessionIdto guard against regression.tools/callflattening structured content — author confirmed this matches current frontend rendering; revisit when the UI supports rich content.- Telemetry — pre-existing gap was acknowledged as non-blocking.
Mocking in the new tests is appropriate (mocks at the AWS SDK boundary and at resolveInvokeTarget, which has its own dedicated unit tests in resolve.test.ts).
LGTM.
Summary
resolveInvokeTargetinto a reusable module shared by CLI invoke and web-ui handlersdeploymentTargetsfrom the resources API so agent inspector can build target picker?target=deployedpath to the MCP proxy (/api/mcp) for deployed MCP tool listing and callingres.end()Details
Deployed invocation (
POST /invocations?target=deployed)agentName,targetName,prompt,sessionId,userIdin the request bodyDeployed MCP proxy (
POST /api/mcp?target=deployed)initialize,tools/list, andtools/callJSON-RPC methodsmcpInitSession,mcpListTools,mcpCallToolfrom the AWS SDK layerResources API (
GET /api/resources)deploymentTargets: [{ name, region, description? }]fromaws-targets.jsonError handling
for awaitin try/catch/finallyTest plan
agentcore dev→ toggle to Deployed mode → invoke HTTP/A2A/AGUI agentagentcore dev→ MCP agent → toggle Deployed → verify tools/list and tools/call work