Uh oh!
There was an error while loading. Please reload this page.
feat(gateway): add interactive invoke console - #1978
Conversation
196b394 to
9c11253Compared312820 to
ad207aeComparead207ae to
858db4dCompareCodecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## refactor #1978 +/- ##
============================================
+ Coverage 96.97% 97.00% +0.02%
============================================
Files 364 366 +2 Lines 20844 21373 +529 ============================================
+ Hits 20213 20732 +519 - Misses 631 641 +10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| metadata: metadata(response), | ||
| state: "streaming", | ||
| }); | ||
| const kind = classifyStreamingResponse(response.contentType); |
There was a problem hiding this comment.
I think we need the same statusCode !== 204 && statusCode !== 205 clause here that we did in the headless invoke PR
There was a problem hiding this comment.
Good catch. Fixed in e14e8848 by exempting HTTP 204 and 205 from the TUI binary/unknown-content guard, matching the headless invoke behavior. I also added screen coverage for both statuses with an empty content type and body; the focused Gateway invoke screen suite passes 31/31.
858db4d to
40b3457Compare| } | ||
| const jsonOutput = ctx.require(JsonKey); | ||
| if (flags.payload === undefined) { |
There was a problem hiding this comment.
does this break the pattern of tui only no flags? If so, could this be confusing for customers?
There was a problem hiding this comment.
Yes, but Runtime invoke already follows this pattern where a missing --payload launches the TUI. I think it technically could be a little bit confusing, but I personally think there's are a couple good reasons for why we should keep it this way. For one, we wanted to keep the TUI simple to start. When building runtime invoke I built an advanced options menu, which was accessible in the invoke TUI via a ctrl + key command. This added complexity to the code and we thought it would be better to start simple and layer this complexity on top in the future. So to keep these configurations accessible, I opted to not open the TUI on no --payload. Second, I think for things like --session-id it is more intuitive to pass it in via the CLI before opening the agent than after opening up the agent.
There was a problem hiding this comment.
The harness invoke TUI is also not strictly no-flags only so I think it's okay to break the pattern on invoke for now.
I think it'd be worthwhile though to have a conversation about this with the team though.
| io, | ||
| ); | ||
| } catch (error) { | ||
| if (error instanceof InvalidEnvironmentError) { |
There was a problem hiding this comment.
why do we need to re-wrap this? Is there a specific InvalidEnvironmentError that should be a InputValidationError?
There was a problem hiding this comment.
Tbh this only exists to change the exit code. I agree with what I think you may be getting at which is that this is unnecessary complexity. I'll remove this.
Runtime invoke also uses this, and I'll update it there as well. I think what I could do is have this here throw a new InvalidEnvironmentError(message, { exitCode: ExitCode.USAGE }) instead.
https://github.com/aws/agentcore-cli/blob/refactor/src/tui/index.tsx#L49-L51
Then remove the catch/rewrite here and in Runtime.
| test("deep-links an id-only invoke and seeds interactive request context", async () => { | ||
| const core = configuredCore(); | ||
| const output = captureIO(); | ||
| const render = spyOn(tui, "renderTuiAt").mockResolvedValue(undefined); |
There was a problem hiding this comment.
is there a way to avoid spying by leveraging the existing dependency injection?
| test("preserves unexpected TUI bearer-token source failures", async () => { | ||
| const failure = new TypeError("source failed"); | ||
| const resolve = spyOn(SourceResolver.prototype, "resolveText").mockRejectedValue(failure); |
There was a problem hiding this comment.
same question as above. Is there way we can inject real implementations?
| ): Promise<string | undefined> { | ||
| if (source === "-") { | ||
| throw new InputValidationError( | ||
| "stdin bearer tokens are not available when launching the interactive console", |
There was a problem hiding this comment.
don't we need to support stdin for sensitive fields?
There was a problem hiding this comment.
This is unfortunately a trade off of using the TUI. Headless invoke supports --bearer-token -. Interactive invoke deliberately does not because Ink needs the same stdin for keyboard input. Consuming it for the token would drain or block the TUI. Interactive mode supports inline and file:// tokens. So you still can pass bearer token securely through a file for interactive. Runtime has the same restriction.
| try { | ||
| return await new SourceResolver({ stdin }).resolveText("bearer-token", source); | ||
| } catch (error) { | ||
| if (error instanceof SourceResolutionError) { |
There was a problem hiding this comment.
whats wrong with the sourceResolutionError bubbling up?
e14e884 to
fdbfe3aCompare
Hweinstock
left a comment
There was a problem hiding this comment.
lgtm! thanks for answering my earlier questions, those decisions make sense.
Uh oh!
There was an error while loading. Please reload this page.
Summary
agentcore gateway invokeUX
Ctrl+Tswitches Gateway and clears Gateway-specific stateCtrl+Pedits the Gateway-relative path; blank uses the exact Gateway URLCtrl+Vtoggles completed JSON between raw and pretty formsVerification
206 pass, 0 failfocused Gateway/Runtime invoke tests1197 pass, 0 failfullbun test srcbun run typecheckbun run lint:checkbun run format:checkbun run buildStack
Depends on #1967. Review this PR against
feat/gateway-invoke.