Skip to content

Share one JSON-RPC error writer across HTTP denial paths - #6086

Merged
JAORMX merged 1 commit into
mainfrom
jsonrpc-error-contract-followups
Jul 28, 2026
Merged

Share one JSON-RPC error writer across HTTP denial paths#6086
JAORMX merged 1 commit into
mainfrom
jsonrpc-error-contract-followups

Conversation

@JAORMX

@JAORMX JAORMX commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Summary

Why. Three HTTP denial paths — authz handleUnauthorized and both webhook sendErrorResponses — each carry a private copy of the same block: encode a *jsonrpc2.Response via jsonrpc2.EncodeMessage before writing any header, fall back to a hardcoded conformant body on the (unreachable) encode failure, set Content-Type, write status, write body. That block exists because of #5950: encoding/json on a jsonrpc2 type reflects Go-capitalized keys, drops the mandatory "jsonrpc":"2.0" tag, and renders every id as {} — and the only enforcement of "never json.Marshal a jsonrpc2 type" has been a greppable convention across hand-rolled copies. #6055's reviewer notes rejected a map-building shared helper (an any-typed id parameter is exactly how a caller reintroduces the {} id bug) but said a narrower helper taking a jsonrpc2 message "is worth a follow-up". This is that follow-up.

What. mcp.EncodeJSONRPCError and mcp.WriteJSONRPCError (pkg/mcp/jsonrpc_write.go) take a typed *jsonrpc2.Response — no any-typed id anywhere — encode before any header is written, and fall back to a hardcoded conformant -32603 body on encode failure. The three sites collapse onto them; each keeps its own semantics (status, code via #6066's mcp.JSONRPCCodeForStatus, message policy). Behavior is unchanged except the per-site fallback bodies, which were unreachable and now uniformly report -32603 rather than echoing a code whose integrity is unknown once encoding has failed.

Scope: deliberately three sites, not four. The fourth carrier of this block, pkg/authz/response_filter.go, is being rewritten upstream — #6087 restructures its error write path and #6088 rewrites it again — so collapsing it here would guarantee conflicts and duplicate that work. It keeps its private copy for now and can adopt the helper in a follow-up once those land; this PR touches that file zero lines. Three of four sites is still real consolidation, and the helper is the landing spot for the fourth.

Honest scoping against #6066: it changed which code each site emits and scrubbed leaked text, but kept each site's private copy of the encode-fallback-write block — so the duplication is still real. Had #6066 centralized the write path, the honest answer here would have been "no longer worth it"; it didn't.

Convention + lint stance: the "never json.Marshal a jsonrpc2 type" rule is recorded in .claude/rules/go-style.md (with the omit-absent-id corollary from #6038/#6068). A lint gate was considered and deliberately not added: forbidigo matches call-expression text and cannot see argument types, so it cannot distinguish marshaling a jsonrpc2 value from any other json.Marshal; the only precise option is wiring gocritic/ruleguard, which is not worth the infrastructure for one rule. The helper plus the recorded convention is the enforcement.

Part of the #5950/#6038/#6039 error-contract cleanup line; the helper follow-up named in #6055's "Special notes for reviewers".

Type of change

  • Bug fix
  • New feature
  • Refactoring (no behavior change)
  • Dependency update
  • Documentation
  • Other (describe):

Test plan

  • Unit tests — new pkg/mcp/jsonrpc_write_test.go pins the helper's wire properties (lowercase keys + version tag via case-sensitive JSONEq; absent id omitted, never "id":null; id: 0 survives — omitempty on the wire struct tests IsNil, not zero-ness; status + Content-Type on the write path). All three collapsed sites remain covered by their existing envelope-conformance tests (pkg/authz, pkg/webhook/... — all green), which is the point: a behavior-preserving refactor should need no assertion changes, and it didn't.
  • Linting (task lint-fix) — 0 issues.

task test not used locally (known gotestfmt v2.5.0 panic); CI runs the real thing.

API Compatibility

  • This PR does not break the v1beta1 API, OR the api-break-allowed label is applied and the migration guidance is described above.

Changes

File Change
pkg/mcp/jsonrpc_write.go The two helpers + the doc comment that carries the #5950 rationale
pkg/mcp/jsonrpc_write_test.go Wire-property pins (key case, id omission, zero id, status/content-type)
pkg/authz/middleware.go handleUnauthorized collapses onto mcp.WriteJSONRPCError
pkg/webhook/{validating,mutating}/middleware.go sendErrorResponse collapses onto the helper; keeps #6066's mcp.JSONRPCCodeForStatus
.claude/rules/go-style.md "JSON-RPC Envelopes" convention section

Does this introduce a user-facing change?

No. Wire behavior at all three sites is unchanged; only the unreachable encode-failure fallbacks are unified.

Special notes for reviewers

🤖 Generated with Claude Code

https://claude.ai/code/session_01Y5WXPQr5Da9Cox2nZWhg6n

@github-actions github-actions Bot added the size/S Small PR: 100-299 lines changed label Jul 28, 2026
@codecov

codecov Bot commented Jul 28, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 80.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.28%. Comparing base (e2c4441) to head (5246673).

Files with missing lines Patch % Lines
pkg/mcp/jsonrpc_write.go 75.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6086      +/-   ##
==========================================
+ Coverage   72.26%   72.28%   +0.02%     
==========================================
  Files         728      729       +1     
  Lines       75523    75533      +10     
==========================================
+ Hits        54575    54601      +26     
+ Misses      17045    17029      -16     
  Partials     3903     3903              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

amirejaz
amirejaz previously approved these changes Jul 28, 2026
Three HTTP denial paths each carry a private copy of the same block:
encode a *jsonrpc2.Response via jsonrpc2.EncodeMessage before writing
any header, fall back to a hardcoded conformant body on the unreachable
encode failure, then set Content-Type, write status, write body. That
block exists because of #5950 — encoding/json on a jsonrpc2 type
reflects Go-capitalized keys, drops the mandatory "jsonrpc":"2.0" tag,
and renders every id as {} — so the only thing preventing a fourth
hand-rolled copy from reintroducing that bug has been a greppable
convention.

#6055's review rejected a map-building shared helper, because an
any-typed id parameter is exactly how a caller reintroduces the {} id
bug, but noted a narrower helper taking a jsonrpc2 message avoids the
objection. Add exactly that: mcp.EncodeJSONRPCError and
mcp.WriteJSONRPCError take a typed *jsonrpc2.Response, so no id can
reach them untyped.

Scope is deliberately three sites, not four. The fourth carrier of this
block, pkg/authz/response_filter.go, is being rewritten by #6087 and
again by #6088; collapsing it here would guarantee a conflict and
duplicate that work, so it keeps its private copy for now and can adopt
the helper once those land.

Behavior is unchanged except the per-site fallback bodies, which were
unreachable and now uniformly report -32603 rather than echoing a code
whose integrity is unknown once encoding has failed. Each site keeps its
own status and its own code via #6066's mcp.JSONRPCCodeForStatus.

The 'never json.Marshal a jsonrpc2 type' convention is also recorded in
.claude/rules/go-style.md. A lint gate was considered and deliberately
not added: forbidigo matches call text and cannot see argument types, so
it cannot distinguish marshaling a jsonrpc2 value from any other
json.Marshal call, and wiring up gocritic/ruleguard for one rule is not
worth the infrastructure. The helper plus the recorded rule is the
enforcement.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y5WXPQr5Da9Cox2nZWhg6n
@JAORMX
JAORMX force-pushed the jsonrpc-error-contract-followups branch from 690adb5 to 5246673 Compare July 28, 2026 10:04
@JAORMX
JAORMX changed the base branch from envelope-sse-delivery to main July 28, 2026 10:05
@JAORMX
JAORMX dismissed amirejaz’s stale review July 28, 2026 10:05

The base branch was changed.

@github-actions github-actions Bot added size/S Small PR: 100-299 lines changed and removed size/S Small PR: 100-299 lines changed labels Jul 28, 2026
@github-actions github-actions Bot added size/S Small PR: 100-299 lines changed and removed size/S Small PR: 100-299 lines changed labels Jul 28, 2026
@JAORMX
JAORMX merged commit bbdb633 into main Jul 28, 2026
44 checks passed
@JAORMX
JAORMX deleted the jsonrpc-error-contract-followups branch July 28, 2026 10:33
JAORMX added a commit that referenced this pull request Jul 28, 2026
Main gained CRD-affecting changes (#6092, #6046, #6086) after the
previous merge, so the Generate CRD Docs check fails against the
merge result until docs/operator/crd-api.md is regenerated on top.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y5WXPQr5Da9Cox2nZWhg6n
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S Small PR: 100-299 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants