From 48271d175e5afef876f1813a29243640a35f0ee4 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 14 Aug 2026 07:43:15 +0000 Subject: [PATCH 1/3] Initial plan From 9564ef6a404e2b85fbdbff64ed0a9ee01d3329ad Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 14 Aug 2026 07:52:49 +0000 Subject: [PATCH 2/3] refactor: embed aggregated base in MCP failure summary Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/logs_display_fields_test.go | 50 +++++++++++++++++++++++++---- pkg/cli/logs_models.go | 9 ++---- pkg/cli/logs_report_mcp.go | 8 +++-- pkg/cli/logs_report_test.go | 20 +++++++----- 4 files changed, 63 insertions(+), 24 deletions(-) diff --git a/pkg/cli/logs_display_fields_test.go b/pkg/cli/logs_display_fields_test.go index 3ef3a065e57..75929e44fdd 100644 --- a/pkg/cli/logs_display_fields_test.go +++ b/pkg/cli/logs_display_fields_test.go @@ -3,6 +3,7 @@ package cli import ( + "encoding/json" "strings" "testing" @@ -58,11 +59,13 @@ func TestMCPFailureSummaryDisplayFields(t *testing.T) { // Create a MCPFailureSummary with populated Display field summaries := []MCPFailureSummary{ { - ServerName: "github-mcp-server", - Count: 3, - Workflows: []string{"workflow-a", "workflow-b"}, - WorkflowsDisplay: "workflow-a, workflow-b", // This should be rendered - RunIDs: []int64{1, 2, 3}, + ServerName: "github-mcp-server", + AggregatedSummaryBase: AggregatedSummaryBase{ + Count: 3, + Workflows: []string{"workflow-a", "workflow-b"}, + WorkflowsDisplay: "workflow-a, workflow-b", // This should be rendered + RunIDs: []int64{1, 2, 3}, + }, }, } @@ -78,10 +81,43 @@ func TestMCPFailureSummaryDisplayFields(t *testing.T) { if !strings.Contains(output, "Server") { t.Errorf("Server header not found in console output") } - if !strings.Contains(output, "Failures") { - t.Errorf("Failures header not found in console output") + if !strings.Contains(output, "Occurrences") { + t.Errorf("Occurrences header not found in console output") } if !strings.Contains(output, "Workflows") { t.Errorf("Workflows header not found in console output") } } + +// TestMCPFailureSummaryJSONFields verifies that embedding AggregatedSummaryBase keeps +// the shared JSON fields flattened at the MCP failure summary level. +func TestMCPFailureSummaryJSONFields(t *testing.T) { + summary := MCPFailureSummary{ + ServerName: "github-mcp-server", + AggregatedSummaryBase: AggregatedSummaryBase{ + Count: 3, + Workflows: []string{"workflow-a", "workflow-b"}, + RunIDs: []int64{1, 2, 3}, + }, + } + + data, err := json.Marshal(summary) + if err != nil { + t.Fatalf("json.Marshal failed: %v", err) + } + + output := string(data) + for _, expected := range []string{ + `"server_name":"github-mcp-server"`, + `"count":3`, + `"workflows":["workflow-a","workflow-b"]`, + `"run_ids":[1,2,3]`, + } { + if !strings.Contains(output, expected) { + t.Errorf("Expected %s in JSON output: %s", expected, output) + } + } + if strings.Contains(output, "AggregatedSummaryBase") { + t.Errorf("AggregatedSummaryBase should not appear as a nested JSON field: %s", output) + } +} diff --git a/pkg/cli/logs_models.go b/pkg/cli/logs_models.go index 5672dd5686a..b689ccae250 100644 --- a/pkg/cli/logs_models.go +++ b/pkg/cli/logs_models.go @@ -166,7 +166,7 @@ type SkillActivation struct { } // AggregatedSummaryBase holds the shared tail fields that appear byte-for-byte identically -// in MissingToolSummary and MissingDataSummary (and as a subset in MCPFailureSummary). +// in MissingToolSummary, MissingDataSummary, and MCPFailureSummary. // Embedding this struct removes copy-paste drift risk across the aggregated-report types. type AggregatedSummaryBase struct { Count int `json:"count" console:"header:Occurrences"` @@ -185,11 +185,8 @@ type MissingToolSummary struct { // MCPFailureSummary aggregates MCP server failure reports across runs type MCPFailureSummary struct { - ServerName string `json:"server_name" console:"header:Server"` - Count int `json:"count" console:"header:Failures"` - Workflows []string `json:"workflows" console:"-"` // List of workflow names that had this server fail - WorkflowsDisplay string `json:"-" console:"header:Workflows,maxlen:60"` // Formatted display of workflows - RunIDs []int64 `json:"run_ids" console:"-"` // List of run IDs where this server failed + ServerName string `json:"server_name" console:"header:Server"` + AggregatedSummaryBase } // MissingDataSummary aggregates missing data reports across runs diff --git a/pkg/cli/logs_report_mcp.go b/pkg/cli/logs_report_mcp.go index d443c38ba41..f71dae79158 100644 --- a/pkg/cli/logs_report_mcp.go +++ b/pkg/cli/logs_report_mcp.go @@ -27,9 +27,11 @@ func buildMCPFailuresSummary(processedRuns []ProcessedRun) []MCPFailureSummary { func(failure MCPFailureReport) *MCPFailureSummary { return &MCPFailureSummary{ ServerName: failure.ServerName, - Count: 1, - Workflows: []string{failure.WorkflowName}, - RunIDs: []int64{failure.RunID}, + AggregatedSummaryBase: AggregatedSummaryBase{ + Count: 1, + Workflows: []string{failure.WorkflowName}, + RunIDs: []int64{failure.RunID}, + }, } }, // updateSummary: update existing summary with new occurrence diff --git a/pkg/cli/logs_report_test.go b/pkg/cli/logs_report_test.go index fb0eee2bd21..5591a00a4fa 100644 --- a/pkg/cli/logs_report_test.go +++ b/pkg/cli/logs_report_test.go @@ -83,16 +83,20 @@ func TestRenderLogsConsoleUnified(t *testing.T) { }, MCPFailures: []MCPFailureSummary{ { - ServerName: "github-mcp-server", - Count: 2, - Workflows: []string{"workflow-a", "workflow-b"}, - WorkflowsDisplay: "workflow-a, workflow-b", + ServerName: "github-mcp-server", + AggregatedSummaryBase: AggregatedSummaryBase{ + Count: 2, + Workflows: []string{"workflow-a", "workflow-b"}, + WorkflowsDisplay: "workflow-a, workflow-b", + }, }, { - ServerName: "playwright", - Count: 1, - Workflows: []string{"browser-test"}, - WorkflowsDisplay: "browser-test", + ServerName: "playwright", + AggregatedSummaryBase: AggregatedSummaryBase{ + Count: 1, + Workflows: []string{"browser-test"}, + WorkflowsDisplay: "browser-test", + }, }, }, LogsLocation: "/tmp/logs", From fb3f2e381a6bc51b6300b297ab547c8203c5d2b2 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 14 Aug 2026 11:32:03 +0000 Subject: [PATCH 3/3] Preserve MCP failure output schema Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- pkg/cli/README.md | 2 +- pkg/cli/logs_display_fields_test.go | 20 ++++++++++---------- pkg/cli/logs_models.go | 23 ++++++++++++++++++++--- pkg/cli/logs_report.go | 29 ++++++++++++++++++++++++++--- pkg/cli/logs_report_test.go | 27 +++++++++++++++++++++++++++ pkg/console/render.go | 6 ++++++ pkg/console/render_test.go | 17 +++++++++++++++++ 7 files changed, 107 insertions(+), 17 deletions(-) diff --git a/pkg/cli/README.md b/pkg/cli/README.md index 0d09b0c4f79..b0980e7e9f5 100644 --- a/pkg/cli/README.md +++ b/pkg/cli/README.md @@ -624,7 +624,7 @@ This appendix is generated from the current non-test Go source files in this pac | `logs_github_rate_limit_usage.go` | `GitHubRateLimitUsage` | `type GitHubRateLimitUsage struct { TotalRequestsMade int `json:"total_requests_made" console:"header:Total GitHub API Calls,format:number"` CoreConsumed int `json:"core_consumed" console:"header:Core Quota Consumed,format:number"` CoreConsumedSource string `json:"core_consumed_source,omitempty" console:"-"` CoreRemaining int `json:"core_remaining" console:"header:Core Remaining,format:number"` CoreLimit int `json:"core_limit" console:"header:Core Limit,format:number"` Resources []*GitHubRateLimitResourceUsage `json:"resources,omitempty"` }` | GitHubRateLimitUsage provides an aggregated view of GitHub API quota consumed by a single workflow run. | | `logs_models.go` | `AggregatedSummaryBase` | `type AggregatedSummaryBase struct { Count int `json:"count" console:"header:Occurrences"` 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 }` | AggregatedSummaryBase holds the shared tail fields that appear byte-for-byte identically in MissingToolSummary and MissingDataSummary (and as a subset in MCPFailureSummary). | | `logs_models.go` | `JobStep` | `type JobStep struct { Name string `json:"name"` Status string `json:"status,omitempty"` Conclusion string `json:"conclusion,omitempty"` }` | JobStep represents basic information about an individual workflow job step. | -| `logs_models.go` | `MCPFailureSummary` | `type MCPFailureSummary struct { ServerName string `json:"server_name" console:"header:Server"` Count int `json:"count" console:"header:Failures"` Workflows []string `json:"workflows" console:"-"` // List of workflow names that had this server fail WorkflowsDisplay string `json:"-" console:"header:Workflows,maxlen:60"` // Formatted display of workflows RunIDs []int64 `json:"run_ids" console:"-"` // List of run IDs where this server failed }` | MCPFailureSummary aggregates MCP server failure reports across runs | +| `logs_models.go` | `MCPFailureSummary` | `type MCPFailureSummary struct { ServerName string `json:"server_name" console:"header:Server"` AggregatedSummaryBase `console:"-"` }` | MCPFailureSummary aggregates MCP server failure reports across runs | | `logs_models.go` | `ReportProvenance` | `type ReportProvenance struct { Timestamp string `json:"timestamp"` WorkflowName string `json:"workflow_name,omitempty"` // Tracks which workflow reported this RunID int64 `json:"run_id,omitempty"` // Tracks which run reported this ExperimentName string `json:"experiment_name,omitempty"` // Assigned experiment name for this run (if present) Variant string `json:"variant,omitempty"` // Assigned variant value for ExperimentName (if present) }` | ReportProvenance holds the shared provenance fields common to all report record types. | | `logs_orchestrator_types.go` | `LogsDownloadOptions` | `type LogsDownloadOptions struct { WorkflowName string Count int StartDate string EndDate string OutputDir string Engine string Ref string BeforeRunID int64 AfterRunID int64 RepoOverride string Verbose bool ToolGraph bool NoStaged bool FirewallOnly bool NoFirewall bool Parse bool JSONOutput bool TimeoutMinutes int SummaryFile string SafeOutputType string FilteredIntegrity bool EvalsOnly bool Train bool Format string ArtifactSets []string After string ReportFile string }` | LogsDownloadOptions holds parameters for DownloadWorkflowLogs. | | `logs_orchestrator_types.go` | `StdinLogsOptions` | `type StdinLogsOptions struct { RunURLs []string OutputDir string Engine string RepoOverride string Verbose bool ToolGraph bool NoStaged bool FirewallOnly bool NoFirewall bool Parse bool JSONOutput bool Timeout int SummaryFile string SafeOutputType string FilteredIntegrity bool EvalsOnly bool Train bool Format string ReportFile string // ArtifactSets defaults to nil (download all artifacts) when this API is used // programmatically. The CLI passes ["usage"] to match the logs command default. ArtifactSets []string }` | StdinLogsOptions holds parameters for DownloadWorkflowLogsFromStdin. | diff --git a/pkg/cli/logs_display_fields_test.go b/pkg/cli/logs_display_fields_test.go index 75929e44fdd..86f5fcd27b7 100644 --- a/pkg/cli/logs_display_fields_test.go +++ b/pkg/cli/logs_display_fields_test.go @@ -54,7 +54,7 @@ func TestMissingToolSummaryDisplayFields(t *testing.T) { } } -// TestMCPFailureSummaryDisplayFields verifies that Display fields are used by console rendering +// TestMCPFailureSummaryDisplayFields verifies that MCP display fields retain their specific tags. func TestMCPFailureSummaryDisplayFields(t *testing.T) { // Create a MCPFailureSummary with populated Display field summaries := []MCPFailureSummary{ @@ -69,8 +69,7 @@ func TestMCPFailureSummaryDisplayFields(t *testing.T) { }, } - // Render using console.RenderStruct - output := console.RenderStruct(summaries) + output := console.RenderStruct(mcpFailureSummaryDisplays(summaries)) // Verify that Display field is included in output if !strings.Contains(output, "workflow-a, workflow-b") { @@ -81,8 +80,8 @@ func TestMCPFailureSummaryDisplayFields(t *testing.T) { if !strings.Contains(output, "Server") { t.Errorf("Server header not found in console output") } - if !strings.Contains(output, "Occurrences") { - t.Errorf("Occurrences header not found in console output") + if !strings.Contains(output, "Failures") { + t.Errorf("Failures header not found in console output") } if !strings.Contains(output, "Workflows") { t.Errorf("Workflows header not found in console output") @@ -95,9 +94,10 @@ func TestMCPFailureSummaryJSONFields(t *testing.T) { summary := MCPFailureSummary{ ServerName: "github-mcp-server", AggregatedSummaryBase: AggregatedSummaryBase{ - Count: 3, - Workflows: []string{"workflow-a", "workflow-b"}, - RunIDs: []int64{1, 2, 3}, + Count: 3, + Workflows: []string{"workflow-a", "workflow-b"}, + FirstReason: "not part of the MCP failure schema", + RunIDs: []int64{1, 2, 3}, }, } @@ -117,7 +117,7 @@ func TestMCPFailureSummaryJSONFields(t *testing.T) { t.Errorf("Expected %s in JSON output: %s", expected, output) } } - if strings.Contains(output, "AggregatedSummaryBase") { - t.Errorf("AggregatedSummaryBase should not appear as a nested JSON field: %s", output) + if strings.Contains(output, "first_reason") { + t.Errorf("first_reason must not be added to the MCP failure JSON schema: %s", output) } } diff --git a/pkg/cli/logs_models.go b/pkg/cli/logs_models.go index b689ccae250..2cd5c6bdbb9 100644 --- a/pkg/cli/logs_models.go +++ b/pkg/cli/logs_models.go @@ -1,6 +1,7 @@ package cli import ( + "encoding/json" "errors" "time" @@ -166,7 +167,7 @@ type SkillActivation struct { } // AggregatedSummaryBase holds the shared tail fields that appear byte-for-byte identically -// in MissingToolSummary, MissingDataSummary, and MCPFailureSummary. +// 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 { Count int `json:"count" console:"header:Occurrences"` @@ -185,8 +186,24 @@ type MissingToolSummary struct { // MCPFailureSummary aggregates MCP server failure reports across runs type MCPFailureSummary struct { - ServerName string `json:"server_name" console:"header:Server"` - AggregatedSummaryBase + ServerName string `json:"server_name" console:"header:Server"` + AggregatedSummaryBase `console:"-"` +} + +// MarshalJSON preserves the MCP failure JSON schema while sharing aggregation state with +// the other summary types. +func (s MCPFailureSummary) MarshalJSON() ([]byte, error) { + return json.Marshal(struct { + ServerName string `json:"server_name"` + Count int `json:"count"` + Workflows []string `json:"workflows"` + RunIDs []int64 `json:"run_ids"` + }{ + ServerName: s.ServerName, + Count: s.Count, + Workflows: s.Workflows, + RunIDs: s.RunIDs, + }) } // MissingDataSummary aggregates missing data reports across runs diff --git a/pkg/cli/logs_report.go b/pkg/cli/logs_report.go index b94af02f21f..4e71d432d9b 100644 --- a/pkg/cli/logs_report.go +++ b/pkg/cli/logs_report.go @@ -13,6 +13,7 @@ import ( "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/logger" + "github.com/github/gh-aw/pkg/sliceutil" "github.com/github/gh-aw/pkg/timeutil" "github.com/github/gh-aw/pkg/workflow" ) @@ -31,7 +32,7 @@ type LogsData struct { ErrorsAndWarnings []ErrorSummary `json:"errors_and_warnings,omitempty" console:"title:Errors and Warnings,omitempty"` MissingTools []MissingToolSummary `json:"missing_tools,omitempty" console:"title:🛠️ Missing Tools Summary,omitempty"` MissingData []MissingDataSummary `json:"missing_data,omitempty" console:"title:📊 Missing Data Summary,omitempty"` - MCPFailures []MCPFailureSummary `json:"mcp_failures,omitempty" console:"title:⚠️ MCP Server Failures,omitempty"` + MCPFailures []MCPFailureSummary `json:"mcp_failures,omitempty" console:"-"` AccessLog *AccessLogSummary `json:"access_log,omitempty" console:"title:Access Log Analysis,omitempty"` FirewallLog *FirewallLogSummary `json:"firewall_log,omitempty" console:"title:🔥 Firewall Log Analysis,omitempty"` RedactedDomains *RedactedDomainsLogSummary `json:"redacted_domains,omitempty" console:"title:🔒 Redacted URL Domains,omitempty"` @@ -658,8 +659,14 @@ func renderLogsConsoleToWriter(w io.Writer, data LogsData) { reportLog.Printf("Rendering logs data to console: %d runs, %d errors, %d warnings", data.Summary.TotalRuns, data.Summary.TotalErrors, data.Summary.TotalWarnings) - // Use unified console rendering for the entire logs data structure - fmt.Fprint(w, console.RenderStruct(data)) + // Use unified console rendering for the entire logs data structure. + mcpFailures := data.MCPFailures + consoleData := data + consoleData.MCPFailures = nil + fmt.Fprint(w, console.RenderStruct(consoleData)) + fmt.Fprint(w, console.RenderStruct(struct { + MCPFailures []mcpFailureSummaryDisplay `console:"title:⚠️ MCP Server Failures,omitempty"` + }{MCPFailures: mcpFailureSummaryDisplays(mcpFailures)})) // Display concise summary at the end fmt.Fprintln(os.Stderr, "") // Blank line for spacing @@ -688,6 +695,22 @@ func renderLogsConsoleToWriter(w io.Writer, data LogsData) { } } +type mcpFailureSummaryDisplay struct { + ServerName string `console:"header:Server"` + Count int `console:"header:Failures"` + WorkflowsDisplay string `console:"header:Workflows,maxlen:60"` +} + +func mcpFailureSummaryDisplays(summaries []MCPFailureSummary) []mcpFailureSummaryDisplay { + return sliceutil.Map(summaries, func(summary MCPFailureSummary) mcpFailureSummaryDisplay { + return mcpFailureSummaryDisplay{ + ServerName: summary.ServerName, + Count: summary.Count, + WorkflowsDisplay: summary.WorkflowsDisplay, + } + }) +} + // renderLogsConsole outputs the logs data as formatted console output to os.Stdout. func renderLogsConsole(data LogsData) { renderLogsConsoleToWriter(os.Stdout, data) diff --git a/pkg/cli/logs_report_test.go b/pkg/cli/logs_report_test.go index 5591a00a4fa..8dbc9081594 100644 --- a/pkg/cli/logs_report_test.go +++ b/pkg/cli/logs_report_test.go @@ -7,6 +7,7 @@ import ( "encoding/json" "os" "path/filepath" + "strings" "testing" "time" @@ -114,6 +115,32 @@ func TestRenderLogsConsoleUnified(t *testing.T) { renderLogsConsoleToWriter(&buf, data) } +func TestRenderLogsConsoleMCPFailureSchema(t *testing.T) { + var buf bytes.Buffer + renderLogsConsoleToWriter(&buf, LogsData{ + MCPFailures: []MCPFailureSummary{{ + ServerName: "github-mcp-server", + AggregatedSummaryBase: AggregatedSummaryBase{ + Count: 2, + WorkflowsDisplay: "workflow-a, workflow-b", + FirstReasonDisplay: "not part of the MCP failure schema", + }, + }}, + }) + + output := buf.String() + for _, expected := range []string{"MCP Server Failures", "Server", "Failures", "Workflows"} { + if !strings.Contains(output, expected) { + t.Errorf("expected %q in MCP failure console output: %s", expected, output) + } + } + for _, unexpected := range []string{"Occurrences", "First Reason"} { + if strings.Contains(output, unexpected) { + t.Errorf("unexpected %q in MCP failure console output: %s", unexpected, output) + } + } +} + // TestBuildToolUsageSummaryPopulatesDisplay tests that buildToolUsageSummary works correctly func TestBuildToolUsageSummaryPopulatesDisplay(t *testing.T) { processedRuns := []ProcessedRun{ diff --git a/pkg/console/render.go b/pkg/console/render.go index 31c6e1a4d27..a387c993ec7 100644 --- a/pkg/console/render.go +++ b/pkg/console/render.go @@ -127,6 +127,9 @@ func walkInlineFields(val reflect.Value, visit func(field reflect.Value, fieldTy fieldType := typ.Field(i) if fieldType.Anonymous { + if parseConsoleTag(fieldType.Tag.Get("console")).skip { + continue + } if embedded, ok := embeddedStructValue(field); ok { walkInlineFields(embedded, visit) continue @@ -292,6 +295,9 @@ func collectTableFields(t reflect.Type, prefix []int) []tableField { fieldPath[len(prefix)] = i if field.Anonymous { + if parseConsoleTag(field.Tag.Get("console")).skip { + continue + } if embeddedType, ok := embeddedStructType(field.Type); ok { fields = append(fields, collectTableFields(embeddedType, fieldPath)...) continue diff --git a/pkg/console/render_test.go b/pkg/console/render_test.go index b6d7bc1cdd1..2c71e8767fd 100644 --- a/pkg/console/render_test.go +++ b/pkg/console/render_test.go @@ -574,6 +574,23 @@ func TestRenderSlice_EmbeddedStruct(t *testing.T) { assert.Contains(t, output, "disabled", "output should contain second status") } +func TestRenderSlice_SkippedEmbeddedStruct(t *testing.T) { + type Base struct { + Name string `console:"header:Name"` + } + type Extended struct { + Base `console:"-"` + Status string `console:"header:Status"` + } + + output := RenderStruct([]Extended{{Base: Base{Name: "wf-1"}, Status: "active"}}) + + assert.NotContains(t, output, "Name") + assert.NotContains(t, output, "wf-1") + assert.Contains(t, output, "Status") + assert.Contains(t, output, "active") +} + func TestRenderSlice_EmbeddedPointerStruct(t *testing.T) { type Base struct { Name string `console:"header:Name"`