fix(ci): promote performance baseline only on successful runs - #310
Merged
Merged
Conversation
The "Save new baseline" and "Cache updated baseline for next run" steps in performance.yml ran under always(), so a weekly run that failed the >20% regression check still cached its slower numbers as the next baseline. Each regression lowered the bar for the following run: the 2026-09-13 run compared `apps` against 2.732s, the exact figure from the failed 2026-09-06 run, failed again, and cached 4.257s -- which let the 2026-09-20 run pass trivially. Gate both steps on success() instead. A deliberate re-baseline is done by deleting the perf-baseline-macOS-* caches: compare_performance.py exits 0 when no baseline exists, so the next run becomes the baseline. Add test_performance_baseline_promoted_only_on_success to TestProjectConsistency to guard the step conditions and their ordering after the regression check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Reviewer's GuideThe weekly performance workflow now promotes and caches a new baseline only when the complete run succeeds, preventing failed regression runs from lowering the following week's comparison bar. A workflow-level regression test verifies the guards and ordering, and the intentional cache-deletion re-baseline procedure is documented in the workflow and changelog. Sequence diagram for performance baseline handlingsequenceDiagram
participant Workflow
participant Benchmark
participant BaselineCache
Workflow->>Benchmark: Run performance benchmark
Benchmark-->>Workflow: performance_results.json
Workflow->>Workflow: Compare against baseline
alt regression check succeeds
Workflow->>Workflow: cp performance_results.json performance_baseline.json
Workflow->>BaselineCache: Save updated baseline
else regression check fails
Workflow-->>BaselineCache: Keep existing baseline
end
Flow diagram for successful performance baseline promotionflowchart TD
A[Run performance benchmark] --> B[Compare against baseline]
B --> C{Regression check passes}
C -->|Yes| D[Save new baseline]
D --> E[Cache updated baseline for next run]
C -->|No| F[Keep existing baseline]
F --> G[Run fails without promotion]
H[No baseline after deliberate cache deletion] --> I[Comparison exits successfully]
I --> D
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Strix is installed on this repository, but we couldn't run this PR security review because this workspace's trial has ended. Add a card to resume code reviews here. So far, Strix has reviewed 30 pull requests across this workspace. |
🔒 Security Analysis ReportSecurity Analysis ReportGenerated: Wed Sep 30 17:15:55 UTC 2026 Bandit Security ScanSafety Check ResultsPip-Audit Results |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path=".github/workflows/performance.yml" line_range="123-124" />
<code_context>
- if: always() && (github.event.inputs.benchmark_target == 'all' || github.event.inputs.benchmark_target == '')
+ # success(), not always(): a run that fails the regression check must
+ # not become the next baseline, or each regression lowers the bar for
+ # the run after it. To accept a slowdown deliberately, delete the
+ # perf-baseline-macOS-* caches (`gh cache list --key perf-baseline-`,
+ # then `gh cache delete <key>`); the next run has no baseline, passes,
+ # and becomes the new one.
+ if: success() && (github.event.inputs.benchmark_target == 'all' || github.event.inputs.benchmark_target == '')
run: cp performance_results.json performance_baseline.json
</code_context>
<issue_to_address>
**issue:** `gh cache list --key perf-baseline-` filters for a cache whose key is exactly `perf-baseline-`, while the workflow creates keys such as `perf-baseline-macOS-<run_id>`, so the documented command returns no caches and the stale baseline remains in use.
**Triggers:** When an operator follows the documented cache-clearing procedure to deliberately re-baseline performance.
**Suggested fix:** List caches without the exact-key filter and select keys beginning with `perf-baseline-macOS-`, or use the GitHub API/CLI with a proper prefix filter before deleting each returned cache.
```suggestion
# perf-baseline-macOS-* caches (`gh cache list --limit 100 --json key --jq '.[] | select(.key | startswith("perf-baseline-macOS-")) | .key' | while read -r key; do gh cache delete "$key"; done`); the next run has no baseline, passes,
```
</issue_to_address>Sourcery assessment
Needs a human reviewer. 1 finding to address first, and if the success gate is wrong, a valid baseline may fail to be promoted and later runs may compare against a stale cache. Reverting the workflow and rerunning the benchmark repairs this; no irreversible production change or data loss is introduced.
Blocking findings: .github/workflows/performance.yml:124
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
🔒 Security Analysis ReportSecurity Analysis ReportGenerated: Wed Sep 30 17:47:49 UTC 2026 Bandit Security ScanSafety Check ResultsPip-Audit Results |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The weekly Performance Testing workflow promoted failing runs to baseline. The
Save new baselineandCache updated baseline for next runsteps ran underalways(), so a run that failed the >20% regression check still cached its slower numbers as the next run's baseline. Each regression lowered the bar for the following week.Evidence from the scheduled runs (dependency-only changes in between):
appsbaseline → currentThe 09-13 run's baseline (2.732s) is exactly the figure from the failed 09-06 run.
Fix
success()instead ofalways(). The existing full-run (benchmark_target == 'all') guard is unchanged.perf-baseline-macOS-*caches.compare_performance.pyexits 0 when no baseline exists, so the next run becomes the new baseline.TestProjectConsistency.test_performance_baseline_promoted_only_on_successasserts both steps are gated onsuccess(), neveralways(), and run after the regression check (verified red before the fix, green after).[Unreleased] → Fixed.Not addressed here (follow-up)
The benchmark times live Homebrew API calls, so
appsswings ~2× week to week (1.94s → 4.26s) against a 20% threshold. That's why 3 of the last 4 scheduled runs failed. This PR keeps the baseline honest but won't make the weekly run less flaky.Testing
mypy.ini), coverage 88.19%actionlint: no new findings (the same 8 pre-existing shellcheck info/style notes in untouched steps as onmain)🤖 Generated with Claude Code
Summary by Sourcery
Keep weekly performance baselines honest by promoting new results only when the full benchmark run passes its regression check.
Bug Fixes:
Enhancements:
Tests: