Add coverage-aware perf gating for allocation-focused custom linters - #51573
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.
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. All changes are to linter implementation files (.github/skills/go-linters/SKILL.md and pkg/linters/*.go). Test Quality Sentinel skipped.
|
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. 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
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ 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
- "proxy.golang.org"See Network Configuration for more information.
|
There was a problem hiding this comment.
Pull request overview
Adds coverage-aware gating to performance-focused Go linters, though the shared coverage package is missing from the PR.
Changes:
- Registers configurable hot-path thresholds across 13 linters.
- Gates diagnostics using test coverage data.
- Documents the mechanism and implementation pattern.
Show a summary per file
| File | Description |
|---|---|
.github/skills/go-linters/SKILL.md |
Documents coverage-gating integration. |
pkg/linters/README.md |
Describes behavior and gated linters. |
pkg/linters/appendbytestring/appendbytestring.go |
Gates redundant conversion findings. |
pkg/linters/appendoneelement/appendoneelement.go |
Gates append simplification findings. |
pkg/linters/bytesbufferstring/bytesbufferstring.go |
Gates buffer conversion findings. |
pkg/linters/bytescomparestring/bytescomparestring.go |
Gates byte comparison findings. |
pkg/linters/lenstringsplit/lenstringsplit.go |
Gates split-allocation findings. |
pkg/linters/mapclearloop/mapclearloop.go |
Gates map-clearing findings. |
pkg/linters/seenmapbool/seenmapbool.go |
Gates set-representation findings. |
pkg/linters/sortslice/sortslice.go |
Gates sort findings and extracts call matching. |
pkg/linters/stringbytesroundtrip/stringbytesroundtrip.go |
Gates conversion round-trip findings. |
pkg/linters/stringsconcatloop/stringsconcatloop.go |
Gates loop-concatenation findings. |
pkg/linters/stringsjoinone/stringsjoinone.go |
Gates single-element join findings. |
pkg/linters/tolowerequalfold/tolowerequalfold.go |
Gates case-folding findings. |
pkg/linters/writebytestring/writebytestring.go |
Gates writer allocation findings. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 15/15 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /codebase-design, and /grill-with-docs — requesting changes on a build-breaking omission and missing test coverage.
📋 Key Themes & Highlights
Blocking Issue
- Missing
coveragepackage:pkg/linters/internal/coverageis imported by all 13 modified linters but does not exist in the repository. The PR cannot build as-is.
Test Coverage Gap
- No tests for the coverage-gating behaviour. The four contract cases (no profile, threshold=0, below threshold, meets threshold) need to be exercised or any future refactor can silently break the gating.
Design Consistency
stringbytesroundtripgates at two separate helper call sites instead of once in the outer loop, inconsistent with all other linters.seenmapboolusescontinuefor the coverage gate while all other linters usereturn— minor but worth aligning for readability.
Positive Highlights
- ✅
matchSortCallextraction insortsliceis a clean, well-motivated refactor. - ✅ Consistent
init()pattern prevents analyzer initialization cycles. - ✅ Fully permissive fallback (no profile = no behaviour change) is the right default.
- ✅ Correct scoping: purely stylistic linters are intentionally excluded.
- ✅ README and skill doc updates are thorough.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 57.4 AIC · ⌖ 7.91 AIC · ⊞ 7.1K
Comment /matt to run again
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ADR Required — Draft Committed to BranchStatus: Draft ADR added to This PR adds 197 new lines to A draft ADR has been committed directly to this branch so you can review and accept it without a separate workflow step. What to do next
Why ADRs MatterArchitecture Decision Records create a durable paper trail for decisions that future contributors will encounter but cannot deduce from code alone. The new ADR Format Reference (Michael Nygard)Required sections: Context, Decision, Alternatives Considered, Consequences (positive + negative). Status values:
|
There was a problem hiding this comment.
Review: Coverage-aware perf gating
🔴 Blocking: Missing pkg/linters/internal/coverage package
All 12 linter files modified in this PR import "github.com/github/gh-aw/pkg/linters/internal/coverage" (providing RegisterHotThresholdFlag and ShouldApply), but this package does not exist anywhere in the repository. The build will fail immediately with cannot find package.
The PR must add pkg/linters/internal/coverage/coverage.go (and tests) with at minimum:
RegisterHotThresholdFlag(a *analysis.Analyzer) *int— registers a-hot-thresholdflag on the analyzer and returns a pointer to itShouldApply(pass *analysis.Pass, pos token.Pos, threshold int) bool— loadsGH_AW_LINT_COVERAGE_PROFILEand gates based on hit count; returnstruewhen no profile is set or threshold is 0
🟡 Non-blocking: Coverage gate in writebytestring placed after expression building
sExpr and writerArg are allocated before the coverage gate fires, wasting work on skipped findings. Move the gate before those calls (see inline comment).
✅ What looks good
- The
init()-based flag registration correctly avoids the analyzer/flag initialization cycle. - The pattern is applied consistently across all 12 targeted perf linters.
- Purely stylistic linters are correctly excluded from coverage gating.
- The
sortslicerefactor to extractmatchSortCallis clean and improves testability. - Documentation (SKILL.md and README.md) is accurate and well-structured.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 91.5 AIC · ⌖ 7.39 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Ponytail review (over-engineering only): two findings — repeated per-linter boilerplate for wiring the hot-threshold flag, and a doc section duplicated between README.md and SKILL.md. Both are small, contained additions; the rest of the gating logic is proportional to its stated purpose.
net: -60 lines possible.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
Generated by ✂️ Ponytail Reviewer for #51573 · auto · 63.4 AIC · ⌖ 5.33 AIC · ⊞ 6.8K
Comment /ponytail to run again
Comments that could not be inline-anchored
pkg/linters/appendbytestring/appendbytestring.go:90
yagni: identical 7-line var hotThreshold *int + init(){ hotThreshold = coverage.RegisterHotThresholdFlag(Analyzer) } block copy-pasted verbatim across 12 linter files. analyzerutil.New could accept coverage gating as an option and own this wiring once.
pkg/linters/README.md:74
shrink: Coverage-aware perf gating section duplicates the same explanation (profile env var, hot-threshold semantics, permissive default) already spelled out in .github/skills/go-linters/SKILL.md. One doc could link to the other instead of repeating ~15 lines.
|
@copilot Quick triage nudge for this PR. Please refresh the branch if GitHub can update it cleanly, address the current blocking review feedback, review the ADR draft now on the branch, run the Open review context (newest first):
Branch refresh was requested. Run:
|
…writebytestring gate position, accept ADR Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done in the latest commit:
|
Re-confirmed on current head — no code changes needed:
Confirmed: only human actions remain:
|
PR TriageCategory: feature · Risk: high · Priority: medium · Score: 48/100 Score breakdown
Recommended action: Automated triage via PR Triage Agent.
|
|
@copilot Quick triage nudge for this PR. Please refresh the branch if GitHub can update it cleanly, re-check the current maintainer-facing state, and run the Open review context (newest first):
Branch refresh was requested.
|
Re-confirmed on current head — no code changes needed:
Confirmed: only human actions remain:
|
|
@copilot PR sous-chef triage: there are open blocking reviews from github-actions[bot]. Please address the review feedback, refresh the branch, and run the pr-finisher skill before handing back to maintainers. Run details: https://github.com/github/gh-aw/actions/runs/31382838939
|
Re-confirmed on current head — no code changes needed:
The two Remaining human actions required:
|
|
@copilot PR sous-chef triage: please refresh the current maintainer-facing state on the latest head, resolve any addressed unresolved review threads, refresh the branch if GitHub can update it cleanly, and run the Open review context (newest first):
No failed checks were listed in the compact candidate snapshot.
|
Re-confirmed on current head — no code changes needed:
The two Remaining human actions required:
|
Triage: Add coverage-aware perf gating for allocation-focused custom linters
|
|
@copilot PR sous-chef triage: please refresh the current maintainer-facing state on the latest head, resolve any addressed unresolved review threads, refresh the branch if GitHub can update it cleanly, and run the Open review context (newest first):
No failed checks were listed in the compact candidate snapshot.
|
Both issues are already addressed on the current head:
Remaining human actions required:
|
|
🎉 This pull request is included in a new release. Release: |
Perf/allocation-focused custom linters flag micro-optimizations that only matter on hot paths; applying them uniformly to dead or rarely-executed code adds review churn without measurable benefit. Rules need a way to consult test coverage data to decide whether a finding is worth reporting.
Centralized coverage helper
pkg/linters/internal/coveragepackage loads a Go coverage profile (produced bygo test -covermode=count -coverprofile=<path>) referenced by theGH_AW_LINT_COVERAGE_PROFILEenvironment variable.ShouldApply(pass, pos, threshold)to gate a finding on the line's recorded execution hit count, andRegisterHotThresholdFlag(analyzer)to register a per-linter-hot-thresholdflag (default1;0disables gating).Wired into perf-oriented linters
Reviewed every custom linter and applied coverage gating only where the fix has a genuine performance rationale (extra allocations, O(n²) growth, etc.):
stringsconcatloop,appendoneelement,appendbytestring,bytesbufferstring,bytescomparestring,lenstringsplit,mapclearloop,seenmapbool,sortslice,stringbytesroundtrip,stringsjoinone,tolowerequalfold,writebytestring.Purely stylistic/readability linters (
stringsindexcontains,stringsindexhasprefix,stringscountcontains,lenstringzero, etc.) are left ungated since their rewrites carry no measurable perf difference.Docs
pkg/linters/README.mddocuments the gating mechanism and which linters use it..github/skills/go-linters/SKILL.mddocuments the pattern for wiring new perf linters into coverage gating, including theinit()requirement to avoid an analyzer initialization cycle.Branch refresh requested by PR Sous Chef run https://github.com/github/gh-aw/actions/runs/31319713324> Generated by 👨🍳 PR Sous Chef · gpt54 · 15.9 AIC · ⌖ 5.45 AIC · ⊞ 6.1K · ◷
Run: https://github.com/github/gh-aw/actions/runs/31330381966> Generated by 👨🍳 PR Sous Chef · gpt54 · 12.9 AIC · ⌖ 5.27 AIC · ⊞ 6.1K · ◷
Requested branch update from PR Sous Chef run https://github.com/github/gh-aw/actions/runs/31336577085> Generated by 👨🍳 PR Sous Chef · gpt54 · 10.2 AIC · ⌖ 5.19 AIC · ⊞ 8.5K · ◷
Branch refresh requested by PR Sous Chef run https://github.com/github/gh-aw/actions/runs/31338952148> Generated by 👨🍳 PR Sous Chef · gpt54 · 27.7 AIC · ⌖ 5.43 AIC · ⊞ 6.1K · ◷
Requested branch update from PR Sous Chef run https://github.com/github/gh-aw/actions/runs/31339757619> Generated by 👨🍳 PR Sous Chef · gpt54 · 10.8 AIC · ⌖ 5.2 AIC · ⊞ 8.5K · ◷
Run: https://github.com/github/gh-aw/actions/runs/31341574451> Generated by 👨🍳 PR Sous Chef · gpt54 · 20.6 AIC · ⌖ 5.23 AIC · ⊞ 6.1K · ◷
Run: https://github.com/github/gh-aw/actions/runs/31354788221> Generated by 👨🍳 PR Sous Chef · gpt54 · 39.6 AIC · ⌖ 6.68 AIC · ⊞ 8.5K · ◷
Run: https://github.com/github/gh-aw/actions/runs/31386523734> Generated by 👨🍳 PR Sous Chef · gpt54 · 15.8 AIC · ⌖ 7.79 AIC · ⊞ 6.1K · ◷