feat: add managed tool catalog and build environments - #397
Conversation
# Conflicts: # lambda/v2-orchestrator/index.js
jeromevdl
left a comment
There was a problem hiding this comment.
This feature is quite complex and there is legitimately complexity in the PR to handle this, but the PR adds 20-30% incidental complexity on top of it because of duplicated subsystems:
- recipe v1 vs v2
- 3 different ways to evaluate if environment is ready:
readPublishedEnvironment,resolveEnvironmentSnapshot,publishedBase - one stack for environments, one for tools very close to the environment (createStore, createHandler, startBuild, createStatusHandler, ... in index/status/store vs tools-index/tools-status/tools-store, both modeled as: a versioned entity with DRAFT/BUILDING/READY/PUBLISHED-style revisions, built via CodeBuild, whose status is reconciled by a polling handler, fronted by a CRUD API handler backed by a DynamoDB store. That shared lifecycle is implemented twice. I've compared the shapes and signatures here, not diffed the two stacks line by line, so I can't put a precise "X% identical" number on it.
Can you check what could be merged (schema v1/v2 ? duplications of env and tools ? one environment resolution function ?) and maybe others to simplify / reduce the amount of code?
|
Also, agents/index.js:79-80,107-108 (fetchRuntimeCapabilities, verifyMcpServers) still hit the core AGENTCORE_RUNTIME_ARN, never a project's managed environment. |
|
@jeromevdl Addressed the |
|
@jeromevdl I worked through the duplicated-subsystems change request:
I kept the two domain state machines explicit because their publication semantics differ materially: environments supersede revisions and provision/verify AgentCore runtimes, while tools maintain version aliases/recommendations/dependency graphs and finish at image readiness. Merging those layers would replace visible domain rules with callback/configuration branching rather than reducing complexity. |
eipasteur
left a comment
There was a problem hiding this comment.
Verdict: comment, non-blocking. Read tool-catalog.js, catalog-recipe.js, agents/index.js and managed-environments/main.tf at 14883fe; jeromevdl's points look addressed in 170a2a6, 60ccee6, 3f1ebfd, 656d145, pending his re-review. IAM is well scoped; the only wildcards are ecr:GetAuthorizationToken (unavoidable) and the AgentCore statement at 383 to 391, where Get* and TagResource could probably use local.managed_runtime_arn. Two inline notes below on guards that read stronger than they are; both are fine to defer.
| )) { | ||
| lines.push(`ENV ${name}=${JSON.stringify(value)}`); | ||
| } | ||
| for (const command of recipe.buildCommands) lines.push(`RUN ${command}`); |
There was a problem hiding this comment.
Build commands run as USER root (334) before sha256sum -c at 371, and the manifest they are checked against is generated in the same build at 337, so a root RUN can regenerate it and FORBIDDEN_COMMAND (21) is a string denylist that cd /opt && ... > managed/protected-runtime.sha256 walks around. I think that is fine if admin build commands are trusted by design, which seems to be the model here. Could we say so in a comment at 369 so the checksum step is not read as an integrity guarantee?
| ); | ||
| }; | ||
|
|
||
| const assertPublic = async (url, allowQuery = false) => { |
There was a problem hiding this comment.
assertPublic checks dns.lookup results but fetch(url) at 806 resolves the name again unpinned, and privateAddress (768) misses ::ffff: mapped addresses and CGNAT. The DOCKER-USER rules at 1040 are the actual backstop, so no exposure today. Might be worth a comment saying the JS check is advisory and iptables is load-bearing, or pinning via an undici Agent with connect.lookup if you'd rather close it.
Summary
Tool ingestion
Publisher verifiedevidence fromPlatform pinnedimportsCompatibility
Verification
npm run format:checknpm run lintnpm run secretlintnpm run sdk:checknpm --prefix frontend run typechecknpm --prefix frontend run buildterraform fmt -recursive -check terraformTerraform was not planned or applied against AWS. The deployed-stack workflow and .NET catalog example are documented in
docs/development/testing.md.Closes #390