Codecov Report

❌ Patch coverage is 90.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.29%. Comparing base (1c60f0f) to head (e618f4b).

@@            Coverage Diff             @@
##             main    #5800      +/-   ##
==========================================
- Coverage   71.32%   71.29%   -0.04%     
==========================================
  Files         693      693              
  Lines       70587    70598      +11     
==========================================
- Hits        50347    50332      -15     
- Misses      16605    16631      +26     
  Partials     3635     3635              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

Most of this diff is generated swagger docs, not your rate-limiting code. Likely cause: swag isn't pinned, so different runs can produce different output with no real API change.

Can you rerun task docs on your PR branch and push the result? If the diff is still large after that, rebase on #5802 (pins the swag version) and try again.

Most of this diff is generated swagger docs, not your rate-limiting code. Likely cause: swag isn't pinned, so different runs can produce different output with no real API change.

Can you rerun task docs on your PR branch and push the result? If the diff is still large after that, rebase on #5802 (pins the swag version) and try again.

Thanks, Jeremy. I reran task docs and reproduced the large Swagger-only diff locally. I also tested the pinned swag version from #5802, but my local run still rewrote generated schema names without any API change.
I removed the generated docs/server changes and the temporary pin commit to keep this PR scoped. The PR now contains only the intended rate-limit observability changes, and Docs / Verify Swagger Documentation passes in CI on e618f4b. It looks like the original diff was local generator/environment churn rather than a required API documentation update.

Heads-up on an interaction with #6136 (fixes #6133), since we'd otherwise discover it after one of us merges.

The overlap. #6136 makes the mutating webhook republish the parsed MCP request, because ParsingMiddleware parses once and the webhook was replacing r.Body without refreshing the cached parse — so authz was evaluating Cedar policy against the tool name and arguments that arrived rather than the ones the backend ran. Today telemetry sits after the webhook layers, so republishing fixes telemetry's view too.

This PR moves telemetry to immediately after the parser. That puts telemetry outside the mutation, and republishing can't reach it there: a derived context only flows downstream, and pkg/telemetry/middleware.go reads the parse solely via GetParsedMCPRequest(ctx) (:174, :227, :300, :417, :687, :723) with no ParsedRequestHolder. recordMetrics runs after next returns but against the ctx built at :217, so it's pre-mutation as well.

Net effect once both land in this order: span names, mcp_resource_id, and toolhive_mcp_tool_calls{tool} would attribute a call to the tool the caller requested rather than the one that executed, whenever a webhook rewrites params/name or method. Those two are documented contracts (docs/observability.md:203, :235), which is why I don't think "document it as as-received" is a good out.

No textual conflict. #6136 doesn't touch pkg/runner/middleware.go, so git will merge both cleanly and nothing will flag this. That's the reason I'm commenting rather than waiting.

One option you may not have considered. The requirement here is a live span before the limiter runs, which you can get by moving the limiter later instead of telemetry earlier:

parser → mutating → validating → usagemetrics → telemetry → ratelimit → authz

The limiter's only stated constraint is the one in its own comment at pkg/runner/middleware.go:166-167 — "Positioned after MCP parser (needs tool name from context)". Nothing requires it ahead of the webhooks. I checked the things that could object: vMCP rate limiting is a separate core.VMCP decorator (pkg/vmcp/ratelimit/decorator.go:37), not in the HTTP chain, so it's unaffected; and no existing test pins telemetry/limiter/webhook relative order (AuditWrapsChain:1366, StripAuthOrdering:1406, AWSStsOrdering:870 don't). It would also close the rate-limit half of #6134, where the limiter debits the bucket for the requested tool while the backend runs the mutated one.

But I'm not recommending it outright, because it has a real cost: rate-limited requests would then pay the webhook round-trips before being rejected. For a quota control that's backwards — outbound HTTP per rejected request is amplification on exactly the traffic you're shedding. If webhooks aren't configured it's free, but that's not something to rely on.

So the alternative worth weighing is splitting span creation (early, so the limiter has something to annotate) from MCP attribute population (late, post-mutation) — span.SetName and attribute-setting can both happen after next in principle. More churn inside the telemetry middleware, no cost to rejected requests. Your call, and observability ownership matters more than my opinion here.

If this merges first, #6136 will reconcile — I'd update the ordering list and the limitations bullet in docs/middleware.md, the comment at the republish site, and the RepublishParsedMCPRequest doc comment, which currently names the rate limiter as an uncorrected consumer.

If #6136 merges first (it's small, green, and fixes a live mis-authorization, whereas this one has changes requested), this PR would need those same four spots updated, plus a decision on the telemetry-accuracy question above.

No action needed from you on #6136 itself — flagging so the second one through isn't a surprise.