Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
40 changes: 8 additions & 32 deletions pkg/cli/audit_diff_render.go
Original file line number Diff line number Diff line change
Expand Up @@ -150,10 +150,7 @@ func renderFirewallDiffMarkdownSection(diff *FirewallDiff) {
for _, entry := range diff.NewDomains {
total := entry.Run2Allowed + entry.Run2Blocked
statusIcon := firewallStatusEmoji(entry.Run2Status)
anomalyTag := ""
if entry.IsAnomaly {
anomalyTag = " ⚠️"
}
anomalyTag := formatAnomalyTag(entry.IsAnomaly)
fmt.Fprintf(os.Stdout, "- %s `%s` (%d requests, %s)%s\n", statusIcon, entry.Domain, total, entry.Run2Status, anomalyTag)
}
fmt.Fprintln(os.Stdout)
Expand All @@ -173,10 +170,7 @@ func renderFirewallDiffMarkdownSection(diff *FirewallDiff) {
for _, entry := range diff.StatusChanges {
icon1 := firewallStatusEmoji(entry.Run1Status)
icon2 := firewallStatusEmoji(entry.Run2Status)
anomalyTag := ""
if entry.IsAnomaly {
anomalyTag = " ⚠️"
}
anomalyTag := formatAnomalyTag(entry.IsAnomaly)
fmt.Fprintf(os.Stdout, "- `%s`: %s %s → %s %s%s\n", entry.Domain, icon1, entry.Run1Status, icon2, entry.Run2Status, anomalyTag)
}
fmt.Fprintln(os.Stdout)
Expand Down Expand Up @@ -205,10 +199,7 @@ func renderMCPToolsDiffMarkdownSection(diff *MCPToolsDiff) {
if len(diff.NewTools) > 0 {
fmt.Fprintf(os.Stdout, "**New tools (%d)**\n", len(diff.NewTools))
for _, entry := range diff.NewTools {
anomalyTag := ""
if entry.IsAnomaly {
anomalyTag = " ⚠️"
}
anomalyTag := formatAnomalyTag(entry.IsAnomaly)
fmt.Fprintf(os.Stdout, "- `%s/%s` (%d calls)%s\n", entry.ServerName, entry.ToolName, entry.Run2CallCount, anomalyTag)
}
fmt.Fprintln(os.Stdout)
Expand All @@ -225,10 +216,7 @@ func renderMCPToolsDiffMarkdownSection(diff *MCPToolsDiff) {
if len(diff.ChangedTools) > 0 {
fmt.Fprintf(os.Stdout, "**Changed tools (%d)**\n", len(diff.ChangedTools))
for _, entry := range diff.ChangedTools {
anomalyTag := ""
if entry.IsAnomaly {
anomalyTag = " ⚠️"
}
anomalyTag := formatAnomalyTag(entry.IsAnomaly)
errInfo := ""
if entry.Run1ErrorCount > 0 || entry.Run2ErrorCount > 0 {
errInfo = fmt.Sprintf(", errors: %d → %d", entry.Run1ErrorCount, entry.Run2ErrorCount)
Expand Down Expand Up @@ -327,10 +315,7 @@ func renderFirewallDiffPrettySection(diff *FirewallDiff) {
}
for _, entry := range diff.NewDomains {
total := entry.Run2Allowed + entry.Run2Blocked
anomalyNote := ""
if entry.IsAnomaly {
anomalyNote = "⚠️ " + entry.AnomalyNote
}
anomalyNote := formatAnomalyNote(entry.IsAnomaly, entry.AnomalyNote)
config.Rows = append(config.Rows, []string{
entry.Domain,
firewallStatusEmoji(entry.Run2Status) + " " + entry.Run2Status,
Expand Down Expand Up @@ -365,10 +350,7 @@ func renderFirewallDiffPrettySection(diff *FirewallDiff) {
Rows: make([][]string, 0, len(diff.StatusChanges)),
}
for _, entry := range diff.StatusChanges {
anomalyNote := ""
if entry.IsAnomaly {
anomalyNote = "⚠️ " + entry.AnomalyNote
}
anomalyNote := formatAnomalyNote(entry.IsAnomaly, entry.AnomalyNote)
config.Rows = append(config.Rows, []string{
entry.Domain,
firewallStatusEmoji(entry.Run1Status) + " " + entry.Run1Status,
Expand Down Expand Up @@ -415,10 +397,7 @@ func renderMCPToolsDiffPrettySection(diff *MCPToolsDiff) {
Rows: make([][]string, 0, len(diff.NewTools)),
}
for _, entry := range diff.NewTools {
anomalyNote := ""
if entry.IsAnomaly {
anomalyNote = "⚠️ " + entry.AnomalyNote
}
anomalyNote := formatAnomalyNote(entry.IsAnomaly, entry.AnomalyNote)
config.Rows = append(config.Rows, []string{
entry.ServerName,
entry.ToolName,
Expand Down Expand Up @@ -452,10 +431,7 @@ func renderMCPToolsDiffPrettySection(diff *MCPToolsDiff) {
Rows: make([][]string, 0, len(diff.ChangedTools)),
}
for _, entry := range diff.ChangedTools {
anomalyNote := ""
if entry.IsAnomaly {
anomalyNote = "⚠️ " + entry.AnomalyNote
}
anomalyNote := formatAnomalyNote(entry.IsAnomaly, entry.AnomalyNote)
config.Rows = append(config.Rows, []string{
entry.ServerName,
entry.ToolName,
Expand Down
9 changes: 2 additions & 7 deletions pkg/cli/audit_expanded.go
Original file line number Diff line number Diff line change
Expand Up @@ -485,10 +485,7 @@ func buildMCPServerHealth(mcpToolUsage *MCPToolUsageData, mcpFailures []MCPFailu
health.TotalRequests += server.RequestCount
health.TotalErrors += server.ErrorCount

errorRate := 0.0
if server.RequestCount > 0 {
errorRate = float64(server.ErrorCount) / float64(server.RequestCount) * 100
}
errorRate := safePercent(server.ErrorCount, server.RequestCount)

status := "✅ healthy"
if _, isFailed := failedServers[server.ServerName]; isFailed {
Expand Down Expand Up @@ -543,9 +540,7 @@ func buildMCPServerHealth(mcpToolUsage *MCPToolUsageData, mcpFailures []MCPFailu
health.HealthySvrs = health.TotalServers - health.FailedSvrs - health.DegradedSvrs

// Calculate overall error rate
if health.TotalRequests > 0 {
health.ErrorRate = float64(health.TotalErrors) / float64(health.TotalRequests) * 100
}
health.ErrorRate = safePercent(health.TotalErrors, health.TotalRequests)

// Sort servers by request count (highest first)
slices.SortFunc(health.Servers, func(a, b MCPServerHealthDetail) int {
Expand Down
18 changes: 18 additions & 0 deletions pkg/cli/audit_math_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,3 +57,21 @@ func formatFloatDelta(value1, value2 float64) string {
}
return fmt.Sprintf("%.3f", delta)
}

// formatAnomalyTag returns a warning emoji suffix for markdown rendering

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.

[/improve-codebase-architecture] formatAnomalyTag and formatAnomalyNote are string-formatting helpers, not arithmetic — placing them in audit_math_helpers.go stretches the file beyond its conceptual boundary. The math helpers file already contains safePercent, formatPercent, formatVolumeChange, and formatFloatDelta; the anomaly helpers belong in a separate audit_format_helpers.go alongside renderFirewallDiff* concerns, or at least noted as a deliberate placement choice.

💡 Suggestion

Move both functions to pkg/cli/audit_format_helpers.go (new file) so that audit_math_helpers.go stays focused on numeric calculations and the anomaly formatting has a natural home near the render functions that use it.

// when isAnomaly is true, otherwise returns an empty string.
func formatAnomalyTag(isAnomaly bool) string {
if isAnomaly {
return " ⚠️"
}
return ""
}

// formatAnomalyNote returns a formatted anomaly note for table rendering
// with a warning emoji prefix when isAnomaly is true, otherwise returns an empty string.
func formatAnomalyNote(isAnomaly bool, anomalyNote string) string {
if isAnomaly {
return "⚠️ " + anomalyNote
}
return ""
}
13 changes: 4 additions & 9 deletions pkg/cli/audit_report.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"github.com/github/gh-aw/pkg/github"
"github.com/github/gh-aw/pkg/logger"
"github.com/github/gh-aw/pkg/sliceutil"
"github.com/github/gh-aw/pkg/stringutil"
"github.com/github/gh-aw/pkg/timeutil"
)

Expand Down Expand Up @@ -687,9 +688,7 @@ func extractPreAgentStepErrors(logsPath string) []ErrorInfo {

if len(errorLines) > 0 {
message := strings.Join(errorLines, "\n")
if len(message) > maxMessageLen {
message = message[:maxMessageLen] + "..."
}
message = stringutil.Truncate(message, maxMessageLen)
auditReportLog.Printf("Extracted ##[error] annotations from flat job log %s (job %d)", jobName, num)
errorAnnotations = append(errorAnnotations, ErrorInfo{
Type: "step_failure",
Expand Down Expand Up @@ -743,9 +742,7 @@ func extractPreAgentStepErrors(logsPath string) []ErrorInfo {

if len(errorLines) > 0 {
message := strings.Join(errorLines, "\n")
if len(message) > maxMessageLen {
message = message[:maxMessageLen] + "..."
}
message = stringutil.Truncate(message, maxMessageLen)
auditReportLog.Printf("Extracted ##[error] annotations from %s (step %d)", stepKey, num)
errorAnnotations = append(errorAnnotations, ErrorInfo{
Type: "step_failure",
Expand Down Expand Up @@ -778,9 +775,7 @@ func extractPreAgentStepErrors(logsPath string) []ErrorInfo {
return nil
}

if len(message) > maxMessageLen {
message = message[:maxMessageLen] + "..."
}
message = stringutil.Truncate(message, maxMessageLen)

auditReportLog.Printf("Extracted pre-agent step error from %s (step %d) as fallback", lastStep.stepKey, lastStep.num)
return []ErrorInfo{{
Expand Down
6 changes: 2 additions & 4 deletions pkg/cli/audit_report_analysis.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (

"github.com/github/gh-aw/pkg/console"
"github.com/github/gh-aw/pkg/sliceutil"
"github.com/github/gh-aw/pkg/stringutil"
"github.com/github/gh-aw/pkg/timeutil"
)

Expand Down Expand Up @@ -52,10 +53,7 @@ func generateFindings(processedRun ProcessedRun, metrics MetricsData, errors []E
// Append a truncated first error message to help quickly identify the root cause.
// Keep descriptions short enough to be useful in a key findings summary.
const maxErrMsgLen = 200
msg := errors[0].Message
if len(msg) > maxErrMsgLen {
msg = msg[:maxErrMsgLen] + "..."
}
msg := stringutil.Truncate(errors[0].Message, maxErrMsgLen)
desc += ": " + msg
}
}
Expand Down
26 changes: 6 additions & 20 deletions pkg/cli/deps_report.go
Original file line number Diff line number Diff line change
Expand Up @@ -119,10 +119,7 @@ func DisplayDependencyReport(report *DependencyReport) {
fmt.Fprintf(os.Stderr, "Outdated: %d (%.0f%%)\n", len(report.Outdated), outdatedPercentage)
fmt.Fprintf(os.Stderr, "Security advisories: %d\n", len(report.Advisories))

v0Percentage := 0.0
if report.TotalDeps > 0 {
v0Percentage = float64(report.V0Count) / float64(report.TotalDeps) * 100
}
v0Percentage := safePercent(report.V0Count, report.TotalDeps)
fmt.Fprintf(os.Stderr, "v0.x dependencies: %d (%.0f%%)", report.V0Count, v0Percentage)
if v0Percentage > 30 {
fmt.Fprintf(os.Stderr, " ⚠️")
Expand Down Expand Up @@ -157,16 +154,10 @@ func DisplayDependencyReport(report *DependencyReport) {
}
fmt.Fprintln(os.Stderr, "")

v1Percentage := 0.0
if report.TotalDeps > 0 {
v1Percentage = float64(report.V1PlusCount) / float64(report.TotalDeps) * 100
}
v1Percentage := safePercent(report.V1PlusCount, report.TotalDeps)
fmt.Fprintf(os.Stderr, "v1.x (stable): %d (%.0f%%)\n", report.V1PlusCount, v1Percentage)

v2Percentage := 0.0
if report.TotalDeps > 0 {
v2Percentage = float64(report.V2PlusCount) / float64(report.TotalDeps) * 100
}
v2Percentage := safePercent(report.V2PlusCount, report.TotalDeps)
fmt.Fprintf(os.Stderr, "v2+ (mature): %d (%.0f%%)\n", report.V2PlusCount, v2Percentage)
fmt.Fprintln(os.Stderr, "")

Expand Down Expand Up @@ -201,14 +192,9 @@ func DisplayDependencyReportJSON(report *DependencyReport) error {
outdatedPercentage = float64(len(report.Outdated)) / float64(report.DirectDeps) * 100
}

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.

[/zoom-out] outdatedPercentage immediately above the migrated block still uses the verbose zero-guard pattern that this PR eliminates elsewhere. safePercent(len(report.Outdated), report.DirectDeps) would make the function internally consistent with the three lines directly below it.

💡 Suggested change
// Before (unchanged by this PR, but inconsistent):
outdatedPercentage := 0.0
if report.DirectDeps > 0 {
    outdatedPercentage = float64(len(report.Outdated)) / float64(report.DirectDeps) * 100
}

// After:
outdatedPercentage := safePercent(len(report.Outdated), report.DirectDeps)

The same pattern also applies to the outdatedPercentage guard in DisplayDependencyReport (not in this diff).

v0Percentage := 0.0
v1Percentage := 0.0
v2Percentage := 0.0
if report.TotalDeps > 0 {
v0Percentage = float64(report.V0Count) / float64(report.TotalDeps) * 100
v1Percentage = float64(report.V1PlusCount) / float64(report.TotalDeps) * 100
v2Percentage = float64(report.V2PlusCount) / float64(report.TotalDeps) * 100
}
v0Percentage := safePercent(report.V0Count, report.TotalDeps)
v1Percentage := safePercent(report.V1PlusCount, report.TotalDeps)
v2Percentage := safePercent(report.V2PlusCount, report.TotalDeps)

// Build JSON-friendly output structure
output := map[string]any{
Expand Down
5 changes: 1 addition & 4 deletions pkg/cli/experiments_analyze_statistics.go
Original file line number Diff line number Diff line change
Expand Up @@ -142,10 +142,7 @@ func computeExperimentAnalysis(exp ExperimentVariantStats, cfg *workflow.Experim
// Populate per-variant entries.
for i, name := range variantNames {
count := exp.Variants[name]
obsPct := 0.0
if exp.Total > 0 {
obsPct = float64(count) / float64(exp.Total) * 100
}
obsPct := safePercent(count, exp.Total)
a.Variants = append(a.Variants, VariantAnalysis{
Name: name,
Count: count,
Expand Down
7 changes: 2 additions & 5 deletions pkg/cli/health_metrics.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,10 +93,7 @@ func CalculateWorkflowHealth(workflowName string, runs []WorkflowRun, threshold
}

totalRuns := len(runs)
successRate := 0.0
if totalRuns > 0 {
successRate = float64(successCount) / float64(totalRuns) * 100
}
successRate := safePercent(successCount, totalRuns)

avgDuration := time.Duration(durationStats.Mean())
avgTokens := int(tokenStats.Mean())
Expand Down Expand Up @@ -175,7 +172,7 @@ func calculateSuccessRate(runs []WorkflowRun) float64 {
}
}

return float64(successCount) / float64(len(runs)) * 100
return safePercent(successCount, len(runs))

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] The NaN→0 behaviour change in calculateSuccessRate is the PR's only real correctness fix, but there is no regression test pinning the empty-runs case. Before this PR the function returned NaN for an empty slice; after the fix it returns 0.0. A missing test means this could silently regress.

💡 Suggested test
func TestCalculateSuccessRateEmpty(t *testing.T) {
    got := calculateSuccessRate(nil)
    if got != 0.0 {
        t.Errorf("expected 0.0 for empty runs, got %v", got)
    }
}

The fix is correct and important — zero-guarding before the division prevents NaN propagation into health summary structs. Anchoring it with a test makes the intent explicit and prevents future regression.

}

// CalculateHealthSummary calculates aggregated health metrics across all workflows
Expand Down
14 changes: 4 additions & 10 deletions pkg/cli/logs_report_errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import (
"cmp"
"slices"
"strings"

"github.com/github/gh-aw/pkg/sliceutil"
)

// ErrorSummary contains aggregated error/warning statistics
Expand All @@ -18,14 +20,6 @@ type ErrorSummary struct {
PatternID string `json:"pattern_id,omitempty" console:"-"`
}

// addUniqueWorkflow adds a workflow to the list if it's not already present
func addUniqueWorkflow(workflows []string, workflow string) []string {
if slices.Contains(workflows, workflow) {
return workflows
}
return append(workflows, workflow)
}

// aggregateSummaryItems is a generic helper that aggregates items from processed runs into summaries
// It handles the common pattern of grouping by key, counting occurrences, tracking unique workflows, and collecting run IDs
func aggregateSummaryItems[TItem any, TSummary any](
Expand Down Expand Up @@ -93,7 +87,7 @@ func buildMissingToolsSummary(processedRuns []ProcessedRun) []MissingToolSummary
// updateSummary: update existing summary with new occurrence
func(summary *MissingToolSummary, tool MissingToolReport) {
summary.Count++
summary.Workflows = addUniqueWorkflow(summary.Workflows, tool.WorkflowName)
summary.Workflows = sliceutil.MergeUnique(summary.Workflows, tool.WorkflowName)
summary.RunIDs = append(summary.RunIDs, tool.RunID)
},
// finalizeSummary: populate display fields for console rendering
Expand Down Expand Up @@ -137,7 +131,7 @@ func buildMissingDataSummary(processedRuns []ProcessedRun) []MissingDataSummary
// updateSummary: update existing summary with new occurrence
func(summary *MissingDataSummary, data MissingDataReport) {
summary.Count++
summary.Workflows = addUniqueWorkflow(summary.Workflows, data.WorkflowName)
summary.Workflows = sliceutil.MergeUnique(summary.Workflows, data.WorkflowName)
summary.RunIDs = append(summary.RunIDs, data.RunID)
},
// finalizeSummary: populate display fields for console rendering
Expand Down
3 changes: 2 additions & 1 deletion pkg/cli/logs_report_mcp.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"strings"
"time"

"github.com/github/gh-aw/pkg/sliceutil"
"github.com/github/gh-aw/pkg/timeutil"
)

Expand Down Expand Up @@ -34,7 +35,7 @@ func buildMCPFailuresSummary(processedRuns []ProcessedRun) []MCPFailureSummary {
// updateSummary: update existing summary with new occurrence
func(summary *MCPFailureSummary, failure MCPFailureReport) {
summary.Count++
summary.Workflows = addUniqueWorkflow(summary.Workflows, failure.WorkflowName)
summary.Workflows = sliceutil.MergeUnique(summary.Workflows, failure.WorkflowName)
summary.RunIDs = append(summary.RunIDs, failure.RunID)
},
// finalizeSummary: populate display fields for console rendering
Expand Down
6 changes: 4 additions & 2 deletions pkg/cli/logs_report_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import (
"path/filepath"
"testing"
"time"

"github.com/github/gh-aw/pkg/sliceutil"
)

// TestRenderLogsConsoleUnified tests the unified console rendering
Expand Down Expand Up @@ -222,7 +224,7 @@ func TestAddUniqueWorkflow(t *testing.T) {

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
result := addUniqueWorkflow(tt.workflows, tt.workflow)
result := sliceutil.MergeUnique(tt.workflows, tt.workflow)

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.

[/zoom-out] TestAddUniqueWorkflow references a function that no longer exists. The test now exercises sliceutil.MergeUnique, which presumably has its own unit tests in pkg/sliceutil. Consider renaming to TestSliceutil_MergeUnique_Deduplication or removing it entirely if the behaviour is already covered downstream — stale names in tests mislead future contributors searching for coverage.

💡 Suggested fix

Option A — rename in place:

// TestMergeUniqueDeduplicatesWorkflows verifies that MergeUnique correctly
// deduplicates workflow names as previously guaranteed by addUniqueWorkflow.
func TestMergeUniqueDeduplicatesWorkflows(t *testing.T) {

Option B — remove it and rely on pkg/sliceutil tests directly.

if len(result) != len(tt.expected) {
t.Errorf("Expected length %d, got %d", len(tt.expected), len(result))
}
Expand Down Expand Up @@ -429,7 +431,7 @@ func TestAggregateSummaryItems(t *testing.T) {
},
func(summary *MissingToolSummary, tool MissingToolReport) {
summary.Count++
summary.Workflows = addUniqueWorkflow(summary.Workflows, tool.WorkflowName)
summary.Workflows = sliceutil.MergeUnique(summary.Workflows, tool.WorkflowName)
summary.RunIDs = append(summary.RunIDs, tool.RunID)
},
func(summary *MissingToolSummary) {
Expand Down
6 changes: 2 additions & 4 deletions pkg/cli/mcp_inspect_mcp.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"github.com/github/gh-aw/pkg/console"
"github.com/github/gh-aw/pkg/logger"
"github.com/github/gh-aw/pkg/parser"
"github.com/github/gh-aw/pkg/stringutil"
"github.com/modelcontextprotocol/go-sdk/mcp"
)

Expand Down Expand Up @@ -386,10 +387,7 @@ func displayServerCapabilities(info *parser.MCPServerInfo, toolFilter string) {
rows := make([][]string, 0, len(info.Resources))

for _, resource := range info.Resources {
description := resource.Description
if len(description) > 40 {
description = description[:37] + "..."
}
description := stringutil.Truncate(resource.Description, 40)

mimeType := resource.MIMEType
if mimeType == "" {
Expand Down
Loading
Loading