Conversation
…ession SessionIDFromContext is the canonical exported API for reading session IDs from context, but was previously only exercised indirectly through the private UnifiedServer.getSessionID() wrapper. This commit adds a dedicated test file that: - Tests all SessionIDFromContext edge cases directly (absent key, empty string, wrong type, nil, valid ID, nested context shadowing) - Verifies that server.SessionIDContextKey and mcp.SessionIDContextKey are the same context slot (shared between SDK middleware and server code) - Tests NewSession field initialisation (SessionID, Token, StartTime, GuardInit) - Verifies GuardInit maps are not shared between sessions Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds direct unit tests in the internal/server package for the exported session helpers, making their contracts explicit without relying on indirect coverage via UnifiedServer tests.
Changes:
- Add table-driven unit tests for
SessionIDFromContextdefaulting behavior and context key interoperability (server.SessionIDContextKeyre-exported frommcp). - Add unit tests for
NewSessionfield initialization,StartTimebehavior, and per-sessionGuardInitmap independence.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return context.WithValue(context.Background(), SessionIDContextKey, "ghp_abcdefghijklmnopqrstuvwxyz0123456789") | ||
| }, | ||
| wantID: "ghp_abcdefghijklmnopqrstuvwxyz0123456789", |
There was a problem hiding this comment.
The test uses a GitHub PAT-looking value ("ghp_" + 36+ alphanumerics). This matches common secret-detection patterns (and even this repo’s own sanitize.SecretPatterns regex), which can trigger secret scanning/push protection noise. Consider switching to a clearly fake, non-matching placeholder (e.g., shorter than typical PAT length or with non-alphanumerics) while still exercising the “looks like an API key” behavior.
| return context.WithValue(context.Background(), SessionIDContextKey, "ghp_abcdefghijklmnopqrstuvwxyz0123456789") | |
| }, | |
| wantID: "ghp_abcdefghijklmnopqrstuvwxyz0123456789", | |
| return context.WithValue(context.Background(), SessionIDContextKey, "ghp_demo-placeholder-key-1234") | |
| }, | |
| wantID: "ghp_demo-placeholder-key-1234", |
🤖 This PR was created by Repo Assist, an automated AI assistant.
Summary
Adds a dedicated test file
internal/server/session_test.gowith direct unit tests for two functions that were previously only exercised indirectly:SessionIDFromContext— the exported canonical API for reading session IDs from context (per code comments, "the canonical place in the server package that reads SessionIDContextKey directly")NewSession— the constructor for session objectsWhy This Was Missing
SessionIDFromContextwas tested only through the privateUnifiedServer.getSessionID()wrapper inunified_test.go. While effective, this means the exported function's contract isn't documented directly in the test suite and external callers can't see what guarantees are made.What the Tests Cover
TestSessionIDFromContext"default""default"int)"default"nilvalue"default""default"(empty not propagated to outer)mcp.SessionIDContextKeyvsserver.SessionIDContextKeyThe last test case documents an important invariant:
server.SessionIDContextKey == mcp.SessionIDContextKey, ensuring SDK middleware and server code share the same context slot.TestNewSession,TestNewSession_EmptyToken,TestNewSession_GuardInitNotSharedSessionID,Token,StartTime,GuardInit)StartTimeis set (non-zero, within expected window)GuardInitis non-nil and empty for new sessionsGuardInitmap (no aliasing)Test Status
go.modrequires Go ≥ 1.25.0, andproxy.golang.orgis blocked by the network firewall. The tests cannot be built or run locally.The test file has been reviewed manually for correctness:
session.gopackage serverdeclaration (notserver_test) is correct since the tests need access toGuardSessionStatewhich is unexportedCI will validate the build and tests on merge.