Add opt-out for automatic audit baseline downloads - #63012
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Full audit caches ignore the requested mode and can also preserve an opt-out result for later default audits.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Adds a --no-baseline option to avoid automatic baseline discovery and downloads during single-run audits.
Changes:
- Propagates the opt-out through CLI and audit configuration.
- Skips comparison generation for fresh and logs-cached reports.
- Adds flag and rendering tests.
| File | Description |
|---|---|
pkg/cli/audit.go |
Adds baseline opt-out state. |
pkg/cli/audit_run_pipeline.go |
Propagates the option through execution. |
pkg/cli/audit_run_pipeline_test.go |
Tests configuration propagation. |
pkg/cli/audit_render_output.go |
Conditionally skips baseline generation. |
pkg/cli/audit_render_output_test.go |
Tests fresh and logs-cache behavior. |
pkg/cli/audit_command.go |
Registers and forwards the CLI flag. |
pkg/cli/audit_command_test.go |
Tests flag registration and parsing. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| func buildRenderedAuditComparison(ctx context.Context, processedRun ProcessedRun, createdItems []CreatedItemReport, runOutputDir string, opts AuditOptions) *AuditComparisonData { | ||
| if opts.NoBaseline { | ||
| return &AuditComparisonData{BaselineFound: false} |
There was a problem hiding this comment.
Fixed in af29bc1. renderAuditReport now drops the comparison from a full cache hit when --no-baseline is set, and an opt-out result is no longer persisted as a full cache entry, so a later default audit still computes the baseline comparison. Both invocation orders are covered by TestRenderAuditReportDropsCachedComparisonWithNoBaseline and TestRenderAuditReportDoesNotCacheNoBaselineResult.
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning Firewall blocked 5 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"
- "o205451.ingest.us.sentry.io"
- "proxy.golang.org"
- "storage.googleapis.com"
- "sum.golang.org"See Network Configuration for more information.
|
|
🔎 PR Code Quality Reviewer is reviewing code quality for this pull request... |
|
✅ 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.
|
|
✅ 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
- "o205451.ingest.us.sentry.io"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.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Applied Impeccable harden and audit modes (bug-fix change adding a new CLI opt-out flag with edge-state handling).
Findings
🟠 HIGH — --no-baseline is not cache-safe: a pre-existing full audit cache (written by a prior run without the flag) is served as-is by renderAuditReport, bypassing buildRenderedAuditComparison entirely, so the flag silently has no effect. See inline comment on pkg/cli/audit_render_output.go.
No other blocking issues found in the diff (flag wiring, config threading, and the logs-cache/fresh-build paths look correct and are covered by the new tests).
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
o205451.ingest.us.sentry.io
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "o205451.ingest.us.sentry.io"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 57 AIC · ⌖ 13.2 AIC · ⊞ 8.1K
Comments that could not be inline-anchored
pkg/cli/audit_render_output.go:25
--no-baseline doesn't take effect when a full audit cache already exists on disk. renderAuditReport checks loadCachedAuditData(runOutputDir, processedRun.Run, auditCacheSourceFull) first and, on a cache hit, returns the cached AuditData (including its Comparison) directly — buildRenderedAuditComparison/opts.NoBaseline is never consulted in that path (see pkg/cli/audit_render_output.go lines 23-31).
Repro: run gh aw audit <run> once (populates the full cache with a real co…
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on a confirmed cache-safety bug.
📋 Key Themes & Highlights
Key Themes
- Cache-safety bug (confirmed by reproduction):
renderAuditReportshort-circuits on a pre-existing full audit cache beforebuildRenderedAuditComparisonever runs, so--no-baselineis silently ignored when a cache entry already exists (and the reverse: aBaselineFound:falseresult cached under--no-baselinepersists after removing the flag). I wrote and ran a local reproduction confirming both directions. This duplicates and confirms an existing unresolved review comment from Copilot on the same line. - Test coverage gap: the two new tests exercise
buildRenderedAuditData/buildRenderedAuditDataFromCachedirectly, which never touches the buggy cache short-circuit inrenderAuditReport. A regression test through the real entry point is needed to catch this.
Positive Highlights
- ✅ Clean, minimal flag threading through
AuditOptions→auditRunConfig→ CLI, consistent with existing patterns. - ✅
buildRenderedAuditComparisonis a good extraction that removes duplication between the two build paths. - ✅ Flag registration test coverage for
--no-baselineis present and correct.
@copilot please address the review comments above.
Warning
Firewall blocked 5 domains
The following domains were blocked by the firewall during workflow execution:
github.como205451.ingest.us.sentry.ioproxy.golang.orgstorage.googleapis.comsum.golang.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "github.com"
- "o205451.ingest.us.sentry.io"
- "proxy.golang.org"
- "storage.googleapis.com"
- "sum.golang.org"See Network Configuration for more information.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 95 AIC · ⌖ 14.7 AIC · ⊞ 10.1K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/cli/audit_render_output.go:23
[/diagnosing-bugs] --no-baseline is not cache-safe: loadCachedAuditData never checks opts.NoBaseline, so a stale full-audit cache written without the flag is replayed verbatim when --no-baseline is passed later (and vice versa — a BaselineFound:false cache from a --no-baseline run sticks even after the flag is removed). I reproduced this locally by seeding a full cache with BaselineFound:true and then calling renderAuditReport with NoBaseline:true; the stale true value…
pkg/cli/audit_render_output_test.go:92
[/tdd] Both new tests call buildRenderedAuditData/buildRenderedAuditDataFromCache directly, bypassing renderAuditReport's loadCachedAuditData(..., auditCacheSourceFull) short-circuit — so the cache-bypass bug flagged in the sibling comment on audit_render_output.go has no failing test to catch it. A regression test should call renderAuditReport twice (with a pre-seeded full-cache file, and with NoBaseline toggled between calls) to assert the flag is honored even when a cache e…
|
@copilot Please move this PR forward. Outstanding review feedback remains unresolved and the compact candidate set still shows failed checks:
Please address the open review finding, inspect the failed runs, refresh the branch if needed, and run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in af29bc1:
Local validation ( |
Single-run audits automatically fetched a previous run and downloaded its artifacts for comparison, adding unnecessary network and runtime cost when callers only needed the requested run.
CLI
--no-baselinefor single-run audits.Audit rendering
baseline_found: false.Requested branch refresh from PR Sous Chef run https://github.com/github/gh-aw/actions/runs/35916643961