Fix silent pagination truncation in gh aw mcp inspect, dedupe connect logic - #51193
Conversation
…helpers Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
gh aw mcp inspect, dedupe connect logic
|
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
|
|
There was a problem hiding this comment.
Pull request overview
Fixes MCP inspector pagination truncation and consolidates capability querying.
Changes:
- Uses SDK pagination iterators for tools, resources, and prompts.
- Shares capability-query logic across stdio and HTTP transports.
- Adds an in-memory pagination regression test.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/mcp_inspect_mcp.go |
Adds paginated shared capability querying. |
pkg/cli/mcp_inspect_mcp_test.go |
Verifies complete multi-page retrieval. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
🧪 Test Quality Sentinel Report
📊 Metrics (1 test)
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Test Quality Sentinel: 100/100 (with violation). Missing (go/redacted):build build tag on newly added pkg/cli/mcp_inspect_mcp_test.go. All test files in this package require (go/redacted):build !integration (or (go/redacted):build integration) as line 1. Review flagged tests in the comment above.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — the pagination bug fix and deduplication are solid; flagging a few targeted gaps.
📋 Key Themes & Highlights
Positive Highlights
- ✅ Root cause properly addressed: switching to SDK iterators is the right fix, not cursor-loop patching
- ✅ Shared
queryServerCapabilitieshelper is a clean deep-module move — simpler interface, same behaviour - ✅ IIFE closures for each iterator section cleanly scope the per-operation
context.WithTimeoutandcancel - ✅
TestQueryServerCapabilities_PaginationwithPageSize:1is a strong regression guard for the core bug
Issues Found
-
Partial results not surfaced to the user (
mcp_inspect_mcp.go~line 284) — iterator errors only print a warning whenverboseis true; otherwise the caller receives a silently truncated list with no indication it is incomplete. This re-introduces a weaker form of the original truncation bug on transient errors. -
info.Rootsunasserted in the test (mcp_inspect_mcp_test.goline 70) —extractRootsFromResourcesruns but is never checked, so a future breakage in that function would go undetected by this test. -
No error-path test case — the test only validates the happy path. A sub-test that simulates a mid-stream error would lock in the
break-on-first-error behaviour.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 32.1 AIC · ⊞ 7.1K
Comment /matt to run again
| if len(info.Resources) != itemCount { | ||
| t.Errorf("expected %d resources, got %d", itemCount, len(info.Resources)) | ||
| } | ||
| if len(info.Prompts) != itemCount { |
There was a problem hiding this comment.
[/tdd] info.Roots is not asserted — extractRootsFromResources runs but its output goes unverified, leaving a silent gap in regression coverage.
💡 Suggested addition
if len(info.Roots) == 0 {
t.Errorf("expected roots to be inferred from resources, got none")
}Adding even a basic non-empty check here would catch a future breakage in extractRootsFromResources.
@copilot please address this.
There was a problem hiding this comment.
Added the //go:build !integration tag and now assert info.Roots in pkg/cli/mcp_inspect_mcp_test.go in fb3eeac.
|
|
||
| client := mcp.NewClient(&mcp.Implementation{Name: "test-client", Version: "1.0.0"}, nil) | ||
| serverTransport, clientTransport := mcp.NewInMemoryTransports() | ||
|
|
There was a problem hiding this comment.
[/tdd] The test does not cover error handling — there is no case where the server returns an error mid-iteration to verify that verbose-mode warnings fire and iteration stops cleanly.
💡 Why this matters
The new iterator loop uses break on first error; a test that injects an error would lock in that behaviour and prevent a silent regression if the error-handling path is changed.
Consider adding a second sub-test with a server that surfaces an error partway through listing, and assert that info.Tools contains only the items received before the error.
@copilot please address this.
There was a problem hiding this comment.
Added an error-path regression in fb3eeac that forces tools/list to fail on the second page and asserts the first-page tool is preserved while info.Error is populated.
| if err != nil { | ||
| if verbose { | ||
| console.PrintWarningMessage(fmt.Sprintf("Failed to list resources: %v", err)) | ||
| } |
There was a problem hiding this comment.
[/diagnosing-bugs] The iterator breaks on the first error, silently dropping any items a server may have already buffered after the failing page. This means a transient mid-stream error produces a partial (and potentially misleading) capability list with no indication in the returned *MCPServerInfo that it is incomplete.
💡 Options to consider
- Set
info.Connected = false(or add anErrorfield toMCPServerInfo) when any iteration is cut short, so callers know the result is partial. - Return the partial list but surface a structured warning in the output rendered by the caller, not just a console
PrintWarningMessagethat is suppressed whenverboseis false.
The current code only warns when verbose is true, so a real-world silent truncation due to a transient network error would be invisible to the user.
@copilot please address this.
There was a problem hiding this comment.
fb3eeac now records iterator failures on info.Error, and displayServerCapabilities always emits a partial-results warning so the truncation is visible even when --verbose is off.
Draft generated by Design Decision Gate for PR #51193.
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (129 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
|
@copilot Quick triage nudge for this PR. Please refresh the branch if GitHub can update it cleanly, address the remaining review feedback, and run the Review items to address (newest first):
Branch refresh was requested. Run: https://github.com/github/gh-aw/actions/runs/31222749409
|
|
@copilot Quick triage nudge for this PR. Please fix the missing Run: https://github.com/github/gh-aw/actions/runs/31225438989
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in fb3eeac: partial iterator errors are surfaced to users,
Addressed in fb3eeac. The missing build tag is fixed, the remaining review feedback is covered, and the branch is already up to date with |
|
@copilot Quick triage nudge for this PR. Please refresh the branch if GitHub can update it cleanly, ensure the ADR at Review items to address (newest first):
Run: https://github.com/github/gh-aw/actions/runs/31230238662
|
TriageCategory: bug | Risk: high | Score: 50/100
Recommended action:
|
|
@copilot Quick triage nudge for this PR. Please refresh the branch if GitHub can update it cleanly, ensure the remaining ADR and blocking review requirements are fully cleared, and run the Open items (newest first):
Branch refresh was requested. Run: https://github.com/github/gh-aw/actions/runs/31233305116
|
|
@copilot Quick triage nudge for this PR. Please refresh the branch if GitHub can update it cleanly, ensure the ADR is linked in the PR body if still required, verify any remaining blocking review state is cleared or superseded, and run the Open items (newest first):
Run: https://github.com/github/gh-aw/actions/runs/31235166066
|
The go-fan review of
modelcontextprotocol/go-sdkv1.7.0 flagged a correctness bug ingh aw mcp inspect:ListTools/ListResourceswere called once with no cursor handling, so MCP servers that paginate their tool/resource lists would be silently truncated in the inspector output. It also flagged ~70 duplicated lines betweenconnectStdioMCPServerandconnectHTTPMCPServer.Pagination fix
pkg/cli/mcp_inspect_mcp.goto the SDK's iterator-based pagination helpers (session.Tools,session.Resources,session.Prompts), which follow cursors automatically instead of requiring hand-rolled cursor loops (previously onlyListPromptshad one).De-duplication
queryServerCapabilities(ctx, config, session, verbose) *parser.MCPServerInfohelper, called by bothconnectStdioMCPServerandconnectHTTPMCPServerafter each establishes its transport-specific session.deferred context cancel, so timeouts can't leak if the loop body changes later.Example
Also confirmed no test or fixture asserts on absence of
readOnlyHint/idempotentHintfields, so the v1.7.0 wire-format change (bareboolinstead ofomitempty) requires no follow-up here. AddedTestQueryServerCapabilities_Pagination, which spins up an in-memory MCP server/client with a small page size to verify all items are retrieved across multiple pages.