Cache complete workflow run metadata in logs - #59257
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft.
|
|
✅ PR Code Quality Reviewer completed the code quality review. 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
- "github.com"See Network Configuration for more information.
|
|
✅ 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.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff 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
- "github.com"See Network Configuration for more information.
|
ADR RequiredThis PR triggers the design-decision gate because it adds more than 100 lines in business-logic code ( I did not find an existing ADR in the PR body, and the latest ADR on the branch ( Based on the PR description and diff, I generated a draft ADR at Evidence used
Next action
|
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. 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
- "github.com"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
The new metadata cache introduces at least one correctness bug: run.json is reused on run ID alone, so a stale or cross-repo cache entry can silently stamp the wrong repository/actor/branch metadata onto a report.
Blocking theme
The cache validation is too weak for persisted workflow-run metadata. Returning a parsed payload without verifying that it still belongs to the expected repository context turns cache pollution into a silent data-integrity problem instead of a harmless miss.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 56.1 AIC · ⌖ 7.43 AIC · ⊞ 21.8K
Comment /review to run again
| func fetchAndCacheWorkflowRunMetadata(ctx context.Context, runID int64, outputDir, owner, repo, hostname string, verbose bool) (WorkflowRun, error) { | ||
| responsePath := filepath.Join(outputDir, runAPIResponseFileName) | ||
| if output, err := os.ReadFile(responsePath); err == nil { | ||
| if run, parseErr := parseWorkflowRunAPIResponse(output); parseErr == nil && run.DatabaseID == runID { |
There was a problem hiding this comment.
This metadata cache trusts any run.json whose numeric run ID matches, but it never verifies that the cached payload still belongs to the expected repository. If a stale or polluted cache directory is reused, gh aw logs can silently attach the wrong repository, actor, branch, and event to the report.
💡 Tighten the cache key, not just the parser
The new cache read path in fetchAndCacheWorkflowRunMetadata only checks run.DatabaseID == runID before returning the cached payload. That is not enough once the same logs directory can be copied, restored from another repo, or reused across contexts.
A safer check is to reject cached metadata unless it also matches the expected repository context when owner/repo are known, or at least unless the cached html_url / repository.full_name points at the same run namespace you are about to process.
if run.Repository != "" && owner != "" && repo != "" && !strings.EqualFold(run.Repository, owner+"/"+repo) {
// treat as cache miss
}Without that guard, this cache can produce structurally valid but semantically wrong reports, which is worse than a cache miss because downstream consumers will trust the bad metadata.
Test Quality Sentinel 🧪PR #59257 — "Cache complete workflow run metadata in logs" Analysis SummaryTest Coverage Overview
Quality Metrics
New & Modified TestsBehavioral Test Coverage (11 tests)logs_github_api_test.go (36 lines added)
logs_json_test.go (31 lines added)
logs_summary_test.go (2 line change)
logs_summary_integration_test.go (2 line change)
Red Flag Assessment✅ No red flags detected:
Quality Score CalculationVerdict✅ APPROVE — Test Quality Sentinel: 86/100 Rationale:
Implementation Test Ratio: 16% (threshold: ≤30%) ✅ Pass 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
- "github.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on one rate-limit gap before merge.
📋 Key Themes & Highlights
Key Themes
- Missing rate-limit guard on cache-refresh path:
tryLoadCachedRunResultcallsfetchAndCacheWorkflowRunMetadata(a realgh apicall on first cache-miss forrun.json) without thewaitForConfiguredRateLimitthrottle used on the fresh-download path. This could cause a rate-limit burst when backfilling metadata across many pre-existing cached runs. - Fragile change-detection via struct equality:
metadataApplied := result.Run != runBeforeMetadatadepends onWorkflowRunstaying comparable (==). Returning a bool from the merge function would be more robust as the struct grows.
Positive Highlights
- ✅ Clean cache-then-fetch pattern in
fetchAndCacheWorkflowRunMetadata, with sensible fallback on stale/mismatched cache entries. - ✅ Good test coverage added for the new metadata fetch/cache round-trip (
TestFetchAndCacheWorkflowRunMetadata) and for thebuildLogsDataGitHub-metadata-without-aw_info path. - ✅ Consistent handling of
run.jsonin storage-pruning and artifact-listing exclusions across all touched call sites.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 67.9 AIC · ⌖ 14.7 AIC · ⊞ 10.3K
Comment /matt to run again
| Cached: true, | ||
| } | ||
| runBeforeMetadata := result.Run | ||
| if metadata, err := fetchAndCacheWorkflowRunMetadata(ctx, run.DatabaseID, runOutputDir, params.dlOwner, params.dlRepo, params.dlHost, params.verbose); err != nil { |
There was a problem hiding this comment.
[/diagnosing-bugs] The cache-hit path now makes an unthrottled gh api call via fetchAndCacheWorkflowRunMetadata, unlike the fresh-download path which calls waitForConfiguredRateLimit first (line 251). This silently bypasses --max-github-api-rate-limit on tryLoadCachedRunResult.
💡 Why this matters
On first run after upgrading, every pre-existing cached run (no run.json yet) will trigger a real network call here with zero throttling. For a bulk gh aw logs invocation over months of history this can burst far past the configured rate-limit ceiling that the rest of the pipeline carefully respects (see logsRunPreflightAPIReserve and the reserve-budget comment above processSingleRunDownload). Recommend calling waitForConfiguredRateLimit(ctx, params.verbose, params.maxGitHubAPIRateLimit, 1) before this fetch, mirroring the fresh-download path.
@copilot please address this.
| } else { | ||
| applyWorkflowRunMetadata(&result.Run, metadata) | ||
| } | ||
| metadataApplied := result.Run != runBeforeMetadata |
There was a problem hiding this comment.
[/tdd] metadataApplied := result.Run != runBeforeMetadata relies on WorkflowRun being comparable with ==. That works today, but it's a fragile invariant — any future field addition of a slice/map to WorkflowRun (quite plausible given this PR just added three scalar fields) will break compilation with a cryptic error far from this line, or worse, silently change semantics if someone works around it with reflect.DeepEqual inconsistently elsewhere.
💡 Suggestion
Consider having applyWorkflowRunMetadata return a bool indicating whether any field changed, instead of relying on struct equality of the whole WorkflowRun. That keeps the change-detection logic colocated with the merge logic and removes the comparability constraint on the struct.
@copilot please address this.
There was a problem hiding this comment.
🟡 Changes recommended
Cache freshness and artifact-detection regressions can produce stale or empty run reports.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Caches complete workflow-run API responses and uses them to enrich log reports.
Changes:
- Adds
run.jsonfetching, parsing, caching, and pruning protection. - Enriches reports with repository, actor, attempt, event, and commit metadata.
- Adds coverage for caching and artifact-inventory exclusions.
File summaries
| File | Description |
|---|---|
pkg/cli/workflow_run_metadata.go |
Shares raw run API argument construction. |
pkg/cli/logs_summary_test.go |
Tests excluding run.json from inventories. |
pkg/cli/logs_summary_integration_test.go |
Extends integration coverage for exclusions. |
pkg/cli/logs_storage_limit.go |
Preserves run.json during pruning. |
pkg/cli/logs_run_processor.go |
Fetches and applies metadata to fresh and cached runs. |
pkg/cli/logs_report.go |
Adds API metadata baselines and artifact overrides. |
pkg/cli/logs_models.go |
Defines cached filename and metadata fields. |
pkg/cli/logs_json_test.go |
Tests report metadata enrichment. |
pkg/cli/logs_github_api.go |
Implements run-response parsing and caching. |
pkg/cli/logs_github_api_test.go |
Tests metadata fetching and cache reuse. |
pkg/cli/logs_download_artifacts.go |
Excludes the metadata cache from inventories. |
Review details
Suppressed comments (1)
pkg/cli/logs_github_api.go:306
- A matching run ID does not make this cache current: GitHub reruns reuse the same run ID while incrementing
run_attempt, and queued/in-progress runs continue changing status and timestamps. This can indefinitely overwrite freshergh run listdata with an old attempt/status/conclusion. Validate the cache against the current run's attempt orupdated_at, and refresh mutable runs.
if output, err := os.ReadFile(responsePath); err == nil {
if run, parseErr := parseWorkflowRunAPIResponse(output); parseErr == nil && run.DatabaseID == runID {
return run, nil
- Files reviewed: 12/12 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| if err := os.MkdirAll(runOutputDir, constants.DirPermSensitive); err != nil { | ||
| return fmt.Errorf("failed to create run output directory: %w", err) | ||
| } | ||
| if metadata, err := fetchAndCacheWorkflowRunMetadata(ctx, run.DatabaseID, runOutputDir, perRunParams.dlOwner, perRunParams.dlRepo, perRunParams.dlHost, params.verbose); err != nil { |
|
|
||
| // Skip directories and synthesized cache/summary files | ||
| if info.IsDir() || filepath.Base(path) == runSummaryFileName || filepath.Base(path) == jobsAPIResponseFileName { | ||
| if info.IsDir() || filepath.Base(path) == runSummaryFileName || filepath.Base(path) == jobsAPIResponseFileName || filepath.Base(path) == runAPIResponseFileName { |
| if metadata.WorkflowName != "" { | ||
| run.WorkflowName = metadata.WorkflowName | ||
| } |
| runBeforeMetadata := result.Run | ||
| if metadata, err := fetchAndCacheWorkflowRunMetadata(ctx, run.DatabaseID, runOutputDir, params.dlOwner, params.dlRepo, params.dlHost, params.verbose); err != nil { | ||
| logsOrchestratorLog.Printf("Failed to refresh cached workflow run metadata for run %d: %v", run.DatabaseID, err) |
There was a problem hiding this comment.
Reviewed the workflow-run metadata caching change (Go backend, no UI — Impeccable modes not applicable; performed a standard correctness/reliability review instead).
Overall the implementation is solid: WorkflowRun/RunData merging is mostly guarded against empty-value clobbering, caching/staleness handling (run.DatabaseID == runID check) is correct, and the artifact-listing/storage-pruning exclusions were updated consistently across all three related test files.
Two non-blocking observations left as inline comments:
logsRunPreflightAPIReservecomment now claims to account for the new metadata fetch, but the constant value is unchanged — worth double-checking headroom.applyGitHubMetadataToRunDataassigns fields unconditionally (unlike its sibling merge functions), which is currently safe due to call order but is a latent risk if reused/reordered.
No blocking issues found.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 109 AIC · ⌖ 13.5 AIC · ⊞ 8.3K
Comments that could not be inline-anchored
pkg/cli/logs_run_processor.go:223
The comment now says the preflight reserve accounts for the workflow-run metadata fetch in addition to the existing calls, but logsRunPreflightAPIReserve is left at 8 (unchanged). If this constant was already tightly sized for the prior call count, adding a new mandatory gh api call (fetchAndCacheWorkflowRunMetadata) inside the same runDownloadDeferred closure without increasing the reserve could let rate-limit usage exceed the configured ceiling before the next check, since `waitFo…
pkg/cli/logs_report.go:398
applyGitHubMetadataToRunData unconditionally assigns runData.Repository, runData.SHA, runData.Actor, and runData.EventName from run.Repository/run.HeadSha/run.Actor/run.Event even when those are empty strings. Contrast this with applyAwInfoToRunData just above (guarded by if awInfo.X != "") and applyWorkflowRunMetadata in logs_github_api.go (also guarded). For older cached runs where the new run.json metadata fetch fails or the run predates this feature, `run.Repositor…
@copilot Please take the next forward-progress pass on PR #59257.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the cache freshness, repository validation, artifact detection, display-name, and rate-limit concerns in df9a68a. Local validation and security scanning passed; the PR-finisher handoff is ready. |
gh aw logsomitted workflow-run metadata that downstream consumers had to retrieve separately withgh api. This change collects that metadata during log processing and caches it for reuse.Changes
Metadata collection
Caching
run.jsonalongside each run’s artifacts.run.jsonduring storage pruning and exclude it from artifact inventories.Metadata precedence
aw_info.jsonvalues as authoritative overrides.