Add ETag conditional requests to the REST transport

[joshfree]

Summary

Adds HTTP conditional-request (ETag / If-None-Match) support to the REST transport so unchanged resources are revalidated with a 304 Not Modified instead of being re-downloaded in full.

Why

Every REST request is currently issued unconditionally: the ETag returned by the GitHub API is never stored or replayed, so repeated tool calls that read the same resource (for example pull request reads, file and commit listings, and reviews) download the full response each time.

Fixes #3025

What changed

• Added ETagTransport, a bounded (LRU), concurrency-safe http.RoundTripper that caches the ETag and body of cacheable GET responses and sends If-None-Match on the next identical request; a 304 Not Modified is served from the cached body.
• Wired ETagTransport into createGitHubClients() below the user-agent and auth layers, so cached entries are scoped by the request's Authorization header and never shared across tokens.
• Rate-limit headers are surfaced from the live 304 response so downstream rate-limit accounting stays correct.
• Added ETag / If-None-Match header constants.

Every request is still sent to the server, so responses are always revalidated and never served stale. Per the GitHub REST API docs, a 304 Not Modified does not count against the token's primary rate limit, so repeated reads conserve rate-limit budget and bandwidth while returning identical data.

MCP impact

• No tool or API changes

Security / limits

• Auth / permissions considered — cache keys include a hash of the Authorization header, so cached bodies are never served across different tokens.
• Data exposure, filtering, or token/size limits considered — the cache is bounded (default 512 entries, LRU eviction) and in-memory only.

Tool renaming

• I am not renaming tools as part of this PR

Lint & tests

• Tested locally with go test ./pkg/http/transport/... (new etag_test.go covers the 304-serves-cached-body flow, per-token scoping, rate-limit header pass-through, GET-only caching, and caller-supplied conditional headers). go build ./... and go vet ./... pass.

Docs

• Not needed

[Copilot]

Pull request overview

Adds ETag-based conditional revalidation to GitHub REST requests to reduce bandwidth and rate-limit usage.

Changes:

• Adds a concurrency-safe LRU ETag transport.
• Integrates it into REST client construction.
• Adds header constants and transport tests.

Show a summary per file

File	Description
pkg/http/transport/etag.go	Implements ETag caching and revalidation.
pkg/http/transport/etag_test.go	Tests conditional-request behavior.
pkg/http/headers/headers.go	Adds ETag header constants.
internal/ghmcp/server.go	Enables the transport for REST clients.

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 2
• Review effort level: Balanced

[SamMorrowDrums]

Thanks for this, the aims make sense, however most of our usage is through the hosted remote server, which is horizontally scaled and so would need to think about this because it's pretty expensive to add a distributed store there, and additional Redis usage, extra network request per request that would normally be false adding latency, in-memory store would likely be much less useful and we also handle hundreds of RPS so really would need to think about this in that context.

[joshfree]

That concern is now addressed by scoping the cache to the local install (ed5fdb83).

The conditional-request cache is wired only into the long-lived stdio server (internal/ghmcp/createGitHubClients). The hosted remote server builds a fresh REST client per request via RequestDeps.GetClient (pkg/github/dependencies.go), which does not use this transport — so nothing changes for the horizontally-scaled deployment: no distributed/Redis store, no extra network hop, and no added latency at hundreds of RPS.

For the local server the in-memory cache is bounded by entry count and by per-entry (1 MiB) and total (32 MiB) byte budgets with LRU eviction, and the raw-content client uses a separate transport without the cache so large file reads are streamed rather than retained. The two inline nits (Cache-Control: no-store / Vary: *, and the byte budget) are handled in the same commit.

Happy to keep this strictly local-only, or to expose it behind an explicit opt-in flag if that is preferable.

[joshfree]

Gentle nudge on this PR. CI is fully green (13/13 checks) and the branch is current. It adds ETag conditional-request support to reduce redundant API calls (ref #3025), with tests and updated tool snapshots/docs per CONTRIBUTING. There is one approving review, but it does not satisfy the required CODEOWNER review, so it is ready for a maintainer whenever there is bandwidth. Any feedback will be addressed promptly.

[IrynaKulakova]

Approving. My earlier concern about the unbounded io.ReadAll is fully addressed in 05fa854 — the read is now capped with io.LimitReader, oversized bodies are handed back as the buffered prefix plus the remaining stream, and TestETagTransport_StreamsOversizedUnknownLengthBody guards the unknown-length case that the original ContentLength check missed. Verified locally: go test -race ./pkg/http/transport/... passes and CI is green.

The design is sound overall — always revalidate, never serve stale, per-token cache scoping, LRU with both entry and byte budgets, and the raw-content client kept off the cache so large file bodies stream.

One non-blocking follow-up (happy for it to land separately): the cache key doesn't include Accept, even though the API returns Vary: Accept and a distinct ETag per representation. I confirmed this against api.github.com — the same PR URL yields different ETags for application/vnd.github+json and application/vnd.github.diff. In this repo that's exercised by pullrequests.go (JSON read vs RawOptions{Type: Diff} on the same URL) and by search with text-match. No wrong body is served today, since the differing ETags simply produce a 200, but the two representations continually evict each other so those URLs get no cache hits while still paying the buffering cost. It's also latent on GHES or proxies that compute a representation-independent ETag. Adding Accept to cacheKey would close it.

Two smaller nits, also non-blocking: cacheKey truncates the Authorization hash to 8 bytes even though it's a tenant-isolation boundary (the full sum costs nothing), and the error path returns resp, err where the RoundTripper contract asks for a nil response alongside a non-nil error.