Skip to content

refactor(logs): extract AggregatedSummaryBase from near-duplicate summary structs #42552

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 5 commits into from
Jun 30, 2026
Merged

refactor(logs): extract AggregatedSummaryBase from near-duplicate summary structs #42552

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
76eee44
Initial plan
Copilot Jun 30, 2026
f0707bb
refactor: extract AggregatedSummaryBase from near-duplicate summary s…
Copilot Jun 30, 2026
c18403c
Merge branch 'main' into copilot/deep-report-extract-shared-base
github-actions[bot] Jun 30, 2026
36663d8
chore: start PR finisher triage
Copilot Jun 30, 2026
108c60a
Merge branch 'main' into copilot/deep-report-extract-shared-base
pelikhan Jun 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/pr-triage-agent.lock.yml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 9 additions & 7 deletions pkg/cli/logs_display_fields_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,15 @@ func TestMissingToolSummaryDisplayFields(t *testing.T) {
// Create a MissingToolSummary with populated Display fields
summaries := []MissingToolSummary{
{
Tool: "test-tool",
Count: 5,
Workflows: []string{"workflow1", "workflow2", "workflow3"},
WorkflowsDisplay: "workflow1, workflow2, workflow3", // This should be rendered
FirstReason: "Tool not found in MCP server",
FirstReasonDisplay: "Tool not found in MCP server", // This should be rendered
RunIDs: []int64{1, 2, 3},
Tool: "test-tool",
AggregatedSummaryBase: AggregatedSummaryBase{
Count: 5,
Workflows: []string{"workflow1", "workflow2", "workflow3"},
WorkflowsDisplay: "workflow1, workflow2, workflow3", // This should be rendered
FirstReason: "Tool not found in MCP server",
FirstReasonDisplay: "Tool not found in MCP server", // This should be rendered
RunIDs: []int64{1, 2, 3},
},
},

@github-actions github-actions Bot Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

}

Expand Down
26 changes: 14 additions & 12 deletions pkg/cli/logs_models.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,15 +149,22 @@ type MCPFailureReport struct {
ReportProvenance
}

// MissingToolSummary aggregates missing tool reports across runs
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).

@github-actions github-actions Bot Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@github-actions github-actions Bot Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

// Embedding this struct removes copy-paste drift risk across the aggregated-report types.
type AggregatedSummaryBase struct {

@github-actions github-actions Bot Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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 occurrences
  • WorkflowOccurrence — what each aggregated item tracks
  • AggregatedOccurrence — 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.

Count int `json:"count" console:"header:Occurrences"`
Workflows []string `json:"workflows" console:"-"` // List of workflow names that reported this tool
Workflows []string `json:"workflows" console:"-"` // List of workflow names
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 tool was reported
RunIDs []int64 `json:"run_ids" console:"-"` // List of run IDs
}

// MissingToolSummary aggregates missing tool reports across runs
type MissingToolSummary struct {
Tool string `json:"tool" console:"header:Tool"`
AggregatedSummaryBase
}
Comment on lines +164 to 168

// MCPFailureSummary aggregates MCP server failure reports across runs
Expand All @@ -171,13 +178,8 @@ type MCPFailureSummary struct {

// 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
}
Comment on lines 179 to 183

// MCPToolUsageSummary aggregates MCP tool usage across all runs
Expand Down
24 changes: 14 additions & 10 deletions pkg/cli/logs_report_errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,11 +77,13 @@ func buildMissingToolsSummary(processedRuns []ProcessedRun) []MissingToolSummary
// createSummary: create new summary for first occurrence
func(tool MissingToolReport) *MissingToolSummary {
return &MissingToolSummary{
Tool: tool.Tool,
Count: 1,
Workflows: []string{tool.WorkflowName},
FirstReason: tool.Reason,
RunIDs: []int64{tool.RunID},
Tool: tool.Tool,
AggregatedSummaryBase: AggregatedSummaryBase{
Count: 1,
Workflows: []string{tool.WorkflowName},
FirstReason: tool.Reason,
RunIDs: []int64{tool.RunID},
},
}
},
// updateSummary: update existing summary with new occurrence
Expand Down Expand Up @@ -121,11 +123,13 @@ func buildMissingDataSummary(processedRuns []ProcessedRun) []MissingDataSummary
// createSummary: create new summary for first occurrence
func(data MissingDataReport) *MissingDataSummary {
return &MissingDataSummary{
DataType: data.DataType,
Count: 1,
Workflows: []string{data.WorkflowName},
FirstReason: data.Reason,
RunIDs: []int64{data.RunID},
DataType: data.DataType,
AggregatedSummaryBase: AggregatedSummaryBase{
Count: 1,
Workflows: []string{data.WorkflowName},
FirstReason: data.Reason,
RunIDs: []int64{data.RunID},
},
}
},
// updateSummary: update existing summary with new occurrence
Expand Down
40 changes: 23 additions & 17 deletions pkg/cli/logs_report_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,20 +59,24 @@ func TestRenderLogsConsoleUnified(t *testing.T) {
},
MissingTools: []MissingToolSummary{
{
Tool: "terraform",
Count: 5,
Workflows: []string{"workflow-a", "workflow-b", "workflow-c"},
WorkflowsDisplay: "workflow-a, workflow-b, workflow-c",
FirstReason: "Infrastructure automation needed",
FirstReasonDisplay: "Infrastructure automation needed",
Tool: "terraform",
AggregatedSummaryBase: AggregatedSummaryBase{
Count: 5,
Workflows: []string{"workflow-a", "workflow-b", "workflow-c"},
WorkflowsDisplay: "workflow-a, workflow-b, workflow-c",
FirstReason: "Infrastructure automation needed",
FirstReasonDisplay: "Infrastructure automation needed",
},
},
{
Tool: "kubectl",
Count: 3,
Workflows: []string{"k8s-deploy"},
WorkflowsDisplay: "k8s-deploy",
FirstReason: "K8s management required",
FirstReasonDisplay: "K8s management required",
Tool: "kubectl",
AggregatedSummaryBase: AggregatedSummaryBase{
Count: 3,
Workflows: []string{"k8s-deploy"},
WorkflowsDisplay: "k8s-deploy",
FirstReason: "K8s management required",
FirstReasonDisplay: "K8s management required",
},
},
},
MCPFailures: []MCPFailureSummary{
Expand Down Expand Up @@ -443,11 +447,13 @@ func TestAggregateSummaryItems(t *testing.T) {
},
func(tool MissingToolReport) *MissingToolSummary {
return &MissingToolSummary{
Tool: tool.Tool,
Count: 1,
Workflows: []string{tool.WorkflowName},
FirstReason: tool.Reason,
RunIDs: []int64{tool.RunID},
Tool: tool.Tool,
AggregatedSummaryBase: AggregatedSummaryBase{
Count: 1,
Workflows: []string{tool.WorkflowName},
FirstReason: tool.Reason,
RunIDs: []int64{tool.RunID},
},
}
},
func(summary *MissingToolSummary, tool MissingToolReport) {
Expand Down
Loading