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 3ef3a065e57..86f5fcd27b7 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" @@ -53,21 +54,22 @@ 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{ { - 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}, + }, }, } - // 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") { @@ -85,3 +87,37 @@ func TestMCPFailureSummaryDisplayFields(t *testing.T) { 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"}, + FirstReason: "not part of the MCP failure schema", + 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, "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 5672dd5686a..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" @@ -185,11 +186,24 @@ 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 `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_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..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" @@ -83,16 +84,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", @@ -110,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"`