-
Notifications
You must be signed in to change notification settings - Fork 467
Fix critical validation gap for run volume parameters #324
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR addresses a critical validation gap where volume parameter templates in server configurations could reference parameters from different servers without validation, leading to runtime failures. The fix introduces server name prefix validation for volume parameters, matching the existing validation pattern used for environment variables.
Key Changes:
- Added
isRunVolumesValid()function to validate volume parameter prefixes - Integrated volume validation into the main validation flow
- Added comprehensive test coverage for volume validation scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| cmd/validate/main.go | Implements isRunVolumesValid() to enforce server name prefix requirements for volume parameters and integrates it into the validation pipeline |
| cmd/validate/main_test.go | Adds test coverage for volume validation and updates Test_areSecretsValid with proper working directory setup |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return nil | ||
| } | ||
|
|
||
| // Check volume parameter usage is valid |
Copilot
AI
Dec 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment should follow Go conventions by starting with the function name. Change to: // isRunVolumesValid checks volume parameter usage is valid
| // Check volume parameter usage is valid | |
| // isRunVolumesValid checks volume parameter usage is valid |
| // Skip if it's a filter expression (contains |) | ||
| paramContent := param[2 : len(param)-2] | ||
| if strings.Contains(paramContent, "|") { | ||
| // Extract just the parameter name before the first | | ||
| paramName := strings.Split(paramContent, "|")[0] |
Copilot
AI
Dec 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment on line 294 is misleading - the code doesn't skip filter expressions but rather handles them specially. Update to: // Handle filter expressions (contains |) by extracting the parameter name before the first |
| // Change to repo root for tests | ||
| originalWd, err := os.Getwd() |
Copilot
AI
Dec 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The directory setup logic is duplicated between Test_areSecretsValid and Test_isRunVolumesValid. Consider extracting this into a helper function or using t.TempDir() with test fixtures to improve maintainability.
Summary
The registry validator had a blind spot: while
isConfigEnvValid()enforces that environment variable templates are namespaced with the server name, there was no equivalent guard forserver.Run.Volumes. As a result, any volume mapping could reference parameters from a completely different server and still pass validation, only to explode later at runtime with an opaque Docker error.Problem
Here’s the kind of misconfiguration we were silently accepting:
Because nothing double-checked
server.Run.Volumes,{{wrong-name.storage_path}}skated by unchecked. When Docker spun up the container it couldn’t resolve that parameter and failed with a cryptic message.Impact
More than twenty production servers rely on templated volumes today, including:
arxiv-mcp-server→{{arxiv-mcp-server.storage_path}}:/app/papersmarkdownify→{{markdownify.paths|volume|into}}aks→{{aks.azure_dir}}:/home/mcp/.azureAny typo in those prefixes would have slipped through CI and landed in the registry, leaving operators to discover the failure only after deployment.
Solution
isRunVolumesValid()to mirror the existing environment validation rules.{{server.param}}) or a filtered expression ({{server.param|filter|into}}).Before
$ go run ./cmd/validate --name my-server ✅ Config env is valid ✅ License validation skipped # Invalid volume parameter silently accepted!After
Testing
arxiv-mcp-server,markdownify, andaksall pass with the new guardrails.This closes the validation gap and surfaces configuration mistakes where they belong—in CI, not in production.