Skip to content

Add regression guard for auth context propagation through Close()#4781

Merged
yrobla merged 2 commits intomainfrom
issue-4725
Apr 14, 2026
Merged

Add regression guard for auth context propagation through Close()#4781
yrobla merged 2 commits intomainfrom
issue-4725

Conversation

@yrobla
Copy link
Copy Markdown
Contributor

@yrobla yrobla commented Apr 13, 2026

Summary

Adds an integration test for the ListCapabilities → Close() path that would fail if the context.Background() regression from #4613 were reintroduced. Uses a proper streamable-HTTP server (NewStreamableHTTPServer) so mcp-go issues a session ID and actually sends a DELETE on Close().

The assertion is a server-side atomic flag: if auth fails for the DELETE, authRoundTripper drops the request before it reaches the server, the flag stays false, and the test fails.

Fixes #4725

Type of change

  • Bug fix
  • New feature
  • Refactoring (no behavior change)
  • Dependency update
  • Documentation
  • Other (describe):

Test plan

  • Unit tests (task test)
  • E2E tests (task test-e2e)
  • Linting (task lint-fix)
  • Manual testing (describe below)

Changes

File Change

Does this introduce a user-facing change?

Implementation plan

Approved implementation plan

Special notes for reviewers

@yrobla yrobla requested a review from Copilot April 13, 2026 15:07
@github-actions github-actions bot added the size/XS Extra small PR: < 100 lines changed label Apr 13, 2026
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

Adds an integration regression test to ensure auth/health-check context is still propagated through the full ListCapabilities → deferred Close() path for streamable HTTP backends, preventing a recurrence of the context.Background() teardown regression fixed in #4613.

Changes:

  • Add a new integration test that spins up a real mcp-go Streamable HTTP server and calls ListCapabilities.
  • Assert server-side receipt of the DELETE emitted by Close() to detect auth-context loss that would cause the request to be dropped pre-flight.

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

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 13, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.01%. Comparing base (32c23f4) to head (4fb5a18).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4781      +/-   ##
==========================================
+ Coverage   68.97%   69.01%   +0.03%     
==========================================
  Files         517      517              
  Lines       54798    54829      +31     
==========================================
+ Hits        37799    37842      +43     
+ Misses      14087    14066      -21     
- Partials     2912     2921       +9     

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

Copy link
Copy Markdown
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

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

LGTM — well-documented regression guard with a clear atomic flag assertion.

Extended reasoning...

Overview

This PR adds a single new integration test file () that guards against a regression in auth context propagation through . No production code is modified.

Security risks

None. This is a test-only change. The test itself exercises auth context propagation, which is a security-relevant path, but only adds coverage rather than altering behavior.

Level of scrutiny

Low. Single new test file, no production code changes, well-commented with references to the original bug (#4613) and the issue being fixed (#4725). The test logic is straightforward: spin up a real streamable-HTTP server, make a ListCapabilities call with a health-check context, and assert the DELETE from Close() reaches the server (proving auth context was propagated correctly).

Other factors

No bugs found by the automated system. The test uses standard patterns (atomic flag, httptest.NewServer, context with timeout). The approach of using a server-side flag to detect whether the DELETE reached the server is a solid regression guard strategy.

@github-actions github-actions bot added size/S Small PR: 100-299 lines changed and removed size/XS Extra small PR: < 100 lines changed labels Apr 13, 2026
Adds an integration test for the ListCapabilities → Close() path that
would fail if the context.Background() regression from #4613 were
reintroduced. Uses a proper streamable-HTTP server (NewStreamableHTTPServer)
so mcp-go issues a session ID and actually sends a DELETE on Close().

The assertion is a server-side atomic flag: if auth fails for the DELETE,
authRoundTripper drops the request before it reaches the server, the flag
stays false, and the test fails.

Closes #4725
@github-actions github-actions bot added size/S Small PR: 100-299 lines changed and removed size/S Small PR: 100-299 lines changed labels Apr 14, 2026
@github-actions github-actions bot added size/S Small PR: 100-299 lines changed and removed size/S Small PR: 100-299 lines changed labels Apr 14, 2026
@yrobla yrobla merged commit 0ef6368 into main Apr 14, 2026
40 checks passed
@yrobla yrobla deleted the issue-4725 branch April 14, 2026 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S Small PR: 100-299 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add regression acceptance test for auth context propagation through full ListCapabilities → Close() path

4 participants