refactor(config): centralize stdio server type helpers#8098
Conversation
|
@copilot resolve the merge conflicts in this pull request |
There was a problem hiding this comment.
Pull request overview
This PR centralizes the “stdio-family” server type predicate and normalization logic in internal/config, removing duplicated checks and fixing a validation gap where an empty type wasn’t treated as stdio in one validation path.
Changes:
- Added
IsStdioServerTypeandNormalizeServerTypehelpers to canonicalize stdio-type handling. - Replaced repeated
"" || "stdio" || "local"checks withIsStdioServerTypein validation and schema validation. - Added table-driven unit tests for the new helpers.
Show a summary per file
| File | Description |
|---|---|
| internal/config/server_type.go | Introduces centralized stdio-type predicate + normalization helpers. |
| internal/config/server_type_test.go | Adds unit coverage for helper behavior across expected/edge inputs. |
| internal/config/validation.go | Uses helpers to normalize types and consistently enforce stdio container requirements. |
| internal/config/validation_schema.go | Uses helper for stdio container-pattern validation. |
| internal/config/config_stdin.go | Uses normalization helper when converting stdin server configs. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
internal/config/validation.go:60
- This change fixes a validation gap: an empty server type now correctly behaves like stdio and requires
container. There doesn't appear to be a regression test covering the failing case (Type == "" with missing container) in internal/config/validation_test.go; adding one would prevent reintroducing this silent acceptance.
if IsStdioServerType(server.Type) {
if server.Container == "" {
logValidation.Printf("Validation failed: %s, name=%s, type=%s", "stdio server missing container field", name, server.Type)
return MissingRequired("container", "stdio", jsonPath, "Add a 'container' field (e.g., \"ghcr.io/owner/image:tag\")")
}
- Files reviewed: 5/5 changed files
- Comments generated: 1
| @@ -490,13 +490,7 @@ func convertStdinServerConfig(name string, server *StdinServerConfig, customSche | |||
| } | |||
|
|
|||
| // Normalize type: "local" is an alias for "stdio" (backward compatibility) | |||
Resolve conflict in config_stdin.go: drop redundant NormalizeServerType call since server.Type is already normalized by validateServerConfigWithCustomSchemas. Also picks up UnsupportedField -> InvalidValue consolidation from main.
Resolved. The only conflict was in |
The "is stdio" predicate (
t == "" || t == "stdio" || t == "local") and the normalization pattern were duplicated across 7 sites in 3 files, with one site (validateStandardServerConfig) missing the""case — a silent validation gap.Changes
internal/config/server_type.go— new file with two helpers:IsStdioServerType(t string) bool— canonical check covering"","stdio","local"NormalizeServerType(t string) string— maps""/"local"→"stdio", passes rest throughvalidation.go— 3 sites updated:ifchecks to oneNormalizeServerTypecallvalidateStandardServerConfig(line 62):"stdio" || "local"→IsStdioServerType(also fixes the missing""case)validateTOMLStdioContainerization(line 489): triple OR →IsStdioServerTypeconfig_stdin.go— 4-line normalization block replaced with singleNormalizeServerTypecallvalidation_schema.go— triple OR invalidateStringPatternsreplaced withIsStdioServerTypeinternal/config/server_type_test.go— table-driven tests for both helpers covering all type values including wrong-case strings