[log] Add debug logging to config validation rules#958
Conversation
- Add logger import and declaration to internal/config/rules/rules.go - Add logging to 5 key validation functions for better debugging: * UnsupportedType: Log unsupported type detection * PortRange: Log port validation with pass/fail outcomes * TimeoutPositive: Log timeout validation with values * MountFormat: Log mount format validation with context * AbsolutePath: Log path validation for Unix/Windows paths Helps troubleshoot configuration validation issues by providing visibility into which validation rules are triggered and why they fail or succeed.
|
PR Titles:
|
There was a problem hiding this comment.
Pull request overview
This PR adds debug logging to the config validation rules package to improve troubleshooting of configuration validation issues. It introduces a logger instance and adds 10 debug logging statements across 5 validation functions, providing visibility into validation entry points, decision paths, and failure conditions.
Changes:
- Added logger declaration using
config:rulescategory following thepkg:filenameconvention - Enhanced 5 validation functions (UnsupportedType, PortRange, TimeoutPositive, MountFormat, AbsolutePath) with debug logging at entry points, error paths, and success paths
- All logging uses simple value arguments with no side effects, consistent with project guidelines
Comments suppressed due to low confidence (1)
internal/config/rules/rules.go:199
- The MountFormat function performs multiple validation checks (empty source, absolute source path, empty dest, absolute dest path, valid mode), but only the first check failure (invalid part count) is logged. For consistency with other validation functions and better debugging, consider adding debug logging for the other validation failure paths as well.
func MountFormat(mount, jsonPath string, index int) *ValidationError {
log.Printf("Validating mount format: mount=%s, jsonPath=%s, index=%d", mount, jsonPath, index)
parts := strings.Split(mount, ":")
if len(parts) < 2 || len(parts) > 3 {
log.Printf("Mount format validation failed: invalid part count=%d", len(parts))
return &ValidationError{
Field: "mounts",
Message: fmt.Sprintf("invalid mount format '%s' (expected 'source:dest' or 'source:dest:mode')", mount),
JSONPath: fmt.Sprintf("%s.mounts[%d]", jsonPath, index),
Suggestion: "Use format 'source:dest' or 'source:dest:mode' where mode is 'ro' (read-only) or 'rw' (read-write)",
}
}
source := parts[0]
dest := parts[1]
mode := ""
if len(parts) == 3 {
mode = parts[2]
}
// Validate source is not empty
if source == "" {
return &ValidationError{
Field: "mounts",
Message: fmt.Sprintf("mount source cannot be empty in '%s'", mount),
JSONPath: fmt.Sprintf("%s.mounts[%d]", jsonPath, index),
Suggestion: "Provide a valid absolute source path (e.g., '/host/path')",
}
}
// Validate source is an absolute path (MCP spec requirement)
if !strings.HasPrefix(source, "/") {
return &ValidationError{
Field: "mounts",
Message: fmt.Sprintf("mount source must be an absolute path, got '%s'", source),
JSONPath: fmt.Sprintf("%s.mounts[%d]", jsonPath, index),
Suggestion: "Use an absolute path starting with '/' (e.g., '/var/data' instead of 'data')",
}
}
// Validate dest is not empty
if dest == "" {
return &ValidationError{
Field: "mounts",
Message: fmt.Sprintf("mount destination cannot be empty in '%s'", mount),
JSONPath: fmt.Sprintf("%s.mounts[%d]", jsonPath, index),
Suggestion: "Provide a valid absolute destination path (e.g., '/app/data')",
}
}
// Validate dest is an absolute path (MCP spec requirement)
if !strings.HasPrefix(dest, "/") {
return &ValidationError{
Field: "mounts",
Message: fmt.Sprintf("mount destination must be an absolute path, got '%s'", dest),
JSONPath: fmt.Sprintf("%s.mounts[%d]", jsonPath, index),
Suggestion: "Use an absolute path starting with '/' (e.g., '/app/data' instead of 'app/data')",
}
}
// Validate mode if provided
if mode != "" && mode != "ro" && mode != "rw" {
return &ValidationError{
Field: "mounts",
Message: fmt.Sprintf("invalid mount mode '%s' (must be 'ro' or 'rw')", mode),
JSONPath: fmt.Sprintf("%s.mounts[%d]", jsonPath, index),
Suggestion: "Use 'ro' for read-only or 'rw' for read-write",
}
}
return nil
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "github.com/github/gh-aw-mcpg/internal/logger" | ||
| ) | ||
|
|
||
| var log = logger.New("config:rules") |
There was a problem hiding this comment.
According to AGENTS.md guidelines, descriptive logger variable names are strongly preferred over generic 'log' (20 out of 23 logger declarations use descriptive names). Consider renaming to 'var logRules = logger.New("config:rules")' for consistency with other config validation files like 'logValidation', 'logEnv', and 'logSchema'.
| @@ -226,9 +240,11 @@ func AbsolutePath(value, fieldName, jsonPath string) *ValidationError { | |||
| if len(value) >= 3 && | |||
| ((value[0] >= 'A' && value[0] <= 'Z') || (value[0] >= 'a' && value[0] <= 'z')) && | |||
| value[1] == ':' && value[2] == '\\' { | |||
| log.Printf("Valid Windows absolute path: %s", value) | |||
There was a problem hiding this comment.
Logging success paths in validation functions can create noisy debug output, especially for frequently called functions. Consider removing the success case logging (lines 234 and 243) and only keeping entry and failure logging, which is the pattern used in other validation functions like PortRange and TimeoutPositive.
Summary
This PR enhances
internal/config/rules/rules.gowith debug logging to help troubleshoot configuration validation issues.Changes Made
Logger Declaration
loggerimport and declaration:var log = logger.New("config:rules")Logging Enhancements (5 functions)
UnsupportedType: Logs when unsupported server types are detected with contextPortRange: Logs port validation attempts and failures with port valuesTimeoutPositive: Logs timeout validation with field names and valuesMountFormat: Logs mount format validation with detailed parsing contextAbsolutePath: Logs path validation for both Unix and Windows paths, including success/failure outcomesTotal Impact
Benefits
Testing Notes
The changes follow the project's logging guidelines from AGENTS.md:
pkg:filenameconvention (config:rules)Example Debug Output
When
DEBUG=config:rulesis enabled: