feat(templates): wire in memory to the runtime templates - #2116
feat(templates): wire in memory to the runtime templates#2116Hweinstock wants to merge 3 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## refactor #2116 +/- ##
=========================================
Coverage 97.22% 97.22%
=========================================
Files 463 464 +1
Lines 28160 28227 +67
=========================================
+ Hits 27378 27445 +67
Misses 782 782 ☔ 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
I traced through the refactor and the new strands-http-python template. The extraction into templates/{project,runtime,harness,renderer,types,fsTree}.ts is coherent, the manager's addResource rollback semantics are preserved (mutating projectSpec in place is fine because it isn't written on the error path), the new fromTextFile correctly reads Dockerfile content from disk (so getHarnessTemplateResolver matches the old copyFile behavior, minus the pre-write existence check — the error is still thrown at write time via InputValidationError), and the manifest snapshot confirms memory/ is filtered out when --memory is not set.
Two very minor observations that are not blocking:
mergeSpecEntriesinsrc/core/project/templates/project.tsmergesruntimes/credentials/memoriesbut ignoresharnesses, even thoughSpecEntriesincludes it. Currently unreachable sincecreateProjectTreeonly invokes the runtime resolver, so the omission is harmless — worth fixing if a future template contributes harnesses at project-create time.src/assets/templates/strands-http-python/main.pyreferences apyJsonStrhelper ({{pyJsonStr inputSchema}}and{{pyJsonStr litellmAdditionalParams}}) that isn't registered inHandlebarsTemplateRenderer. Guarded behindinlineFunctionTools/litellmAdditionalParams, both of which are never set in the currentruntime.tscontext, so it's latent — but it will blow up the day someone flips those flags on.
Neither of these needs to hold up the merge.
|
Claude Security Review: no high-confidence findings. (run) |
|
Claude Security Review: no high-confidence findings. (run) |
1b25e3b to
f07f985
Compare
|
Claude Security Review: no high-confidence findings. (run) |
|
Claude Security Review: no high-confidence findings. (run) |
f07f985 to
9088f13
Compare
|
Claude Security Review: no high-confidence findings. (run) |
9088f13 to
2f80ed2
Compare
|
Claude Security Review: no high-confidence findings. (run) |
2f80ed2 to
311ee32
Compare
|
Claude Security Review: no high-confidence findings. (run) |
311ee32 to
13f6e95
Compare
|
Claude Security Review: no high-confidence findings. (run) |
Dependent on #2099 (ignore this until that is merged)
Problem
Memory is currently hardcoded to none. The old CLI defaulted to a real memory, and allowed
none | longAndShort | shortoptions.Solutions
Testing