fix: create global entrypoint for tui#1365
Conversation
|
Claude Security Review: no high-confidence findings. (run) |
Package TarballHow to installgh release download pr-1365-tarball --repo aws/agentcore-cli --pattern "*.tgz" --dir /tmp/pr-tarball
npm install -g /tmp/pr-tarball/aws-agentcore-0.14.2.tgz |
agentcore-cli-automation
left a comment
There was a problem hiding this comment.
Thanks for unifying the TUI entrypoint — the RenderTUIOptions shape and InitialRoute typing are nice. I have a few concerns that I think need to be addressed before this merges.
Summary of issues
-
Telemetry regression for
invokeTUI: The PR removes thewithCommandRunTelemetry('invoke', …)wrapper that previously wrapped the TUI invoke flow. No replacement was added, socli.command_run(withhas_stream/has_session_id/auth_type/agent_protocol/duration/exit_reason) is no longer emitted foragentcore invokeinteractive mode. See inline comment. -
Behavior change: TUI flows no longer auto-exit on success when launched from CLI commands. Previously
agentcore add,agentcore remove, etc. rendered their flows withisInteractive={false}, which makes screens likeAddFlow,RemoveAllScreen,RemoveFlow,AddSuccessScreen,AddMemoryFlow,AddEvaluatorFlow,AddOnlineEvalFlow,AddIdentityFlowauto-exit after success (see e.g.AddFlow.tsx:206,RemoveAllScreen.tsx:30). Going throughAppnow means they always receiveisInteractive={true}, so the user has to press escape to leave after every successful run. See inline comment. -
Double
TelemetryClientAccessor.init()for command-routed paths. Whenagentcore add(or any of the migrated commands) runs,main()callsTelemetryClientAccessor.init(args[0], 'cli')(cli.ts:271) and then the action handler callsrenderTUI()which callsTelemetryClientAccessor.init(initialRoute.name, 'tui')(cli.ts:127). The first client (mode=cli) is overwritten in the singleton and nevershutdown()'d. See inline comment. -
Circular import.
src/cli/cli.tsimportsregisterInvoke/registerAdd/etc. from./commands/*, and those command files now importrenderTUIfrom'../../cli'. It works today becauserenderTUIis only referenced inside async action handlers, but it's fragile. See inline comment.
Items 1 and 3 are the most important — 1 is a clear telemetry regression and 3 partially undoes the telemetry-mode fix this PR is trying to land.
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-cli-automation
left a comment
There was a problem hiding this comment.
Thanks for the cleanup — the unified renderTUI() entrypoint is a nice improvement and the recent commits address most of the earlier review feedback (double init, isInteractive threading, render-tui module extraction).
Flagging two new issues below; both involve information being silently dropped on the way through renderTUI. The other pre-existing review comments still appear open but their underlying concerns look addressed in the latest commits — happy to leave those for the original reviewer to confirm.
9456078 to
bce4cf9
Compare
Create a unified renderTUI() entrypoint that all TUI-rendering code paths use instead of inline Ink render() calls. This fixes telemetry never being emitted for bare 'agentcore' TUI mode and mislabeling TUI sessions as CLI. - Add renderTUI() with RenderTUIOptions for configurable behavior - Migrate add, deploy, create, remove, invoke commands to use renderTUI() - Add InitialRoute type for type-safe route navigation - Add actionOnBack option to control escape/back behavior - Add enterAltScreen option for inline vs full-screen rendering - Initialize and shutdown TelemetryClientAccessor within renderTUI() Fixes aws#895
Move the withCommandRunTelemetry('invoke', ...) call into useInvokeFlow
so that a cli.command_run event is emitted regardless of how the invoke
screen is launched (bare agentcore TUI or agentcore invoke command).
This restores the telemetry emission that was lost when the invoke
command was migrated to use renderTUI().
…avior Commands launched via CLI (agentcore add, deploy, etc.) previously rendered with isInteractive=false, causing screens to auto-exit after success. The renderTUI migration broke this by hard-coding isInteractive=true. Add isInteractive option to RenderTUIOptions (default: true) and thread it through App to all screens. CLI command handlers pass isInteractive=false to preserve the previous auto-exit behavior.
|
Claude Security Review: no high-confidence findings. (run) |
The renderTUI migration dropped diffMode — agentcore deploy --diff silently fell back to a regular deploy flow. Extend the deploy Route variant with diffMode and pass it through to DeployScreen. Add test that verifies diffMode reaches renderTUI via initialRoute.
The invoke telemetry was hard-coding agent_protocol='unknown'. Read the project spec before the telemetry wrapper so the actual protocol from the first runtime is used in the metric attributes.
274c6ff to
8acb65f
Compare
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-cli-automation
left a comment
There was a problem hiding this comment.
Reviewed the latest commits — the author has addressed all the major issues raised in prior reviews:
- Telemetry regression for
invokeTUI is fixed (withCommandRunTelemetrynow wraps the config-load effect inuseInvokeFlow). isInteractiveis now threaded throughrenderTUI→App→ individual screens, preserving auto-exit behavior for CLI-launched flows.TelemetryClientAccessor.init()now flushes the prior client before re-initializing, so the cli→tui mode switch no longer leaks an unflushed sink.renderTUIwas extracted intosrc/cli/tui/render.ts, breaking the would-be circular import from command files.diffModeis threaded through thedeployroute intoDeployScreen, with a unit test asserting the wiring.agent_protocolis now resolved from the project spec instead of being hard-coded to'unknown'.
The duration-semantics point on cli.command_run (now measuring config-load time rather than full session length for TUI invoke) was acknowledged by the author and is acceptable.
No new blocking issues from me — looks good to merge.
|
Claude Security Review: no high-confidence findings. (run) |
|
Claude Security Review: no high-confidence findings. (run) |
Hweinstock
left a comment
There was a problem hiding this comment.
few comments to help with review.
| const { unmount } = render( | ||
| <DeployScreen | ||
| isInteractive={false} | ||
| autoConfirm={options.autoConfirm} |
There was a problem hiding this comment.
this field is dropped because it wasn't actually used: https://github.com/aws/agentcore-cli/blob/4adc9c08/src/cli/commands/deploy/command.tsx#L211-L214
| headers = parseHeaderFlags(cliOptions.header); | ||
| } | ||
|
|
||
| const tuiResult = await withCommandRunTelemetry( |
There was a problem hiding this comment.
telemetry is moved inside the invoke logic since the client is now initialized in renderTUI.
| isInteractive={false} | ||
| onExit={() => { | ||
| unmount(); | ||
| process.exit(0); |
There was a problem hiding this comment.
killing the process here can prevent clean up from running which includes telemetry client shutdown.
| import { type UpdateCheckResult, printUpdateNotification } from './update-notifier'; | ||
|
|
||
| export function printTelemetryNotice(): void { | ||
| const yellow = '\x1b[33m'; |
There was a problem hiding this comment.
unrelated to work here, but I want to centralize these. Its very confusing as a reader of code to see \x1b[33m and know its yellow.
Description
Previously, commands like
agentcore add,deploy, etc. rendered TUI screens inline via directrender()calls. This made it difficult to distinguish CLI vs TUI code paths, caused telemetry to be mislabeled asmode="cli"for interactive flows, and meant bareagentcorenever initialized or shut down the telemetry client.This PR creates a unified
renderTUI()entrypoint that owns the telemetry lifecycle and replaces all inlinerender()calls. As part of this:src/cli/tui/render.tsand shared notices intosrc/cli/notices.tsto break circular importsadd,deploy,create,remove,invoketo userenderTUI()TelemetryClientAccessor.init()handle re-initialization safelydiffMode,isInteractive,actionOnBack)This refactor fixes #895.
Type of Change
Testing
basic-flows.mov
Manually tested flows:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.