diff --git a/pkg/cli/audit.go b/pkg/cli/audit.go index 1da31970af6..31a927c6b21 100644 --- a/pkg/cli/audit.go +++ b/pkg/cli/audit.go @@ -327,6 +327,17 @@ func AuditWorkflowRun(ctx context.Context, runID int64, owner, repo, hostname st fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to analyze firewall logs: %v", err))) } + // Supplement firewall analysis with blocked domains extracted directly from + // agent-stdio.log (e.g., Codex CLI emits "--allow-domains " warnings + // when the sandbox firewall denies a network request). + if agentLogFirewall := extractFirewallFromAgentLog(runOutputDir, verbose); agentLogFirewall != nil { + if firewallAnalysis == nil { + firewallAnalysis = agentLogFirewall + } else { + firewallAnalysis.AddMetrics(agentLogFirewall) + } + } + // Analyze firewall policy artifacts if available (policy-manifest.json + audit.jsonl) policyAnalysis, err := analyzeFirewallPolicy(runOutputDir, verbose) if err != nil && verbose { diff --git a/pkg/cli/audit_report.go b/pkg/cli/audit_report.go index 221f6eca699..677bc3985e9 100644 --- a/pkg/cli/audit_report.go +++ b/pkg/cli/audit_report.go @@ -3,6 +3,7 @@ package cli import ( "bufio" "encoding/json" + "math" "os" "path/filepath" "strconv" @@ -274,6 +275,15 @@ func buildAuditData(processedRun ProcessedRun, metrics LogMetrics, mcpToolUsage WarningCount: run.WarningCount, } + // Populate ActionMinutes from run duration so it is always visible even + // when token/turn metrics are zero (e.g. Codex runs that exit early). + // Use math.Ceil to match the billable-minute rounding used elsewhere. + if run.ActionMinutes > 0 { + metricsData.ActionMinutes = run.ActionMinutes + } else if run.Duration > 0 { + metricsData.ActionMinutes = math.Ceil(run.Duration.Minutes()) + } + // Build job data jobs := sliceutil.Map(processedRun.JobDetails, func(jobDetail JobInfoWithDuration) JobData { job := JobData{ diff --git a/pkg/cli/audit_report_analysis.go b/pkg/cli/audit_report_analysis.go index ed33c5810d6..832934e4f0f 100644 --- a/pkg/cli/audit_report_analysis.go +++ b/pkg/cli/audit_report_analysis.go @@ -143,11 +143,23 @@ func generateFindings(processedRun ProcessedRun, metrics MetricsData, errors []E // Firewall findings if processedRun.FirewallAnalysis != nil && processedRun.FirewallAnalysis.BlockedRequests > 0 { + blockedDomains := processedRun.FirewallAnalysis.GetBlockedDomains() + var desc string + switch { + case len(blockedDomains) == 1: + desc = "Agent attempted to access blocked domain: " + blockedDomains[0] + case len(blockedDomains) > 1 && len(blockedDomains) <= 3: + desc = "Agent attempted to access blocked domains: " + strings.Join(blockedDomains, ", ") + case len(blockedDomains) > 3: + desc = fmt.Sprintf("Agent attempted to access %d blocked domains, including: %s", len(blockedDomains), strings.Join(blockedDomains[:3], ", ")) + default: + desc = fmt.Sprintf("%d network request(s) were blocked by firewall", processedRun.FirewallAnalysis.BlockedRequests) + } findings = append(findings, Finding{ Category: "network", Severity: "medium", Title: "Blocked Network Requests", - Description: fmt.Sprintf("%d network requests were blocked by firewall", processedRun.FirewallAnalysis.BlockedRequests), + Description: desc, Impact: "Blocked requests may indicate missing network permissions or unexpected behavior", }) } @@ -238,13 +250,24 @@ func generateRecommendations(processedRun ProcessedRun, metrics MetricsData, fin }) } - // Recommendations for firewall blocks - if processedRun.FirewallAnalysis != nil && processedRun.FirewallAnalysis.BlockedRequests > 10 { + // Recommendations for firewall blocks – trigger on any block so even a single + // domain denial (e.g. Codex CLI reporting one blocked domain) surfaces an action. + if processedRun.FirewallAnalysis != nil && processedRun.FirewallAnalysis.BlockedRequests > 0 { + blockedDomains := processedRun.FirewallAnalysis.GetBlockedDomains() + var example string + if len(blockedDomains) > 0 { + example = fmt.Sprintf( + "Add the blocked domain(s) to your workflow frontmatter:\n\n```yaml\nnetwork:\n allowed:\n - %s\n```", + strings.Join(blockedDomains, "\n - "), + ) + } else { + example = "Add allowed domains to network configuration or review firewall rules" + } recommendations = append(recommendations, Recommendation{ Priority: "medium", - Action: "Review network access configuration", - Reason: "Many blocked requests suggest missing network permissions", - Example: "Add allowed domains to network configuration or review firewall rules", + Action: "Add blocked domains to the workflow network allow-list", + Reason: "Firewall-blocked domains prevent the agent from completing its tasks", + Example: example, }) } diff --git a/pkg/cli/audit_report_test.go b/pkg/cli/audit_report_test.go index ef699910d0b..c7823f8f2b1 100644 --- a/pkg/cli/audit_report_test.go +++ b/pkg/cli/audit_report_test.go @@ -1534,3 +1534,107 @@ func TestExtractPreAgentStepErrors(t *testing.T) { assert.Contains(t, files, "agent/Run agent", "Should include subdirectory step error") }) } + +func TestBuildAuditDataActionMinutes(t *testing.T) { + // Verify that action_minutes is populated from run.Duration even when + // token/turn metrics are zero (e.g. Codex runs that exit early). + // math.Ceil should be applied, and a pre-set run.ActionMinutes should take precedence. + t.Run("derived from Duration with ceil", func(t *testing.T) { + processedRun := ProcessedRun{ + Run: WorkflowRun{ + DatabaseID: 42, + WorkflowName: "Codex Test", + Status: "completed", + Conclusion: "failure", + Duration: 6*time.Minute + 30*time.Second, // 6.5 minutes → ceil = 7 + TokenUsage: 0, + Turns: 0, + ErrorCount: 1, + WarningCount: 0, + }, + } + + metrics := workflow.LogMetrics{} + auditData := buildAuditData(processedRun, metrics, nil) + + assert.InDelta(t, 7.0, auditData.Metrics.ActionMinutes, 0.01, + "ActionMinutes should be ceil of duration minutes (6.5m → 7)") + assert.Equal(t, 0, auditData.Metrics.TokenUsage, + "TokenUsage should be 0 when no log metrics available") + assert.Equal(t, 0, auditData.Metrics.Turns, + "Turns should be 0 when no log metrics available") + }) + + t.Run("uses pre-set ActionMinutes when available", func(t *testing.T) { + processedRun := ProcessedRun{ + Run: WorkflowRun{ + DatabaseID: 43, + WorkflowName: "Pre-set Test", + Status: "completed", + Conclusion: "success", + Duration: 6 * time.Minute, + ActionMinutes: 8.0, // pre-set by orchestrator, takes precedence + }, + } + + metrics := workflow.LogMetrics{} + auditData := buildAuditData(processedRun, metrics, nil) + + assert.InDelta(t, 8.0, auditData.Metrics.ActionMinutes, 0.01, + "Pre-set ActionMinutes should take precedence over Duration") + }) +} + +func TestGenerateFindingsFirewallWithBlockedDomains(t *testing.T) { + // A single blocked domain should produce a finding naming the domain. + pr := createTestProcessedRun() + fw := &FirewallAnalysis{ + TotalRequests: 1, + BlockedRequests: 1, + AllowedRequests: 0, + RequestsByDomain: map[string]DomainRequestStats{}, + } + fw.SetBlockedDomains([]string{"chatgpt.com"}) + pr.FirewallAnalysis = fw + + findings := generateFindings(pr, MetricsData{}, nil, nil) + + var networkFinding *Finding + for i := range findings { + if findings[i].Category == "network" { + networkFinding = &findings[i] + break + } + } + require.NotNil(t, networkFinding, "Should generate a network finding when firewall blocks a domain") + assert.Contains(t, networkFinding.Description, "chatgpt.com", + "Finding description should name the blocked domain") +} + +func TestGenerateRecommendationsFirewallSingleBlock(t *testing.T) { + // Even a single blocked request (threshold changed from >10 to >0) + // should generate a recommendation with the domain name in the example. + pr := createTestProcessedRun() + fw := &FirewallAnalysis{ + TotalRequests: 1, + BlockedRequests: 1, + AllowedRequests: 0, + RequestsByDomain: map[string]DomainRequestStats{}, + } + fw.SetBlockedDomains([]string{"chatgpt.com"}) + pr.FirewallAnalysis = fw + + findings := []Finding{{Category: "network", Severity: "medium", Title: "Blocked Network Requests"}} + recs := generateRecommendations(pr, MetricsData{}, findings) + + var networkRec *Recommendation + for i := range recs { + if strings.Contains(recs[i].Action, "network") || strings.Contains(recs[i].Action, "blocked") { + networkRec = &recs[i] + break + } + } + require.NotNil(t, networkRec, "Should generate a network recommendation for any firewall block") + assert.Contains(t, networkRec.Example, "chatgpt.com", + "Recommendation example should include the blocked domain name") +} diff --git a/pkg/cli/firewall_log.go b/pkg/cli/firewall_log.go index 9a8b80b363a..b05d89775a1 100644 --- a/pkg/cli/firewall_log.go +++ b/pkg/cli/firewall_log.go @@ -20,6 +20,15 @@ var firewallLogLog = logger.New("cli:firewall_log") // Pre-compiled regexes for firewall log parsing (performance optimization) var ( firewallLogFieldSplitter = regexp.MustCompile(`(?:[^\s"]+|"[^"]*")+`) + + // agentLogAllowDomainsPattern matches Codex CLI firewall warning lines that suggest + // adding a blocked domain to the allow-list. + // Captures the full token after --allow-domains (up to whitespace) to support + // hostnames, protocol prefixes, wildcard patterns, and ports. + // Example: "add --allow-domains chatgpt.com to your command" + // Example: "add --allow-domains chatgpt.com,other.com to your command" + // Example: "add --allow-domains https://api.example.com:443 to your command" + agentLogAllowDomainsPattern = regexp.MustCompile(`--allow-domains\s+([^\s]+)`) ) // Firewall Log Parser @@ -127,7 +136,44 @@ func (f *FirewallAnalysis) AddMetrics(other LogAnalysis) { f.AllowedRequests += otherFirewall.AllowedRequests f.BlockedRequests += otherFirewall.BlockedRequests + // Merge blocked domain lists + if len(otherFirewall.BlockedDomains) > 0 { + domainSet := make(map[string]bool, len(f.BlockedDomains)+len(otherFirewall.BlockedDomains)) + for _, d := range f.BlockedDomains { + domainSet[d] = true + } + for _, d := range otherFirewall.BlockedDomains { + domainSet[d] = true + } + merged := make([]string, 0, len(domainSet)) + for d := range domainSet { + merged = append(merged, d) + } + sort.Strings(merged) + f.SetBlockedDomains(merged) + } + + // Merge allowed domain lists + if len(otherFirewall.AllowedDomains) > 0 { + domainSet := make(map[string]bool, len(f.AllowedDomains)+len(otherFirewall.AllowedDomains)) + for _, d := range f.AllowedDomains { + domainSet[d] = true + } + for _, d := range otherFirewall.AllowedDomains { + domainSet[d] = true + } + merged := make([]string, 0, len(domainSet)) + for d := range domainSet { + merged = append(merged, d) + } + sort.Strings(merged) + f.SetAllowedDomains(merged) + } + // Merge request stats by domain + if f.RequestsByDomain == nil { + f.RequestsByDomain = make(map[string]DomainRequestStats) + } for domain, stats := range otherFirewall.RequestsByDomain { existing := f.RequestsByDomain[domain] existing.Allowed += stats.Allowed @@ -399,3 +445,67 @@ func analyzeMultipleFirewallLogs(logsDir string, verbose bool) (*FirewallAnalysi }, ) } + +// extractFirewallFromAgentLog scans agent-stdio.log for firewall-blocked domain warnings. +// This supplements dedicated proxy firewall logs (e.g., Squid access logs) by extracting +// network-block information from the agent's own output when proxy logs are unavailable. +// +// The Codex CLI emits lines containing "--allow-domains " when a domain is blocked +// by the sandbox firewall. For example: +// +// "[WARN] chatgpt.com is not in the allowed domains. To allow access, add --allow-domains chatgpt.com to your command." +// +// Returns nil when no agent-stdio.log exists or no blocked domains are found. +func extractFirewallFromAgentLog(logsPath string, verbose bool) *FirewallAnalysis { + agentStdioPath := filepath.Clean(filepath.Join(logsPath, "agent-stdio.log")) + content, err := os.ReadFile(agentStdioPath) // #nosec G304 -- path is cleaned via filepath.Clean and logsPath is a trusted run output directory + if err != nil { + // File not present is normal (agent didn't run, or run used a different log path) + firewallLogLog.Printf("No agent-stdio.log found at %s: %v", agentStdioPath, err) + return nil + } + + blockedDomainsSet := make(map[string]bool) + for line := range strings.SplitSeq(string(content), "\n") { + if matches := agentLogAllowDomainsPattern.FindStringSubmatch(line); len(matches) > 1 { + // Domains can be comma-separated in the suggestion + for domain := range strings.SplitSeq(matches[1], ",") { + if d := strings.TrimSpace(domain); d != "" { + blockedDomainsSet[d] = true + } + } + } + } + + if len(blockedDomainsSet) == 0 { + firewallLogLog.Printf("No blocked domains found in agent-stdio.log at %s", agentStdioPath) + return nil + } + + blockedDomains := make([]string, 0, len(blockedDomainsSet)) + for d := range blockedDomainsSet { + blockedDomains = append(blockedDomains, d) + } + sort.Strings(blockedDomains) + + analysis := &FirewallAnalysis{ + TotalRequests: len(blockedDomains), + AllowedRequests: 0, + BlockedRequests: len(blockedDomains), + RequestsByDomain: make(map[string]DomainRequestStats), + } + analysis.SetBlockedDomains(blockedDomains) + for _, d := range blockedDomains { + analysis.RequestsByDomain[d] = DomainRequestStats{Blocked: 1} + } + + firewallLogLog.Printf("Extracted %d firewall-blocked domain(s) from agent-stdio.log: %s", len(blockedDomains), strings.Join(blockedDomains, ", ")) + if verbose { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf( + "Found %d firewall-blocked domain(s) in agent log: %s", + len(blockedDomains), strings.Join(blockedDomains, ", "), + ))) + } + + return analysis +} diff --git a/pkg/cli/firewall_log_test.go b/pkg/cli/firewall_log_test.go index 9a7e2c1188e..4d3b77bd066 100644 --- a/pkg/cli/firewall_log_test.go +++ b/pkg/cli/firewall_log_test.go @@ -794,3 +794,181 @@ func TestParseFirewallLogInternalSquidErrorEntriesDashDash(t *testing.T) { t.Errorf("BlockedDomains: got %v, want [blocked.example.com:443]", analysis.BlockedDomains) } } + +func TestExtractFirewallFromAgentLog(t *testing.T) { + tests := []struct { + name string + logContent string + wantNil bool + wantBlocked []string + wantTotalReqs int + wantBlockedReqs int + }{ + { + name: "single blocked domain from Codex CLI warning", + logContent: `[2026-01-01T10:00:00] thinking +[2026-01-01T10:00:01] [WARN] chatgpt.com is not in the allowed domains. To allow access, add --allow-domains chatgpt.com to your command. +[2026-01-01T10:00:01] agent exiting with code 1`, + wantNil: false, + wantBlocked: []string{"chatgpt.com"}, + wantTotalReqs: 1, + wantBlockedReqs: 1, + }, + { + name: "multiple blocked domains", + logContent: `[2026-01-01T10:00:00] thinking +add --allow-domains openai.com to your command +add --allow-domains anthropic.com to your command`, + wantNil: false, + wantBlocked: []string{"anthropic.com", "openai.com"}, + wantTotalReqs: 2, + wantBlockedReqs: 2, + }, + { + name: "comma-separated domains in single warning", + logContent: `add --allow-domains chatgpt.com,openai.com to access the API`, + wantNil: false, + wantBlocked: []string{"chatgpt.com", "openai.com"}, + wantTotalReqs: 2, + wantBlockedReqs: 2, + }, + { + name: "deduplicated repeated warnings for same domain", + logContent: `add --allow-domains chatgpt.com to your command +add --allow-domains chatgpt.com to your command +add --allow-domains chatgpt.com to your command`, + wantNil: false, + wantBlocked: []string{"chatgpt.com"}, + wantTotalReqs: 1, + wantBlockedReqs: 1, + }, + { + name: "no blocked domains in log", + logContent: `[2026-01-01T10:00:00] thinking +[2026-01-01T10:00:01] tool github.list_pull_requests({"owner":"org","repo":"repo"}) +[2026-01-01T10:00:02] tokens used: 1234`, + wantNil: true, + }, + { + name: "empty log", + logContent: "", + wantNil: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tempDir := testutil.TempDir(t, "agent-log-*") + logPath := filepath.Join(tempDir, "agent-stdio.log") + if err := os.WriteFile(logPath, []byte(tt.logContent), 0644); err != nil { + t.Fatalf("Failed to write agent-stdio.log: %v", err) + } + + result := extractFirewallFromAgentLog(tempDir, false) + + if tt.wantNil { + if result != nil { + t.Errorf("expected nil result, got %+v", result) + } + return + } + + if result == nil { + t.Fatal("expected non-nil result, got nil") + } + if result.TotalRequests != tt.wantTotalReqs { + t.Errorf("TotalRequests: got %d, want %d", result.TotalRequests, tt.wantTotalReqs) + } + if result.BlockedRequests != tt.wantBlockedReqs { + t.Errorf("BlockedRequests: got %d, want %d", result.BlockedRequests, tt.wantBlockedReqs) + } + if result.AllowedRequests != 0 { + t.Errorf("AllowedRequests: got %d, want 0", result.AllowedRequests) + } + if len(result.GetBlockedDomains()) != len(tt.wantBlocked) { + t.Errorf("BlockedDomains length: got %d, want %d (domains: %v)", + len(result.GetBlockedDomains()), len(tt.wantBlocked), result.GetBlockedDomains()) + } else { + for i, d := range tt.wantBlocked { + if result.GetBlockedDomains()[i] != d { + t.Errorf("BlockedDomains[%d]: got %q, want %q", i, result.GetBlockedDomains()[i], d) + } + } + } + }) + } +} + +func TestExtractFirewallFromAgentLogNoFile(t *testing.T) { + tempDir := testutil.TempDir(t, "no-agent-log-*") + // No agent-stdio.log created + result := extractFirewallFromAgentLog(tempDir, false) + if result != nil { + t.Errorf("expected nil when agent-stdio.log is missing, got %+v", result) + } +} + +func TestFirewallAnalysisAddMetricsMergesDomains(t *testing.T) { + base := &FirewallAnalysis{ + TotalRequests: 2, + AllowedRequests: 1, + BlockedRequests: 1, + RequestsByDomain: map[string]DomainRequestStats{}, + } + base.SetBlockedDomains([]string{"blocked-a.com"}) + base.SetAllowedDomains([]string{"allowed-a.com"}) + + other := &FirewallAnalysis{ + TotalRequests: 2, + AllowedRequests: 1, + BlockedRequests: 1, + RequestsByDomain: map[string]DomainRequestStats{}, + } + other.SetBlockedDomains([]string{"blocked-b.com"}) + other.SetAllowedDomains([]string{"allowed-b.com"}) + + base.AddMetrics(other) + + if base.TotalRequests != 4 { + t.Errorf("TotalRequests: got %d, want 4", base.TotalRequests) + } + if base.BlockedRequests != 2 { + t.Errorf("BlockedRequests: got %d, want 2", base.BlockedRequests) + } + if base.AllowedRequests != 2 { + t.Errorf("AllowedRequests: got %d, want 2", base.AllowedRequests) + } + + blocked := base.GetBlockedDomains() + if len(blocked) != 2 || blocked[0] != "blocked-a.com" || blocked[1] != "blocked-b.com" { + t.Errorf("BlockedDomains: got %v, want [blocked-a.com, blocked-b.com]", blocked) + } + + allowed := base.GetAllowedDomains() + if len(allowed) != 2 || allowed[0] != "allowed-a.com" || allowed[1] != "allowed-b.com" { + t.Errorf("AllowedDomains: got %v, want [allowed-a.com, allowed-b.com]", allowed) + } +} + +func TestFirewallAnalysisAddMetricsDeduplicatesDomains(t *testing.T) { + base := &FirewallAnalysis{ + TotalRequests: 1, + BlockedRequests: 1, + RequestsByDomain: map[string]DomainRequestStats{}, + } + base.SetBlockedDomains([]string{"chatgpt.com"}) + + // Same domain added again (e.g. from two different log sources) + other := &FirewallAnalysis{ + TotalRequests: 1, + BlockedRequests: 1, + RequestsByDomain: map[string]DomainRequestStats{}, + } + other.SetBlockedDomains([]string{"chatgpt.com"}) + + base.AddMetrics(other) + + if len(base.GetBlockedDomains()) != 1 { + t.Errorf("BlockedDomains should be deduplicated: got %v", base.GetBlockedDomains()) + } +}