feat(project): add project-aware invoke - #2115
Conversation
|
Claude Security Review: no high-confidence findings. (run) |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## refactor #2115 +/- ##
============================================
- Coverage 97.25% 97.25% -0.01%
============================================
Files 472 479 +7
Lines 29018 29491 +473
============================================
+ Hits 28221 28681 +460
- Misses 797 810 +13 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
AgentCore Harness Review
Verdict: Looks good
Nice split: the shared invokeRuntimeTarget / invokeHarnessTurn helpers cleanly refactor the existing handlers into reusable operations, the new project-level invoke composes those without duplication, and the resolveDeployedResource layering (manager → backend → deployment helpers) is easy to follow. Tests use real temp dirs + fake backends, and only mock at true I/O boundaries (readStack, per the guidelines).
One thing worth double-checking before this fully lights up end-to-end:
src/core/project/backends/cdk/deployment.ts:28looks up harnesses by CloudFormation export name${stackName}-Harness-${resourceName}-Id. Scanningagentcore-l3-cdk-constructs, the harness constructs (AgentCoreHarnessEnvironment,AgentCoreHarnessRole,AgentCoreApplication) currently only emitHarness-<name>-RoleArnandHarness-<name>-ImageUrioutputs — noHarness-<name>-Id. Runtime lookup will work today (AgentEnvironment.tsemits<agentName>-RuntimeId), butagentcore invoke --harness ...will always fail with the "not deployed" error until the L3 emits that export. If a coordinated L3 change is in flight this is fine; if not, this handler and its export-name contract will need to land together with the construct change (and ideally the runtime path inAgentEnvironment.tsshould probably also be prefixed like-Runtime-<name>-Idfor symmetry withHarness-<name>-Id, but that's a naming choice).
Not blocking — tests all pass with a fake backend, and if the harness export is a known follow-up this is just a heads-up. Everything else (validation, mutual-exclusion, --json requiring content, session-id length rule, bearer-token restricted to runtime, TUI launch with inputMode: "prompt", region override from the resolved target) reads correctly.
f2c164c to
c6796be
Compare
|
Claude Security Review: no high-confidence findings. (run) |
|
Claude Security Review: no high-confidence findings. (run) |
|
Claude Security Review: no high-confidence findings. (run) |
|
Claude Security Review: no high-confidence findings. (run) |
|
Claude Security Review: no high-confidence findings. (run) |
|
Claude Security Review: no high-confidence findings. (run) |
|
Claude Security Review: no high-confidence findings. (run) |
|
Claude Security Review: no high-confidence findings. (run) |
|
Claude Security Review: no high-confidence findings. (run) |
|
Claude Security Review: no high-confidence findings. (run) |
|
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.
thanks for making this change, I know its a big, but critical one. I honestly didn't get to all of it, but saw a few patterns I wanted to comment on:
- not all of the invoke runtime logic lives in core, so we end up importing across handlers, and breaking dependency inversion boundary to share this functionality, but it feels like we're fighting the framework/architecture. Might be worth chatting w/ @AlexanderRichey to hear his vision on where this logic should live since he knows it best.
- the tui being conditionally rendered rather than only on no flags adds a lot of complexity downstream with that launch context being passed around, and I'm wondering if this is required?
| qualifier: flags.qualifier, | ||
| bearerToken, | ||
| }; | ||
| await renderInvokeTui( |
There was a problem hiding this comment.
wouldn't this mean if I do agentcore project invoke --session-id .. --qualifier .. --target .. --bearer-token we would open the TUI?
I thought the convention was that no flags => open TUI.
I'm open to breaking this, but I do think we need to be consistent since it might confuse a customer (or their agent) if it varies by command what determines if a TUI opens.
On the flip side, I do think this existing convention helps keep things simple. For example, we don't need the branching logic here to conditionally render the tui in a few places.
There was a problem hiding this comment.
Yeah this is legacy from the old CLI. Runtime and Harness invoke already treat session, qualifier, authentication, and target values as TUI launch configuration. Project invoke is consistent with those commands.
I definitely agree with the concern about the pattern, like you mention. But I also agree with what you say where it keeps things simple. This makes it so that I am able to do things like pass a bearer token or an session id, before i open the TUI. If this wasn't an option we would need to provide a way for the user to do that within the TUI. Which we wanted to avoid for right now.
| throw new InputValidationError("--bearer-token is only valid with --runtime"); | ||
| } | ||
| if ( | ||
| selected.resourceType === "harness" && |
There was a problem hiding this comment.
doesn't a similar constraint also apply to runtime (with a higher cap)? https://docs.aws.amazon.com/bedrock-agentcore/latest/APIReference/API_InvokeAgentRuntime.html#API_InvokeAgentRuntime_RequestSyntax
unreleated to changes here but I wonder why they are different max length?
|
|
||
| function targetContext(ctx: Context, region: string): Context { | ||
| return ctx.withValue(RegionKey, region); | ||
| } |
There was a problem hiding this comment.
what's the advantage of making this a function? I feel like it adds an unnecessary layer of indirection.
There was a problem hiding this comment.
Thought I had removed this. Will fix this.
| import { argument, createHandler, flag, ProjectKey, type Context } from "../../../router"; | ||
| import { JsonRendererKey, renderTuiAt } from "../../../tui"; | ||
| import { RuntimeInvokeLaunchContextKey } from "../../runtime/invoke/launchContext"; | ||
| import { invokeRuntimeTarget } from "../../runtime/invoke/operation"; |
There was a problem hiding this comment.
this feels strange to me. I'm not sure we want handlers from project depending on imperative handlers. I feel like core should be where this shared functionality lives.
My understanding of the setup was that handlers serve as a "ui shell" over the functionality that handle routing and validation, with core implementing the core logic. However, I think @AlexanderRichey would know better.
There was a problem hiding this comment.
Yeah, I had the same concern. I extracted these operations because duplicating the Runtime and Harness invocation flows under project felt incorrect. I agree the dependency direction is unusual, though, and invokeRuntimeTarget in particular may belong behind CoreRuntimeClient.
The complication is that the current Runtime operation composes handler-owned request normalization, while the Harness operation folds service events into the handler-owned transcript model. I don’t think source resolution, terminal output, TUI state, or transcript presentation should move into Core, but there
may be a cleaner split where Core owns the Runtime lookup/normalization/invocation workflow and handlers retain presentation policy.
I’d definitely like to check with @AlexanderRichey before changing this too much so we follow the intended architecture
| } | ||
|
|
||
| if (selected.resourceType === "harness") { | ||
| const result = await invokeHarnessTurn( |
There was a problem hiding this comment.
where we do handle user cancellations for harness? I see the wrapper for runtime below.
| agentcore project invoke | ||
|
|
||
| # The project-aware shorthand has the same behavior inside a project. | ||
| agentcore invoke |
There was a problem hiding this comment.
thanks for updating the docs!
Also, I like this idea of making it project aware, but I wonder if we should revisit this with all project commands together to make sure the behavior is consistent. If the team has already aligned this is where we're heading than I'm good to leave it in.
| expect(UserCancellationError.resolve(new Error("failed"))).toBeUndefined(); | ||
| }); | ||
|
|
||
| test("RuntimeAgentResponseError is a visible service failure", () => { |
There was a problem hiding this comment.
nit: could this be part of the test.each below? Its doing the same thing with one more check for a check for silent, which could be added as a parameter to the tabled test.
ex.
test.each([
...
[
"RuntimeAgentResponseError",
new RuntimeAgentResponseError("Model access denied"),
"service",
false" // not silent
]
...
| }); | ||
|
|
||
| describe("FsProjectManager.resolveDeployedResource", () => { | ||
| const targets: AwsDeploymentTarget[] = [ |
There was a problem hiding this comment.
q: what behavior are we testing here, that we don't get through the handlers?
| @@ -0,0 +1,125 @@ | |||
| import { parseAgentEvent } from "../../../core/project/agentEventParser"; | |||
There was a problem hiding this comment.
isn't this breaking the dependency inversion?
| @@ -0,0 +1,60 @@ | |||
| export type AgentEvent = | |||
There was a problem hiding this comment.
I'm kind of confused where this fits in. Isn't this assuming a certain response shape from the customer's runtime?
|
Claude Security Review: no high-confidence findings. (run) |
|
Claude Security Review: no high-confidence findings. (run) |
|
Claude Security Review: no high-confidence findings. (run) |
|
Claude Security Review: no high-confidence findings. (run) |
|
Claude Security Review: no high-confidence findings. (run) |
Description
Add project-aware invocation for Runtimes and Harnesses declared by the project enclosing the current directory.
agentcore project invokeas an interactive picker for deployed project Runtimes and Harnesses on the default targetagentcore project invoke runtime --name <name> --payload <payload>agentcore project invoke harness --name <name> --prompt <prompt>--nameto be omitted when the project declares exactly one resource of that typeThis PR intentionally does not define a project-specific Runtime payload or response contract. Runtime responses retain the existing raw invoke behavior, including raw SSE. Gateway project invoke remains outside this scope.
The project deployment prerequisites are already present on
refactor:Related Issue
N/A
Documentation PR
N/A - documentation is included.
Type of Change
Testing
bun test src:2236 pass, 0 fail149 pass, 0 failbun run typecheckbun run lint:checkbun run format:checkbun run secrets:checkbun run buildgit diff --checkLive verification in account
603141041947, regionus-west-2, using the retainedInvokeMatrix827project:--name, preserved the native raw SSE response, and returnedREBASED_RUNTIME_OK--name, preserved the transcript response, and returnedREBASED_HARNESS_OKChecklist