Add cached audit output to logs command - #59270
Conversation
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.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ 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.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ponytail review: one meaningful simplification found in this diff.
net: -6 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 #59270 · codex · gpt53codex · 5.83 AIC · ⌖ 3.13 AIC · ⊞ 13.9K
Comment /ponytail to run again
| } | ||
| return "" | ||
| } | ||
|
|
There was a problem hiding this comment.
pkg/cli/audit_cache.go:L23: shrink: separate existingAuditPath helper exists only for one callsite. Inline os.Stat check directly in newRunData to avoid an extra abstraction layer.
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (>100 new lines) but does not have a linked Architecture Decision Record (ADR). 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. 🔎 PR evidence used to draft the ADR
❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on a cache-correctness gap between the two audit code paths.
📋 Key Themes & Highlights
Key Themes
- Stale/impoverished cache reuse:
renderAuditReport(fullgh aw audit) will reuse anaudit.jsonwritten bylogs --audit, which is missingOutcomes/OutcomeSummaryand uses a narrower, network-free comparison. Since the cache key is only run ID + status + conclusion, this silently downgrades the full audit output indefinitely once a--audit-cached file exists. - Undocumented scope difference: comparisons built during
logs --auditonly look at runs already downloaded in the current batch, whereas the standaloneauditcommand can fetch historical baselines over the network. This distinction isn't tested or documented in the flag help text.
Positive Highlights
- ✅ Good test coverage for the new cache read/write/invalidate logic in
audit_cache_test.go(state-change invalidation is well tested). - ✅ Clean extraction of
buildLocalAuditDatafrombuildAuditData, keeping the local-only path free of GitHub API calls as intended. - ✅ Reasonable, minimal wiring of the new
--audit/Auditflag through the options structs.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 29.1 AIC · ⌖ 14.7 AIC · ⊞ 10.3K
Comment /matt to run again
| runOutputDir := opts.OutputDir | ||
| processedRun.Run.SafeItemsCount = len(extractCreatedItemsFromManifest(runOutputDir)) | ||
| auditData := buildRenderedAuditData(ctx, processedRun, metrics, mcpToolUsage, runOutputDir, opts) | ||
| auditData, ok := loadCachedAuditData(runOutputDir, processedRun.Run) |
There was a problem hiding this comment.
[/diagnosing-bugs] Cache correctness gap: loadCachedAuditData accepts any audit.json matching run ID/status/conclusion, but the file may have been written by the local-only path (writeLogsAuditFiles -> buildLocalAuditData), which skips addAuditOutcomeSummary (no Outcomes/OutcomeSummary) and uses an in-memory comparison instead of the network baseline comparison used here. If a user runs gh aw logs --audit first and then gh aw audit on the same run, this cache hit silently returns the impoverished data forever (state fields still match), even though the full command normally computes outcomes/network-based comparisons.
💡 Suggested fix
Tag cached audit data with its provenance (e.g. an Overview.Source field like "local" vs "full"), and have renderAuditReport only reuse the cache when it was produced by an equivalent-or-richer code path. Alternatively, only let logs --audit cache be reused by logs --audit itself, not by the full gh aw audit command.
@copilot please address this.
| return fmt.Errorf("failed to marshal audit data: %w", err) | ||
| } | ||
| if err := os.MkdirAll(runOutputDir, constants.DirPermSensitive); err != nil { | ||
| return fmt.Errorf("failed to create audit output directory: %w", err) |
There was a problem hiding this comment.
[/tdd] writeLogsAuditFiles builds comparisons only from the already-downloaded processedRuns set (buildAuditComparisonForProcessedRuns), unlike the full gh aw audit path which can download baseline artifacts over the network (buildAuditComparisonForRun). This behavioral difference between --audit-cached data and full-audit data isn't covered by a test, and isn't called out in the flag help text (--audit help just says "Generate audit.json...").
💡 Suggested test / doc
Add a test asserting writeLogsAuditFiles comparison only considers runs present in the current batch (no network calls), and update the --audit flag description to mention that comparisons are limited to downloaded runs in the batch.
@copilot please address this.
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.
Requesting changes
The new audit-cache path adds useful functionality, but it currently turns a per-run cache write failure into a fatal error for the entire gh aw logs command.
Blocking theme
--audit now writes audit.json during prepareLogsData, and writeLogsAuditFiles bubbles the first per-run filesystem failure up as failed to write audit files. That means a single bad cache directory can break rendering for every other successfully processed run, which is too fragile for a post-download reporting feature.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 27.6 AIC · ⌖ 7.35 AIC · ⊞ 21.8K
Comment /review to run again
There was a problem hiding this comment.
🟡 Changes recommended
Cache reuse can return incomplete or stale audit data, and the audit file uses an overly permissive mode.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds cached per-run audit generation to gh aw logs.
Changes:
- Adds and propagates the
--auditoption across all log modes. - Generates, caches, links, and reuses
audit.json. - Adds command and cache tests.
File summaries
| File | Description |
|---|---|
pkg/cli/logs_report.go |
Exposes audit_path in run data. |
pkg/cli/logs_orchestrator.go |
Propagates audit configuration. |
pkg/cli/logs_orchestrator_types.go |
Adds audit option fields. |
pkg/cli/logs_orchestrator_stdin.go |
Enables audits for stdin mode. |
pkg/cli/logs_orchestrator_render.go |
Generates audits before rendering. |
pkg/cli/logs_multi.go |
Enables audits for multi-target mode. |
pkg/cli/logs_models.go |
Defines the audit cache filename. |
pkg/cli/logs_command.go |
Adds the CLI flag and help text. |
pkg/cli/logs_command_test.go |
Tests flag registration and parsing. |
pkg/cli/audit_report.go |
Separates local and enriched audit construction. |
pkg/cli/audit_render_output.go |
Persists and reuses standalone audits. |
pkg/cli/audit_cache.go |
Implements audit cache handling. |
pkg/cli/audit_cache_test.go |
Tests cache reuse and audit paths. |
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 3
- Review effort level: Balanced
| if _, ok := loadCachedAuditData(runOutputDir, processedRun.Run); ok { | ||
| continue | ||
| } |
| auditData, ok := loadCachedAuditData(runOutputDir, processedRun.Run) | ||
| if !ok { | ||
| auditData = buildRenderedAuditData(ctx, processedRun, metrics, mcpToolUsage, runOutputDir, opts) |
There was a problem hiding this comment.
Impeccable review — backend/CLI change (audit caching)
This PR is a Go CLI change (no UI), so Impeccable UI modes don't directly apply; reviewed for correctness/reliability instead.
Found: gh aw audit can silently reuse a partial audit.json written by gh aw logs --audit (missing Outcomes/OutcomeSummary, weaker Comparison), since loadCachedAuditData only checks run id/status/conclusion, not which pipeline produced the file. See inline comment for detail and a suggested fix.
Everything else (flag wiring, file permission usage matching existing conventions, cache invalidation on status/conclusion change, new tests) looks solid.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 90.4 AIC · ⌖ 13.5 AIC · ⊞ 8.3K
| runOutputDir := opts.OutputDir | ||
| processedRun.Run.SafeItemsCount = len(extractCreatedItemsFromManifest(runOutputDir)) | ||
| auditData := buildRenderedAuditData(ctx, processedRun, metrics, mcpToolUsage, runOutputDir, opts) | ||
| auditData, ok := loadCachedAuditData(runOutputDir, processedRun.Run) |
There was a problem hiding this comment.
renderAuditReport (used by gh aw audit <run>) now short-circuits whenever a cached audit.json exists with matching run id/status/conclusion — including files written by the new logs --audit cache path (writeLogsAuditFiles/buildLocalAuditData).
That cache path deliberately builds a partial AuditData: it skips addAuditOutcomeSummary (so Outcomes/OutcomeSummary are never populated) and uses buildAuditComparisonForProcessedRuns instead of buildAuditComparisonForRun (a lighter, local-only comparison that can't look up prior successful runs via the GitHub API). If a user runs gh aw logs --audit first and later runs gh aw audit <run> for the same run, they will silently get this degraded report — no outcomes, weaker comparison data — until the run's status/conclusion changes and the cache is invalidated. That's a soft regression for gh aw audit users who expect full data.
Consider tagging the cached payload with which code path produced it (e.g. a source/complete field) and only reusing a cache for renderAuditReport when it was produced by the full pipeline (or by calling addAuditOutcomeSummary/buildAuditComparisonForRun to backfill missing sections instead of reusing as-is).
@copilot please address this.
@copilot Please take the next forward-progress pass on PR #59270.
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
[!TIP] tools:
github:
mode: gh-proxySee GitHub Tools for more information on To allow these domains, add them to the network:
allowed:
- defaults
- "api.github.com"See Network Configuration for more information.
|
|
@copilot make sure to integrate the Drain 3 fitting inline so that it continues to be updated horizontally without requiring an additional task when auditing a large number of workflow runs. All of this should be done concurrently using concurrency-safe data stores. |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Implemented in f01a332. |
|
/matt |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /codebase-design, and /tdd to the new audit-caching path in gh aw logs --audit. go test -race on the new/changed tests passed cleanly (no data race detected), so the concurrent audit+drain3 training path looks safe in practice — flagged as a nice-to-have to document that explicitly.
📋 Key Themes & Highlights
Key Themes
- Cache invalidation is coarse:
loadCachedAuditDataonly checks run ID/status/conclusion/source, so a re-downloaded run with unchanged status can serve stale content (audit_cache.go). - No cache bypass for
gh aw audit: unlike the append-onlylogs --auditcache, the standalone audit command now also silently reusesaudit.jsonwith no--force/--no-cacheescape hatch (audit_render_output.go). - Concurrency safety unverified in comments: the new concurrent audit+drain3-training path (
writeLogsAuditsAndTrain) is untested for data races in its own right, though the broader test suite passes with-race.
Positive Highlights
- ✅ Solid parallelization of both
writeLogsAuditFilesandTrainDrain3Weightsusingerrgroup, verified safe under-race. - ✅ Good test coverage added for cache hit/miss/refresh behavior (
audit_cache_test.go). - ✅ Clear ADR documenting the caching decision and alternatives considered.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 63.6 AIC · ⌖ 15.1 AIC · ⊞ 10.3K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/cli/audit_cache.go:110
[/diagnosing-bugs] Cache validity is keyed only on RunID/Status/Conclusion/CacheSource. If a run is re-downloaded (e.g. artifacts refreshed, log parsing changed) while status/conclusion stay the same, loadCachedAuditData returns the stale audit.json from the prior data.
<details>
<summary>💡 Suggestion</summary>
Consider also validating against a hash/mtime of source artifacts (or a SafeItemsCount/artifact-set fingerprint) so a completed run re-downloaded with different conten…
pkg/cli/audit_render_output.go:349
[/codebase-design] renderAuditReport now silently reuses whatever audit.json is on disk (via auditCacheSourceFull), even though the standalone gh aw audit command's contract previously always recomputed fresh output including GitHub API enrichment. If a user runs gh aw logs --audit first (cache source logs, no API calls) they get a cache miss here (different source) and it's fine, but if gh aw audit is run twice in a row after any renderer/schema change, stale output is served…
pkg/cli/logs_orchestrator_render.go:128
[/tdd] writeLogsAuditsAndTrain runs writeLogsAuditFiles and TrainDrain3Weights concurrently, both operating over the same processedRuns slice and both writing files under opts.outputDir/each run's LogsPath. There's no test asserting these two concurrent paths don't race on shared state (e.g. processedRun.Run fields) or interleave badly when verbose triggers concurrent stderr writes from both paths.
<details>
<summary>💡 Suggestion</summary>
Run go test -race on the new `…
@copilot Please take the next forward-progress pass on PR #59270.
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.
|
|
@copilot Add integration tests near the other |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the active cache correctness feedback in 2c91af8: logs cache entries now invalidate when the run update timestamp changes, and standalone |
|
@copilot gh aw audit should reuse the cached data to recompute the audit as much as possible. It is critical to avoid gh api calls when possible. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Implemented in 228c230. Added |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Implemented in 25c6bf1. |
@copilot Please take the next forward-progress pass on PR #59270.
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.
|
|
🎉 This pull request is included in a new release. Release: |
Adds
gh aw logs --auditto generate per-run audit reports from already-downloaded data without additional GitHub API calls. The standalone audit command now persists the same report format.Audit generation
audit.jsonin each run cache directory.Caching
audit.jsonwhen run status and conclusion are unchanged.Output
audit_pathto each run in logs JSON and summary output.Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
api.github.com[!TIP]
api.github.comis blocked because GitHub API access uses the built-in GitHub tools by default. Instead of addingapi.github.comtonetwork.allowed, usetools.github.mode: gh-proxyfor direct pre-authenticated GitHub CLI access without requiring network access toapi.github.com:See GitHub Tools for more information on
gh-proxymode.To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.
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.
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.