refactor(logs): extract AggregatedSummaryBase from near-duplicate summary structs - #42552
Conversation
…tructs MissingToolSummary and MissingDataSummary had byte-identical trailing fields (Count, Workflows, WorkflowsDisplay, FirstReason, FirstReasonDisplay, RunIDs). Extract them into an embedded AggregatedSummaryBase to eliminate copy-paste drift risk. MCPFailureSummary is intentionally left inline — its Count and WorkflowsDisplay carry different console tags (header:Failures, maxlen:60) that make a shared base impractical. Closes #42504 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #42552 does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (60 additions detected, threshold is 100). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Refactors the aggregated logs-report summary models to remove duplicated trailing fields by extracting a shared embedded struct, updating the report builders and tests to construct the embedded portion explicitly.
Changes:
- Introduces
AggregatedSummaryBaseto centralize shared aggregated-summary fields used by missing-tool and missing-data summaries. - Updates
buildMissingToolsSummary/buildMissingDataSummaryto initialize the embedded struct in composite literals. - Updates tests to reflect the new composite literal shape.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/logs_models.go | Adds AggregatedSummaryBase and embeds it into MissingToolSummary / MissingDataSummary to eliminate duplicated fields. |
| pkg/cli/logs_report_errors.go | Updates summary construction to populate the embedded base struct fields. |
| pkg/cli/logs_report_test.go | Updates test data and helper literals to use the embedded base struct syntax. |
| pkg/cli/logs_display_fields_test.go | Updates display-field rendering test to populate fields via AggregatedSummaryBase. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Low
| // MissingToolSummary aggregates missing tool reports across runs | ||
| type MissingToolSummary struct { | ||
| Tool string `json:"tool" console:"header:Tool"` | ||
| AggregatedSummaryBase | ||
| } |
| // MissingDataSummary aggregates missing data reports across runs | ||
| type MissingDataSummary struct { | ||
| DataType string `json:"data_type" console:"header:Data Type"` | ||
| Count int `json:"count" console:"header:Occurrences"` | ||
| Workflows []string `json:"workflows" console:"-"` // List of workflow names that reported this data | ||
| WorkflowsDisplay string `json:"-" console:"header:Workflows,maxlen:40"` // Formatted display of workflows | ||
| FirstReason string `json:"first_reason" console:"-"` // Reason from the first occurrence | ||
| FirstReasonDisplay string `json:"-" console:"header:First Reason,maxlen:50"` // Formatted display of first reason | ||
| RunIDs []int64 `json:"run_ids" console:"-"` // List of run IDs where this data was reported | ||
| DataType string `json:"data_type" console:"header:Data Type"` | ||
| AggregatedSummaryBase | ||
| } |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: refactor(logs): extract AggregatedSummaryBase
Clean, well-scoped refactoring with no correctness or security issues.
What works well:
- JSON field order is preserved — Go promotes embedded struct fields exactly as if they were declared inline, so no serialization behaviour changes.
console:struct tags are unaffected; the rendering pipeline will see the same promoted fields and tags.updateSummaryfunctions inlogs_report_errors.gocorrectly access promoted fields (summary.Count,summary.Workflows, etc.) without changes — field promotion handles this transparently.MCPFailureSummaryis correctly excluded with the rationale documented in the PR body.- All composite literals (production + tests) are consistently updated.
One non-blocking suggestion — the AggregatedSummaryBase doc comment's parenthetical mentioning MCPFailureSummary is slightly misleading (see inline comment).
Otherwise LGTM. ✅
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 28.5 AIC · ⌖ 6.57 AIC · ⊞ 4.9K
| type MissingToolSummary struct { | ||
| Tool string `json:"tool" console:"header:Tool"` | ||
| // AggregatedSummaryBase holds the shared tail fields that appear byte-for-byte identically | ||
| // in MissingToolSummary and MissingDataSummary (and as a subset in MCPFailureSummary). |
There was a problem hiding this comment.
The parenthetical (and as a subset in MCPFailureSummary) is misleading — MCPFailureSummary does not embed AggregatedSummaryBase; it keeps its own flat fields with intentionally different console: tags (header:Failures, maxlen:60). A reader who hasn't seen the PR description may assume MCPFailureSummary shares the base, or be confused why it's mentioned here.
Suggested wording:
// AggregatedSummaryBase holds the shared tail fields that appear byte-for-byte identically
// in MissingToolSummary and MissingDataSummary. MCPFailureSummary is intentionally excluded:
// its Count (header:Failures) and WorkflowsDisplay (maxlen:60) carry different console: tags.
// Embedding this struct removes copy-paste drift risk across the aggregated-report types.@copilot please address this.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 80/100 — Excellent
📊 Metrics & Test Classification (3 tests analyzed)
Go: 3 ( Test inflation check:
Build tag compliance: both test files carry Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
There was a problem hiding this comment.
🔎 Code quality review by PR Code Quality Reviewer · 72.7 AIC · ⌖ 7.44 AIC · ⊞ 1.6K
Comment /review to run again
Comments that could not be inline-anchored
pkg/cli/logs_models.go:153
Misleading doc comment: the parenthetical "(and as a subset in MCPFailureSummary)" implies MCPFailureSummary uses this type, but it does not — and it was explicitly left out of this refactor.
<details>
<summary>💡 Suggested fix</summary>
The PR description is clear that MCPFailureSummary was intentionally left flat. The comment should reflect that rather than imply a structural relationship that does not exist in code:
// AggregatedSummaryBase holds the shared tail fields that …
</details>
<details><summary>pkg/cli/logs_report_errors.go:97</summary>
**Incomplete refactor**: the `updateSummary` and `finalizeSummary` closures in `buildMissingToolsSummary` and `buildMissingDataSummary` are still byte-identical — the copy-paste drift risk this PR targets is only half-fixed.
<details>
<summary>💡 Suggested fix</summary>
Since both types now embed `AggregatedSummaryBase`, the shared mutation logic belongs there as methods. This eliminates the remaining duplication and makes future types that embed the base automatically correct:
```go
// on A…
</details>
<details><summary>pkg/cli/logs_models.go:155</summary>
**Unnecessary export**: `AggregatedSummaryBase` is an intra-package implementation detail with no intended external callers — exporting it widens the public API surface for free.
<details>
<summary>💡 Suggested fix</summary>
Rename to `aggregatedSummaryBase` (unexported). External callers of `MissingToolSummary` and `MissingDataSummary` still access all promoted fields (`Count`, `Workflows`, `RunIDs`, etc.) normally — Go promotes them regardless of the embedding type's visibility.
The only c…
</details>
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — leaving 3 comments, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Doc comment accuracy: The
AggregatedSummaryBasecomment mentionsMCPFailureSummaryin a way that implies an embedding relationship that does not exist — see inline comment on line 153. - Naming convention:
Baseis an OOP inheritance idiom; Go prefers names that describe what the type is, not its hierarchy role. - Test symmetry:
MissingDataSummarygained embedding but lacks its ownTestMissingDataSummaryDisplayFieldsto mirror the existingMissingToolSummaryandMCPFailureSummarytests.
Positive Highlights
- ✅ Clear motivation: The PR description and comment both explain why the duplication existed and precisely why
MCPFailureSummaryis excluded (differentconsole:tag values). That level of rationale in the code is rare and valuable. - ✅ Tight scope: Only the two structs that are genuinely byte-identical get the base type; the third is consciously left flat. Good restraint.
- ✅ Test coverage maintained: All four changed files include updated test literals — the refactor does not reduce test confidence.
- ✅ Field promotion works correctly: The
updateSummarycallbacks access promoted fields (summary.Count++,summary.Workflows, etc.) idiomatically without needing to route throughsummary.AggregatedSummaryBase.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 63.8 AIC · ⌖ 7.83 AIC · ⊞ 6.6K
Comment /matt to run again
| type MissingToolSummary struct { | ||
| Tool string `json:"tool" console:"header:Tool"` | ||
| // AggregatedSummaryBase holds the shared tail fields that appear byte-for-byte identically | ||
| // in MissingToolSummary and MissingDataSummary (and as a subset in MCPFailureSummary). |
There was a problem hiding this comment.
[/codebase-design] The doc comment says (and as a subset in MCPFailureSummary), but MCPFailureSummary does not embed AggregatedSummaryBase. A reader unfamiliar with the PR description may infer an embedding relationship that does not exist.
💡 Suggested rewording
// AggregatedSummaryBase holds the shared tail fields that appear byte-for-byte identically
// in MissingToolSummary and MissingDataSummary. MCPFailureSummary carries overlapping fields
// but with different console: tag values (header:Failures, maxlen:60), so it does not embed
// this type.Being explicit about why MCPFailureSummary is excluded makes the design rationale self-documenting, rather than relying on the PR description.
@copilot please address this.
| // AggregatedSummaryBase holds the shared tail fields that appear byte-for-byte identically | ||
| // in MissingToolSummary and MissingDataSummary (and as a subset in MCPFailureSummary). | ||
| // Embedding this struct removes copy-paste drift risk across the aggregated-report types. | ||
| type AggregatedSummaryBase struct { |
There was a problem hiding this comment.
[/codebase-design] The Base suffix implies OOP-style inheritance, which is not idiomatic in Go. Go composition is best named by what the type is, not its role in a hierarchy.
💡 Naming alternatives
Consider a name that describes the data these fields represent:
OccurrenceSummary— the thing being counted across workflow occurrencesWorkflowOccurrence— what each aggregated item tracksAggregatedOccurrence— keeps "aggregated" from the original, drops "Base"
Base will lead future readers to ask "base of what?" whereas a descriptive noun answers the domain question immediately.
@copilot please address this.
| FirstReasonDisplay: "Tool not found in MCP server", // This should be rendered | ||
| RunIDs: []int64{1, 2, 3}, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
[/tdd] TestMissingToolSummaryDisplayFields exists, and there is a symmetric TestMCPFailureSummaryDisplayFields, but no TestMissingDataSummaryDisplayFields. Since MissingDataSummary now embeds AggregatedSummaryBase, a parallel test would confirm that console.RenderStruct promotes the embedded headers (Occurrences, Workflows, First Reason) correctly for MissingDataSummary too.
💡 Suggested test skeleton
func TestMissingDataSummaryDisplayFields(t *testing.T) {
summaries := []MissingDataSummary{
{
DataType: "context",
AggregatedSummaryBase: AggregatedSummaryBase{
Count: 3,
WorkflowsDisplay: "workflow1, workflow2",
FirstReasonDisplay: "context data missing",
},
},
}
output := console.RenderStruct(summaries)
if !strings.Contains(output, "workflow1, workflow2") {
t.Errorf("WorkflowsDisplay not found in output:\n%s", output)
}
if !strings.Contains(output, "context data missing") {
t.Errorf("FirstReasonDisplay not found in output:\n%s", output)
}
}@copilot please address this.
|
@copilot please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Completed. I ran the PR-finisher pass and validated locally on commit |
|
@copilot please run the Feel free to edit this comment to add additional instructions or context for the pr-finisher skill.
|
Ran |
|
🎉 This pull request is included in a new release. Release: |
MissingToolSummaryandMissingDataSummaryinlogs_models.gohad six byte-identical trailing fields, creating silent copy-paste drift risk across the aggregated-report types.Changes
logs_models.go— introducesAggregatedSummaryBaseembeddingCount,Workflows,WorkflowsDisplay,FirstReason,FirstReasonDisplay,RunIDs; bothMissingToolSummaryandMissingDataSummarynow embed it, retaining only their identity field inlinelogs_report_errors.go— updates composite literals inbuildMissingToolsSummary/buildMissingDataSummaryto use embedded-struct syntaxlogs_display_fields_test.go,logs_report_test.go— corresponding literal updatesMCPFailureSummaryis intentionally left flat: itsCount(header:Failures) andWorkflowsDisplay(maxlen:60) carry differentconsole:tags, so it cannot share the same base without overriding behaviour.