Skip to content

refactor: inject deps via context instead of closures - #1640

Merged
SamMorrowDrums merged 1 commit into
mainfrom
context-deps-injection
Dec 18, 2025
Merged

refactor: inject deps via context instead of closures#1640
SamMorrowDrums merged 1 commit into
mainfrom
context-deps-injection

Conversation

@SamMorrowDrums

Copy link
Copy Markdown
Collaborator

Summary

This refactor changes how dependencies are passed to tool handlers from a closure-based pattern to a context-based pattern. This eliminates the need to create new closures for every tool on each request, which is critical for the remote server where one server instance is created per request.

Problem

In the closure-based pattern, NewTool stored a function that took deps and returned a handler:

// Old pattern - creates closure on every call to Handler(deps)funcNewTool(..., handlerfunc(deps) handlerFn) {
returnServerTool{
Handler: func(depsany) handlerFn {
returnhandler(deps.(ToolDependencies)) // Creates new closure each time
}
}
}

For the remote server with ~89 tools, this meant 178 allocations per request just for tool handler setup.

Solution

The new pattern injects dependencies into the context once, and handlers extract them:

// New pattern - no closure creation at request timefuncNewTool(..., handlerfunc(ctx, deps, req, args) result) {
returnServerTool{
Handler: func(ctxany) handlerFn {
returnfunc(ctx, req) result {
deps:=MustDepsFromContext(ctx) // Extract from contextreturnhandler(ctx, deps, req, args)
}
}
}
}

Benchmark Results

Throughput (89 tools, 5s benchmark, 3 runs)

Benchmarkmain (old)newSpeedup
Sequential9,345 ns/op4,250 ns/op2.2x faster
Parallel2,751 ns/op779 ns/op3.5x faster

Memory

MetricmainnewReduction
Bytes/op7,832 B2,136 B73% less
Allocs/op1788950% fewer

Scaling (tools from 13 to 1,300)

ToolsmainnewSpeedupMemory Saved
131,261 ns811 ns1.6x54%
656,420 ns3,771 ns1.7x54%
13014,653 ns7,797 ns1.9x54%
65068,722 ns38,872 ns1.8x54%
1,300145,135 ns67,292 ns2.2x54%

Changes

New APIs in pkg/github/dependencies.go

  • ContextWithDeps(ctx, deps) - inject dependencies into context
  • DepsFromContext(ctx) - extract dependencies (returns nil if not present)
  • MustDepsFromContext(ctx) - extract dependencies (panics if not present)

Updated in pkg/inventory/server_tool.go

  • NewServerToolWithContextHandler - new constructor for context-based handlers
  • NewServerToolWithRawContextHandler - new constructor for raw handlers
  • Old constructors marked as deprecated

Converted (89 tool handlers across 15 files)

All tool handlers converted from closure pattern to direct context-based pattern.

Test updates (17 test files)

All tests updated to use ContextWithDeps(context.Background(), deps).

Backwards Compatibility

  • Old NewServerTool and NewServerToolFromHandler are deprecated but still work
  • dynamic_tools.go intentionally uses the old pattern (with nolint comment) since it wraps user-provided handlers

Testing

  • All existing tests pass
  • script/lint passes
  • script/test passes

This refactor addresses performance issues in per-request server scenarios
where creating ~90 handler closures per request was causing latency.
Changes:
- Add ContextWithDeps, DepsFromContext, MustDepsFromContext to dependencies.go
- Add NewServerToolWithContextHandler, NewServerToolWithRawContextHandler to inventory
- Convert all 89 tool handlers from closure pattern to direct context-based deps
- Update all tests to inject deps into context before calling handlers
- Mark old NewServerTool and NewServerToolFromHandler as deprecated
The new pattern:
- Before: func(deps) handler { return func(ctx, req, args) { use deps } }
- After: func(ctx, deps, req, args) { use deps }
Dependencies are now injected into context once (via ContextWithDeps) and
extracted by NewTool internally before passing to handlers. This eliminates
closure creation on the hot path for remote servers.
CopilotAI review requested due to automatic review settings December 18, 2025 10:50
@SamMorrowDrums
SamMorrowDrums requested a review from a team as a code ownerDecember 18, 2025 10:50

