Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4304 +/- ##
==========================================
- Coverage 68.61% 68.29% -0.33%
==========================================
Files 478 478
Lines 48450 48619 +169
==========================================
- Hits 33243 33203 -40
- Misses 12367 12552 +185
- Partials 2840 2864 +24 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
aponcedeleonch
left a comment
There was a problem hiding this comment.
Thanks for the fix — the root cause analysis is spot on and the merge logic is solid. Left two comments on things that could bite us.
c7001e8 to
b56db14
Compare
When --runtime-add-package is used without --runtime-image,
loadRuntimeConfig returned the override as-is with an empty
BuilderImage, producing an invalid Dockerfile ("FROM AS builder").
Even with --runtime-image, AdditionalPackages replaced the defaults
instead of merging.
Additionally, the npx, go, and uvx templates only installed
AdditionalPackages in the builder stage. The runtime stage hardcoded
ca-certificates, so packages needed at runtime (e.g. git) were missing.
Fix both issues:
- Add mergeRuntimeConfig to fill empty BuilderImage from defaults and
deduplicate-merge AdditionalPackages
- Update all three templates to install AdditionalPackages in the
runtime stage
Fixes #4301
b56db14 to
21ab354
Compare
There was a problem hiding this comment.
Large PR Detected
This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.
How to unblock this PR:
Add a section to your PR description with the following format:
## Large PR Justification
[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformationAlternative:
Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.
See our Contributing Guidelines for more details.
This review will be automatically dismissed once you add the justification section.
There was a problem hiding this comment.
Both previous comments addressed — nice work. Two minor nits below, neither blocking. You might need to regenerate the swagger docs with task docs for the CI to pass though
The empty-packages nit I had in mind is already covered by TestEmptyAdditionalPackagesDoesNotBreakBuild — great to see that included.
| @@ -26,7 +26,8 @@ type RuntimeConfig struct { | |||
| // Examples: "golang:1.25-alpine", "node:22-alpine", "python:3.13-slim" | |||
| BuilderImage string `json:"builder_image" yaml:"builder_image"` | |||
There was a problem hiding this comment.
Nit: the BuilderImage field comment says "full image reference" but doesn't mention that an empty value is valid and means "inherit from defaults during merge." The Validate() godoc already mentions this, but callers reading the struct definition might miss it.
| BuilderImage string `json:"builder_image" yaml:"builder_image"` | |
| // BuilderImage is the full image reference for the builder stage. | |
| // An empty string signals "use the default for this transport type" during config merging. | |
| // Examples: "golang:1.25-alpine", "node:22-alpine", "python:3.13-slim" | |
| BuilderImage string `json:"builder_image" yaml:"builder_image"` |
| if !strings.Contains(runtimeStage, tt.wantInRuntime) { | ||
| t.Errorf("runtime stage does not install %q.\nRuntime stage:\n%s", tt.wantInRuntime, runtimeStage) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Nit: nice that this test exists — it's exactly the scenario I was worried about. Consider also adding a nil AdditionalPackages case (as opposed to empty slice) since {{if}} treats both as falsy but they behave differently in Go. Not blocking.
Summary
--runtime-add-packagewithout--runtime-imagecreated aRuntimeConfigoverride with an emptyBuilderImage, whichloadRuntimeConfigreturned as-is — bypassing defaults and producing an invalid Dockerfile (FROM AS builder). Even with--runtime-image, the override'sAdditionalPackagesreplaced the defaults instead of merging with them.Separately, the npx, go, and uvx Dockerfile templates only installed
AdditionalPackagesin the builder stage. The runtime stage hardcodedca-certificates, so runtime dependencies likegitadded via--runtime-add-packagewere missing from the final container.Fixes first half of #4301
Type of change
Test plan
task test)Built
thvfrom the branch and ranthv run --transport stdio --runtime-add-package git --name awesome npx://awesome-agent-skills-mcp. Verified:FROM node:22-alpine AS builder(not empty)apk add --no-cache git ca-certificatesChanges
pkg/runner/protocol.gomergeRuntimeConfigthat fills emptyBuilderImagefrom defaults and dedup-mergesAdditionalPackages; updateloadRuntimeConfigto call itpkg/runner/protocol_test.goTestMergeRuntimeConfig(5 table cases) andTestLoadRuntimeConfigMergesOverrideWithDefaultspkg/container/templates/npx.tmplAdditionalPackagesinstead of hardcodedca-certificatespkg/container/templates/go.tmplpkg/container/templates/uvx.tmplpkg/container/templates/templates_test.goTestRuntimeStageInstallsAdditionalPackagescovering all three transportsDoes this introduce a user-facing change?
--runtime-add-packagenow works correctly without requiring--runtime-image, and packages are available at runtime inside the container.