Cache GitHub Actions job metadata in logs output - #59039
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
Non-blocking from this diff alone. The change preserves more of the jobs payload and wires the cache through the existing run summary path without introducing a clear correctness regression in the touched lines.
Highlights
- The expanded
JobInfo/JobStepmodels are still backward-compatible with partial JSON payloads. fetchJobStatusesintentionally avoids writingjobs.json, which keeps the lightweight status path cheap.- The one grumpy-coder finding was dropped because
gh api --paginate --slurpis expected to return the paginated JSON array shape this code unmarshals, and the existing behavior already suppresses upstream fetch failures for callers.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 23.8 AIC · ⌖ 8.18 AIC · ⊞ 21.8K
Comment /review to run again
There was a problem hiding this comment.
🟡 Changes recommended
Cache-write error handling, overwrite permissions, and concurrent artifact enumeration can produce incomplete or insecure cached output.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Caches complete paginated GitHub Actions job metadata and exposes expanded job and step details in run summaries.
Changes:
- Stores raw paginated responses in owner-only
jobs.json. - Expands job and step metadata.
- Preserves duration and failure calculations.
File summaries
| File | Description |
|---|---|
pkg/cli/logs_run_processor.go |
Passes the run directory for caching. |
pkg/cli/logs_models.go |
Adds cache filename and metadata fields. |
pkg/cli/logs_github_api.go |
Fetches, caches, and parses paginated jobs. |
pkg/cli/logs_github_api_test.go |
Tests metadata, pagination flags, and permissions. |
pkg/cli/audit_analysis_fanout.go |
Enables job-response caching during audits. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Balanced
| continue | ||
| if outputDir != "" { | ||
| responsePath := filepath.Join(outputDir, jobsAPIResponseFileName) | ||
| if err := os.WriteFile(responsePath, output, constants.FilePermSensitive); err != nil { |
|
|
||
| launchMetricsAnalysis(g, gctx, results, runOutputDir, verbose, run.WorkflowPath) | ||
| launchJobDetailsAnalysis(g, gctx, results, run.DatabaseID, verbose) | ||
| launchJobDetailsAnalysis(g, gctx, results, run.DatabaseID, runOutputDir, verbose) |
| if err := os.WriteFile(responsePath, output, constants.FilePermSensitive); err != nil { | ||
| return nil, 0, fmt.Errorf("failed to cache jobs API response: %w", err) |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd on the jobs-API caching change.
📋 Key Themes & Highlights
Key Themes
- Resilience regression: switching from per-line to whole-response
json.Unmarshalmeans one malformed page now discards all job data for the run, where previously bad lines were skipped individually. - Untested error path: the new
os.WriteFilecache-write can fail (missing dir, permissions) and that failure currently propagates as a hard error fromfetchJobDetailsWithCounts, with no test exercising it.
Positive Highlights
- ✅ Good coverage of the new richer
JobInfo/JobStepfields in the updated unit tests. - ✅ Consistent use of
constants.FilePermSensitive(0600) for the cached response, matching the project's sensitive-file convention. - ✅ The plumbing of
outputDir/runOutputDirthroughfetchJobDetailsWithCounts→fetchJobDetails→launchJobDetailsAnalysisis clean and consistent across both the logs and audit call paths.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 79.1 AIC · ⌖ 13.7 AIC · ⊞ 10.3K
Comment /matt to run again
| var responses []struct { | ||
| Jobs []JobInfo `json:"jobs"` | ||
| } | ||
| if err := json.Unmarshal(output, &responses); err != nil { |
There was a problem hiding this comment.
[/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.
| continue | ||
| if outputDir != "" { | ||
| responsePath := filepath.Join(outputDir, jobsAPIResponseFileName) | ||
| if err := os.WriteFile(responsePath, output, constants.FilePermSensitive); err != nil { |
There was a problem hiding this comment.
[/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.
There was a problem hiding this comment.
Reviewed as a backend/CLI change (not UI), so Impeccable modes were not applicable; performed a direct correctness/reliability review focused on the jobs-API caching change.
Two non-blocking issues found:
logs_github_api.go— a cache-write failure (os.WriteFileforjobs.json) now aborts job parsing entirely instead of degrading gracefully, which can regress previously-reliable job-status reporting on transient filesystem errors.logs_run_processor.go— the newly cachedjobs.jsonfile isn't excluded fromlistArtifacts, so it leaks intoRunSummary.ArtifactsListas if it were a downloaded artifact.
Both are suggestions for hardening/consistency rather than blockers; tests for the changed behavior pass (TestFetchJobDetailsWithCountsIncludesSteps, TestFetchJobDetailsWithCountsNullConclusion, TestCollectAuditAnalysisResultsReturnsContextCancellation).
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 134.5 AIC · ⌖ 14.4 AIC · ⊞ 8.3K
Comments that could not be inline-anchored
pkg/cli/logs_github_api.go:100
When outputDir != "", a cache-write failure now aborts job parsing entirely (return nil, 0, fmt.Errorf(...)), even though the GitHub API call already succeeded and output holds valid data. fetchJobDetails/fetchJobStatuses swallow errors from this function and return nil/0, so a transient filesystem issue (permission problem, disk full, read-only mount) now causes complete loss of job details/duration/failed-job counts — a capability that previously only failed when the API call it…
pkg/cli/logs_run_processor.go:550
listArtifacts (used to populate RunSummary.ArtifactsList) walks the entire runOutputDir and only excludes runSummaryFileName; it does not exclude jobsAPIResponseFileName. Since finalizeAndSaveRunSummary fetches job details (writing jobs.json into runOutputDir) before calling listArtifacts, the newly-cached jobs.json will now show up in ArtifactsList as if it were a downloaded GitHub Actions artifact, even though it's synthesized metadata rather than an artifact. Consider e…
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
@copilot Please take the next forward-progress pass on PR #59039.
Recent review focus:
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
The logs command discarded GitHub’s jobs API response, limiting cached run data to a reduced projection.
Changes
jobs.jsonfiles.