Share MCPServerStatsBase across duplicated MCP server health/stats structs - #53693
Conversation
|
Hey The PR is currently in draft status with a clear checklist. Here's what's needed to move forward:
Once the implementation is complete and tests pass, mark the PR as ready for review. If you'd like assistance from your coding agent, you can assign this:
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Shares common MCP server identity and usage fields through MCPServerStatsBase while preserving existing JSON output schemas.
Changes:
- Embeds the shared base across three MCP reporting types.
- Updates aggregation, rendering, and tests to standardized field names.
- Adds custom JSON marshaling and schema-preservation tests.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/README.md |
Updates generated type documentation. |
pkg/cli/logs_usage_activity.go |
Populates the shared base. |
pkg/cli/logs_models.go |
Defines MCPServerStatsBase. |
pkg/cli/logs_mcp_tool_usage_test.go |
Updates MCP usage fixtures. |
pkg/cli/gateway_logs_mcp.go |
Builds stats using the base. |
pkg/cli/audit_report.go |
Embeds the base in server stats. |
pkg/cli/audit_expanded.go |
Refactors health details and JSON encoding. |
pkg/cli/audit_expanded_test.go |
Tests health-detail JSON output. |
pkg/cli/audit_cross_run.go |
Refactors cross-run health and JSON encoding. |
pkg/cli/audit_cross_run_test.go |
Updates fixtures and JSON tests. |
pkg/cli/audit_cross_run_render.go |
Uses standardized count fields. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 11/11 changed files
- Comments generated: 2
- Review effort level: Balanced
| func (h MCPServerCrossRunHealth) MarshalJSON() ([]byte, error) { | ||
| return json.Marshal(struct { | ||
| ServerName string `json:"server_name"` | ||
| RunsConnected int `json:"runs_connected"` | ||
| TotalRuns int `json:"total_runs"` | ||
| TotalCalls int `json:"total_calls"` | ||
| TotalErrors int `json:"total_errors"` | ||
| ErrorRate float64 `json:"error_rate"` | ||
| Unreliable bool `json:"unreliable"` | ||
| }{ | ||
| ServerName: h.ServerName, | ||
| RunsConnected: h.RunsConnected, | ||
| TotalRuns: h.TotalRuns, | ||
| TotalCalls: h.ToolCallCount, | ||
| TotalErrors: h.ErrorCount, | ||
| ErrorRate: h.ErrorRate, | ||
| Unreliable: h.Unreliable, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Added a matching UnmarshalJSON on MCPServerCrossRunHealth in 9611a0f, plus a round-trip assertion in TestMCPServerCrossRunHealthJSONSchema.
| func (d MCPServerHealthDetail) MarshalJSON() ([]byte, error) { | ||
| return json.Marshal(struct { | ||
| ServerName string `json:"server_name"` | ||
| RequestCount int `json:"request_count"` | ||
| ToolCalls int `json:"tool_calls"` | ||
| ErrorCount int `json:"error_count"` | ||
| ErrorRate float64 `json:"error_rate"` | ||
| ErrorRateStr string `json:"error_rate_str"` | ||
| AvgLatency string `json:"avg_latency"` | ||
| Status string `json:"status"` | ||
| }{ | ||
| ServerName: d.ServerName, | ||
| RequestCount: d.RequestCount, | ||
| ToolCalls: d.ToolCallCount, | ||
| ErrorCount: d.ErrorCount, | ||
| ErrorRate: d.ErrorRate, | ||
| ErrorRateStr: d.ErrorRateStr, | ||
| AvgLatency: d.AvgLatency, | ||
| Status: d.Status, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Added a matching UnmarshalJSON on MCPServerHealthDetail in 9611a0f, plus a round-trip assertion in TestMCPServerHealthDetailJSONSchema.
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
This refactor keeps the old JSON keys only on the write path and breaks round-tripping on the read path.
Blocking themes
MCPServerCrossRunHealthnow marshalstotal_calls/total_errors, but unmarshaling that same payload back into the struct drops both values.MCPServerHealthDetailhas the same regression fortool_calls.
These are wire-compatibility regressions, not just internal renames: persisted reports and downstream readers will silently lose counts after deserialization.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 16.5 AIC · ⌖ 9.09 AIC · ⊞ 7K
Comment /review to run again
|
|
||
| // MarshalJSON preserves the cross-run MCP health JSON schema while sharing the | ||
| // per-server stat fields with the other MCP server report types. | ||
| func (h MCPServerCrossRunHealth) MarshalJSON() ([]byte, error) { |
There was a problem hiding this comment.
This refactor only preserves the legacy wire keys when encoding; any existing JSON consumers that unmarshal total_calls/total_errors back into MCPServerCrossRunHealth will now silently drop those fields, so round-tripping the report loses the call/error counts.
💡 Why this blocks merge
MarshalJSON emits total_calls and total_errors, but the struct now only has embedded fields tagged as tool_call_count / error_count. Without a matching UnmarshalJSON, json.Unmarshal ignores the legacy keys and leaves ToolCallCount/ErrorCount at zero.
That is a real compatibility break for any code that deserializes stored reports, even though the encoder tests pass.
Suggested fix:
func (h *MCPServerCrossRunHealth) UnmarshalJSON(data []byte) error {
var aux struct {
ServerName string `json:"server_name"`
RunsConnected int `json:"runs_connected"`
TotalRuns int `json:"total_runs"`
TotalCalls int `json:"total_calls"`
TotalErrors int `json:"total_errors"`
ErrorRate float64 `json:"error_rate"`
Unreliable bool `json:"unreliable"`
}
if err := json.Unmarshal(data, &aux); err != nil { return err }
h.MCPServerStatsBase = MCPServerStatsBase{ServerName: aux.ServerName, ToolCallCount: aux.TotalCalls, ErrorCount: aux.TotalErrors}
h.RunsConnected, h.TotalRuns, h.ErrorRate, h.Unreliable = aux.RunsConnected, aux.TotalRuns, aux.ErrorRate, aux.Unreliable
return nil
}Add a round-trip test (Marshal + Unmarshal) so this regression stays covered.
There was a problem hiding this comment.
Fixed in 9611a0f: added UnmarshalJSON on MCPServerCrossRunHealth mapping total_calls/total_errors back into the embedded base, plus a round-trip test.
|
|
||
| // MarshalJSON preserves the MCP server health detail JSON schema while sharing the | ||
| // per-server stat fields with the other MCP server report types. | ||
| func (d MCPServerHealthDetail) MarshalJSON() ([]byte, error) { |
There was a problem hiding this comment.
MCPServerHealthDetail has the same one-way compatibility break: the custom marshaler still emits tool_calls, but decoding now relies on the embedded base tag tool_call_count, so any round-trip through JSON zeros the per-server tool count.
💡 Why this blocks merge
This type is part of the audit report payload (servers under mcp_server_health). After this change, json.Marshal still produces the old schema, but json.Unmarshal of that same JSON back into MCPServerHealthDetail will ignore tool_calls and keep ToolCallCount == 0.
That means persisted reports and any downstream consumer that deserializes them lose data without an error, which is worse than an explicit schema change.
Suggested fix: mirror the MarshalJSON shim with an UnmarshalJSON that reads tool_calls into ToolCallCount, then add a round-trip test covering the zero and non-zero cases.
There was a problem hiding this comment.
Fixed in 9611a0f: added UnmarshalJSON on MCPServerHealthDetail mapping tool_calls back into the embedded base, plus a round-trip test.
There was a problem hiding this comment.
This is a clean refactoring that reduces duplication across MCPServerStats, MCPServerHealthDetail, and MCPServerCrossRunHealth by extracting the shared ServerName/ToolCallCount/ErrorCount fields into MCPServerStatsBase.
The MarshalJSON overrides on MCPServerCrossRunHealth and MCPServerHealthDetail correctly preserve the legacy JSON key names (total_calls, total_errors, tool_calls) for downstream consumers. The schema-preservation tests (TestMCPServerCrossRunHealthJSONSchema, TestMCPServerHealthDetailJSONSchema, TestMCPServerHealthDetailJSONKeepsZeroErrorCount) are a good safeguard.
Existing inline comments already flag the decode asymmetry (encode → legacy keys; decode → new field names). Since these structs are report-output types and are never round-tripped through their own JSON, this is low risk in practice.
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 30.6 AIC · ⌖ 8.82 AIC · ⊞ 5.7K
There was a problem hiding this comment.
Ponytail review: over-engineering only.
Two of the three MCP struct consolidations require a full MarshalJSON override (~20 lines each, plus new dedicated schema tests) to keep the old wire format, because the shared base's JSON tags don't match those types. That's roughly as much new code as the duplication it removes, and the goal — collapsing 4 field spellings into 1 — could largely be achieved by sharing just ServerName (or accepting the JSON key rename) without the override machinery.
net: -40 lines possible.
Generated by ✂️ Ponytail Reviewer for #53693 · auto · 32.3 AIC · ⌖ 3.94 AIC · ⊞ 7.3K
Comment /ponytail to run again
|
|
||
| // MarshalJSON preserves the cross-run MCP health JSON schema while sharing the | ||
| // per-server stat fields with the other MCP server report types. | ||
| func (h MCPServerCrossRunHealth) MarshalJSON() ([]byte, error) { |
There was a problem hiding this comment.
L92-113: yagni: 20-line MarshalJSON override just to rename ToolCallCount->total_calls and ErrorCount->total_errors for JSON output. Two of three embedders need this bypass, undercutting the shared-base rationale. Simpler: keep TotalCalls/TotalErrors as this type own fields (as before) and only pull ServerName into the base.
There was a problem hiding this comment.
Keeping the shared base with a marshal/unmarshal shim rather than reverting to type-local TotalCalls/TotalErrors fields, since the goal is a single shared representation (MCPServerStatsBase) for all three server-stat types, with the shim only needed to preserve each type's existing wire format.
|
|
||
| // MarshalJSON preserves the MCP server health detail JSON schema while sharing the | ||
| // per-server stat fields with the other MCP server report types. | ||
| func (d MCPServerHealthDetail) MarshalJSON() ([]byte, error) { |
There was a problem hiding this comment.
L101-122: yagni: same pattern - a 22-line MarshalJSON override to rename ToolCallCount->tool_calls, needed only because the shared base JSON tags do not match this type wire format. Two overrides (here and in audit_cross_run.go) plus dedicated schema tests are pure overhead introduced by the base; scoping the base to ServerName only would avoid both.
There was a problem hiding this comment.
Same rationale as the audit_cross_run.go thread — keeping the shared MCPServerStatsBase and the marshal/unmarshal shim so all three types genuinely share one struct, rather than reverting to per-type duplicated fields.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — requesting changes on three targeted issues.
📋 Key Themes & Highlights
Key Themes
- Encode/decode asymmetry (2 existing Copilot comments + my comment on
audit_cross_run.go):MarshalJSONoverrides emit legacy keys (total_calls,tool_calls) that the default decoder can't map back into the base fields. The types are effectively write-only; that constraint should be documented or enforced with round-trip tests. - Base-field
omitemptyfootgun (logs_models.go):ErrorCount'somitemptytag satisfiesMCPServerStatsbut silently constrains every future embedder. The obligation to overrideMarshalJSONneeds to be visible at the type level. - Incomplete schema-override test assertions (
audit_cross_run_test.go): Tests check that override keys exist but not that base-field keys don't appear, leaving a class of regressions (e.g. pointer-vs-value receiver flip) uncaught.
Positive Highlights
- ✅ Clean application of the existing
AggregatedSummaryBasecomposition pattern — consistent with the broader codebase vocabulary. - ✅ Wire-format preservation via
MarshalJSONanonymous-struct idiom is the right approach (same asMCPFailureSummary). - ✅ New tests cover the zero-
error_countedge case — good instinct. - ✅
MCPServerHealthexclusion is well-reasoned in the PR description.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 44.1 AIC · ⌖ 10.3 AIC · ⊞ 7.8K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/cli/logs_models.go:582
[/codebase-design] omitempty in the base struct leaks an embedder-specific serialisation concern into the shared type — a footgun for future embedders.
<details>
<summary>💡 Why this matters and how to document it</summary>
A future embedder that wants error_count to always appear in JSON output will silently inherit the omit-when-zero behaviour and will need to remember to override MarshalJSON. The inline comment captures intent but sits inside the struct where it's easy to miss. E…
pkg/cli/audit_cross_run.go:110
[/codebase-design] The MarshalJSON override is encode-only: json.Unmarshal into MCPServerCrossRunHealth will map tool_call_count (not total_calls) — so a round-trip of your own JSON output drops the call count silently.
<details>
<summary>💡 Impact and options</summary>
If MCPServerCrossRunHealth values are ever read back from stored JSON (e.g. audit report files), the total_calls key written by MarshalJSON will be ignored on decode and ToolCallCount will be zero. Same i…
pkg/cli/audit_cross_run_test.go:178
[/tdd] The JSON schema tests verify the marshal output keys but don't assert the full key set — tool_call_count (the base field's own key) could appear alongside tool_calls if the override breaks, and the test wouldn't catch it.
<details>
<summary>💡 Add a negative assertion</summary>
// In TestMCPServerCrossRunHealthJSONSchema, after existing asserts:
assert.NotContains(t, decoded, "tool_call_count",
"base field key must not leak into output; override should replace it")
as…
</details>
…tern Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (241 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 run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Three per-server MCP report structs in
pkg/climodeled the same "server name + call count + error count" shape with four different field spellings (TotalCalls/ToolCalls/ToolCallCount,TotalErrors/ErrorCount). This applies theAggregatedSummaryBasecomposition pattern already used byMissingToolSummary/MCPFailureSummary.Shared base
MCPServerStatsBaseinpkg/cli/logs_models.go, adjacent toAggregatedSummaryBase.MCPServerStats(audit_report.go),MCPServerHealthDetail(audit_expanded.go) andMCPServerCrossRunHealth(audit_cross_run.go); call sites and tests updated to the standardized names.Wire format preserved
MCPServerHealthDetailandMCPServerCrossRunHealthgetMarshalJSONoverrides emitting their original keys (tool_calls,total_calls,total_errors) — same technique asMCPFailureSummary.MCPServerStats'serror_count,omitemptytags, since it is the only embedder serializing base tags directly.error_countcase.Out of scope
MCPServerHealth(also named in the issue) is left unchanged: it has no server name, and itsTotalRequests/TotalErrorsare rollups over theServersslice rather than per-server stats, so the base does not apply.Note:
console:"omitempty"is only honored byRenderStruct, not by table rendering, so relocating theErrorscolumn inMCPServerStatsdoes not change table output beyond column order.pkg/cli/README.md's type table was updated to match.