feat: add validation command for vmcp

[yrobla]

Related-to: https://github.com/stacklok/stacklok-epics/issues/153

[codecov]

Codecov Report

❌ Patch coverage is 47.04762% with 278 lines in your changes missing coverage. Please review.
✅ Project coverage is 53.79%. Comparing base (720b794) to head (294e52d).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/vmcp/config/validator.go	50.00%	90 Missing and 39 partials ⚠️
pkg/vmcp/config/yaml_loader.go	48.76%	108 Missing and 16 partials ⚠️
cmd/vmcp/app/commands.go	0.00%	25 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2364      +/-   ##
==========================================
- Coverage   54.26%   53.79%   -0.48%     
==========================================
  Files         242      251       +9     
  Lines       23446    24490    +1044     
==========================================
+ Hits        12724    13174     +450     
- Misses       9514    10034     +520     
- Partials     1208     1282      +74     

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

[JAORMX]

@claude please review this in context of docs/proposals/THV-2106-virtual-mcp-server.md

[claude]

Claude finished @JAORMX's task —— View job

---

Code Review: vmcp Validation Command

I've reviewed the PR in context of the Virtual MCP Server proposal (docs/proposals/THV-2106-virtual-mcp-server.md). Overall, this is a solid foundational implementation that aligns well with the proposal's design. Here's my detailed feedback:

---

Summary

Strengths:

• ✅ Configuration model accurately reflects the proposal specification
• ✅ Clean separation of concerns (loader, validator, config types)
• ✅ Platform-agnostic design supporting both CLI and future Kubernetes deployment
• ✅ Comprehensive validation with good error messages
• ✅ Good test coverage for core validation logic

Areas for Improvement:

• ⚠️ Test coverage is low (22.04%) - need integration tests for YAML loader
• ⚠️ Environment variable handling could be more robust
• ⚠️ Missing validation for circular dependencies in composite tool workflows
• ℹ️ Minor code quality improvements possible

---

Detailed Findings

1. Alignment with Proposal ✅

The implementation correctly maps to the proposal's configuration schema (lines 310-519 of the proposal):

Well-implemented aspects:

• IncomingAuth configuration (OIDC, anonymous, local) - matches proposal lines 318-327
• OutgoingAuth with multiple backend strategies - matches proposal lines 339-378
• Aggregation with conflict resolution strategies - matches proposal lines 387-415
• Token cache configuration - matches proposal lines 381-385
• Composite tools with workflow steps - matches proposal lines 438-519
• Operational configuration with timeouts and failure handling - matches proposal lines 417-436

Configuration model correctness:

• pkg/vmcp/config/config.go provides a clean platform-agnostic model as intended by the proposal's "unified configuration model" (lines 132-143)
• Types match the YAML structure exactly, enabling future Kubernetes adapter implementation

2. Code Quality & Architecture

pkg/vmcp/config/validator.go - Overall good quality with minor improvements possible:

Strengths:

• Comprehensive validation covering all configuration sections
• Clear validation error messages that guide users
• Good separation of concerns with helper methods for each config section
• Proper use of domain errors (vmcp.ErrInvalidConfig)

Issues to address:

1. Missing circular dependency validation (validator.go:396-456)

   • The validateWorkflowSteps function validates that depends_on references exist but doesn't detect circular dependencies
   • Example problematic config:

     steps:
       - id: step1
         depends_on: [step2]
       - id: step2
         depends_on: [step1]

   • Recommendation: Add cycle detection using a topological sort or DFS-based cycle detection algorithm

2. Incomplete dependency validation (validator.go:435-440)

   for _, depID := range step.DependsOn {
       if !stepIDs[depID] {
           return fmt.Errorf("step[%d].depends_on references non-existent step: %s", i, depID)
       }
   }

   • This only checks if referenced steps exist in the map, but the map is built as we iterate
   • Steps can only reference previously defined steps, not future ones
   • Recommendation: Track step IDs separately and validate forward references are caught

