Fix args handling for protocol builds

[danbarr]

Fixes #2623

Abandons the unimplemented MCPArgs build-time expansion in favor of simpler runtime args handling. This also eliminates potential
duplication of args.

Root cause: command args were working for Go and uvx protocol builds because their Dockerfile templates implemented direct ENTRYPOINT execution, while npx has an interim shell script that was not handling runtime args. An alternate approach to the fix (#2627) led to duplication of args.

Changes:

• Remove unused MCPArgs field from TemplateData struct
• Update go.tmpl, npx.tmpl, and uvx.tmpl to use runtime args ($@)
• Update all test expectations in templates_test.go and protocol_test.go

All Dockerfile templates now accept arguments at runtime instead of baking them in at build time, making the containers more flexible.

Testing:

I tested with Go, uvx, and npx and confirmed the resulting Docker command was as expected for all three:

./bin/thv run go://github.com/aantti/mcp-netbird/cmd/mcp-netbird@latest -- --some-arg true
./bin/thv run uvx://awslabs.aws-diagram-mcp-server@1.0.12 -- --some-arg true
./bin/thv run npx://@launchdarkly/mcp-server@latest -- start

docker ps --no-trunc --format "table {{.Names}}\t{{.Command}}"

Output:

NAMES                                                  COMMAND
awslabs-aws-diagram-mcp-server-1-0-12                  "sh -c 'package='awslabs.aws-diagram-mcp-server@1.0.12'; exec \"${package%%@*}\"' -- --some-arg true"
launchdarkly-mcp-server-latest                         "./entrypoint.sh start"
github-com-aantti-mcp-netbird-cmd-mcp-netbird-latest   "/app/mcp-server --some-arg true"

Signed-off-by: Dan Barr 6922515+danbarr@users.noreply.github.com

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 55.47%. Comparing base (ffef594) to head (6a1568d).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2630   +/-   ##
=======================================
  Coverage   55.47%   55.47%           
=======================================
  Files         312      312           
  Lines       29715    29714    -1     
=======================================
  Hits        16485    16485           
+ Misses      11790    11789    -1     
  Partials     1440     1440           

☔ 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.

[danbarr]

@claude review these changes to confirm they fix the issue described here and don't introduce unintended effects #2623

[claude]

Claude finished @danbarr's task —— View job

---

Code Review Complete ✅

I've reviewed the changes in this PR and confirmed they correctly fix issue #2623 regarding args handling for protocol builds.

Summary

The PR successfully removes the unimplemented build-time args expansion (MCPArgs field in TemplateData) and updates all Dockerfile templates to properly accept runtime arguments. This is the correct approach and aligns with how Docker containers should work.

Changes Analysis

1. Template Changes (go.tmpl, npx.tmpl, uvx.tmpl)

• ✅ go.tmpl: Changed from ENTRYPOINT ["/app/mcp-server"{{range .MCPArgs}}, "{{.}}"{{end}}] to ENTRYPOINT ["/app/mcp-server"]
• ✅ uvx.tmpl: Changed from ENTRYPOINT ["sh", "-c", "package='{{.MCPPackage}}'; exec \"${package%%@*}\" {{range .MCPArgs}}\"{{.}}\" {{end}}\"$@\"", "--"] to ENTRYPOINT ["sh", "-c", "package='{{.MCPPackage}}'; exec \"${package%%@*}\"", "--"]
• ✅ npx.tmpl: Changed from embedding args in the entrypoint script to using exec npx {{.MCPPackage}} \"$@\", which properly passes runtime args using shell's $@

The npx.tmpl change also correctly removes the complex sed expression that was trying to strip version suffixes at runtime, simplifying the code while maintaining functionality.

2. Code Changes

• ✅ templates.go: Removed unused MCPArgs []string field from TemplateData struct
• ✅ protocol.go: Removed initialization of unused MCPArgs field
• ✅ Test files: All test expectations updated to match new templates

Verification of Fix

The changes correctly address the root cause described in the PR:

• Before: Args were being baked into the Dockerfile at build time via the MCPArgs field (which was never actually populated)
• After: Args are passed at runtime via Docker's Cmd field, which gets appended to the ENTRYPOINT

Runtime args flow (verified by code review):

1. User provides args via -- separator in CLI: thv run <protocol>://<package> -- --arg1 --arg2
2. Args stored in RunConfig.CmdArgs via WithCmdArgs() builder option (config_builder.go:113)
3. Passed through runner.go:211 → runtime.Setup() → DeployWorkload() → createMcpContainer()
4. Set as container.Config.Cmd in Docker API (client.go:1427)
5. Docker appends Cmd to ENTRYPOINT at container runtime

For the three protocol builds:

• Go: ENTRYPOINT ["/app/mcp-server"] + runtime args → /app/mcp-server --arg1 --arg2
• uvx: sh -c 'exec "${package%%@*}"' + -- + runtime args → properly passes args to the Python executable
• npx: ./entrypoint.sh which runs exec npx <package> "$@" → properly passes args to the npm package

Potential Issues Checked

✅ No args duplication: The old approach could have led to args being specified both at build time and runtime. This is now impossible.

✅ Shell script handling: The npx template correctly uses "$@" (quoted) to preserve argument boundaries and handle arguments with spaces.

✅ Version handling: npx template correctly keeps {{.MCPPackage}} with version specifier, while uvx uses shell parameter expansion ${package%%@*} to strip version at runtime.

✅ Test coverage: All tests updated to reflect new behavior, with proper escaping of shell syntax.

Minor Observations

1. npx template improvement: The change from $(echo {{.MCPPackage}} | sed 's/@[^@/]*$//') to just {{.MCPPackage}} is cleaner and correct since npx can handle the version specifier.

2. Consistency: All three templates now follow the same pattern of accepting runtime args, making the codebase more maintainable.

3. No breaking changes: Since MCPArgs was never populated in the code (always empty slice), removing it doesn't break existing functionality.

Recommendation

LGTM ✅

This PR correctly fixes the args handling issue. The changes are:

• Architecturally sound (runtime args vs build-time args)
• Properly implemented across all three protocol types
• Well-tested with updated test expectations
• Simple and maintainable

The fix eliminates potential confusion and makes the system work as users would expect: args provided at thv run time are passed to the container at runtime, not baked into the image.

---