Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ 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.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based 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! 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
|
There was a problem hiding this comment.
🟡 Changes recommended
Cache identity checks are bypassed in discovery mode, artifact processing still occurs, and several cached aggregates are inaccurate.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds reusable JSON caching to gh aw logs to avoid repeated run processing.
Changes:
- Adds
--cached-jsonand cache validation/lookup. - Reuses eligible cached run records.
- Recomputes report aggregates and adds tests.
File summaries
| File | Description |
|---|---|
pkg/cli/logs_run_processor.go |
Reuses cached download results. |
pkg/cli/logs_report.go |
Aggregates cached run metrics. |
pkg/cli/logs_orchestrator_types.go |
Adds cache option fields. |
pkg/cli/logs_orchestrator_stdin.go |
Integrates caching for stdin mode. |
pkg/cli/logs_orchestrator_download.go |
Integrates caching into downloads. |
pkg/cli/logs_models.go |
Adds cached-record state. |
pkg/cli/logs_command.go |
Adds flag and help text. |
pkg/cli/logs_command_test.go |
Tests flag configuration. |
pkg/cli/logs_cached_json.go |
Implements cache loading and reuse. |
pkg/cli/logs_cached_json_test.go |
Tests cache behavior and aggregation. |
Review details
- Files reviewed: 10/10 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.
|
|
||
| // Each download task runs concurrently with context awareness. | ||
| for i, run := range runs { | ||
| if cachedResult, ok := cachedJSONDownloadResult(run, opts.cachedRuns, opts.filters); ok { |
There was a problem hiding this comment.
Fixed in 47882d5. The default discovery path now requests attempt, fills WorkflowRun.Repository before cache lookup, and cache reuse requires exact repository/attempt/update-time identity.
| MissingToolCount: data.MissingToolCount, | ||
| MissingDataCount: data.MissingDataCount, | ||
| SafeItemsCount: data.SafeItemsCount, | ||
| LogsPath: data.LogsPath, |
There was a problem hiding this comment.
Fixed in 47882d5. Cached logs_path is preserved only through the cached RunData; ProcessedRun.Run.LogsPath is left empty so downstream artifact readers skip cached records.
| agg.totalTemporaryIDMappings += run.TemporaryIDMappings | ||
| agg.totalChainedTargets += run.ChainedTargetCount | ||
| agg.totalChainedFollowupActions += run.ChainedFollowupActionCount | ||
| agg.totalClosedTempTargets += run.ClosedTempTargetCount |
There was a problem hiding this comment.
Fixed in 47882d5. Cached chain metrics now use the same aggregate counter path, so runs_with_* counters are populated along with totals.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ponytail pass focused only on deletable complexity in changed lines. Suggested cuts target duplicate aggregation paths, extra wrapper concepts, and avoidable coupling between cache and processed models.
net: -35 lines possible.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
ab.chatgpt.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
Generated by ✂️ Ponytail Reviewer for #59572 · codex · gpt53codex · 8.01 AIC · ⌖ 4.29 AIC · ⊞ 13.9K
Comment /ponytail to run again
| MissingDataCount: data.MissingDataCount, | ||
| SafeItemsCount: data.SafeItemsCount, | ||
| LogsPath: data.LogsPath, | ||
| }, |
There was a problem hiding this comment.
pkg/cli/logs_cached_json.go:L107: yagni: pointer in only bypasses recomputation. Keep in and build output directly, no extra cross-type coupling.
There was a problem hiding this comment.
Addressed in 47882d5 by removing cached logs_path from ProcessedRun.Run. I kept cachedData because it preserves the original cached RunData for output without artifact reprocessing.
| agg.totalSafeItems += run.SafeItemsCount | ||
| } | ||
|
|
||
| func (agg *logsAggregate) accumulateCachedRunTotals(run RunData) { |
There was a problem hiding this comment.
pkg/cli/logs_report.go:L255: shrink: 25-line duplicates . Convert cached record to and call one aggregator path.
There was a problem hiding this comment.
Partially addressed in 47882d5. The cached aggregate path now reuses chain aggregation and includes the missing counters; I kept a separate cached accumulator so cached output can preserve the original RunData exactly.
| if cached.Status != "completed" || run.Status != "completed" || cached.Conclusion != run.Conclusion { | ||
| return RunData{}, false | ||
| } | ||
| if cached.RunAttempt != "" && run.Attempt > 0 && cached.RunAttempt != strconv.Itoa(run.Attempt) { |
There was a problem hiding this comment.
pkg/cli/logs_cached_json.go:L47: yagni: dual run-attempt branches ( and ) encode legacy shape in lookup. Normalize attempt once when loading cache, then compare one field.
There was a problem hiding this comment.
Fixed in 47882d5. Cached run_attempt is normalized once while loading the cache, and lookup now compares a single normalized field.
| @@ -156,6 +158,11 @@ func downloadRunArtifactsConcurrent(ctx context.Context, runs []WorkflowRun, opt | |||
|
|
|||
There was a problem hiding this comment.
pkg/cli/logs_run_processor.go:L158: shrink: inline cache hit handling into and return early result; removes loop-local branching and duplicated completion bookkeeping.
There was a problem hiding this comment.
No code change for this one. I kept the cache-hit branch localized in downloadRunArtifactsConcurrent because the current early-continue path keeps the concurrent scheduling and completion bookkeeping explicit.
| return cached, true | ||
| } | ||
|
|
||
| func cachedJSONCanSatisfy(artifactFilter []string, parse, audit, train, toolGraph bool) bool { |
There was a problem hiding this comment.
pkg/cli/logs_cached_json.go:L74: yagni: dedicated wrapper adds an extra concept around a single predicate. Inline at callsite.
There was a problem hiding this comment.
No code change for this one. I kept cachedJSONCanSatisfy centralized because the same usage-only predicate guards cache loading in both download and stdin paths.
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.
|
There was a problem hiding this comment.
The cached JSON path is a useful optimization, but the reuse checks are still too loose for a merge-safe logs cache.
Blocking themes
- cached entries can be reused for a different workflow run from the same repository when the run ID is recycled across repos or the current API response omits repository metadata
- cached records can report stale aggregate values because the reuse guard does not validate immutable identity fields beyond status/conclusion/attempt/update time
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 33.6 AIC · ⌖ 7.34 AIC · ⊞ 23.5K
Comment /review to run again
🏗️ ADR Required — draft added for PR #59572I added a draft ADR to this PR branch:
Why this gate triggeredThis PR exceeds the default ADR volume threshold for business-logic code:
Evidence usedThe PR introduces a new Next actionPlease review and refine the draft ADR, especially the decision wording and trade-offs, before merging this PR.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd to the new cached-JSON reuse path in gh aw logs.
Overall this is a well-tested, carefully-guarded feature — the lookup() match conditions (repository, attempt, conclusion, updated-at, engine, artifact-dependent filters) are thorough and each has a matching test in logs_cached_json_test.go. Two issues worth addressing before merge:
📋 Key Findings
- Steering-events aggregate silently dropped for cached runs (
logs_report.go):accumulateCachedRunTotalsmirrorsaccumulateRunTotalsfield-by-field but omitsTotalSteeringEventsfromRunData.TokenUsageSummary, so--cached-jsonsummaries under-report this metric with no error or warning. - Attempt-match gap when both sides are unknown (
logs_cached_json.go): whencached.RunAttempt == ""andrun.Attempt == 0, neither guard clause rejects the pair, so a stale cache entry could be reused across attempts if attempt metadata happens to be missing on both sides.
Both are called out inline with suggested fixes/tests. Neither blocks merge outright (the feature is additive and gated behind --cached-json), but the first is a real correctness gap in a documented aggregate.
@copilot please address the review comments above.
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 Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 72.7 AIC · ⌖ 14.9 AIC · ⊞ 10.3K
Comment /matt to run again
| func (agg *logsAggregate) accumulateCachedRunTotals(run RunData) { | ||
| agg.totalDuration += parseDurationString(run.Duration) | ||
| agg.totalAIC += run.AIC | ||
| agg.totalTokens += run.TokenUsage |
There was a problem hiding this comment.
[/diagnosing-bugs] accumulateCachedRunTotals silently drops steering-event counts for cached runs, so --cached-json output undercounts TotalSteeringEvents in the summary even though the data is present.
💡 Details
The non-cached path (accumulateRunTotals, line 236) adds pr.TokenUsage.TotalSteeringEvents to agg.totalSteeringEvents. RunData carries the equivalent value in TokenUsageSummary.TotalSteeringEvents (see token_usage_types.go), but accumulateCachedRunTotals never reads it, so any run served from the cache is silently missing from this aggregate — no error, no fallback, just a quietly wrong total.
Suggested fix:
func (agg *logsAggregate) accumulateCachedRunTotals(run RunData) {
...
if run.TokenUsageSummary != nil {
agg.totalSteeringEvents += run.TokenUsageSummary.TotalSteeringEvents
}
}Also add a regression test analogous to TestBuildLogsDataAggregatesSteeringEvents (logs_report_test.go:1190) but exercising processedRunFromCachedData/cached path, so future changes to either accumulation function can't silently diverge again.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 47882d5. Cached steering events now aggregate from TokenUsageSummary, and the cached-run regression test covers the value.
| if !cached.UpdatedAt.IsZero() && !run.UpdatedAt.IsZero() && !cached.UpdatedAt.Equal(run.UpdatedAt) { | ||
| return RunData{}, false | ||
| } | ||
| if cached.Repository != "" && run.Repository != "" && !strings.EqualFold(cached.Repository, run.Repository) { |
There was a problem hiding this comment.
[/tdd] The attempt-matching fallback treats a missing cached.RunAttempt ("") plus run.Attempt == 0 (unknown/unset) as a match, since neither branch on lines 53/56 rejects that combination. That's the one case where both sides are genuinely unknown, so a rerun with a different actual attempt could be silently reused from cache.
💡 Suggested test
func TestCachedLogsLookupRejectsUnknownAttemptOnBothSides(t *testing.T) {
runs := cachedLogsRuns{42: {RunID: 42, Status: "completed", Conclusion: "success"}} // RunAttempt == ""
run := WorkflowRun{DatabaseID: 42, Status: "completed", Conclusion: "success"} // Attempt == 0
_, ok := runs.lookup(run, runFilterOpts{})
assert.False(t, ok, "should not reuse cache when attempt is unknown on both sides")
}If this is intentionally accepted as a safe default (e.g. attempt is always populated in practice), consider a comment explaining why, since it's the one gap in an otherwise thorough set of match conditions.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 47882d5. Cache lookup now rejects unknown attempt identity, including the both-sides-unknown case, and regression coverage was added.
There was a problem hiding this comment.
Reviewed the new cached-JSON reuse logic for gh aw logs (pkg/cli/logs_cached_json.go and its call sites).
Blocking issue: the attempt/repository staleness checks in cachedLogsRuns.lookup() are effectively bypassed on the default (non---stdin) download path, because WorkflowRun.Attempt and .Repository are never populated before the cache lookup runs there (see inline comment). This means a cached record from a prior attempt or different repository could be silently reused for usage-only queries.
Everything else (loading, filter gating via cachedJSONCanSatisfy, test coverage for the lookup() function itself) looks solid; the gap is specifically in how/when WorkflowRun fields are populated before the cache check in the primary flow.
Additional lower-confidence note (not blocking)
Cached RunData.LogsPath values come from a previous run/machine and may not exist on the current filesystem. Cross-run rendering (deriveGradersClusterValue/deriveEvalsClusterValue in logs_orchestrator_render.go) reads pr.Run.LogsPath unconditionally (not gated by cachedJSONCanSatisfy's artifact-filter restrictions), so if a stale path is dereferenced there it could produce empty/incorrect cluster values rather than a crash — worth a quick existence check, but not confirmed as a real bug on this pass.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 169.4 AIC · ⌖ 14.6 AIC · ⊞ 8.3K
| if cached.Status != "completed" || run.Status != "completed" || cached.Conclusion != run.Conclusion { | ||
| return RunData{}, false | ||
| } | ||
| if cached.RunAttempt != "" && run.Attempt > 0 && cached.RunAttempt != strconv.Itoa(run.Attempt) { |
There was a problem hiding this comment.
The PR description promises cache reuse only when repository, attempt, conclusion, and update timestamp all match, and lookup() implements those checks — but for the primary (non---stdin) download path, run.Attempt and run.Repository are never populated before this function is called.
listWorkflowRunsWithPagination (pkg/cli/logs_github_api.go:406) queries gh run list --json databaseId,number,url,status,conclusion,workflowName,createdAt,startedAt,updatedAt,event,headBranch,headSha,displayTitle — it omits attempt entirely (a valid gh run list --json field) and there is no repository field available from gh run list at all. applyWorkflowRunMetadata only back-fills run.Attempt/run.Repository from a follow-up API call made inside processSingleRunDownload, i.e. after the cache short-circuit in downloadRunArtifactsConcurrent (pkg/cli/logs_run_processor.go) has already decided whether to reuse the cache.
Concretely, for every run reached through the default (non-stdin) path:
run.Attemptis always0, so both attempt guards (cached.RunAttempt != "" && run.Attempt > 0 && ...andcached.RunAttempt == "" && run.Attempt > 1) can never reject a stale attempt.run.Repositoryis always"", so the repository guard (cached.Repository != "" && run.Repository != "" && ...) is also always skipped.
updatedAt happens to change on re-run so it provides some incidental protection today, but that's coincidental — the attempt/repository checks documented in the PR description are effectively dead code on this path. Please add attempt to the gh run list --json field list (and populate WorkflowRun.Repository from opts.RepoOverride where available) so the safety checks in lookup() actually run against real values in the default flow, not just the --stdin path where fetchWorkflowRunMetadata fully populates the run first.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 47882d5. gh run list now requests attempt, repository is populated from the target repo before lookup, and lookup requires exact populated attempt/repository/update timestamp values.
|
Great work! 🎉 This PR from the Copilot SWE agent adds a valuable optimization to the logs command — caching and reusing run records from previous JSON output. What looks good:
The implementation demonstrates solid engineering: graceful fallback to normal processing for stale records or artifact-dependent filters shows robustness. This is ready for review by the team!
|
@copilot Please take the next forward-progress pass on PR #59572.
Blocking review highlights:
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.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Fixed in 47882d5. I tightened default-path cache identity, re-checked the remaining review notes, ran |
@copilot Please take the next forward-progress pass on PR #59572.
|
…and-cached-json Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Completed in 7b3a606. I re-checked the cache identity and aggregate fixes on current HEAD, merged latest |
gh aw logsrecomputed records already available in earlier JSON output. This adds--cached-jsonto reuse unchanged run records while allowing approximate aggregates from compact cached data.Changes
Cache loading
logs --jsonoutput and index records by run ID.Safe reuse
Output
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.