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) |
|
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.
|
Claude Security Review: no high-confidence findings. (run) |
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()Additions to agent inspector (web-ui) APIs
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