Skip to content

perf(http): preserve lazy MCP parsing and compatibility - #10

Open
Daigrin wants to merge 5 commits into
mainfrom
daigrin-perf-mcp-parse-arguments
Open

Daigrin wants to merge 5 commits into
mainfrom
daigrin-perf-mcp-parse-arguments

Conversation

@Daigrin

@Daigrin Daigrin commented Sep 3, 2026 •

Copy link
Copy Markdown
Owner

Summary

Keep MCP request parsing lazy and preserve source compatibility while covering argument-decoding regressions. Also fix Windows icon URI line endings and enforce the repository's lint-tool version.

Why

The original middleware materialized all tool arguments just to read owner/repo. During review, current main introduced a better lazy RawArguments/DecodeArguments() design; this branch now integrates that design instead of retaining eager struct extraction, which also had case-insensitive key-matching regressions. No linked issue.

What changed

  • Merge current main without rewriting history, resolve all parser/context conflicts, and preserve upstream argument-dependent scope checks and their fast paths. mcp_parse.go now matches main.
  • Retain deprecated Owner, Repo, and Arguments fields for source compilation compatibility. The middleware does not populate them; consumers explicitly use DecodeArguments() instead. Decoded maps are not shared or cached.
  • Add coverage for exact and escaped keys, case variants, duplicate/null/malformed values, independent decoded maps, raw nested arguments, request-body preservation, and parse-only allocation benchmarks.
  • Strip CRLF terminators from embedded icon URI records. Verify canonical URI equality against all embedded PNGs, both line endings, missing final newlines, and malformed records, without regenerating snapshots.
  • Enforce cached golangci-lint v2.9.0, align the exact CI pin, and correct Go/source-build and workflow guidance.

MCP impact

  • No tool or API changes
  • Tool schema or behavior changed
  • New tool added

No tool names or wire schemas change relative to current main. Request metadata uses lazy decoding, with deprecated library fields retained solely for source compatibility; raw arguments remain available to policy middleware and tool validation.

Prompts tested (tool changes only)

N/A: no live GitHub tool prompts were executed. Automated request/argument tests cover the affected middleware behavior.

Security / limits

  • No security or limits impact
  • Auth / permissions considered
  • Data exposure, filtering, or token/size limits considered

Preserves main's scope-challenge workflow/file-path inputs and maximum-scopes fast path. Request-size enforcement and downstream body contents remain intact. Case-varied keys stay distinct rather than overriding lowercase owner/repo keys.

Tool renaming

  • I am renaming tools as part of this PR (e.g. a part of a consolidation effort)
    • I have added the new tool aliases in deprecated_tool_aliases.go
  • I am not renaming tools as part of this PR

No tools are renamed.

Note: 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

  • Linted locally with ./script/lint
  • Tested locally with ./script/test

Post-integration validation uses Go 1.25.12, pinned golangci-lint v2.9.0, and MinGW GCC with CGO_ENABLED=1: full lint (0 issues), full race suite, go build ./..., go mod tidy -diff, and generated-documentation consistency all pass. Windows scripts run through Git Bash with set -o igncr; module files were normalized to LF to match CI.

Independent QA also passed 12 isolated linter-cache/install/error-path scenarios. On Windows amd64, the ~128 KiB nested-payload benchmark measured 139,856 B / 14 allocations for lazy envelope parsing versus 324,896 B / 1,058 allocations for envelope parsing plus eager map decoding. These exclude HTTP body reading and explicit lazy decoding by downstream consumers.

Docs

  • Not needed
  • Updated (README / docs / examples)

Updated installation and contributor/toolchain guidance. Generated documentation is unchanged relative to current main.

WithMCPParse unmarshaled the entire tools/call arguments payload into a
map[string]any on every request just to extract owner and repo. Decode
into a small typed struct with only those two fields instead, and drop
the now-unused Arguments field from MCPMethodInfo (grep confirmed the
only consumer was the middleware's own test).

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

It introduces a breaking exported API change (MCPMethodInfo field removal) and tightens argument decoding in a way that can drop valid repo extraction on partial type mismatches.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR optimizes HTTP-mode MCP request parsing by avoiding full map[string]any argument decoding when handling tools/call, extracting only owner/repo to reduce per-request allocations and parsing work.

Changes:

  • Updated WithMCPParse to decode tools/call arguments into a small typed struct containing only owner/repo.
  • Removed Arguments map[string]any from MCPMethodInfo.
  • Updated middleware tests to stop asserting full arguments and added coverage for large extra argument fields.
File summaries
File Description
pkg/http/middleware/mcp_parse.go Switches argument parsing to decode only owner/repo for tools/call.
pkg/context/mcp_info.go Removes Arguments from MCPMethodInfo.
pkg/http/middleware/mcp_parse_test.go Adjusts assertions to match new parsing behavior and adds a “large extra field” case.
Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread pkg/context/mcp_info.go Outdated
Comment thread pkg/http/middleware/mcp_parse.go Outdated
Copilot AI added 4 commits September 3, 2026 05:27
…s field

Address review feedback: decode owner/repo as json.RawMessage and
unmarshal each individually so a wrongly typed field no longer blocks
extraction of the other. Restore the MCPMethodInfo.Arguments field as
deprecated (no longer populated) to avoid breaking external consumers
of pkg/context.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Strip CRLF terminators from embedded icon data URIs so Windows checkouts match the existing tool snapshots. Cover both line endings and exercise tolerant argument extraction, large nested payloads, and body restoration.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Validate cached golangci-lint versions, align CI's exact pin, correct Go/build and workflow guidance, and add independent canonical icon URI and malformed-entry coverage.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Resolve parser conflicts using upstream RawArguments and DecodeArguments, retaining deprecated legacy fields for source compatibility. Preserve scope-policy inputs and fast paths, and add exact-key, malformed-input, body-preservation and lazy-allocation regression coverage.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@Daigrin Daigrin changed the title perf(http): avoid full argument map allocation in MCP parse middleware perf(http): preserve lazy MCP parsing and compatibility Sep 6, 2026
Copilot AI mentioned this pull request Sep 6, 2026
1 of 7 tasks

Copilot AI commented Sep 7, 2026

Copy link
Copy Markdown

🟡 Changes recommended

It introduces a breaking exported API change (MCPMethodInfo field removal) and tightens argument decoding in a way that can drop valid repo extraction on partial type...

The two requested fixes from this review are already present on the branch in commit f9760d5: MCPMethodInfo.Arguments is retained for compatibility (deprecated/unpopulated), and argument handling keeps tolerant behavior via RawArguments + DecodeArguments(). I verified with targeted tests (go test ./pkg/context ./pkg/http/middleware), so no additional code changes were needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants