Skip to content

Populate audit job steps in structured audit output #42222

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 3 commits into from
Jun 29, 2026
+126 −8
Merged

Populate audit job steps in structured audit output #42222

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6ffd6f5
Initial plan
Copilot Jun 29, 2026
7ff8be5
Populate audit job steps
Copilot Jun 29, 2026
8dd1d02
Coerce null conclusion to empty string in jq projection
Copilot Jun 29, 2026
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
19 changes: 15 additions & 4 deletions pkg/cli/audit_report.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,10 +115,18 @@ type MetricsData struct {

// JobData contains information about individual jobs
type JobData struct {
Name string `json:"name" console:"header:Name"`
Status string `json:"status" console:"header:Status"`
Conclusion string `json:"conclusion,omitempty" console:"header:Conclusion,omitempty"`
Duration string `json:"duration,omitempty" console:"header:Duration,omitempty"`
Name string `json:"name" console:"header:Name"`
Status string `json:"status" console:"header:Status"`
Conclusion string `json:"conclusion,omitempty" console:"header:Conclusion,omitempty"`
Duration string `json:"duration,omitempty" console:"header:Duration,omitempty"`
Steps []JobStepData `json:"steps,omitempty"`

@github-actions github-actions Bot Jun 29, 2026

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] Steps field has no console struct tag — steps will be silently invisible in console (tabular) rendering.

Every other field in JobData carries a console:"header:..." tag. Omitting it from Steps is probably intentional (nested slices are hard to render in a table), but leaving it undocumented is a subtle inconsistency.

💡 Suggestion

Add an explicit opt-out tag so the intent is clear to future maintainers:

Steps []JobStepData `json:"steps,omitempty" console:"-"`

The console:"-" pattern is already used in this file (see AwContext) for fields that should be excluded from table output.

@copilot please address this.

}

// JobStepData contains information about an individual workflow job step.
type JobStepData struct {
Name string `json:"name"`
Status string `json:"status,omitempty"`
Conclusion string `json:"conclusion,omitempty"`
}

@github-actions github-actions Bot Jun 29, 2026

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] JobStep and JobStepData are structurally identical — the type-cast conversion adds maintenance debt.

The two structs have the same three fields. return JobStepData(step) works via Go's composite-literal type conversion, but any future field added to one will require a matching change in the other (and a updated mapping). Since JobStepData carries no extra behaviour or tags beyond what JobStep already has, consider using a type alias instead:

💡 Simpler alternative 
// In audit_report.go — eliminate the duplicate struct entirely
type JobStepData = JobStep  // type alias, no conversion needed

Then the mapping in buildAuditData reduces to:

Steps: slices.Clone(jobDetail.Steps),

If the two types do need to diverge in the future (e.g., console tags, extra audit-only fields), switching back to a concrete struct is a one-line change.

@copilot please address this.


// FileInfo contains information about downloaded artifact files
Expand Down Expand Up @@ -351,6 +359,9 @@ func buildAuditData(processedRun ProcessedRun, metrics LogMetrics, mcpToolUsage
Name: jobDetail.Name,
Status: jobDetail.Status,
Conclusion: jobDetail.Conclusion,
Steps: sliceutil.Map(jobDetail.Steps, func(step JobStep) JobStepData {
return JobStepData(step)

@github-actions github-actions Bot Jun 29, 2026

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.

The direct struct conversion JobStepData(step) is valid today because JobStep and JobStepData are structurally identical (same field names, types, and tags). Go's type conversion rules mean any field divergence will be caught at compile time, which is safe.

One subtle side-effect: sliceutil.Map calls make([]U, len(slice)), so when jobDetail.Steps is nil (no steps from the API), this returns []JobStepData{} — an empty non-nil slice — rather than nil. The json:"steps,omitempty" tag correctly omits both nil and empty slices from JSON output, so serialized audit reports are unaffected. Just be aware that in-memory checks of job.Steps == nil won't reliably distinguish "no step data" from "zero steps"; prefer len(job.Steps) == 0.

@github-actions github-actions Bot Jun 29, 2026

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.

Brittle named-struct cast will surprise future maintainers: JobStepData(step) silently depends on JobStep and JobStepData having identical field names, types, and order — any divergence causes a compile error whose origin is non-obvious.

💡 Suggested fix

Use explicit field mapping instead:

Steps: sliceutil.Map(jobDetail.Steps, func(step JobStep) JobStepData {
    return JobStepData{Name: step.Name, Status: step.Status, Conclusion: step.Conclusion}
}),

This makes the intent clear — only these three fields flow from the internal API model to the audit output model — and compiles cleanly even if either struct gains fields independently (e.g., Number int on JobStep for step ordering, without leaking it to audit consumers).

}),
}
if jobDetail.Duration > 0 {
job.Duration = timeutil.FormatDuration(jobDetail.Duration)
Expand Down
47 changes: 44 additions & 3 deletions pkg/cli/audit_report_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -686,8 +686,30 @@ func TestBuildAuditDataComplete(t *testing.T) {
LogsPath: tmpDir,
},
JobDetails: []JobInfoWithDuration{
{JobInfo: JobInfo{Name: "build", Status: "completed", Conclusion: "success"}, Duration: 2 * time.Minute},
{JobInfo: JobInfo{Name: "test", Status: "completed", Conclusion: "failure"}, Duration: 5 * time.Minute},
{
JobInfo: JobInfo{
Name: "build",
Status: "completed",
Conclusion: "success",
Steps: []JobStep{
{Name: "Set up job", Status: "completed", Conclusion: "success"},
{Name: "Compile", Status: "completed", Conclusion: "success"},
},
},
Duration: 2 * time.Minute,
},
{
JobInfo: JobInfo{
Name: "test",
Status: "completed",
Conclusion: "failure",
Steps: []JobStep{
{Name: "Set up job", Status: "completed", Conclusion: "success"},
{Name: "Run tests", Status: "completed", Conclusion: "failure"},
},
},
Duration: 5 * time.Minute,
},
},
MissingTools: []MissingToolReport{
{Tool: "special_tool", Reason: "Not configured"},
Expand Down Expand Up @@ -752,6 +774,12 @@ func TestBuildAuditDataComplete(t *testing.T) {
t.Run("Jobs", func(t *testing.T) {
assert.Len(t, auditData.Jobs, 2,
"Should have 2 jobs")
assert.Len(t, auditData.Jobs[1].Steps, 2,
"Should preserve step details for each job")
assert.Equal(t, "Run tests", auditData.Jobs[1].Steps[1].Name,
"Should preserve step names")
assert.Equal(t, "failure", auditData.Jobs[1].Steps[1].Conclusion,
"Should preserve step conclusions")
})

// Verify tool usage
Expand Down Expand Up @@ -895,7 +923,16 @@ func TestRenderJSONComplete(t *testing.T) {
{Priority: "low", Action: "Monitor", Reason: "Test reason"},
},
Jobs: []JobData{
{Name: "test-job", Status: "completed", Conclusion: "success", Duration: "1m30s"},
{
Name: "test-job",
Status: "completed",
Conclusion: "success",
Duration: "1m30s",
Steps: []JobStepData{
{Name: "Set up job", Status: "completed", Conclusion: "success"},
{Name: "Run agent", Status: "completed", Conclusion: "success"},
},
},
},
DownloadedFiles: []FileInfo{
{Path: "test.log", Size: 1024, Description: "Test log"},
Expand Down Expand Up @@ -943,6 +980,10 @@ func TestRenderJSONComplete(t *testing.T) {
"Recommendations should be preserved in JSON")
assert.Len(t, parsed.Jobs, 1,
"Jobs should be preserved in JSON")
assert.Len(t, parsed.Jobs[0].Steps, 2,
"Job steps should be preserved in JSON")
assert.Equal(t, "Run agent", parsed.Jobs[0].Steps[1].Name,
"Job step names should be preserved in JSON")
assert.Len(t, parsed.Errors, 1,
"Errors should be preserved in JSON")
assert.Len(t, parsed.Warnings, 2,
Expand Down
2 changes: 1 addition & 1 deletion pkg/cli/logs_github_api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ func fetchJobDetailsWithCounts(runID int64, verbose bool) ([]JobInfoWithDuration

output, err := workflow.RunGHCombined("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}")
"--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 // \"\")}))}")
if err != nil {
if verbose {
fmt.Fprintln(os.Stderr, console.FormatVerboseMessage(fmt.Sprintf("Failed to fetch job details for run %d: %v", runID, err)))
Expand Down
58 changes: 58 additions & 0 deletions pkg/cli/logs_github_api_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,13 @@ package cli

import (
"encoding/json"
"os"
"path/filepath"
"strings"
"testing"
"time"

"github.com/github/gh-aw/pkg/testutil"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
Expand Down Expand Up @@ -222,6 +226,60 @@ func TestListWorkflowRunsErrorHandling(t *testing.T) {
}
}

func TestFetchJobDetailsWithCountsIncludesSteps(t *testing.T) {
fakeBinDir := testutil.TempDir(t, "fake-gh-*")
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" +
"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(28307653871, 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")
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")

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")

@github-actions github-actions Bot Jun 29, 2026

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.

The assertion confirms the jq projection mentions steps:, which is useful. However, the test only exercises the case where the API returns populated steps. A second sub-test or table-driven case where the fake gh script emits "steps":null or "steps":[] would verify that the // [] fallback in the jq expression works and that fetchJobDetailsWithCounts returns an empty/nil Steps slice without error — providing a backward-compat regression guard for runs fetched before step data was available.

@copilot please address this.

@github-actions github-actions Bot Jun 29, 2026

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.

The assert.Contains(t, ..., "steps:") check does not validate the jq expression: the fake gh binary cats pre-baked JSON regardless of the jq argument passed, so a typo in the jq projection (e.g., .step // [] instead of .steps // []) would still produce correct steps in the test output and the assertion would still pass.

💡 What this means

The test verifies Go-side parsing of step data — which is correct and valuable. But the actual jq expression in logs_github_api.go line 87 is never executed during tests. A regression in that expression (wrong field name, broken fallback, etc.) would only be caught by running against a real GitHub API.

Consider documenting this known gap in a comment, or strengthening the assertion to check for a more specific substring of the jq expression (e.g., .steps // []), at minimum catching obvious field-name typos:

assert.Contains(t, string(argsLog), ".steps // []", "jq must use .steps with empty-array fallback")

}

@github-actions github-actions Bot Jun 29, 2026

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] Missing edge-case test: job with null or absent steps — the (.steps // []) jq guard is untested.

The guard ensures API responses without a steps field still parse cleanly, but no test exercises this path. A job that returns JSON without a steps key could silently produce a nil slice or a parse error without anyone noticing.

💡 Suggested addition

Add a second sub-test (or table-driven variant) feeding output without a steps field and assert jobs[0].Steps is nil/empty:

// Verify a job with no steps field round-trips without error
assert.Empty(t, jobs[0].Steps, "job with no steps field should produce empty Steps slice")

This documents the contract of (.steps // []) and prevents a regression if the jq expression is later simplified.

@copilot please address this.


// TestFetchJobDetailsWithCountsNullConclusion verifies that jobs and steps with null conclusions
// (e.g. in-progress or queued jobs) are still parsed and not silently dropped. The jq projection
// uses (.conclusion // "") to coerce null to empty string before JSON decoding.
func TestFetchJobDetailsWithCountsNullConclusion(t *testing.T) {
fakeBinDir := testutil.TempDir(t, "fake-gh-*")
fakeGH := filepath.Join(fakeBinDir, "gh")
// Simulate jq coercing null -> "" before output (as (.conclusion // "") does).
// 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" +
"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(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")
assert.Equal(t, "in_progress", jobs[0].Status)
assert.Empty(t, jobs[0].Conclusion, "null conclusion should be coerced to empty string")
require.Len(t, jobs[0].Steps, 2)
assert.Empty(t, jobs[0].Steps[1].Conclusion, "null step conclusion should be coerced to empty string")
}

func TestWorkflowRunsSpinnerMessage(t *testing.T) {
tests := []struct {
name string
Expand Down
8 changes: 8 additions & 0 deletions pkg/cli/logs_models.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,14 @@ type JobInfo struct {
Conclusion string `json:"conclusion"`
StartedAt time.Time `json:"started_at,omitzero"`
CompletedAt time.Time `json:"completed_at,omitzero"`
Steps []JobStep `json:"steps,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"`
}

// JobInfoWithDuration extends JobInfo with calculated duration
Expand Down
Loading