feat(templates): support containers for ts http runtime - #2159
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 #2159 +/- ##
=========================================
Coverage 97.25% 97.25%
=========================================
Files 508 508
Lines 33902 33915 +13
=========================================
+ Hits 32972 32985 +13
Misses 930 930 ☔ 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
Small, focused PR that mirrors the existing strands-python + --build Container pattern for TypeScript. The template resolver filter, buildRuntimeSpec's container branch (setting dockerfile: "Dockerfile" and omitting runtimeVersion), and the shortcut's runtimeVersion clearing when overriding to Container all line up correctly. The test coverage matches the existing hello-world-python-container style. Nothing blocking.
A few non-blocking observations the author may want to consider (not required for merge):
npm ci --omit=dev || npm install --omit=dev(Dockerfile.templateL12) silently falls back tonpm installwhenpackage-lock.jsonis missing, which produces a non-reproducible image without any warning. If reproducibility is desired, consider dropping the fallback and requiring the lockfile; if the fallback is intentional for the "nonpm installwas run first" case, an on-image message might help debuggability. Same convenience-vs-reproducibility tradeoff exists in the Python templates, so this is consistent.- No OTel wrapping in the container CMD: the Python container Dockerfiles wrap CMD with
opentelemetry-instrument; this TS one runsnpx tsx main.tsdirectly. That looks consistent with the current TS story (OTel isn't otherwise wired into the TS scaffold), so it's likely fine — just flagging in case OTel-for-TS-container is on the roadmap. entrypoint: "main.js"in the spec while the Dockerfile CMD runsmain.ts: harmless becauseagent.entrypointisn't consulted on the Container deploy path, but slightly confusing to read. Nothing to change.
|
Claude Security Review: no high-confidence findings. (run) |
069bb38 to
e2b721c
Compare
|
responding to reviewer comments:
This is a valid callout for inconsistency with python, addressed as a follow-up to generate the lock file on no-installs
This is intentional since that wrapping is python specific. We will likely want to do something like what https://www.npmjs.com/package/@opentelemetry/auto-instrumentations-node provides, but for now we can mirror whats on mainline and already in production.
This is intentional, the dev command first tries main.ts, and fallsback to main.js if it is missing. |
|
Claude Security Review: no high-confidence findings. (run) |
|
Known failing test: #2140 |
Problem
The strands ts template does not yet support container builds.
Solution
Testing / Verification
went to console and invoked both created agents.