diff --git a/pkg/cli/audit_analysis_fanout.go b/pkg/cli/audit_analysis_fanout.go index 868852b82d7..998b71f2b26 100644 --- a/pkg/cli/audit_analysis_fanout.go +++ b/pkg/cli/audit_analysis_fanout.go @@ -20,6 +20,13 @@ func collectAuditAnalysisResults(ctx context.Context, run WorkflowRun, runOutput if includeFirewallAnalyses { launchFirewallAuditAnalyses(g, gctx, &results, runOutputDir, verbose) } + if err := g.Wait(); err != nil { + return results, err + } + if ctx.Err() != nil { + return results, ctx.Err() + } + g, gctx = errgroup.WithContext(ctx) launchSupplementalAuditAnalyses(g, gctx, &results, runOutputDir, verbose) if err := g.Wait(); err != nil { return results, err @@ -44,7 +51,7 @@ func launchCoreAuditAnalyses(g *errgroup.Group, gctx context.Context, results *a expName, expVariant, _ := firstExperimentAssignment(extractExperimentData(runOutputDir)) launchMetricsAnalysis(g, gctx, results, runOutputDir, verbose, run.WorkflowPath) - launchJobDetailsAnalysis(g, gctx, results, run.DatabaseID, verbose) + launchJobDetailsAnalysis(g, gctx, results, run.DatabaseID, runOutputDir, verbose) runAuditAnalysis(g, gctx, verbose, "extractMissingToolsFromRun", "Failed to extract missing tools", func(v []MissingToolReport) { results.missingTools = v }, func() ([]MissingToolReport, error) { @@ -100,12 +107,12 @@ func launchMetricsAnalysis(g *errgroup.Group, gctx context.Context, results *aud } // launchJobDetailsAnalysis exclusively writes results.jobDetails and results.failedJobCount. -func launchJobDetailsAnalysis(g *errgroup.Group, gctx context.Context, results *auditAnalysisResults, runID int64, verbose bool) { +func launchJobDetailsAnalysis(g *errgroup.Group, gctx context.Context, results *auditAnalysisResults, runID int64, runOutputDir string, verbose bool) { g.Go(func() error { if err := gctx.Err(); err != nil { return err } - jobDetails, failedJobCount, err := fetchJobDetailsWithCounts(gctx, runID, verbose) + jobDetails, failedJobCount, err := fetchJobDetailsWithCounts(gctx, runID, runOutputDir, verbose) if err != nil { if gctx.Err() != nil { return gctx.Err() diff --git a/pkg/cli/logs_download_artifacts.go b/pkg/cli/logs_download_artifacts.go index 452c02cdedb..3a04cd6c3d7 100644 --- a/pkg/cli/logs_download_artifacts.go +++ b/pkg/cli/logs_download_artifacts.go @@ -9,6 +9,7 @@ import ( "context" "fmt" "os" + "path" "path/filepath" "strconv" "strings" @@ -27,9 +28,9 @@ func buildRepoFlag(owner, repo, hostname string) string { return "" } if hostname != "" && hostname != "github.com" { - return hostname + "/" + owner + "/" + repo + return path.Join(hostname, owner, repo) } - return owner + "/" + repo + return path.Join(owner, repo) } // listArtifacts creates a list of all artifact files in the output directory @@ -41,8 +42,8 @@ func listArtifacts(outputDir string) ([]string, error) { return err } - // Skip directories and the summary file itself - if info.IsDir() || filepath.Base(path) == runSummaryFileName { + // Skip directories and synthesized cache/summary files + if info.IsDir() || filepath.Base(path) == runSummaryFileName || filepath.Base(path) == jobsAPIResponseFileName { return nil } @@ -214,54 +215,58 @@ func retryCriticalArtifacts(ctx context.Context, opts downloadArtifactsOptions) logsDownloadLog.Printf("Critical artifact %q already present, skipping retry", name) continue } + retryCriticalArtifact(ctx, opts, repoFlag, name, artifactDir) + } +} - // Stage next to the output directory so promotion can use an atomic same-filesystem rename. - stagingDir, err := os.MkdirTemp(filepath.Dir(opts.outputDir), "."+filepath.Base(opts.outputDir)+"-"+name+"-") - if err != nil { - logsDownloadLog.Printf("Failed to create staging directory for critical artifact %q: %v", name, err) - continue - } +func retryCriticalArtifact(ctx context.Context, opts downloadArtifactsOptions, repoFlag, name, artifactDir string) { + // Stage next to the output directory so promotion can use an atomic same-filesystem rename. + stagingDir, err := os.MkdirTemp(filepath.Dir(opts.outputDir), "."+filepath.Base(opts.outputDir)+"-"+name+"-") + if err != nil { + logsDownloadLog.Printf("Failed to create staging directory for critical artifact %q: %v", name, err) + return + } - retryArgs := []string{"run", "download", strconv.FormatInt(opts.runID, 10), "--name", name, "--dir", stagingDir} - if repoFlag != "" { - retryArgs = append(retryArgs, "-R", repoFlag) - } + retryArgs := []string{"run", "download", strconv.FormatInt(opts.runID, 10), "--name", name, "--dir", stagingDir} + if repoFlag != "" { + retryArgs = append(retryArgs, "-R", repoFlag) + } + + logsDownloadLog.Printf("Retrying individual download for artifact %q: gh %s", name, strings.Join(retryArgs, " ")) + if opts.verbose { + fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Retrying download for missing artifact: "+name)) + } - logsDownloadLog.Printf("Retrying individual download for artifact %q: gh %s", name, strings.Join(retryArgs, " ")) + retryCmd := workflow.ExecGHContext(ctx, retryArgs...) + retryOutput, retryErr := retryCmd.CombinedOutput() + if retryErr != nil { + _ = os.RemoveAll(stagingDir) + logsDownloadLog.Printf("Failed to download artifact %q individually: %v (%s)", name, retryErr, string(retryOutput)) if opts.verbose { - fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Retrying download for missing artifact: "+name)) + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Could not download artifact %q: %v", name, retryErr))) } + return + } - retryCmd := workflow.ExecGHContext(ctx, retryArgs...) - retryOutput, retryErr := retryCmd.CombinedOutput() - if retryErr != nil { - _ = os.RemoveAll(stagingDir) - logsDownloadLog.Printf("Failed to download artifact %q individually: %v (%s)", name, retryErr, string(retryOutput)) - if opts.verbose { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Could not download artifact %q: %v", name, retryErr))) - } - } else { - logsDownloadLog.Printf("Successfully downloaded artifact %q individually", name) - if err := os.RemoveAll(artifactDir); err != nil { - _ = os.RemoveAll(stagingDir) - logsDownloadLog.Printf("Failed to remove existing critical artifact directory %q: %v", artifactDir, err) - continue - } - if err := os.Rename(stagingDir, artifactDir); err != nil { - _ = os.RemoveAll(stagingDir) - logsDownloadLog.Printf("Failed to promote critical artifact %q from staging: %v", name, err) - continue - } - // Marker write failures are non-fatal in the retry path: retryCriticalArtifacts - // is a best-effort recovery after a partial bulk download, so a missing marker - // only causes a redundant re-download on the next run (not data loss). - if err := markArtifactDownloaded(opts.outputDir, name); err != nil { - logsDownloadLog.Printf("Failed to mark artifact %q as downloaded: %v", name, err) - } - if opts.verbose { - fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Downloaded missing artifact: "+name)) - } - } + logsDownloadLog.Printf("Successfully downloaded artifact %q individually", name) + if err := os.RemoveAll(artifactDir); err != nil { + _ = os.RemoveAll(stagingDir) + logsDownloadLog.Printf("Failed to remove existing critical artifact directory %q: %v", artifactDir, err) + return + } + if err := os.Rename(stagingDir, artifactDir); err != nil { + _ = os.RemoveAll(stagingDir) + logsDownloadLog.Printf("Failed to promote critical artifact %q from staging: %v", name, err) + return + } + // Marker write failures are non-fatal in the retry path: retryCriticalArtifacts + // is a best-effort recovery after a partial bulk download, so a missing marker + // only causes a redundant re-download on the next run (not data loss). + if err := markArtifactDownloaded(opts.outputDir, name); err != nil { + logsDownloadLog.Printf("Failed to mark artifact %q as downloaded: %v", name, err) + } + if opts.verbose { + fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Downloaded missing artifact: "+name)) } } diff --git a/pkg/cli/logs_github_api.go b/pkg/cli/logs_github_api.go index 09f17d41e09..5fb7ef43e2d 100644 --- a/pkg/cli/logs_github_api.go +++ b/pkg/cli/logs_github_api.go @@ -16,12 +16,14 @@ import ( "fmt" "os" "os/exec" + "path/filepath" "slices" "strconv" "strings" "time" "github.com/github/gh-aw/pkg/console" + "github.com/github/gh-aw/pkg/constants" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/workflow" ) @@ -76,15 +78,15 @@ func buildCreatedFilter(startDate, endDate, beforeDate string) string { // call and returns the full detail slice together with the count of failed jobs. // It is the single source of truth for the jobs endpoint; fetchJobDetails and // fetchJobStatuses are thin wrappers that each return only the value they need. -func fetchJobDetailsWithCounts(ctx context.Context, runID int64, verbose bool) ([]JobInfoWithDuration, int, error) { +func fetchJobDetailsWithCounts(ctx context.Context, runID int64, outputDir string, verbose bool) ([]JobInfoWithDuration, int, error) { logsGitHubAPILog.Printf("Fetching job details: runID=%d", runID) if verbose { fmt.Fprintln(os.Stderr, console.FormatVerboseMessage(fmt.Sprintf("Fetching job details for run %d", runID))) } output, err := workflow.RunGHCombinedContext(ctx, "Fetching job details...", "api", - fmt.Sprintf("repos/{owner}/{repo}/actions/runs/%d/jobs", runID), - "--jq", ".jobs[] | {name: .name, status: .status, conclusion: (.conclusion // \"\"), started_at: .started_at, completed_at: .completed_at, steps: ((.steps // []) | map({name: .name, status: .status, conclusion: (.conclusion // \"\")}))}") + fmt.Sprintf("repos/{owner}/{repo}/actions/runs/%d/jobs?per_page=100", runID), + "--paginate", "--slurp") if err != nil { if verbose { fmt.Fprintln(os.Stderr, console.FormatVerboseMessage(fmt.Sprintf("Failed to fetch job details for run %d: %v", runID, err))) @@ -92,47 +94,105 @@ func fetchJobDetailsWithCounts(ctx context.Context, runID int64, verbose bool) ( return nil, 0, err } - var jobs []JobInfoWithDuration - failedJobs := 0 - lines := strings.SplitSeq(strings.TrimSpace(string(output)), "\n") - for line := range lines { - if strings.TrimSpace(line) == "" { - continue - } + var responses []struct { + Jobs []json.RawMessage `json:"jobs"` + } + if err := json.Unmarshal(output, &responses); err != nil { + return nil, 0, fmt.Errorf("failed to parse jobs API response: %w", err) + } - var job JobInfo - if err := json.Unmarshal([]byte(line), &job); err != nil { - if verbose { - fmt.Fprintln(os.Stderr, console.FormatVerboseMessage("Failed to parse job info: "+line)) + jobs := []JobInfoWithDuration{} + failedJobs := 0 + for _, response := range responses { + for _, rawJob := range response.Jobs { + var job JobInfo + if err := json.Unmarshal(rawJob, &job); err != nil { + logsGitHubAPILog.Printf("Skipping malformed job in run %d: %v", runID, err) + continue + } + jobWithDuration := JobInfoWithDuration{JobInfo: job} + if !job.StartedAt.IsZero() && !job.CompletedAt.IsZero() { + jobWithDuration.Duration = job.CompletedAt.Sub(job.StartedAt) + } + jobs = append(jobs, jobWithDuration) + + if isFailureConclusion(job.Conclusion) { + failedJobs++ + logsGitHubAPILog.Printf("Found failed job: name=%s, conclusion=%s", job.Name, job.Conclusion) + if verbose { + fmt.Fprintln(os.Stderr, console.FormatVerboseMessage(fmt.Sprintf("Found failed job '%s' with conclusion '%s'", job.Name, job.Conclusion))) + } } - continue - } - - jobWithDuration := JobInfoWithDuration{JobInfo: job} - if !job.StartedAt.IsZero() && !job.CompletedAt.IsZero() { - jobWithDuration.Duration = job.CompletedAt.Sub(job.StartedAt) } - jobs = append(jobs, jobWithDuration) + } - if isFailureConclusion(job.Conclusion) { - failedJobs++ - logsGitHubAPILog.Printf("Found failed job: name=%s, conclusion=%s", job.Name, job.Conclusion) - if verbose { - fmt.Fprintln(os.Stderr, console.FormatVerboseMessage(fmt.Sprintf("Found failed job '%s' with conclusion '%s'", job.Name, job.Conclusion))) - } + if outputDir != "" { + responsePath := filepath.Join(outputDir, jobsAPIResponseFileName) + if err := writeSensitiveFile(responsePath, output); err != nil { + return jobs, failedJobs, &jobDetailsCacheError{err: fmt.Errorf("failed to cache jobs API response: %w", err)} } + logsGitHubAPILog.Printf("Cached jobs API response: path=%s", responsePath) } logsGitHubAPILog.Printf("Job fetch complete: total=%d failed=%d", len(jobs), failedJobs) return jobs, failedJobs, nil } +type jobDetailsCacheError struct { + err error +} + +func (e *jobDetailsCacheError) Error() string { + return e.err.Error() +} + +func (e *jobDetailsCacheError) Unwrap() error { + return e.err +} + +func writeSensitiveFile(path string, data []byte) (err error) { + file, err := os.CreateTemp(filepath.Dir(path), "."+filepath.Base(path)+".tmp-*") + if err != nil { + return err + } + tempPath := file.Name() + closed := false + defer func() { + if !closed { + if closeErr := file.Close(); err == nil { + err = closeErr + } + } + if removeErr := os.Remove(tempPath); err == nil && removeErr != nil && !errors.Is(removeErr, os.ErrNotExist) { + err = removeErr + } + }() + if err := file.Chmod(constants.FilePermSensitive); err != nil { + return err + } + if _, err := file.Write(data); err != nil { + return err + } + if err := file.Close(); err != nil { + return err + } + closed = true + if err := os.Rename(tempPath, path); err != nil { + return err + } + return nil +} + // fetchJobDetails gets detailed job information including durations for a workflow run. // Errors from the underlying API call are suppressed so that callers can continue // processing even when job data is unavailable (e.g. missing permissions). -func fetchJobDetails(ctx context.Context, runID int64, verbose bool) ([]JobInfoWithDuration, error) { - jobs, _, err := fetchJobDetailsWithCounts(ctx, runID, verbose) +func fetchJobDetails(ctx context.Context, runID int64, outputDir string, verbose bool) ([]JobInfoWithDuration, error) { + jobs, _, err := fetchJobDetailsWithCounts(ctx, runID, outputDir, verbose) if err != nil { + var cacheErr *jobDetailsCacheError + if errors.As(err, &cacheErr) { + return jobs, err + } // Don't fail the entire operation if we can't get job info return nil, nil } @@ -143,7 +203,7 @@ func fetchJobDetails(ctx context.Context, runID int64, verbose bool) ([]JobInfoW // Errors from the underlying API call are suppressed so that callers can continue // processing even when job data is unavailable (e.g. missing permissions). func fetchJobStatuses(ctx context.Context, runID int64, verbose bool) (int, error) { - _, failedJobs, err := fetchJobDetailsWithCounts(ctx, runID, verbose) + _, failedJobs, err := fetchJobDetailsWithCounts(ctx, runID, "", verbose) if err != nil { // Don't fail the entire operation if we can't get job info return 0, nil @@ -189,7 +249,7 @@ type ListWorkflowRunsOptions struct { // not the total number of matching runs the user wants to find. // // The processedCount and targetCount parameters are used to display progress in the spinner message. -func listWorkflowRunsWithPagination(opts ListWorkflowRunsOptions) ([]WorkflowRun, int, error) { +func listWorkflowRunsWithPagination(opts ListWorkflowRunsOptions) ([]WorkflowRun, int, error) { //nolint:largefunc // Existing run listing keeps pagination, error classification, and filtering together. logsGitHubAPILog.Printf("Listing workflow runs: workflow=%s, limit=%d, startDate=%s, endDate=%s, ref=%s", opts.WorkflowName, opts.Limit, opts.StartDate, opts.EndDate, opts.Ref) args := []string{"run", "list", "--json", "databaseId,number,url,status,conclusion,workflowName,createdAt,startedAt,updatedAt,event,headBranch,headSha,displayTitle"} diff --git a/pkg/cli/logs_github_api_test.go b/pkg/cli/logs_github_api_test.go index 8dd40b00767..92205a1a991 100644 --- a/pkg/cli/logs_github_api_test.go +++ b/pkg/cli/logs_github_api_test.go @@ -229,30 +229,88 @@ func TestListWorkflowRunsErrorHandling(t *testing.T) { func TestFetchJobDetailsWithCountsIncludesSteps(t *testing.T) { fakeBinDir := testutil.TempDir(t, "fake-gh-*") + outputDir := t.TempDir() fakeGH := filepath.Join(fakeBinDir, "gh") argsLogPath := filepath.Join(fakeBinDir, "gh-args.log") fakeGHScript := "#!/bin/sh\n" + "printf '%s\\n' \"$*\" >> \"" + argsLogPath + "\"\n" + "cat <<'EOF'\n" + - "{\"name\":\"agent\",\"status\":\"completed\",\"conclusion\":\"failure\",\"started_at\":\"2026-06-28T01:31:00Z\",\"completed_at\":\"2026-06-28T01:33:00Z\",\"steps\":[{\"name\":\"Set up job\",\"status\":\"completed\",\"conclusion\":\"success\"},{\"name\":\"Run agent\",\"status\":\"completed\",\"conclusion\":\"failure\"}]}\n" + + "[{\"total_count\":1,\"jobs\":[{\"id\":42,\"run_id\":28307653871,\"run_attempt\":2,\"html_url\":\"https://github.com/github/gh-aw/actions/runs/28307653871/job/42\",\"status\":\"completed\",\"conclusion\":\"failure\",\"created_at\":\"2026-06-28T01:30:00Z\",\"started_at\":\"2026-06-28T01:31:00Z\",\"completed_at\":\"2026-06-28T01:33:00Z\",\"name\":\"agent\",\"runner_name\":\"GitHub Actions 1\",\"steps\":[{\"name\":\"Set up job\",\"status\":\"completed\",\"conclusion\":\"success\",\"number\":1,\"started_at\":\"2026-06-28T01:31:00Z\",\"completed_at\":\"2026-06-28T01:31:10Z\"},{\"name\":\"Run agent\",\"status\":\"completed\",\"conclusion\":\"failure\",\"number\":2,\"started_at\":\"2026-06-28T01:31:10Z\",\"completed_at\":\"2026-06-28T01:33:00Z\"}]}]}]\n" + "EOF\n" require.NoError(t, os.WriteFile(fakeGH, []byte(fakeGHScript), 0o755)) t.Setenv("PATH", fakeBinDir+string(os.PathListSeparator)+os.Getenv("PATH")) - jobs, failedJobs, err := fetchJobDetailsWithCounts(context.Background(), 28307653871, false) + cachePath := filepath.Join(outputDir, jobsAPIResponseFileName) + require.NoError(t, os.WriteFile(cachePath, []byte("old cache"), 0o644)) + + jobs, failedJobs, err := fetchJobDetailsWithCounts(context.Background(), 28307653871, outputDir, false) require.NoError(t, err) require.Len(t, jobs, 1) assert.Equal(t, 1, failedJobs, "failed job count should include failed jobs") assert.Equal(t, 2*time.Minute, jobs[0].Duration, "job duration should still be derived from timestamps") + assert.Equal(t, int64(42), jobs[0].ID, "high-level GitHub job metadata should be preserved") + assert.Equal(t, 2, jobs[0].RunAttempt) + assert.Equal(t, "GitHub Actions 1", jobs[0].RunnerName) require.Len(t, jobs[0].Steps, 2) assert.Equal(t, "Run agent", jobs[0].Steps[1].Name, "step names should be parsed from gh api output") assert.Equal(t, "failure", jobs[0].Steps[1].Conclusion, "step conclusions should be parsed from gh api output") + assert.Equal(t, 2, jobs[0].Steps[1].Number) argsLog, err := os.ReadFile(argsLogPath) require.NoError(t, err) - assert.Contains(t, string(argsLog), "repos/{owner}/{repo}/actions/runs/28307653871/jobs", "should query the run jobs API") - assert.Contains(t, string(argsLog), "steps:", "gh jq projection should request step data") + assert.Contains(t, string(argsLog), "repos/{owner}/{repo}/actions/runs/28307653871/jobs?per_page=100", "should query the run jobs API") + assert.Contains(t, string(argsLog), "--paginate --slurp", "should cache all pages of the jobs API response") + assert.NotContains(t, string(argsLog), "--jq", "should cache the complete API response without a projection") + + cachedResponse, err := os.ReadFile(cachePath) + require.NoError(t, err) + assert.Contains(t, string(cachedResponse), `"total_count":1`) + assert.Contains(t, string(cachedResponse), `"runner_name":"GitHub Actions 1"`) + cachedInfo, err := os.Stat(cachePath) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o600), cachedInfo.Mode().Perm()) +} + +func TestFetchJobDetailsWithCountsReturnsJobsWhenCacheWriteFails(t *testing.T) { + fakeBinDir := testutil.TempDir(t, "fake-gh-*") + fakeGH := filepath.Join(fakeBinDir, "gh") + fakeGHScript := "#!/bin/sh\n" + + "cat <<'EOF'\n" + + "[{\"total_count\":1,\"jobs\":[{\"name\":\"agent\",\"status\":\"completed\",\"conclusion\":\"failure\"}]}]\n" + + "EOF\n" + require.NoError(t, os.WriteFile(fakeGH, []byte(fakeGHScript), 0o755)) + + t.Setenv("PATH", fakeBinDir+string(os.PathListSeparator)+os.Getenv("PATH")) + + missingOutputDir := filepath.Join(t.TempDir(), "missing", "run") + jobs, failedJobs, err := fetchJobDetailsWithCounts(context.Background(), 28307653871, missingOutputDir, false) + + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to cache jobs API response") + require.Len(t, jobs, 1) + assert.Equal(t, "agent", jobs[0].Name) + assert.Equal(t, 1, failedJobs) +} + +func TestFetchJobDetailsWithCountsSkipsMalformedJobs(t *testing.T) { + fakeBinDir := testutil.TempDir(t, "fake-gh-*") + fakeGH := filepath.Join(fakeBinDir, "gh") + fakeGHScript := "#!/bin/sh\n" + + "cat <<'EOF'\n" + + "[{\"total_count\":3,\"jobs\":[{\"name\":\"failed\",\"status\":\"completed\",\"conclusion\":\"failure\"},{\"name\":42,\"status\":\"completed\",\"conclusion\":\"failure\"},{\"name\":\"passed\",\"status\":\"completed\",\"conclusion\":\"success\"}]}]\n" + + "EOF\n" + require.NoError(t, os.WriteFile(fakeGH, []byte(fakeGHScript), 0o755)) + + t.Setenv("PATH", fakeBinDir+string(os.PathListSeparator)+os.Getenv("PATH")) + + jobs, failedJobs, err := fetchJobDetailsWithCounts(context.Background(), 28307653871, "", false) + + require.NoError(t, err) + require.Len(t, jobs, 2) + assert.Equal(t, "failed", jobs[0].Name) + assert.Equal(t, "passed", jobs[1].Name) + assert.Equal(t, 1, failedJobs) } // TestFetchJobDetailsWithCountsNullConclusion verifies that jobs and steps with null conclusions @@ -265,13 +323,13 @@ func TestFetchJobDetailsWithCountsNullConclusion(t *testing.T) { // A job still in progress has conclusion="" for itself and for any pending steps. fakeGHScript := "#!/bin/sh\n" + "cat <<'EOF'\n" + - "{\"name\":\"agent\",\"status\":\"in_progress\",\"conclusion\":\"\",\"started_at\":\"2026-06-28T01:31:00Z\",\"completed_at\":\"0001-01-01T00:00:00Z\",\"steps\":[{\"name\":\"Set up job\",\"status\":\"completed\",\"conclusion\":\"success\"},{\"name\":\"Run agent\",\"status\":\"in_progress\",\"conclusion\":\"\"}]}\n" + + "[{\"total_count\":1,\"jobs\":[{\"name\":\"agent\",\"status\":\"in_progress\",\"conclusion\":null,\"started_at\":\"2026-06-28T01:31:00Z\",\"completed_at\":null,\"steps\":[{\"name\":\"Set up job\",\"status\":\"completed\",\"conclusion\":\"success\"},{\"name\":\"Run agent\",\"status\":\"in_progress\",\"conclusion\":null}]}]}]\n" + "EOF\n" require.NoError(t, os.WriteFile(fakeGH, []byte(fakeGHScript), 0o755)) t.Setenv("PATH", fakeBinDir+string(os.PathListSeparator)+os.Getenv("PATH")) - jobs, failedJobs, err := fetchJobDetailsWithCounts(context.Background(), 28307653871, false) + jobs, failedJobs, err := fetchJobDetailsWithCounts(context.Background(), 28307653871, "", false) require.NoError(t, err) require.Len(t, jobs, 1, "in-progress jobs with null conclusion should not be dropped") assert.Equal(t, 0, failedJobs, "in-progress job should not count as failed") diff --git a/pkg/cli/logs_models.go b/pkg/cli/logs_models.go index e65cecda40a..35707317947 100644 --- a/pkg/cli/logs_models.go +++ b/pkg/cli/logs_models.go @@ -19,6 +19,8 @@ const ( defaultAgentStdioLogPath = "/tmp/gh-aw/agent-stdio.log" // runSummaryFileName is the name of the summary file created in each run folder runSummaryFileName = "run_summary.json" + // jobsAPIResponseFileName is the raw GitHub Actions jobs API response cached for each run + jobsAPIResponseFileName = "jobs.json" // defaultLogsOutputDir is the default directory for downloaded workflow logs defaultLogsOutputDir = ".github/aw/logs" ) @@ -302,19 +304,39 @@ type DownloadResult struct { // JobInfo represents basic information about a workflow job type JobInfo struct { - Name string `json:"name"` - Status string `json:"status"` - Conclusion string `json:"conclusion"` - StartedAt time.Time `json:"started_at,omitzero"` - CompletedAt time.Time `json:"completed_at,omitzero"` - Steps []JobStep `json:"steps,omitempty"` + ID int64 `json:"id,omitempty"` + RunID int64 `json:"run_id,omitempty"` + RunURL string `json:"run_url,omitempty"` + RunAttempt int `json:"run_attempt,omitempty"` + NodeID string `json:"node_id,omitempty"` + HeadSha string `json:"head_sha,omitempty"` + URL string `json:"url,omitempty"` + HTMLURL string `json:"html_url,omitempty"` + Status string `json:"status"` + Conclusion string `json:"conclusion"` + CreatedAt time.Time `json:"created_at,omitzero"` + StartedAt time.Time `json:"started_at,omitzero"` + CompletedAt time.Time `json:"completed_at,omitzero"` + Name string `json:"name"` + Steps []JobStep `json:"steps,omitempty"` + CheckRunURL string `json:"check_run_url,omitempty"` + Labels []string `json:"labels,omitempty"` + RunnerID int64 `json:"runner_id,omitempty"` + RunnerName string `json:"runner_name,omitempty"` + RunnerGroupID int64 `json:"runner_group_id,omitempty"` + RunnerGroupName string `json:"runner_group_name,omitempty"` + WorkflowName string `json:"workflow_name,omitempty"` + HeadBranch string `json:"head_branch,omitempty"` } // JobStep represents basic information about an individual workflow job step. type JobStep struct { - Name string `json:"name"` - Status string `json:"status,omitempty"` - Conclusion string `json:"conclusion,omitempty"` + Name string `json:"name"` + Status string `json:"status,omitempty"` + Conclusion string `json:"conclusion,omitempty"` + Number int `json:"number,omitempty"` + StartedAt time.Time `json:"started_at,omitzero"` + CompletedAt time.Time `json:"completed_at,omitzero"` } // JobInfoWithDuration extends JobInfo with calculated duration diff --git a/pkg/cli/logs_run_processor.go b/pkg/cli/logs_run_processor.go index 3bd6f96b06d..1123596b121 100644 --- a/pkg/cli/logs_run_processor.go +++ b/pkg/cli/logs_run_processor.go @@ -540,10 +540,11 @@ func applyRunUsageMetrics(result *DownloadResult, metrics *LogMetrics, runOutput // RunSummary struct, and writes it to disk. It also sets the agentic-analysis fields on // result directly so they are available to the caller. func finalizeAndSaveRunSummary(ctx context.Context, result *DownloadResult, runOutputDir string, metrics LogMetrics, verbose bool) { - jobDetails, jobErr := fetchJobDetails(ctx, result.Run.DatabaseID, verbose) + jobDetails, jobErr := fetchJobDetails(ctx, result.Run.DatabaseID, runOutputDir, verbose) if jobErr != nil && verbose { fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to fetch job details for run %d: %v", result.Run.DatabaseID, jobErr))) - } else { + } + if jobDetails != nil { result.JobDetails = jobDetails } diff --git a/pkg/cli/logs_summary_integration_test.go b/pkg/cli/logs_summary_integration_test.go index 90dcea747ec..c8885b8a619 100644 --- a/pkg/cli/logs_summary_integration_test.go +++ b/pkg/cli/logs_summary_integration_test.go @@ -228,6 +228,7 @@ func TestListArtifactsExcludesSummary(t *testing.T) { "aw_info.json", "agent-stdio.log", runSummaryFileName, // This should be excluded from the list + jobsAPIResponseFileName, } for _, filename := range testFiles { @@ -243,15 +244,15 @@ func TestListArtifactsExcludesSummary(t *testing.T) { t.Fatalf("Failed to list artifacts: %v", err) } - // Should have 2 artifacts (excluding the summary) + // Should have 2 artifacts (excluding synthesized cache/summary files) if len(artifacts) != 2 { - t.Errorf("Expected 2 artifacts (excluding summary), got %d: %v", len(artifacts), artifacts) + t.Errorf("Expected 2 artifacts (excluding synthesized files), got %d: %v", len(artifacts), artifacts) } - // Verify summary is not in the list + // Verify synthesized files are not in the list for _, artifact := range artifacts { - if artifact == runSummaryFileName { - t.Errorf("Summary file %s should not be in artifacts list", runSummaryFileName) + if artifact == runSummaryFileName || artifact == jobsAPIResponseFileName { + t.Errorf("Synthesized file %s should not be in artifacts list", artifact) } } diff --git a/pkg/cli/logs_summary_test.go b/pkg/cli/logs_summary_test.go index bb16b9e4fac..f9ebaa11fb1 100644 --- a/pkg/cli/logs_summary_test.go +++ b/pkg/cli/logs_summary_test.go @@ -290,6 +290,7 @@ func TestListArtifacts(t *testing.T) { "workflow-logs/job-1.txt", "workflow-logs/job-2.txt", "agent_output/output.json", + jobsAPIResponseFileName, } for _, file := range testFiles { @@ -308,18 +309,26 @@ func TestListArtifacts(t *testing.T) { t.Fatalf("Failed to list artifacts: %v", err) } - // Verify all test files are in the list - for _, expectedFile := range testFiles { + // Verify real artifact files are in the list + expectedFiles := []string{ + "aw_info.json", + "agent-stdio.log", + "safe_output.jsonl", + "workflow-logs/job-1.txt", + "workflow-logs/job-2.txt", + "agent_output/output.json", + } + for _, expectedFile := range expectedFiles { found := slices.Contains(artifacts, expectedFile) if !found { t.Errorf("Expected artifact %s not found in list: %v", expectedFile, artifacts) } } - // Verify the summary file itself is not in the list + // Verify synthesized cache/summary files are not in the list for _, artifact := range artifacts { - if artifact == runSummaryFileName { - t.Errorf("Summary file %s should not be in artifacts list", runSummaryFileName) + if artifact == runSummaryFileName || artifact == jobsAPIResponseFileName { + t.Errorf("Synthesized file %s should not be in artifacts list", artifact) } } }