fix(scaffold): allow certain template values to be overriden - #2130
Conversation
|
Claude Security Review: no high-confidence findings. (run) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## refactor #2130 +/- ##
=========================================
Coverage 97.24% 97.24%
=========================================
Files 471 472 +1
Lines 28911 28954 +43
=========================================
+ Hits 28114 28157 +43
Misses 797 797 ☔ 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 refactor. Moving RUNTIME_TEMPLATE_SHORTCUTS and the new resolveRuntimeTemplateShortcut into their own shortcuts.ts cleans up types.ts, and the "compatible flag overrides" semantics are consistent between create and add runtime.
A few things I verified while reviewing:
- The
--buildoverride path spreadsruntimeVersion: undefinedwhen switching a CodeZip template to Container, which correctly clears the template'sPYTHON_3_14and satisfiesScaffoldRuntimeInputSchema.superRefine(Container must not have aruntimeVersion). Going the other way (Container template +--build CodeZip) setsPYTHON_3_14, also validated. languageandframeworkremain locked when a template is chosen, which is the right call since the template asset tree is keyed on${framework}/${language}inruntime.ts.apiKeyoverride with a Bedrock template correctly falls through to the schema's cross-field refine, and the new test covers this.- In
add/runtime, always forcingruntimeName: flags.namein the template branch is a behavior change from the previous hard-codedhello_world/strands_agent, but it matches how the resourcenameis already used downstream (codeLocation: app/${name}) and how the non-template branch behaves.
No new user-facing surface, so no new telemetry needed. No excessive mocking — the new tests operate on real temp directories through the handler.
Nothing blocking; feel free to merge.
I think we might need to adjust its prompt. I can take a look. |
notgitika
left a comment
There was a problem hiding this comment.
Very easy to follow through PR I just added one comment about build also becoming a locked param imo lmk what you think
| const lockedFlag = (["language", "framework"] as const).find( | ||
| (flagName) => flags[flagName] !== undefined, | ||
| ); | ||
| if (isTemplate && lockedFlag) { | ||
| throw new InputValidationError(`--${lockedFlag} cannot override a template`); | ||
| } | ||
|
|
||
| const isCustom = presentScaffoldingFlags.length > 0; | ||
|
|
||
| const source = new SourceResolver({ stdin: config.io.stdin }); | ||
| const apiKey = await source.resolveSecret("api-key", flags["api-key"]); | ||
|
|
||
| const scaffoldRuntimeInput = isTemplate | ||
| ? RUNTIME_TEMPLATE_SHORTCUTS[flags.template!] | ||
| ? resolveRuntimeTemplateShortcut(flags.template!, { | ||
| runtimeName: flags.name, | ||
| ...(flags.build !== undefined && { | ||
| build: flags.build, | ||
| runtimeVersion: flags.build === "CodeZip" ? "PYTHON_3_14" : undefined, | ||
| }), | ||
| ...(flags["model-provider"] !== undefined && { | ||
| modelProvider: flags["model-provider"], | ||
| }), | ||
| ...(apiKey !== undefined && { apiKey }), | ||
| ...(flags.memory !== undefined && { memory: flags.memory }), | ||
| }) | ||
| : isCustom |
There was a problem hiding this comment.
OOS for your PR but would we see any value in making a shared component for the shared functionality b/w create and runtime?
There was a problem hiding this comment.
YES! I'm hoping to come back to this.
There was a problem hiding this comment.
wait why are adding the build override? shouldn't that also belong to the "locked" param?
for eg: strands-python template can be override to container but the generated files won't contain Dockerfile, while the agentcore.json config would reference one.
There was a problem hiding this comment.
The build override should work since its passed as a parameter to the template. I haven't wired up container support for the strands-python one yet, so if its not rejecting that's a bug.
Update: it is a bug, let me just fix that here.
|
Claude Security Review: no high-confidence findings. (run) |
notgitika
left a comment
There was a problem hiding this comment.
awesome, looks great to me!
|
Claude Security Review: no high-confidence findings. (run) |
aidandaly24
left a comment
There was a problem hiding this comment.
This looks good to me. Just had one readability comment.
| ? resolveRuntimeTemplateShortcut(flags["template"]!, { | ||
| ...(flags["runtime-name"] !== undefined && { | ||
| runtimeName: flags["runtime-name"], | ||
| }), | ||
| ...(flags["build"] !== undefined && { | ||
| build: flags["build"], | ||
| runtimeVersion: flags["build"] === "CodeZip" ? "PYTHON_3_14" : undefined, | ||
| }), | ||
| ...(flags["model-provider"] !== undefined && { | ||
| modelProvider: flags["model-provider"], | ||
| }), | ||
| ...(apiKey !== undefined && { apiKey }), | ||
| ...(flags["memory"] !== undefined && { memory: flags["memory"] }), | ||
| }) |
There was a problem hiding this comment.
Not saying necessarily to change this because it is correct, but this is kind of hard to read at first. To make this more readable we could move the override building logic into resolveRuntimeTemplateShortcut? Passing the optional flag values directly would make this easier to scan and keep the build/runtimeVersion relationship in one place
const scaffoldRuntimeInput = resolveRuntimeTemplateShortcut(flags["template"]!, {
runtimeName: flags["runtime-name"],
build: flags["build"],
modelProvider: flags["model-provider"],
apiKey,
memory: flags["memory"],
});
The resolver could ignore undefined values and own the build/runtimeVersion relationship, which would make both callers easier to scan.
There was a problem hiding this comment.
I like that! I'll have to rebase #2116 on top of this anyway, so I'm going to merge and address this there.
Problem
Not all template fields that we'd expect to be able to overwrite are able to be overwritten. For example, in the create flow runtimeName is fixed since passing runtimeName + template is invalid.
Solution
Testing
went to console and verified the name is now generated from bob with name
testP_bob.