Uh oh!
There was an error while loading. Please reload this page.
feat(templates): wire in memory to the runtime templates - #2116
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## refactor #2116 +/- ##
=========================================
Coverage 97.24% 97.25% =========================================
Files 472 472 Lines 28956 29018 +62 =========================================
+ Hits 28159 28221 +62
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
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
f07f985CompareClaude Security Review: no high-confidence findings. (run) |
Claude Security Review: no high-confidence findings. (run) |
f07f985 to
9088f13Compare
notgitika
left a comment
There was a problem hiding this comment.
LGTM thanks for addressing comments :)
Claude Security Review: the review did not analyze this PR (model took 0 turns). See the run for details; a later push or re-run is needed. |
| } | ||
| {{/if}} | ||
| return AgentCoreMemorySessionManager( |
There was a problem hiding this comment.
This is fine for now since we said we don't want to make to many changes, and I know you said you would eventually like to improve the template. One thing we should change is to use the new AgentCoreMemoryManager and AgentCoreMemoryStore at some point. We should theoretically be using our own best practices.
| f"/episodes/{actor_id}/{session_id}": RetrievalConfig(top_k=5, relevance_score=0.5), | ||
| {{/if}} | ||
| {{#if (includes memoryStrategies "SUMMARIZATION")}} | ||
| f"/summaries/{actor_id}": RetrievalConfig(top_k=3, relevance_score=0.5), |
There was a problem hiding this comment.
I think this retrieval namespace is different from the strategy being created. MEMORY_SHORTCUTS configures SUMMARIZATION as /summaries/{actorId}/{sessionId}, but the generated runtime queries /summaries/{actor_id}. That means summaries written under the configured session namespace will not be retrieved.
I think this should probablyt include session_id. I just looked and this mismatch also exists in the old template, but this PR makes that strategy part of the default memory so we may as well just make it right here.
There was a problem hiding this comment.
I think this is actually intentional. The namespace in the MEMORY_SHORTCUTS is where the LTM records get written (session specific path), and then the agent retrieves those records across all sessions by dropping the sessionId on the retrieval path.
I see a PR from main that fixes this exact behavior: #1660.
There was a problem hiding this comment.
I see, I didn't realize this. This is definitely correct I was treating the retrieval namespace like an exact match, thanks!
| }, | ||
| { rootDirName: input.name }, | ||
| ); | ||
| return { tree, spec: { runtimes: [buildRuntimeSpec(input)] } }; |
There was a problem hiding this comment.
I think the custom hello-world path can silently ignore --memory. Selecting --framework none routes here, but even when scaffoldRuntimeInput.memory is set, this return only adds the Runtime and does not generate a memories[] entry in agentcore.json or the memory template files. Do you think we should reject --memory for this path for cleanliness in UX?
There was a problem hiding this comment.
I think this is indirectly fixed in #2130 (comment). Let me rebase and verify.
| modelProvider: flags["model-provider"], | ||
| apiKey, | ||
| memory: flags["memory"], | ||
| memory: MEMORY_SHORTCUTS[flags["memory"] ?? "longAndShortTerm"](runtimeName), |
There was a problem hiding this comment.
Is memory supposed to be becoming the default here? Don't have a problem with it but like it is a breaking change. In the released CLI, omitting --memory does not create a Memory resource, but this defaults to longAndShortTerm and provisions four strategies.
There was a problem hiding this comment.
Yes, that was my intention. Since harness is also now defaulting to give customers a memory, I thought we should align and give them a memory by default as well. The runtime experience should be better with a memory attached, so I figure we should give them the best experience by default.
Claude Security Review: no high-confidence findings. (run) |
Claude Security Review: no high-confidence findings. (run) |
| /** | ||
| * Expands the flat asset listing under assetDir into a nested tree of nodes. | ||
| * Builds a file tree from assets under `input.assetDir`. |
| assetDir: string, | ||
| rootDirName?: string, | ||
| transform?: (content: string) => string, | ||
| config: { assetSource: AssetSource }, |
There was a problem hiding this comment.
I like how we are using object types here.
Uh oh!
There was an error while loading. Please reload this page.
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
created a project with the memory template then deployed, verified the project and my cdk stack included a memory. Also verified the memory code was included in the asset rendering.
went to console and invoked it, and verified the memory was created.