3. Magic number in elicitation timeout (validator.go:430-432)

   if step.Timeout <= 0 {
       step.Timeout = 5 * time.Minute // Default elicitation timeout
   }

   • Validator is mutating the config by setting defaults
   • Validators should validate, not transform
   • Recommendation: Either document this behavior clearly or move default-setting to a separate function

4. Parameter validation gap (validator.go:484-493)

   // Transform parameters
   for name, paramMap := range rawTool.Parameters {
       param := ParameterSchema{
           Type: paramMap["type"].(string), // UNSAFE type assertion!
       }

   • Unsafe type assertion could panic
   • Recommendation: Add type checking with proper error handling

pkg/vmcp/config/yaml_loader.go - Good structure with security concerns:

Issues to address:

1. Environment variable handling security (yaml_loader.go:248-252, 311-314, 331-334)

   clientSecret := os.Getenv(raw.OIDC.ClientSecretEnv)
   if clientSecret == "" && raw.OIDC.ClientSecretEnv != "" {
       return nil, fmt.Errorf("environment variable %s not set for client_secret", raw.OIDC.ClientSecretEnv)
   }

   • Returns error only if env var name is set but value is empty
   • If ClientSecretEnv is empty string, code silently uses empty secret
   • Recommendation: Make env var name required for OIDC configs, fail if not provided

2. Missing validation for duration strings (yaml_loader.go:387-390, 398-401)

   ttlOffset, err := time.ParseDuration(raw.Config.TTLOffset)
   if err != nil {
       return nil, fmt.Errorf("invalid ttl_offset: %w", err)
   }

   • Good error handling, but should also validate ranges (e.g., TTLOffset shouldn't be negative or absurdly large)
   • Recommendation: Add range validation for durations

3. Unsafe type assertion in parameter transformation (yaml_loader.go:486-492)

   for name, paramMap := range rawTool.Parameters {
       param := ParameterSchema{
           Type: paramMap["type"].(string), // UNSAFE
       }

   • Will panic if "type" key is missing or not a string
   • Recommendation: Use type switches or checked assertions

cmd/vmcp/app/commands.go - Clean CLI implementation:

Minor improvements:

1. Duplicate error handling (commands.go:45-48, 51-54)

   err := viper.BindPFlag("debug", rootCmd.PersistentFlags().Lookup("debug"))
   if err != nil {
       logger.Errorf("Error binding debug flag: %v", err)
   }

   • BindPFlag errors are extremely rare and usually indicate programming errors
   • Logging and continuing could hide issues
   • Recommendation: Consider panicking or returning error to caller

2. Inconsistent output format (commands.go:142-163)

   • Uses logger for output which is good
   • But mixes information levels (Info for both status and results)
   • Recommendation: Consider structured output for machine parsing (e.g., JSON flag)

3. Test Coverage Analysis

Current coverage: 22.04% (382 lines uncovered)

Well-tested:

• pkg/vmcp/config/validator_test.go: Good unit test coverage for validation logic
• Tests cover happy paths and error cases for major validation functions
• Clear test structure with table-driven tests

Missing tests (explaining low coverage):

1. pkg/vmcp/config/yaml_loader.go (234 missing lines)

   • CRITICAL: No tests for YAML loading functionality
   • Should test:
     • Valid YAML parsing
     • Invalid YAML syntax handling
     • Environment variable resolution
     • Missing environment variable errors
     • All transformation functions (transformIncomingAuth, transformOutgoingAuth, etc.)
     • Edge cases (empty configs, nil values, type mismatches)

2. pkg/vmcp/config/validator.go (87 missing lines + 36 partials)

   • Need tests for:
     • Composite tool workflow validation (especially dependency chains)
     • Operational config validation
     • Failure handling and circuit breaker validation
     • Edge cases in aggregation validation

3. cmd/vmcp/app/commands.go (25 missing lines)

   • Should test:
     • Command registration and execution
     • Flag handling
     • Error paths in validate command
     • Integration: full validate command flow with real YAML files

Recommendation: Add comprehensive tests for YAML loader before merging. This is critical infrastructure code.

4. Security Considerations

1. Secret exposure in logs (commands.go:129-130, 139-140)

   logger.Errorf("Failed to load configuration: %v", err)
   return fmt.Errorf("configuration loading failed: %w", err)

   • Error messages might contain sensitive data from config
   • Recommendation: Sanitize errors before logging

2. Environment variable timing attack (yaml_loader.go:249, 311, 331)

   • Using os.Getenv directly is fine, but timing side-channels could leak secret existence
   • Recommendation: Document that vmcp should run in trusted environments

5. Proposal Compliance Notes

The implementation correctly follows the proposal's Phase 1 requirements:

• ✅ Configuration schema matches proposal specification
• ✅ Supports inline and discovered auth sources (proposal lines 341-343)
• ✅ Conflict resolution strategies implemented (proposal lines 759-833)
• ✅ Token cache configuration (proposal lines 912-961)
• ✅ Composite tools with elicitation support (proposal lines 109-127, 438-519)
• ✅ Operational settings (proposal lines 417-436)

Future work needed (Phase 2+, not required for this PR):

• Backend discovery from Kubernetes MCPExternalAuthConfig (proposal lines 699-754)
• Token exchange implementation (proposal lines 234-269, 1004-1028)
• Workflow execution engine (proposal lines 1030-1046)

---

Recommendations Priority

High Priority (should address before merge):

1. Add integration tests for yaml_loader.go - this is critical infrastructure
2. Fix circular dependency validation in workflow steps
3. Fix unsafe type assertions that could panic
4. Make environment variable requirements explicit and consistent

Medium Priority (nice to have):

1. Add range validation for durations
2. Improve error sanitization to prevent secret exposure
3. Add structured output option (JSON) for validation results
4. Separate validation from mutation (default setting)

Low Priority (future improvements):

1. Add integration test for full validate command flow
2. Improve test coverage to >70%
3. Consider adding validation for template syntax in composite tool arguments

---

Example Config Issues

The invalid example (examples/vmcp-config-invalid.yaml) correctly demonstrates validation errors, but could be expanded to cover more edge cases:

Suggested additions:

• Circular dependencies in composite tools
• Invalid duration formats
• Missing required fields in nested structures
• Type mismatches in parameters

---

Conclusion

This is a solid foundational PR that correctly implements the vmcp validation command according to the proposal. The architecture is clean and extensible. The main concern is the lack of tests for the YAML loader, which handles critical functionality like environment variable resolution and type transformation.

Recommendation: Request changes to add YAML loader tests before merging. Once tests are added and the high-priority issues are addressed, this will be ready to merge.

---

[JAORMX]

this LGTM, it just has some linting errors.

[Copilot]

Pull Request Overview

This PR implements YAML configuration loading and validation for the Virtual MCP Server, enabling users to define and validate VMCP configurations through a CLI-based workflow.

• Adds YAMLLoader for parsing YAML configuration files with environment variable resolution
• Implements comprehensive Validator for validating configuration correctness
• Integrates validation into the CLI validate command

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
pkg/vmcp/config/yaml_loader.go	Implements YAML parsing and transformation to internal config model with env var resolution
pkg/vmcp/config/yaml_loader_test.go	Comprehensive test suite for YAML loading including error cases and integration tests
pkg/vmcp/config/validator.go	Implements multi-level configuration validation with detailed error messages
pkg/vmcp/config/validator_test.go	Extensive test coverage for all validation scenarios
pkg/vmcp/config/config.go	Adds token cache provider constants
cmd/vmcp/app/commands.go	Integrates YAML loader and validator into the validate command
examples/vmcp-config-invalid.yaml	Example of an invalid configuration for testing

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.