Skip to content

Add stdin-path test coverage for stdio server auth validation#3769

Merged
lpcox merged 2 commits intomainfrom
copilot/fix-code-based-review-comment
Apr 14, 2026
Merged

Add stdin-path test coverage for stdio server auth validation#3769
lpcox merged 2 commits intomainfrom
copilot/fix-code-based-review-comment

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 14, 2026

Review comment noted missing test coverage for the validateServerAuth path when a stdio server includes an auth block via the stdin JSON config path, leaving the structured error type and message unguarded against regressions.

Changes

  • TestConvertStdinServerConfig_ValidationError — added "stdio with auth block" table case to cover the error message via convertStdinServerConfig (the stdin JSON path)
  • TestConvertStdinServerConfig_StdioWithAuth — focused test asserting the returned error is a *rules.ValidationError with the correct Field, Message, JSONPath, and non-empty Suggestion:
var valErr *rules.ValidationError
require.True(t, errors.As(err, &valErr))
assert.Equal(t, "auth", valErr.Field)
assert.Contains(t, valErr.Message, "auth is only supported for HTTP servers")
assert.Contains(t, valErr.JSONPath, "mcpServers.my-server")
assert.NotEmpty(t, valErr.Suggestion)

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • example.com
    • Triggering command: /tmp/go-build2349467698/b514/launcher.test /tmp/go-build2349467698/b514/launcher.test -test.testlogfile=/tmp/go-build2349467698/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s go_.�� @v1.43.0/semconv-errorsas @v1.43.0/semconv-ifaceassert x_amd64/vet . --gdwarf2 --64 LEXxvrp/NoLlItYx-buildtags -I g_.a -I x_amd64/vet --gdwarf-5 --64 -o x_amd64/vet (dns block)
  • invalid-host-that-does-not-exist-12345.com
    • Triggering command: /tmp/go-build2349467698/b496/config.test /tmp/go-build2349467698/b496/config.test -test.testlogfile=/tmp/go-build2349467698/b496/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build2349467698/b392/vet.cfg g_.a k/gh-aw-mcpg/gh-aw-mcpg/internal-nolocalimports x_amd64/vet -dynout xcontext -dynlinker x_amd64/vet (dns block)
  • nonexistent.local
    • Triggering command: /tmp/go-build2349467698/b514/launcher.test /tmp/go-build2349467698/b514/launcher.test -test.testlogfile=/tmp/go-build2349467698/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s go_.�� @v1.43.0/semconv-errorsas @v1.43.0/semconv-ifaceassert x_amd64/vet . --gdwarf2 --64 LEXxvrp/NoLlItYx-buildtags -I g_.a -I x_amd64/vet --gdwarf-5 --64 -o x_amd64/vet (dns block)
  • slow.example.com
    • Triggering command: /tmp/go-build2349467698/b514/launcher.test /tmp/go-build2349467698/b514/launcher.test -test.testlogfile=/tmp/go-build2349467698/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s go_.�� @v1.43.0/semconv-errorsas @v1.43.0/semconv-ifaceassert x_amd64/vet . --gdwarf2 --64 LEXxvrp/NoLlItYx-buildtags -I g_.a -I x_amd64/vet --gdwarf-5 --64 -o x_amd64/vet (dns block)
  • this-host-does-not-exist-12345.com
    • Triggering command: /tmp/go-build2349467698/b523/mcp.test /tmp/go-build2349467698/b523/mcp.test -test.testlogfile=/tmp/go-build2349467698/b523/testlog.txt -test.paniconexit0 -test.timeout=10m0s -o g_.a -trimpath x_amd64/vet -p ernal/server -lang=go1.25 x_amd64/vet ut-1�� .cfg -buildtags x_amd64/vet ctor -ifaceassert E=3 x_amd64/vet (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI changed the title [WIP] Fix code based on review comment Add stdin-path test coverage for stdio server auth validation Apr 14, 2026
Copilot AI requested a review from lpcox April 14, 2026 15:52
@lpcox lpcox marked this pull request as ready for review April 14, 2026 15:58
Copilot AI review requested due to automatic review settings April 14, 2026 15:58
@lpcox lpcox merged commit cc37331 into main Apr 14, 2026
3 checks passed
@lpcox lpcox deleted the copilot/fix-code-based-review-comment branch April 14, 2026 15:58
Copy link
Copy Markdown
Contributor

Copilot AI left a 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 adds test coverage for the stdin JSON config conversion/validation path to ensure that stdio servers configured with an auth block fail with a stable, structured rules.ValidationError (including message, JSONPath, and suggestion), preventing regressions in error typing/formatting.

Changes:

  • Extends TestConvertStdinServerConfig_ValidationError with a "stdio with auth block" table case to cover the stdin conversion validation message.
  • Adds TestConvertStdinServerConfig_StdioWithAuth to assert the returned error is a *rules.ValidationError with expected Field, Message, JSONPath, and non-empty Suggestion.
Show a summary per file
File Description
internal/config/config_stdin_test.go Adds stdin-path tests asserting stdio+auth is rejected and that the structured validation error remains stable.

Copilot's findings

Tip

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

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

require.True(t, errors.As(err, &valErr), "expected a *rules.ValidationError, got %T: %v", err, err)
assert.Equal(t, "auth", valErr.Field)
assert.Contains(t, valErr.Message, "auth is only supported for HTTP servers")
assert.Contains(t, valErr.JSONPath, "mcpServers.my-server")
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TestConvertStdinServerConfig_StdioWithAuth says it verifies the “correct JSONPath”, but the assertion uses assert.Contains on valErr.JSONPath. Since the expected path is deterministic (mcpServers.my-server), asserting equality would make this test stricter and better at catching regressions (e.g., a different prefix/suffix that still contains the substring).

Suggested change
assert.Contains(t, valErr.JSONPath, "mcpServers.my-server")
assert.Equal(t, "mcpServers.my-server", valErr.JSONPath)

Copilot uses AI. Check for mistakes.
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