Skip to content

Deduplicate shared config validation across TOML and stdin paths#7885

Merged
lpcox merged 4 commits into
mainfrom
copilot/duplicate-code-config-validation
Jun 21, 2026
Merged

Deduplicate shared config validation across TOML and stdin paths#7885
lpcox merged 4 commits into
mainfrom
copilot/duplicate-code-config-validation

Conversation

Copilot AI commented Jun 21, 2026

Copy link
Copy Markdown
Contributor

Server-level config validation had drift-prone duplication between the TOML loader and JSON stdin validation flow. The same auth, tool_response_filters, and gateway payload threshold rules were implemented in parallel, with parity enforced only by convention and comments.

  • Shared validation helpers

    • Extracted validateCommonServerFields(...) to centralize per-server auth and tool_response_filters validation.
    • Extracted validateGatewayPayloadSizeThreshold(...) to centralize positive-integer validation for gateway payload threshold fields across both config formats.
  • TOML path

    • Updated LoadFromFile to delegate shared per-server validation to the new helper.
    • Replaced inline payload_size_threshold validation with the shared gateway helper.
  • JSON stdin path

    • Updated validateStandardServerConfig to use the shared per-server helper instead of revalidating auth and response filters inline.
    • Updated validateGatewayConfig to use the shared gateway payload-threshold helper.
  • Regression coverage

    • Added a parity test asserting TOML loading and stdin validation produce the same error for the same invalid tool_response_filters input.
func validateCommonServerFields(name, serverType string, auth *AuthConfig, filters map[string]string, jsonPath string) error {
	if err := validateServerAuth(auth, serverType, name, jsonPath); err != nil {
		return err
	}
	return validateToolResponseFilters(filters, jsonPath+".tool_response_filters")
}

GitHub Advanced Security started work on behalf of lpcox June 21, 2026 16:09 View session
Copilot AI changed the title [WIP] Refactor duplicated config validation logic for TOML and JSON Deduplicate shared config validation across TOML and stdin paths Jun 21, 2026
Copilot AI requested a review from lpcox June 21, 2026 16:10
Copilot finished work on behalf of lpcox June 21, 2026 16:10
GitHub Advanced Security finished work on behalf of lpcox June 21, 2026 16:11
GitHub Advanced Security started work on behalf of lpcox June 21, 2026 16:18 View session
GitHub Advanced Security finished work on behalf of lpcox June 21, 2026 16:20
@lpcox lpcox marked this pull request as ready for review June 21, 2026 17:09
Copilot AI review requested due to automatic review settings June 21, 2026 17:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 reduces drift risk in configuration validation by centralizing shared validation logic used by both the TOML loader (LoadFromFile) and the JSON-stdin validation flow, and adds a regression test intended to ensure parity for invalid tool_response_filters.

Changes:

  • Introduces validateCommonServerFields(...) to share per-server auth and tool_response_filters validation across TOML and stdin paths.
  • Introduces validateGatewayPayloadSizeThreshold(...) and updates both config formats to use it for gateway payload threshold validation.
  • Adds a parity regression test for invalid tool_response_filters jq expressions.
Show a summary per file
File Description
internal/config/validation.go Adds shared validation helpers and switches stdin gateway validation to use the shared payload threshold validator.
internal/config/config_core.go Switches TOML load-time per-server validation and payload_size_threshold validation to the shared helpers.
internal/config/config_core_test.go Adds a regression test intended to assert parity between TOML and stdin validation errors for invalid jq filters.

Copilot's findings

Tip

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

  • Files reviewed: 3/3 changed files
  • Comments generated: 1

Comment thread internal/config/config_core_test.go Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
GitHub Advanced Security started work on behalf of lpcox June 21, 2026 17:14 View session
GitHub Advanced Security finished work on behalf of lpcox June 21, 2026 17:16
@lpcox

lpcox commented Jun 21, 2026

Copy link
Copy Markdown
Collaborator

@copilot fix failing ci checks

1 similar comment
@lpcox

lpcox commented Jun 21, 2026

Copy link
Copy Markdown
Collaborator

@copilot fix failing ci checks

Copilot AI commented Jun 21, 2026

Copy link
Copy Markdown
Contributor Author

@copilot fix failing ci checks

Fixed in the latest commit. There were three separate issues causing CI failures:

  1. Syntax errorTestLoadFromFile_ToolResponseFilter_InvalidJqExpressionMatchesStdinValidation was missing its closing }, which the Go parser reported as expected '(', found TestLoadFromFile_ToolResponseFilter_WhitespaceOnlyValue.

  2. Duplicate TLS filestls_config.go and tls_config_test.go were accidentally introduced alongside the existing tls.go/tls_test.go, redeclaring MinTLSVersion, NewServerTLSConfig, and NewClientTLSConfig. Removed the duplicates and restored tls_test.go to match main.

  3. Nil interface bug in validateGatewayPayloadSizeThresholdPositiveInteger returns *ValidationError. Returning it directly as error produced a non-nil interface even when the pointer was nil, causing valid thresholds (≥ 1) to be incorrectly flagged as errors. Fixed by explicitly returning nil when the concrete pointer is nil.

GitHub Advanced Security started work on behalf of lpcox June 21, 2026 17:24 View session
Copilot finished work on behalf of lpcox June 21, 2026 17:24
GitHub Advanced Security finished work on behalf of lpcox June 21, 2026 17:25

Copilot AI commented Jun 21, 2026

Copy link
Copy Markdown
Contributor Author

@copilot fix failing ci checks

The CI failures have been resolved in commit ff1038d. All tests now pass:

Fixed issues:

  1. Syntax error - Missing closing brace in test function TestLoadFromFile_ToolResponseFilter_InvalidJqExpressionMatchesStdinValidation
  2. Duplicate TLS files - Removed tls_config.go and tls_config_test.go duplicates
  3. Nil interface bug - Fixed validateGatewayPayloadSizeThreshold to explicitly return nil when validation succeeds

Verification:

  • make lint - passes
  • make test-unit - all tests pass
  • make test-race - no race conditions
  • make agent-finished - full verification succeeds

The workflow status shows "action_required" which indicates pending approvals or required checks, not test failures.

Copilot finished work on behalf of lpcox June 21, 2026 17:31
@lpcox lpcox merged commit b1296c6 into main Jun 21, 2026
28 checks passed
@lpcox lpcox deleted the copilot/duplicate-code-config-validation branch June 21, 2026 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants