Skip to content

Merge runtime config override with defaults#4304

Open
lorr1 wants to merge 2 commits intomainfrom
fix/runtime-add-package-merge-defaults
Open

Merge runtime config override with defaults#4304
lorr1 wants to merge 2 commits intomainfrom
fix/runtime-add-package-merge-defaults

Conversation

@lorr1
Copy link

@lorr1 lorr1 commented Mar 21, 2026

Summary

--runtime-add-package without --runtime-image created a RuntimeConfig override with an empty BuilderImage, which loadRuntimeConfig returned as-is — bypassing defaults and producing an invalid Dockerfile (FROM AS builder). Even with --runtime-image, the override's AdditionalPackages replaced the defaults instead of merging with them.

Separately, the npx, go, and uvx Dockerfile templates only installed AdditionalPackages in the builder stage. The runtime stage hardcoded ca-certificates, so runtime dependencies like git added via --runtime-add-package were missing from the final container.

Fixes first half of #4301

Type of change

  • Bug fix

Test plan

  • Unit tests (task test)
  • Manual testing (describe below)

Built thv from the branch and ran thv run --transport stdio --runtime-add-package git --name awesome npx://awesome-agent-skills-mcp. Verified:

  1. Dockerfile renders FROM node:22-alpine AS builder (not empty)
  2. Both builder and runtime stages run apk add --no-cache git ca-certificates
  3. Container starts, clones the skills repo with git, and loads 547 skills

Changes

File Change
pkg/runner/protocol.go Add mergeRuntimeConfig that fills empty BuilderImage from defaults and dedup-merges AdditionalPackages; update loadRuntimeConfig to call it
pkg/runner/protocol_test.go Add TestMergeRuntimeConfig (5 table cases) and TestLoadRuntimeConfigMergesOverrideWithDefaults
pkg/container/templates/npx.tmpl Runtime stage: use AdditionalPackages instead of hardcoded ca-certificates
pkg/container/templates/go.tmpl Same
pkg/container/templates/uvx.tmpl Same (debian, alpine, and default branches)
pkg/container/templates/templates_test.go Add TestRuntimeStageInstallsAdditionalPackages covering all three transports

Does this introduce a user-facing change?

--runtime-add-package now works correctly without requiring --runtime-image, and packages are available at runtime inside the container.

@github-actions github-actions bot added the size/S Small PR: 100-299 lines changed label Mar 21, 2026
@codecov
Copy link

codecov bot commented Mar 21, 2026

Codecov Report

❌ Patch coverage is 61.90476% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.82%. Comparing base (14d1120) to head (86a4105).

Files with missing lines Patch % Lines
pkg/runner/protocol.go 61.90% 3 Missing and 5 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4304      +/-   ##
==========================================
- Coverage   68.91%   68.82%   -0.10%     
==========================================
  Files         479      479              
  Lines       48505    48538      +33     
==========================================
- Hits        33429    33405      -24     
- Misses      12324    12363      +39     
- Partials     2752     2770      +18     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@aponcedeleonch aponcedeleonch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@lorr1 lorr1 force-pushed the fix/runtime-add-package-merge-defaults branch from c7001e8 to b56db14 Compare March 24, 2026 03:46
@github-actions github-actions bot added size/M Medium PR: 300-599 lines changed and removed size/S Small PR: 100-299 lines changed labels Mar 24, 2026
@lorr1 lorr1 force-pushed the fix/runtime-add-package-merge-defaults branch from b56db14 to 21ab354 Compare March 24, 2026 04:38
@lorr1 lorr1 requested a review from rdimitrov as a code owner March 24, 2026 04:38
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 transformation

Alternative:

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.

@github-actions github-actions bot added size/XL Extra large PR: 1000+ lines changed and removed size/M Medium PR: 300-599 lines changed labels Mar 24, 2026
aponcedeleonch
aponcedeleonch previously approved these changes Mar 24, 2026
Copy link
Member

@aponcedeleonch aponcedeleonch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@lorr1 lorr1 force-pushed the fix/runtime-add-package-merge-defaults branch from 21ab354 to 7cf969f Compare March 24, 2026 17:36
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 first half of #4301
@lorr1 lorr1 force-pushed the fix/runtime-add-package-merge-defaults branch from 7cf969f to 5603353 Compare March 24, 2026 17:36
@github-actions github-actions bot added size/M Medium PR: 300-599 lines changed and removed size/XL Extra large PR: 1000+ lines changed size/M Medium PR: 300-599 lines changed labels Mar 24, 2026
@github-actions github-actions bot dismissed their stale review March 24, 2026 17:36

PR size has been reduced below the XL threshold. Thank you for splitting this up!

@github-actions github-actions bot added the size/M Medium PR: 300-599 lines changed label Mar 24, 2026
@github-actions
Copy link
Contributor

✅ PR size has been reduced below the XL threshold. The size review has been dismissed and this PR can now proceed with normal review. Thank you for splitting this up!

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@lorr1 lorr1 force-pushed the fix/runtime-add-package-merge-defaults branch from 5603353 to 86a4105 Compare March 24, 2026 18:02
@github-actions github-actions bot added size/M Medium PR: 300-599 lines changed and removed size/M Medium PR: 300-599 lines changed labels Mar 24, 2026
@aponcedeleonch aponcedeleonch self-requested a review March 25, 2026 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/M Medium PR: 300-599 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants