Uh oh!
There was an error while loading. Please reload this page.
refactor: align templates with old cli definition - #2094
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## refactor #2094 +/- ##
=========================================
Coverage 97.41% 97.42% =========================================
Files 428 428 Lines 26096 26124 +28 =========================================
+ Hits 25422 25450 +28
Misses 674 674 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
4c16ba0 to
4a80a21Compare| }, | ||
| }; | ||
| function buildRuntimeTemplateKey(input: ScaffoldRuntimeInput): string { |
There was a problem hiding this comment.
i plan to refactor this in a future PR. We're going to want some of these parameters to determine the asset templates to render from, and some of them to be rendered into it with handlebars. However, this felt like the smallest change I could make to keep it functional with the new interface.
This PR is intended to focus on aligning the handlers with what we want, then we work backwards from there to implement the functionality we need to support it.
There was a problem hiding this comment.
What makes this complexity necessary at this stage? Do we expect more than a handful of templates?
4a80a21 to
667fb0eCompareClaude Security Review: no high-confidence findings. (run) |
notgitika
left a comment
There was a problem hiding this comment.
still reviewing the manager and templates files in a couple mins
| /** Set of flags needed to scaffold a new Runtime-based agent **/ | ||
| export const ScaffoldRuntimeInputSchema = z.object({ | ||
| runtimeName: z.string().min(1), |
There was a problem hiding this comment.
maybe not related to this PR but if we are only checking if it is a string, is "../MyAgent" a valid runtime name? it would scaffold cold into <cwd>/.. outside the project then right?
There was a problem hiding this comment.
good question! my understanding is this case is handled for the runtime case in the schema itself, so when the write fails, the scaffolding rollback, but we don't have the same on the create command, so its currently possible to do this already.
runtime schema:
agentcore-cli/src/projectSchemas/runtime.ts
Lines 16 to 23 in e51a676
To allow us to fail earlier and fix the create case, we can validate the same regex we validate in the schema here.
| await expect( | ||
| run([ | ||
| "create", | ||
| "--name", | ||
| "MyAgent", | ||
| "--template", | ||
| "hello-world-python", | ||
| "--build", | ||
| "Container", | ||
| ]), | ||
| ).rejects.toThrow(/--template and --build are mutually exclusive/); | ||
| }); | ||
There was a problem hiding this comment.
just wanted to confirm is our vision that we can probably add certain build time env vars into templates? so I can do like hello-world-python[no-memory]
or do we just reject that idea altogether?
There was a problem hiding this comment.
my implementation here is based on the old CLI. my understanding from our conversation yesterday was that we want to mirror existing behavior, before adding new functionality. Hopefully we get a chance to come back and make this more flexible.
There was a problem hiding this comment.
sounds good, we can revisit this later
| "model provider for the scaffolded runtime code", | ||
| z.enum(["Bedrock"]).optional(), | ||
| ), | ||
| flag( |
There was a problem hiding this comment.
api-key is still not supported fully yet right? like it gets resolved but nothing reads it so it is dropped (assuming its since we only have bedrock for now)
There was a problem hiding this comment.
yes exactly. I put it there so that its wired up in the handler once we're ready, but there is no way to provide it.
Let me add some validation to reject this if we pass bedrock to make this more explicit.
| "model-provider", | ||
| "api-key", | ||
| "memory", | ||
| "runtime-name", |
There was a problem hiding this comment.
wait so is runtime-name also mutually exclusive? I cant have "mydemoagent" as my runtime name with the template? seems like we are forcing the user too much here folks might want custom runtime name but the basic hello world scaffolding in it.
pulling it out of this list also gives you somewhere to hang the codeLocation fix.
There was a problem hiding this comment.
yeah I think I agree we should allow this to be customized, but our templates are not flexible enough to support this yet. The follow-up #2099 refactors the templates to add this functionality which should allow us to allow overrides like this.
| }, | ||
| }); | ||
| function parseScaffoldRuntimeInput(input: Record<string, unknown>) { |
There was a problem hiding this comment.
if i miss a flag, it would surface the raw Zod error.
for eg: create --name MyAgent --runtime-name foo printsInvalid input: expected "none" → at framework
it points users to internal camelCase field paths rather than the --kebab flags they typed. can we check for the missing flags up front? or map it back to flag names
There was a problem hiding this comment.
could be polished in follow up tbh
There was a problem hiding this comment.
good catch. I think it might make sense to circle back here, because this definitely isn't the only case of ugly error messages and I think we should establish some consistent standards.
There was a problem hiding this comment.
Wouldn't these kinds of errors get thrown before the handler itself runs? The framework should handle the validations of the given flags assuming they're populated.
There was a problem hiding this comment.
Most of them yes, but I think the referenced message is coming from https://github.com/Hweinstock/agentcore-cli/blob/667fb0e60f750d4bed85ffb8c1fc790053375e69/src/handlers/project/create/index.ts#L117-L121 which happens after the flags are parsed and is part of a shared validation between this and the create handler for the scaffolding flags.
We could get rid of this if we share the flags themselves rather than a separate schema, but originally wanted to keep the flags explicit.
There was a problem hiding this comment.
this is hardcoded but line 103 now writes the assets to app/${input.runtimeName}.
those are thes ame when runtimeName is hello-world, which is true for the presets but not for the custom flags path. I would say this is a blocker for this PR
There was a problem hiding this comment.
Yeah this is a hack to get the same behavior. let me be consistent with the hardcoding and properly remove it in the follow-up.
Claude Security Review: no high-confidence findings. (run) |
Claude Security Review: no high-confidence findings. (run) |
Claude Security Review: no high-confidence findings. (run) |
| memory: z.enum(["none"]), | ||
| }) | ||
| .refine(({ modelProvider, apiKey }) => !(modelProvider === "Bedrock" && apiKey !== undefined), { | ||
| message: "API keys are not compatible with Bedrock model providers", |
| build: "CodeZip", | ||
| entrypoint: "main.py", | ||
| codeLocation: "app/hello-world", | ||
| codeLocation: "app/hello_world", |
There was a problem hiding this comment.
😭 I forgot - is not supported in runtime
| const isCustom = presentScaffoldingFlags.length > 0; | ||
| const source = new SourceResolver({ stdin: config.io.stdin }); | ||
| const apiKey = await source.resolveSecret("api-key", flags["api-key"]); |
There was a problem hiding this comment.
Will this hang if the api-key param isn't passed? api-key is required only for non-Bedrock model providers, right?
There was a problem hiding this comment.
I believe it handles the undefined case internally https://github.com/Hweinstock/agentcore-cli/blob/667fb0e60f750d4bed85ffb8c1fc790053375e69/src/io/source.ts#L62.
And yeah exactly, we only support bedrock atm so this is effectively a placeholder.
AlexanderRichey
left a comment
There was a problem hiding this comment.
Looks good. Left a couple questions that can be addressed in a follow up if necessary.
Uh oh!
There was an error while loading. Please reload this page.
Problem
The new CLI has a new definition of templates that is not fully flushed out and diverges from the old CLI. We want to get e2e functionality first, before rethinking this.
Solution
Future Work
Want to refactor the templates to split up template parameters from template identifiers (i.e. what determines the base vs rendered in with handlebars. )
Testing