CopilotAI 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 refactors dependency injection from a closure-based pattern to a context-based pattern, improving performance for the remote server by eliminating the need to create closures for each tool on every request. The change reduces allocations from 178 to 89 per request and achieves 2-3.5x throughput improvements.

Key changes:

  • Introduces new context-based constructors (NewServerToolWithContextHandler, NewServerToolWithRawContextHandler) that avoid closure creation at registration time
  • Changes handler signature from func(deps) handler to func(ctx, deps, req, args) result
  • Adds context helper functions ContextWithDeps, DepsFromContext, and MustDepsFromContext in pkg/github/dependencies.go
  • Converts all 89 tool handlers across 15 files to use the new pattern
  • Updates all test files to inject dependencies via context

Reviewed changes

Copilot reviewed 35 out of 35 changed files in this pull request and generated no comments.

Show a summary per file
FileDescription
pkg/inventory/server_tool.goAdds new context-based constructors; deprecates old closure-based constructors
pkg/github/security_advisories.goConverts 4 tool handlers from closure to context pattern
pkg/github/security_advisories_test.goUpdates tests to use ContextWithDeps
pkg/github/secret_scanning.goConverts 2 tool handlers to context pattern
pkg/github/secret_scanning_test.goUpdates tests to use ContextWithDeps
pkg/github/search.goConverts 4 search tool handlers to context pattern, including helper function refactoring
pkg/github/search_test.goUpdates 5 test cases to use ContextWithDeps
pkg/github/repositories.goConverts 18 repository tool handlers to context pattern
pkg/github/repositories_test.goUpdates 17 test cases to use ContextWithDeps
pkg/github/pullrequests.goConverts 10 PR tool handlers to context pattern
pkg/github/pullrequests_test.goUpdates 18 test cases to use ContextWithDeps
pkg/github/projects.goConverts 8 project tool handlers to context pattern
pkg/github/projects_test.goUpdates 8 test cases to use ContextWithDeps
pkg/github/notifications_test.goUpdates 6 test cases to use ContextWithDeps
pkg/github/labels.goConverts 3 label tool handlers to context pattern
pkg/github/labels_test.goUpdates 3 test cases to use ContextWithDeps

The refactoring is comprehensive, consistent, and well-executed. All handler conversions follow the same pattern, tests are properly updated, and the deprecated constructors remain for backward compatibility. No issues were identified during the review.

@SamMorrowDrums
SamMorrowDrums merged commit 3c453dd into mainDec 18, 2025
23 checks passed
@SamMorrowDrums
SamMorrowDrums deleted the context-deps-injection branch December 18, 2025 10:57
CopilotAI added a commit that referenced this pull request Dec 18, 2025
The issue was that after PR #1640 switched from closure-based deps to context-based deps,
the stdio server was missing middleware to inject ToolDependencies into the request context.
This caused tools to panic with "ToolDependencies not found in context" when called.
Added middleware in NewMCPServer() that wraps all requests with github.ContextWithDeps(),
ensuring deps are available to tool handlers via MustDepsFromContext().
Also added tests to verify server creation and toolset resolution logic.
Co-authored-by: SamMorrowDrums <4811358+SamMorrowDrums@users.noreply.github.com>
SamMorrowDrums added a commit that referenced this pull request Dec 18, 2025
The issue was that after PR #1640 switched from closure-based deps to context-based deps,
the stdio server was missing middleware to inject ToolDependencies into the request context.
This caused tools to panic with "ToolDependencies not found in context" when called.
Added middleware in NewMCPServer() that wraps all requests with github.ContextWithDeps(),
ensuring deps are available to tool handlers via MustDepsFromContext().
Also added tests to verify server creation and toolset resolution logic.
Co-authored-by: SamMorrowDrums <4811358+SamMorrowDrums@users.noreply.github.com>
Sign up for freeto 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

@SamMorrowDrums@JoannaaKL