Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 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
6 changes: 3 additions & 3 deletions pkg/cli/audit_analysis_fanout.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,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) {
Expand Down Expand Up @@ -100,12 +100,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()
Expand Down
68 changes: 37 additions & 31 deletions pkg/cli/logs_github_api.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -76,49 +78,53 @@ 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)))
}
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 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))
}
continue
if outputDir != "" {
responsePath := filepath.Join(outputDir, jobsAPIResponseFileName)
if err := os.WriteFile(responsePath, output, constants.FilePermSensitive); err != nil {

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] No test covers os.WriteFile failing (e.g. outputDir not existing, permission denied) — that path currently returns a hard error from fetchJobDetailsWithCounts, which callers treat as "no job data available" even though this is a caching side-effect, not a fetch failure.

💡 Suggested test
func TestFetchJobDetailsWithCounts_CacheWriteFailure(t *testing.T) {
    // outputDir points at a non-existent parent (or a read-only dir)
    // assert jobs/failedJobs are still returned, or that the error is
    // clearly scoped to the cache-write concern.
}

Worth deciding intentionally whether a cache-write failure should fail job detail retrieval at all, since every caller already discards fetchJobDetailsWithCounts errors as non-fatal (fetchJobDetails, fetchJobStatuses, launchJobDetailsAnalysis) — so today it silently degrades, which may be fine, but should be covered by a test rather than implicit.

@copilot please address this.

return nil, 0, fmt.Errorf("failed to cache jobs API response: %w", err)
}
logsGitHubAPILog.Printf("Cached jobs API response: path=%s", responsePath)
}

jobWithDuration := JobInfoWithDuration{JobInfo: job}
if !job.StartedAt.IsZero() && !job.CompletedAt.IsZero() {
jobWithDuration.Duration = job.CompletedAt.Sub(job.StartedAt)
}
jobs = append(jobs, jobWithDuration)
var responses []struct {
Jobs []JobInfo `json:"jobs"`
}
if err := json.Unmarshal(output, &responses); err != nil {

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.

[/diagnosing-bugs] Resilience regression: parsing now aborts entirely on any malformed byte in the whole paginated response, whereas the old per-line loop skipped just the bad job and kept the rest.

💡 Details

The previous implementation iterated line-by-line and logged+skipped any job that failed to unmarshal, so a single corrupted/partial page didn't discard every other job. Now json.Unmarshal(output, &responses) is all-or-nothing — one bad page (e.g. truncated network response, unexpected field type) zeroes out job info for the entire run, even though most pages were fine. Since this directly affects the failed-job count and audit summaries, consider decoding per-page (or per-job) with a fallback that logs and continues, matching the old resilience behavior. Add a test with one malformed page among several valid ones to lock in the desired behavior.

@copilot please address this.

return nil, 0, fmt.Errorf("failed to parse jobs API response: %w", err)
}

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)))
var jobs []JobInfoWithDuration
failedJobs := 0
for _, response := range responses {
for _, job := range response.Jobs {
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)))
}
}
}
}
Expand All @@ -130,8 +136,8 @@ func fetchJobDetailsWithCounts(ctx context.Context, runID int64, verbose bool) (
// 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 {
// Don't fail the entire operation if we can't get job info
return nil, nil
Expand All @@ -143,7 +149,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
Expand Down Expand Up @@ -189,7 +195,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"}

Expand Down
26 changes: 20 additions & 6 deletions pkg/cli/logs_github_api_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -229,30 +229,44 @@ 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)
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(filepath.Join(outputDir, jobsAPIResponseFileName))
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(filepath.Join(outputDir, jobsAPIResponseFileName))
require.NoError(t, err)
assert.Equal(t, os.FileMode(0o600), cachedInfo.Mode().Perm())
}

// TestFetchJobDetailsWithCountsNullConclusion verifies that jobs and steps with null conclusions
Expand All @@ -265,13 +279,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")
Expand Down
40 changes: 31 additions & 9 deletions pkg/cli/logs_models.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion pkg/cli/logs_run_processor.go
Original file line number Diff line number Diff line change
Expand Up @@ -540,7 +540,7 @@ 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 {
Expand Down
Loading