Conversation
list_ toolslist_ tools in default toolsets
There was a problem hiding this comment.
Pull request overview
This PR adds a response optimization package (pkg/response) that reduces token usage in list_ tool responses by 5-94% (depending on the tool). The optimization applies six strategies: nested object flattening with dot-notation keys, URL field elimination (except preserved ones), zero-value removal, whitespace normalization, collection summarization to [N items] or extracted fields, and fill-rate filtering to remove rarely-populated fields.
The package provides two configuration mechanisms:
preservedFields: exempts specific keys (html_url, draft, prerelease) from destructive optimizationscollectionFieldExtractors: controls extraction of important subfields from collections (e.g., labels→name, requested_reviewers→login)
Changes:
- Added
pkg/responsepackage with optimization pipeline and comprehensive test coverage - Wired
response.MarshalItems()into 6 list tools: list_commits (depth 3 for commit.author.name), list_tags, list_releases, list_branches, list_pull_requests, list_issues - Updated existing tests to work with flattened output structure (map[string]any instead of typed structs)
- Intentionally excluded list_issue_types (already flat, no benefit)
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/response/optimize.go | Core optimization pipeline with flattening, URL removal, zero-value elimination, whitespace normalization, collection handling, and fill-rate filtering |
| pkg/response/optimize_test.go | Comprehensive test coverage for all optimization strategies and configuration interactions |
| pkg/github/repositories.go | Integrated response.MarshalItems into list_commits (depth 3), list_branches, list_tags, list_releases |
| pkg/github/repositories_test.go | Updated test assertions to work with flattened map structure instead of MinimalCommit structs |
| pkg/github/pullrequests.go | Integrated response.MarshalItems into list_pull_requests |
| pkg/github/issues.go | Integrated response.MarshalItems into list_issues, renamed response variable to issueResponse to avoid package name shadowing |
| pkg/github/issues_test.go | Updated test assertions to work with map[string]any structure instead of typed structs, removed unused verifyOrder field |
|
I have left a few comments, let me know what you think. |
list_ tools in default toolsetslist_ tools
|
Thematically they fit to our current approach of creating minimal versions. The approach to handle raw json strings and manually cutting down the objects with default values, or having to manually append exceptions imo justifies manual handcrafting of the reduced minimal version. Afterwards there is still room for sensible optimizations for list based responses. Like:
Conversion from original to optimized structRequires: While we do already have a way to reduce bloateds structs. github-mcp-server/pkg/github/pullrequests.go Line 1172 in de12f8f github-mcp-server/pkg/github/pullrequests.go Line 190 in de12f8f Problems I see:
|
|
@tonytrg I've wired minimal types into the 4 tools that were missing ( The optimisation pipeline now does: flatten -> URL removal -> zero-value removal -> whitespace normalisation -> fill rate filtering. Reasoning behind why I kept each (plus the
On get/list tools field parity, I have somewhat mixed feelings here. These tool types serve different enough purposes that they benefit from different field sets imo. List tools calls done first to find items, then get tools calls to drill in.. having fewer fields in list and richer fields in get feels like the right separation to me. The minimal types are now shared between |
| } | ||
|
|
||
| // flattenInto is the recursive worker for flattenTo. | ||
| func flattenInto(item map[string]any, prefix string, result map[string]any, depth int, maxDepth int) { |
There was a problem hiding this comment.
Code style nit: I'd much rather prefer this function to return a parameter than to return void and fill it in the body. Is there any particular reason you chose to do it this way?
If depth >= maxDepth we just return an empty map, maybe we should keep it as-is?
There was a problem hiding this comment.
This was to avoid creating + merging a new map at each recursive layer.
There was a problem hiding this comment.
I think for the 2-3 levels of nesting and each item having ~5 nested objects that'd be 3-5 additional map allocations per nested item. In the grand scheme of things (marshalling, unmarshalling pipeline) it's negligible. I know it's idiomatic go code but I much more prefer clean functions simply returning parameters (although it's a matter of taste so I am not pushing on this).
What needs to be addressed is that we always drop the last level in the nested structures due to depth < maxDepth check. You can reproduce it by copy-pasting this test and running it:
t.Run("silently drops nested maps at maxDepth boundary", func(t *testing.T) {
// With defaultMaxDepth=2, depth starts at 1.
// Result: "user.org" and its contents are silently lost.
input := map[string]any{
"title": "issue",
"user": map[string]any{
"login": "alice",
"org": map[string]any{
"name": "acme",
},
},
}
result := flattenTo(input, defaultMaxDepth)
assert.Equal(t, "issue", result["title"])
assert.Equal(t, "alice", result["user.login"])
// BUG: org data is silently dropped — neither "user.org" nor
// "user.org.name" appear in the result.
assert.Nil(t, result["user.org"], "nested map at maxDepth is silently dropped")
assert.Nil(t, result["user.org.name"], "nested map contents at maxDepth are lost")
})
There was a problem hiding this comment.
Regarding the code style nit, more than happy to address it in a follow up PR.
As for the second point, that is actually intentional. The main reason for this is that deeply nested data is not useful in a list response imo.
I did just notice that the PR description is generic though and focused on flattening only, my bad! It completely slipped off my mind, as this behaviour was part of the nested object flattening strategy as a whole.
Will edit the PR description and add a test to cover this behaviour.
| if !preserved && isURLKey(key) { | ||
| continue | ||
| } | ||
| if !preserved && isZeroValue(value) { |
There was a problem hiding this comment.
Won't isZeroValue cause fields like draft in pull request to be removed? I don't think we want to strip important fields 🤔
There was a problem hiding this comment.
draft specifically is not removed due to being part of WithPreservedFields. However, for fields like merged: false for example on open PRs, zero value removal / isZeroValue is intentional. N * merged: false is noise in a list response where the get_ tool variant can be used as a fallback for details.
There was a problem hiding this comment.
What about protected? That's an example, I didn't go though the whole list of boolean fields that are important but will be stripped. Looks like a lot of additional configuration needs to be added to make sure we aren't truncating important data?
There was a problem hiding this comment.
protected is actually a very good catch, as there's no get_ branch tool as a fallback. I've added it to WithPreservedFields in list_branches.
Regarding the broader concern, there are two alternatives I considered (i.e., what I also briefly mentioned in #2016 (comment)):
- either add
omitemptyon relevant fields (this breaksget_tools)- alternative to not breaking
get_tools is having separate types for get and list, and this is a lot more boilerplate imo
- alternative to not breaking
- no zero value removal at all (-> leads to noisy lists)
Overall, there is a risk of maybe leaving out a field, but other than the one you caught just now, all the other tools have get_ fallbacks, which the model can use.
|
|
||
| // Wrap optimized issues with pagination metadata | ||
| issueResponse := map[string]any{ | ||
| "issues": json.RawMessage(optimizedIssues), |
There was a problem hiding this comment.
For empty issues list we used to return an empty list {"issues":[],...}
which is no longer the case after this optimization
There was a problem hiding this comment.
OptimizeList still produces [] on an empty slice 🤔 I've also just tested it against an issue-less repo and this is what was returned:
{"issues":[],"pageInfo":{"endCursor":"","hasNextPage":false,"hasPreviousPage":false,"startCursor":""},"totalCount":0}
There was a problem hiding this comment.
Added a nil guard in OptimizeList, so that [] is returned in that case too.
|
|
||
| // OptimizeList optimizes a list of items by applying flattening, URL removal, zero-value removal, | ||
| // whitespace normalization, and fill-rate filtering. | ||
| func OptimizeList[T any](items []T, opts ...OptimizeListOption) ([]byte, error) { |
There was a problem hiding this comment.
OptimizeList does marshal + unmarshal + marshal. For large list responses this may introduce noticeable latency.
There was a problem hiding this comment.
This unfortunately is the tradeoff of having a generic optimisation pipeline. An alternative was using Go reflection (which the spike did), but that's significantly more complex (to read and maintain) imo. It also still has some of the drawbacks of this solution.
I do agree with the entire json round trip potentially adding overhead when a large amount of items is requested, but it should be in the low ms range at worst (which I'd dare to say is negligible vs the actual API call?).
| } | ||
|
|
||
| if len(maps) >= minFillRateRows { | ||
| maps = filterByFillRate(maps, defaultFillRateThreshold, cfg) |
There was a problem hiding this comment.
There is another issue with the ordering.
The pipeline order is
- flattenTo
- optimizeItem
- filterByFillRate
So zero-value removal artificially deflates fill rates, causing fill-rate filtering to wipe out keys that had legitimate non-zero values on some items.
Concrete example with list_branches (no preserved fields):
10 branches: 1 has protected: true, 9 have protected: false optimizeItem strips protected: false from 9 items (it's a zero value)
Then filterByFillRate sees protected on only 1/10 items --> minCount = int(0.1 * 10) = 1 → 1 > 1 is false and it's removed
The one branch that was protected: true loses that information entirely.
There was a problem hiding this comment.
That order is intentional (and intentionally aggressive). I might sound like a broken record here, but what the optimisation pipeline should do imo is optimise for a workflow where list tools are for scanning, and get tools are for getting in-depth details.
Stripping everything that isn't high signal at the list level should be fine, the worst thing that could happen in practice is that the model ends up using the get_ tool to get more specifics.
With that being said, I did spend some time exploring a couple of alternatives:
- Swapping the order (fill rate first, then zero value removal):
- this might be better for JSON, but it creates a problem for other output formats (e.g., for CSV like format, sparse bool with 1
trueand 99 empty cells adds noise, which I'm pretty sure scales linearly with amount of items).
- Skipping
boolsin zero value removal entirely:
- no data loss, but keeps N *
falsevalues in the output, which does add up at scale and goes against the entire goal of this work
Keeping the current order and having WithPreservedFields is imo the most explicit way of controlling what is retained (i.e., high priority fields) and it works consistently across formats.
pkg/github/issues.go
Outdated
|
|
||
| optimizedIssues, err := response.OptimizeList(minimalIssues) | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("failed to optimize issues: %w", err) |
There was a problem hiding this comment.
This is different from list_pull_requests, where if a call to OptimizeList fails we return if err != nil { return utils.NewToolResultErrorFromErr("failed to marshal response", err), nil, nil }
|
I added some tests to my branch, just to show you what I mean #2081 |
…dd depth drop test, review error handling
Tried my best at using these as reference and addressing every concern here.. Regarding the whitespace normalisation vs "markdown with code blocks, bullet lists, and intentional line breaks", I don't think there's a quick fix for this without making the approach smarter (e.g., skipping content inside code fences?). It is a valid concern and I'll try revisiting this in a follow up, but overall I am of the idea that Imo this is acceptable for now (I did not consider this specific scenario during evals, but overall, when a prompt such as |
Summary
Adds a generic response optimisation package that reduces token usage in
list_tool responses by applying six strategies at runtime: nested object flattening, URL elimination, zero-value elimination, whitespace normalisation, collection summarisation, and fill-rate filtering.Each relevant tool passes its own config via functional options:
WithPreservedFields- keys exempt from destructive strategiesWithCollectionExtractors- extract important values from nested collections instead of summarising as[N items]WithMaxDepth- override flatten depth if neededWhy
Current JSON responses are token heavy, with lots of redundant nested objects, URL fields, and zero-value noise. A single
list_pull_requesttool call at 25 items can use 180k tokens. This pipeline brings that down to 55k (~69% reduction) without losing anything the model actually needs.What changed
pkg/response- generic optimisation pipeline with per-tool configuration via functional optionsresponse.OptimizeListinto 6 defaultlist_tools (list_pull_requests,list_issues,list_commits,list_tags,list_releases,list_branches)list_issue_types, as its data is already flat and the pipeline produces no benefitToken reduction
list_commits: ~21-25%list_tags: ~73-74%list_releases: ~91-94%list_issues: ~5-15%list_pull_requests: ~54-73%list_branches: ~5-9%Measured using OAI's
tiktokenlibrary (o200k_base) at 2, 10, and 25 items. Model accuracy validated withGPT 5.1andOpus 4.5across 6 prompts per tool, no degradation observed.MCP impact
Prompts tested (tool changes only)
Security / limits
Tool renaming
deprecated_tool_aliases.goNote: if you're renaming tools, you must add the tool aliases. For more information on how to do so, please refer to the official docs.
Lint & tests
./script/lint./script/testDocs