-
Notifications
You must be signed in to change notification settings - Fork 538
Add cached JSON reuse to the logs command #59572
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.
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
Changes from all commits
219ea8d
a31856a
8917406
47882d5
b6450a1
7b3a606
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| # ADR-59572: Reuse compatible logs JSON run records | ||
|
|
||
| **Date**: 2026-09-09 | ||
| **Status**: Draft | ||
| **Deciders**: gh-aw maintainers | ||
|
|
||
| --- | ||
|
|
||
| ### Context | ||
|
|
||
| The `gh aw logs` command currently recomputes run records by downloading and processing artifacts even when equivalent `logs --json` output from an earlier run already exists. This PR adds a new `--cached-json` input, threads it through both standard and stdin-based log collection flows, and rebuilds report output from a mix of reused cached records and newly processed runs. The diff shows strict cache-safety checks around run identity, completion state, attempt, repository, conclusion, and update time, plus explicit fallbacks when requested filters require artifact-level evidence not preserved in compact JSON. The repository needs a documented decision on whether prior JSON output is an acceptable cache source for logs analysis and reporting. | ||
|
|
||
| ### Decision | ||
|
|
||
| We will allow `gh aw logs` to reuse prior `--json` output as a cache source when a cached record can be proven compatible with the current workflow run and requested analysis mode. The implementation will only reuse completed runs with matching run ID, repository, attempt, conclusion, and update timestamp, and it will disable cached reuse for artifact-dependent filters or analysis modes that require richer evidence than compact JSON retains. When reuse is valid, the command will preserve cached run records in rebuilt JSON output and recompute aggregate totals from the combined cached and newly processed data. | ||
|
|
||
| ### Alternatives Considered | ||
|
|
||
| #### Alternative 1: Always re-download and reprocess run artifacts | ||
|
|
||
| The command could continue treating every invocation as a fresh collection pass with no reuse of earlier JSON output. This was considered because it is the simplest behavior and avoids the risk of stale cached data. It was not chosen because the PR explicitly adds compatibility checks and tests to avoid unnecessary artifact work for unchanged completed runs. | ||
|
|
||
| #### Alternative 2: Reuse cached JSON for all runs and filter modes without validation | ||
|
|
||
| Another option would be to accept any previous logs JSON as authoritative and skip most per-run validation and mode checks. This was considered because it would maximize performance improvements and implementation simplicity. It was not chosen because the diff adds explicit guards for repository, attempt, conclusion, updated timestamp, engine filters, and artifact-dependent modes, showing that unvalidated reuse would be unsafe. | ||
|
|
||
| #### Alternative 3: Introduce a dedicated internal cache format instead of reusing prior JSON output | ||
|
|
||
| The project could have created a separate opaque cache artifact tailored specifically for reuse rather than depending on user-visible JSON output. This was considered because a dedicated format could carry richer evidence and fewer compatibility constraints. It was not chosen in this PR because the implementation intentionally reuses existing `logs --json` output, preserving current user workflows and avoiding an additional cache format. | ||
|
|
||
| ### Consequences | ||
|
|
||
| #### Positive | ||
| - Re-running `gh aw logs` can avoid downloading and reprocessing artifacts for unchanged completed runs, reducing cost and latency. | ||
| - Cache reuse remains conservative because compatibility checks reject stale or insufficient cached records. | ||
| - Rebuilt reports can combine cached and fresh records while preserving existing JSON output structure. | ||
|
|
||
| #### Negative | ||
| - The logs pipeline becomes more complex because download, stdin, filtering, and aggregation paths must all account for cached records. | ||
| - Aggregate values may be approximate when compact cached JSON omits detailed evidence that richer artifact processing would have produced. | ||
| - Users must understand when `--cached-json` is ignored due to incompatible filters or analysis modes. | ||
|
|
||
| #### Neutral | ||
| - `LogsDownloadOptions`, `StdinLogsOptions`, and related orchestration types now carry a `CachedJSON` field through multiple entry points. | ||
| - Report aggregation now has a separate path for accumulating totals from cached `RunData` values. | ||
| - The feature relies on previous JSON output remaining parseable and structurally compatible with the current `LogsData` schema. | ||
|
|
||
| --- | ||
|
|
||
| *ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,127 @@ | ||
| package cli | ||
|
|
||
| import ( | ||
| "encoding/json" | ||
| "errors" | ||
| "fmt" | ||
| "os" | ||
| "strconv" | ||
| "strings" | ||
| ) | ||
|
|
||
| type cachedLogsRuns map[int64]RunData | ||
|
|
||
| func loadCachedLogsJSON(path string) (cachedLogsRuns, error) { | ||
| if path == "" { | ||
| return nil, nil | ||
| } | ||
| data, err := os.ReadFile(path) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to read cached logs JSON: %w", err) | ||
| } | ||
| var logsData LogsData | ||
| if err := json.Unmarshal(data, &logsData); err != nil { | ||
| return nil, fmt.Errorf("failed to parse cached logs JSON: %w", err) | ||
| } | ||
| if logsData.Runs == nil { | ||
| return nil, errors.New("failed to parse cached logs JSON: missing runs array") | ||
| } | ||
| runs := make(cachedLogsRuns, len(logsData.Runs)) | ||
| for _, run := range logsData.Runs { | ||
| if err := normalizeCachedLogRun(&run); err != nil { | ||
| return nil, err | ||
| } | ||
| if run.RunID != 0 { | ||
| runs[run.RunID] = run | ||
| } | ||
| } | ||
| logsCacheLog.Printf("Loaded %d run records from cached logs JSON", len(runs)) | ||
| return runs, nil | ||
| } | ||
|
|
||
| func (runs cachedLogsRuns) lookup(run WorkflowRun, filters runFilterOpts) (RunData, bool) { | ||
| cached, ok := runs[run.DatabaseID] | ||
| if !ok { | ||
| return RunData{}, false | ||
| } | ||
| 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) { | ||
| return RunData{}, false | ||
| } | ||
| 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) { | ||
| return RunData{}, false | ||
| } | ||
| if filters.engine != "" && | ||
| !strings.EqualFold(filters.engine, cached.EngineID) && | ||
| !strings.EqualFold(filters.engine, cached.Engine) && | ||
| !strings.EqualFold(filters.engine, cached.Agent) { | ||
| return RunData{}, false | ||
| } | ||
| // Compact logs JSON does not retain enough per-run evidence to safely | ||
| // re-evaluate these artifact-dependent filters. | ||
| if filters.runtime != "" || filters.noStaged || filters.firewallOnly || filters.noFirewall || | ||
| filters.safeOutputType != "" || filters.filteredIntegrity || filters.evalsOnly || filters.gradersOnly { | ||
| return RunData{}, false | ||
| } | ||
| return cached, true | ||
| } | ||
|
|
||
| func normalizeCachedLogRun(run *RunData) error { | ||
| if run.RunAttempt == "" { | ||
| return nil | ||
| } | ||
| attempt, err := strconv.Atoi(run.RunAttempt) | ||
| if err != nil || attempt <= 0 { | ||
| return fmt.Errorf("failed to parse cached logs JSON: invalid run_attempt for run %d", run.RunID) | ||
| } | ||
| run.RunAttempt = strconv.Itoa(attempt) | ||
| return nil | ||
| } | ||
|
|
||
| func cachedJSONCanSatisfy(artifactFilter []string, parse, audit, train, toolGraph bool) bool { | ||
| return isUsageOnlyArtifactFilter(artifactFilter) && !parse && !audit && !train && !toolGraph | ||
| } | ||
|
|
||
| func processedRunFromCachedData(data RunData) ProcessedRun { | ||
| return ProcessedRun{ | ||
| Run: WorkflowRun{ | ||
| DatabaseID: data.RunID, | ||
| Number: data.Number, | ||
| URL: data.URL, | ||
| Status: data.Status, | ||
| Conclusion: data.Conclusion, | ||
| WorkflowName: data.WorkflowName, | ||
| WorkflowPath: data.WorkflowPath, | ||
| CreatedAt: data.CreatedAt, | ||
| StartedAt: data.StartedAt, | ||
| UpdatedAt: data.UpdatedAt, | ||
| Event: data.Event, | ||
| HeadBranch: data.Branch, | ||
| HeadSha: data.HeadSHA, | ||
| DisplayTitle: data.DisplayTitle, | ||
| Repository: data.Repository, | ||
| Actor: data.Actor, | ||
| Duration: parseDurationString(data.Duration), | ||
| ActionMinutes: data.ActionMinutes, | ||
| TokenUsage: data.TokenUsage, | ||
| Turns: data.Turns, | ||
| ErrorCount: data.ErrorCount, | ||
| WarningCount: data.WarningCount, | ||
| MissingToolCount: data.MissingToolCount, | ||
| MissingDataCount: data.MissingDataCount, | ||
| SafeItemsCount: data.SafeItemsCount, | ||
| }, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. pkg/cli/logs_cached_json.go:L107: yagni: pointer in only bypasses recomputation. Keep in and build output directly, no extra cross-type coupling.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in 47882d5 by removing cached |
||
| AwContext: data.AwContext, | ||
| TaskDomain: data.TaskDomain, | ||
| BehaviorFingerprint: data.BehaviorFingerprint, | ||
| AgenticAssessments: data.AgenticAssessments, | ||
| TokenUsage: data.TokenUsageSummary, | ||
| WorkingSet: data.WorkingSet, | ||
| cachedData: &data, | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,191 @@ | ||
| //go:build !integration | ||
|
|
||
| package cli | ||
|
|
||
| import ( | ||
| "context" | ||
| "encoding/json" | ||
| "os" | ||
| "path/filepath" | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/github/gh-aw/pkg/constants" | ||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestLoadCachedLogsJSON(t *testing.T) { | ||
| path := filepath.Join(t.TempDir(), "logs.json") | ||
| data, err := json.Marshal(LogsData{Runs: []RunData{ | ||
| {RunID: 42, WorkflowName: "cached-workflow"}, | ||
| {RunID: 0, WorkflowName: "invalid"}, | ||
| }}) | ||
| require.NoError(t, err) | ||
| require.NoError(t, os.WriteFile(path, data, 0o600)) | ||
|
|
||
| runs, err := loadCachedLogsJSON(path) | ||
|
|
||
| require.NoError(t, err) | ||
| require.Len(t, runs, 1) | ||
| assert.Equal(t, "cached-workflow", runs[42].WorkflowName) | ||
| } | ||
|
|
||
| func TestLoadCachedLogsJSONRejectsInvalidInput(t *testing.T) { | ||
| path := filepath.Join(t.TempDir(), "logs.json") | ||
| require.NoError(t, os.WriteFile(path, []byte(`{"summary":{}}`), 0o600)) | ||
|
|
||
| _, err := loadCachedLogsJSON(path) | ||
|
|
||
| require.ErrorContains(t, err, "missing runs array") | ||
| } | ||
|
|
||
| func TestLoadCachedLogsJSONRejectsInvalidRunAttempt(t *testing.T) { | ||
| path := filepath.Join(t.TempDir(), "logs.json") | ||
| require.NoError(t, os.WriteFile(path, []byte(`{"runs":[{"run_id":42,"run_attempt":"bogus"}]}`), 0o600)) | ||
|
|
||
| _, err := loadCachedLogsJSON(path) | ||
|
|
||
| require.ErrorContains(t, err, "invalid run_attempt") | ||
| } | ||
|
|
||
| func TestCachedLogsLookupHonorsRepositoryAndFilters(t *testing.T) { | ||
| updatedAt := time.Date(2026, time.September, 1, 0, 0, 0, 0, time.UTC) | ||
| runs := cachedLogsRuns{ | ||
| 42: {RunID: 42, Repository: "github/gh-aw", EngineID: "copilot", Status: "completed", Conclusion: "success", RunAttempt: "1", UpdatedAt: updatedAt}, | ||
| } | ||
| run := WorkflowRun{DatabaseID: 42, Repository: "github/gh-aw", Status: "completed", Conclusion: "success", Attempt: 1, UpdatedAt: updatedAt} | ||
|
|
||
| _, ok := runs.lookup(run, runFilterOpts{engine: "copilot"}) | ||
| assert.True(t, ok) | ||
| _, ok = runs.lookup(run, runFilterOpts{engine: "claude"}) | ||
| assert.False(t, ok) | ||
| _, ok = runs.lookup(run, runFilterOpts{runtime: "gvisor"}) | ||
| assert.False(t, ok) | ||
| _, ok = runs.lookup(WorkflowRun{DatabaseID: 42, Repository: "other/repo", Status: "completed", Conclusion: "success", Attempt: 1, UpdatedAt: updatedAt}, runFilterOpts{}) | ||
| assert.False(t, ok) | ||
| } | ||
|
|
||
| func TestCachedLogsLookupRejectsChangedRun(t *testing.T) { | ||
| updatedAt := time.Date(2026, time.September, 1, 0, 0, 0, 0, time.UTC) | ||
| runs := cachedLogsRuns{ | ||
| 42: { | ||
| RunID: 42, | ||
| Status: "completed", | ||
| Conclusion: "success", | ||
| RunAttempt: "1", | ||
| UpdatedAt: updatedAt, | ||
| }, | ||
| } | ||
|
|
||
| tests := []WorkflowRun{ | ||
| {DatabaseID: 42, Status: "in_progress", Conclusion: "", Attempt: 1, UpdatedAt: updatedAt}, | ||
| {DatabaseID: 42, Status: "completed", Conclusion: "failure", Attempt: 1, UpdatedAt: updatedAt}, | ||
| {DatabaseID: 42, Status: "completed", Conclusion: "success", Attempt: 2, UpdatedAt: updatedAt}, | ||
| {DatabaseID: 42, Status: "completed", Conclusion: "success", Attempt: 1, UpdatedAt: updatedAt.Add(time.Minute)}, | ||
| } | ||
| for _, run := range tests { | ||
| _, ok := runs.lookup(run, runFilterOpts{}) | ||
| assert.False(t, ok) | ||
| } | ||
| } | ||
|
|
||
| func TestCachedLogsLookupRejectsUnknownIdentity(t *testing.T) { | ||
| updatedAt := time.Date(2026, time.September, 1, 0, 0, 0, 0, time.UTC) | ||
| runs := cachedLogsRuns{ | ||
| 42: {RunID: 42, Repository: "github/gh-aw", Status: "completed", Conclusion: "success", RunAttempt: "1", UpdatedAt: updatedAt}, | ||
| 43: {RunID: 43, Status: "completed", Conclusion: "success", UpdatedAt: updatedAt}, | ||
| } | ||
|
|
||
| tests := []WorkflowRun{ | ||
| {DatabaseID: 42, Repository: "github/gh-aw", Status: "completed", Conclusion: "success", UpdatedAt: updatedAt}, | ||
| {DatabaseID: 42, Status: "completed", Conclusion: "success", Attempt: 1, UpdatedAt: updatedAt}, | ||
| {DatabaseID: 43, Status: "completed", Conclusion: "success", UpdatedAt: updatedAt}, | ||
| } | ||
| for _, run := range tests { | ||
| _, ok := runs.lookup(run, runFilterOpts{}) | ||
| assert.False(t, ok) | ||
| } | ||
| } | ||
|
|
||
| func TestCachedJSONCanSatisfy(t *testing.T) { | ||
| usageFilter := []string{constants.UsageArtifactName.String()} | ||
| assert.True(t, cachedJSONCanSatisfy(usageFilter, false, false, false, false)) | ||
| assert.False(t, cachedJSONCanSatisfy(nil, false, false, false, false)) | ||
| assert.False(t, cachedJSONCanSatisfy(usageFilter, true, false, false, false)) | ||
| assert.False(t, cachedJSONCanSatisfy(usageFilter, false, true, false, false)) | ||
| assert.False(t, cachedJSONCanSatisfy(usageFilter, false, false, true, false)) | ||
| assert.False(t, cachedJSONCanSatisfy(usageFilter, false, false, false, true)) | ||
| } | ||
|
|
||
| func TestDownloadRunArtifactsConcurrentReusesCachedJSONRecord(t *testing.T) { | ||
| cached := RunData{ | ||
| RunID: 42, | ||
| WorkflowName: "cached-workflow", | ||
| Repository: "github/gh-aw", | ||
| Status: "completed", | ||
| Conclusion: "success", | ||
| RunAttempt: "1", | ||
| UpdatedAt: time.Date(2026, time.September, 1, 0, 0, 0, 0, time.UTC), | ||
| LogsPath: "/previous/run-42", | ||
| } | ||
|
|
||
| results := downloadRunArtifactsConcurrent(context.Background(), []WorkflowRun{{DatabaseID: 42, Repository: "github/gh-aw", Status: "completed", Conclusion: "success", Attempt: 1, UpdatedAt: cached.UpdatedAt}}, runArtifactsConcurrentOptions{ | ||
| outputDir: t.TempDir(), | ||
| maxRuns: 1, | ||
| cachedRuns: cachedLogsRuns{42: cached}, | ||
| storageLimit: newLogsStorageLimit(t.TempDir(), 0, false), | ||
| }) | ||
|
|
||
| require.Len(t, results, 1) | ||
| require.NotNil(t, results[0].CachedRun) | ||
| assert.True(t, results[0].Cached) | ||
| assert.Equal(t, cached, *results[0].CachedRun) | ||
| } | ||
|
|
||
| func TestBuildLogsDataPreservesCachedRunRecord(t *testing.T) { | ||
| cached := RunData{ | ||
| RunID: 42, | ||
| WorkflowName: "cached-workflow", | ||
| Status: "completed", | ||
| Conclusion: "success", | ||
| Duration: "2m0s", | ||
| TokenUsageSummary: &TokenUsageSummary{TotalSteeringEvents: 2}, | ||
| TokenUsage: 1200, | ||
| Turns: 4, | ||
| ErrorCount: 1, | ||
| WarningCount: 2, | ||
| GitHubAPICalls: 3, | ||
| TemporaryIDMappings: 4, | ||
| ChainedTargetCount: 1, | ||
| ChainedFollowupActionCount: 2, | ||
| DelegatedTempTargetCount: 1, | ||
| TemporaryIDMapStatus: temporaryIDMapStatusMissing, | ||
| EngineID: "copilot", | ||
| CreatedAt: time.Date(2026, time.September, 1, 0, 0, 0, 0, time.UTC), | ||
| LogsPath: "/previous/run-42", | ||
| Classification: "normal", | ||
| IntentionalFailure: true, | ||
| } | ||
|
|
||
| processedRun := processedRunFromCachedData(cached) | ||
| assert.Empty(t, processedRun.Run.LogsPath) | ||
|
|
||
| data := buildLogsData([]ProcessedRun{processedRun}, t.TempDir(), nil) | ||
|
|
||
| require.Equal(t, []RunData{cached}, data.Runs) | ||
| assert.Equal(t, 1, data.Summary.TotalRuns) | ||
| assert.Equal(t, "2.0m", data.Summary.TotalDuration) | ||
| assert.Equal(t, 2, data.Summary.TotalSteeringEvents) | ||
| assert.Equal(t, 1200, data.Summary.TotalTokens) | ||
| assert.Equal(t, 4, data.Summary.TotalTurns) | ||
| assert.Equal(t, 3, data.Summary.TotalGitHubAPICalls) | ||
| assert.Equal(t, 1, data.Summary.RunsWithTemporaryIDChains) | ||
| assert.Equal(t, 1, data.Summary.RunsWithDelegatedTempTargets) | ||
| assert.Equal(t, 1, data.Summary.RunsWithMissingTemporaryIDMap) | ||
| assert.Equal(t, 4, data.Summary.TotalTemporaryIDMappings) | ||
| assert.Equal(t, 1, data.Summary.TotalChainedTargets) | ||
| assert.Equal(t, 2, data.Summary.TotalChainedFollowupActions) | ||
| assert.Equal(t, map[string]int{"copilot": 1}, data.Summary.EngineCounts) | ||
| assert.Equal(t, 1, data.Summary.IntentionalFailureRuns) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No code change for this one. I kept
cachedJSONCanSatisfycentralized because the same usage-only predicate guards cache loading in both download and stdin paths.