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
11 changes: 10 additions & 1 deletion pkg/cli/access_log.go
Original file line number Diff line number Diff line change
Expand Up @@ -157,13 +157,22 @@ func parseSquidLogLine(line string) (*AccessLogEntry, error) {
func analyzeAccessLogs(runDir string, verbose bool) (*DomainAnalysis, error) {
accessLogLog.Printf("Analyzing access logs in: %s", runDir)

// Check for access log files in access.log directory
// Check for access log files in access.log directory (legacy path)
accessLogsDir := filepath.Join(runDir, "access.log")
if _, err := os.Stat(accessLogsDir); err == nil {
accessLogLog.Printf("Found access logs directory: %s", accessLogsDir)
return analyzeMultipleAccessLogs(accessLogsDir, verbose)
}

// Check for access logs in sandbox/firewall/logs/ directory (new path after artifact download)
// Firewall logs are uploaded from /tmp/gh-aw/sandbox/firewall/logs/ and the common parent
// /tmp/gh-aw/ is stripped during artifact upload, resulting in sandbox/firewall/logs/ after download
sandboxFirewallLogsDir := filepath.Join(runDir, "sandbox", "firewall", "logs")
if _, err := os.Stat(sandboxFirewallLogsDir); err == nil {
accessLogLog.Printf("Found firewall logs directory: %s", sandboxFirewallLogsDir)
return analyzeMultipleAccessLogs(sandboxFirewallLogsDir, verbose)
}

// No access logs found
accessLogLog.Printf("No access logs directory found in: %s", runDir)
if verbose {
Expand Down
18 changes: 18 additions & 0 deletions pkg/cli/access_log_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,24 @@ func TestAnalyzeAccessLogsDirectory(t *testing.T) {
require.NoError(t, err, "should not error when no logs present")
assert.Nil(t, analysis, "should return nil when no logs found")
})

t.Run("access logs in sandbox/firewall/logs/ (new path)", func(t *testing.T) {
// Test case 3: Access logs in sandbox/firewall/logs/ directory after artifact download
sandboxLogsDir := filepath.Join(tempDir, "run3", "sandbox", "firewall", "logs")
err := os.MkdirAll(sandboxLogsDir, 0755)
require.NoError(t, err, "should create sandbox/firewall/logs directory")

fetchLogContent := `1701234567.123 180 192.168.1.100 TCP_MISS/200 1234 GET http://example.com/api/data - HIER_DIRECT/93.184.216.34 text/html
1701234568.456 250 192.168.1.100 TCP_HIT/200 5678 GET http://api.github.com/repos - HIER_DIRECT/140.82.112.6 application/json`
fetchLogPath := filepath.Join(sandboxLogsDir, "access-1.log")
err = os.WriteFile(fetchLogPath, []byte(fetchLogContent), 0644)
require.NoError(t, err, "should create test access log in sandbox path")

analysis, err := analyzeAccessLogs(filepath.Join(tempDir, "run3"), false)
require.NoError(t, err, "should analyze access logs from sandbox path")
require.NotNil(t, analysis, "should return analysis for logs in sandbox path")
assert.Equal(t, 2, analysis.TotalRequests, "should count requests from sandbox path")
})
}

func TestExtractDomainFromURL(t *testing.T) {
Expand Down
14 changes: 13 additions & 1 deletion pkg/cli/firewall_log.go
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,19 @@ func analyzeFirewallLogs(runDir string, verbose bool) (*FirewallAnalysis, error)
// Look for firewall logs in the run directory
// The logs could be in several locations depending on how they were uploaded

// First, check for directories starting with squid-logs or firewall-logs
// First, check for sandbox/firewall/logs/ directory (new path after artifact download)
// Firewall logs are uploaded from /tmp/gh-aw/sandbox/firewall/logs/ and the common parent
// /tmp/gh-aw/ is stripped during artifact upload, resulting in sandbox/firewall/logs/ after download
sandboxFirewallLogsDir := filepath.Join(runDir, "sandbox", "firewall", "logs")
Comment on lines +314 to +317
Copy link

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

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

The function header comment above still says firewall logs are stored under /tmp/gh-aw/squid-logs-{workflow-name}/, but this function now prefers /tmp/gh-aw/sandbox/firewall/logs/ (downloaded as sandbox/firewall/logs/). Update the comment to describe both layouts (legacy squid-logs* and the sandbox/firewall/logs path) so future readers don’t assume only the legacy structure exists.

Copilot uses AI. Check for mistakes.
if _, err := os.Stat(sandboxFirewallLogsDir); err == nil {
firewallLogLog.Printf("Found firewall logs directory: sandbox/firewall/logs")
if verbose {
fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Found firewall logs directory: sandbox/firewall/logs"))
}
return analyzeMultipleFirewallLogs(sandboxFirewallLogsDir, verbose)
Copy link

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

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

When sandbox/firewall/logs is present, this returns analyzeMultipleFirewallLogs(sandboxFirewallLogsDir), which currently aggregates ".log". That will also pick up squid access logs (e.g., access-.log) if they coexist in this directory, and parseFirewallLogLine will incorrectly treat them as firewall entries (it only validates timestamp). Filter the files (prefer firewall-.log when present, or explicitly exclude access-.log), or harden parseFirewallLogLine to validate expected firewall fields (e.g., status code numeric and/or decision format) so mixed-format directories don’t corrupt firewall metrics.

Suggested change
return analyzeMultipleFirewallLogs(sandboxFirewallLogsDir, verbose)
// Use sandbox/firewall/logs as the run directory so that the existing
// per-file filtering logic below (which excludes access-*.log) is applied.
runDir = sandboxFirewallLogsDir

Copilot uses AI. Check for mistakes.
}

// Second, check for directories starting with squid-logs or firewall-logs (legacy paths)
// The actual directories may have workflow-specific suffixes like:
// - squid-logs-smoke-copilot-firewall
// - squid-logs-changeset-generator
Expand Down
47 changes: 47 additions & 0 deletions pkg/cli/firewall_log_integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -237,3 +237,50 @@ func TestFirewallLogSummaryBuilding(t *testing.T) {
t.Error("workflow-1 not found in ByWorkflow")
}
}

// TestAnalyzeFirewallLogsFromSandboxPath tests that firewall logs can be found
// in sandbox/firewall/logs/ directory (the path after artifact download)
func TestAnalyzeFirewallLogsFromSandboxPath(t *testing.T) {
// Create temp directory for test
runDir := testutil.TempDir(t, "firewall-test-*")

// Create sandbox/firewall/logs directory (path after artifact download)
firewallLogsDir := filepath.Join(runDir, "sandbox", "firewall", "logs")
err := os.MkdirAll(firewallLogsDir, 0755)
if err != nil {
t.Fatalf("should create sandbox/firewall/logs directory: %v", err)
}

// Create test firewall log file
logContent := `# Firewall access log
1701234567.123 172.30.0.20:35288 api.github.com:443 140.82.112.22:443 1.1 CONNECT 200 TCP_TUNNEL:HIER_DIRECT api.github.com:443 "-"
1701234568.456 172.30.0.20:35289 api.npmjs.org:443 151.101.0.162:443 1.1 CONNECT 200 TCP_TUNNEL:HIER_DIRECT api.npmjs.org:443 "-"
1701234569.789 172.30.0.20:35290 blocked.example.com:443 0.0.0.0:0 1.1 CONNECT 403 NONE_NONE:HIER_NONE blocked.example.com:443 "-"
1701234570.012 172.30.0.20:35291 blocked.test.com:443 0.0.0.0:0 1.1 CONNECT 403 NONE_NONE:HIER_NONE blocked.test.com:443 "-"
`
logPath := filepath.Join(firewallLogsDir, "firewall-access.log")
err = os.WriteFile(logPath, []byte(logContent), 0644)
if err != nil {
t.Fatalf("should write test firewall log: %v", err)
}

// Test the analysis
analysis, err := analyzeFirewallLogs(runDir, true)
if err != nil {
t.Fatalf("analyzeFirewallLogs should succeed: %v", err)
}
if analysis == nil {
t.Fatal("Analysis should not be nil")
}

// Verify results
if analysis.TotalRequests != 4 {
t.Errorf("TotalRequests: got %d, want 4", analysis.TotalRequests)
}
if analysis.AllowedRequests != 2 {
t.Errorf("AllowedRequests: got %d, want 2", analysis.AllowedRequests)
}
if analysis.BlockedRequests != 2 {
t.Errorf("BlockedRequests: got %d, want 2", analysis.BlockedRequests)
}
}
27 changes: 24 additions & 3 deletions pkg/cli/gateway_logs.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,12 +80,21 @@ type GatewayMetrics struct {

// parseGatewayLogs parses a gateway.jsonl file and extracts metrics
func parseGatewayLogs(logDir string, verbose bool) (*GatewayMetrics, error) {
// Try root directory first (for older logs where gateway.jsonl was in the root)
gatewayLogPath := filepath.Join(logDir, "gateway.jsonl")

// Check if gateway.jsonl exists
// Check if gateway.jsonl exists in root
if _, err := os.Stat(gatewayLogPath); os.IsNotExist(err) {
gatewayLogsLog.Printf("gateway.jsonl not found at: %s", gatewayLogPath)
return nil, fmt.Errorf("gateway.jsonl not found")
// Try mcp-logs subdirectory (new path after artifact download)
// Gateway logs are uploaded from /tmp/gh-aw/mcp-logs/gateway.jsonl and the common parent
// /tmp/gh-aw/ is stripped during artifact upload, resulting in mcp-logs/gateway.jsonl after download
mcpLogsPath := filepath.Join(logDir, "mcp-logs", "gateway.jsonl")
if _, err := os.Stat(mcpLogsPath); os.IsNotExist(err) {
gatewayLogsLog.Printf("gateway.jsonl not found at: %s or %s", gatewayLogPath, mcpLogsPath)
return nil, fmt.Errorf("gateway.jsonl not found")
}
gatewayLogPath = mcpLogsPath
gatewayLogsLog.Printf("Found gateway.jsonl in mcp-logs subdirectory")
}
Comment on lines +83 to 98
Copy link

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

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

These os.Stat checks only branch on os.IsNotExist(err). If Stat returns another error (notably permission denied), the code will treat the file as present (and even log "Found" in parseGatewayLogs) and then fail later on Open with a less clear flow. Handle non-not-exist errors explicitly (returning a wrapped error), and only accept the path when Stat succeeds, so permission issues don’t masquerade as successful discovery and you still get the intended fallback behavior.

Copilot uses AI. Check for mistakes.

gatewayLogsLog.Printf("Parsing gateway.jsonl from: %s", gatewayLogPath)
Expand Down Expand Up @@ -399,7 +408,19 @@ func extractMCPToolUsageData(logDir string, verbose bool) (*MCPToolUsageData, er
}

// Read gateway.jsonl again to get individual tool call records
// Try root directory first (for older logs where gateway.jsonl was in the root)
gatewayLogPath := filepath.Join(logDir, "gateway.jsonl")

// Check if gateway.jsonl exists in root
if _, err := os.Stat(gatewayLogPath); os.IsNotExist(err) {
// Try mcp-logs subdirectory (new path after artifact download)
mcpLogsPath := filepath.Join(logDir, "mcp-logs", "gateway.jsonl")
if _, err := os.Stat(mcpLogsPath); os.IsNotExist(err) {
return nil, fmt.Errorf("gateway.jsonl not found")
}
gatewayLogPath = mcpLogsPath
}
Comment on lines 410 to +422
Copy link

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

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

The gateway.jsonl path resolution logic is duplicated here and in parseGatewayLogs(). This creates a maintenance risk where future path changes update one place but not the other. Consider extracting a single helper (e.g., resolveGatewayLogPath(logDir)) and reusing it in both functions, or have parseGatewayLogs return the resolved path alongside metrics.

Copilot uses AI. Check for mistakes.
Comment on lines 410 to +422
Copy link

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

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

The new mcp-logs/gateway.jsonl fallback in extractMCPToolUsageData() isn’t covered by tests: existing MCP tool usage tests write gateway.jsonl at the run root. Add a test that writes gateway.jsonl under mcp-logs/ and asserts extractMCPToolUsageData() returns populated Summary/ToolCalls, so this path doesn’t regress independently from parseGatewayLogs().

Copilot uses AI. Check for mistakes.

file, err := os.Open(gatewayLogPath)
if err != nil {
return nil, fmt.Errorf("failed to open gateway.jsonl: %w", err)
Expand Down
33 changes: 33 additions & 0 deletions pkg/cli/gateway_logs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -452,3 +452,36 @@ func TestGatewayLogsParsingIntegration(t *testing.T) {
assert.False(t, metrics.EndTime.IsZero())
assert.True(t, metrics.EndTime.After(metrics.StartTime))
}

func TestParseGatewayLogsFromMCPLogsSubdirectory(t *testing.T) {
// Create temp directory for test
tmpDir := t.TempDir()

// Create mcp-logs subdirectory (path after artifact download)
mcpLogsDir := filepath.Join(tmpDir, "mcp-logs")
err := os.MkdirAll(mcpLogsDir, 0755)
require.NoError(t, err, "should create mcp-logs directory")

// Create test gateway.jsonl in mcp-logs subdirectory
testLogContent := `{"timestamp":"2024-01-15T10:00:00Z","level":"info","event":"tool_call","server_name":"github","tool_name":"search_code","duration":250}
{"timestamp":"2024-01-15T10:00:01Z","level":"info","event":"tool_call","server_name":"github","tool_name":"list_issues","duration":180}
{"timestamp":"2024-01-15T10:00:02Z","level":"error","event":"tool_call","server_name":"github","tool_name":"create_issue","duration":100}
`
gatewayLogPath := filepath.Join(mcpLogsDir, "gateway.jsonl")
err = os.WriteFile(gatewayLogPath, []byte(testLogContent), 0644)
require.NoError(t, err, "should write test gateway.jsonl in mcp-logs")

// Test parsing from mcp-logs subdirectory
metrics, err := parseGatewayLogs(tmpDir, false)
require.NoError(t, err, "should parse gateway logs from mcp-logs subdirectory")
require.NotNil(t, metrics, "metrics should not be nil")

// Verify results
assert.Equal(t, 3, metrics.TotalRequests, "should have 3 total requests")
assert.Len(t, metrics.Servers, 1, "should have 1 server")

// Verify server metrics
githubMetrics, ok := metrics.Servers["github"]
require.True(t, ok, "should have github server metrics")
assert.Equal(t, 3, githubMetrics.RequestCount, "should have 3 total calls for github server")
}
Loading