Add full-repo audit mode to pkg/linters/errormessage - #50695
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds opt-in repository-wide auditing to the error-message analyzer while preserving existing changed-file gating.
Changes:
- Adds full-repository flag and sentinel modes.
- Adds analyzer tests and documentation.
- Adds a non-blocking Make report target.
Show a summary per file
| File | Description |
|---|---|
pkg/linters/errormessage/errormessage.go |
Implements full-repository scope selection. |
pkg/linters/errormessage/errormessage_test.go |
Tests both full-repository entry points. |
pkg/linters/README.md |
Documents the new modes. |
Makefile |
Adds the non-blocking audit target. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Balanced
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (56 lines added, threshold is 100). |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The changes are clean and well-structured. The full-repo audit mode is implemented correctly:
nilchanged-set correctly signals "check every file" inshouldCheckFile- The
fullRepoSentinelconstant avoids a magic string - Tests cover both entry points (
-full-repoflag and-changed-files=allsentinel) - The Makefile target is non-blocking (
|| true) and follows existing conventions - Docs updated consistently in both the analyzer
Docstring andREADME.md
No blocking issues found.> 🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 19.7 AIC · ⌖ 10.4 AIC · ⊞ 5.4K
🧪 Test Quality Sentinel ReportPR: #50695 — "Add full-repo audit mode to pkg/linters/errormessage" SummaryThis PR adds 2 new behavioral tests for the
Quality Metrics
Test AnalysisFlagged Tests (0 found)No suspicious patterns detected. Test-by-Test Breakdown
Infrastructure: None (no Recommendations
Compliance:
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /codebase-design, and /grill-with-docs — requesting changes on test coverage gaps and a binary path collision.
📋 Key Themes & Highlights
Key Issues
- Test scope does not verify scope expansion — the two new full-repo tests run against the same fixture package as the diff-gated baseline; they cannot prove that violations outside the changed-files set are surfaced. A separate fixture sub-package is needed.
- Cleanup ordering risk —
t.Cleanupis registered after theFlags.Setcall in all three tests; a panic inSetwould skip cleanup and leave global flag state dirty for subsequent tests. Registering cleanup first is the safer pattern. - Binary path collision —
lint-error-messages-reportwrites to/tmp/gh-aw-linters, identical togolint-custom. Parallel or sequential CI invocations of both targets could overwrite each other.
Positive Highlights
- ✅ Clean refactor: splitting
run()into mode-detection +runOnFiles()is a clear deep-module improvement - ✅ Backward compatibility is explicitly preserved — empty
-changed-filesstill no-ops - ✅
|| trueon the Makefile target correctly models this as a metric, not a gate - ✅
isFullRepoSentinelis well-named and testable in isolation - ✅ Doc updates in README and
Analyzer.Docare concise and accurate
| analysistest.Run(t, analysistest.TestData(), errormessage.Analyzer, "errormessage") | ||
| } | ||
|
|
||
| func TestErrorMessageFullRepoFlag(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] The test runs on the same testdata/errormessage fixture package as the baseline TestErrorMessage, so it does not verify that full-repo mode checks additional files beyond the -changed-files selection — it cannot distinguish scope expansion from the diff-gated path.
💡 Suggested fix
Create a separate fixture sub-package (e.g. testdata/errormessage/fullrepopkg/) that contains a violation and is not listed in any -changed-files flag. Run the full-repo tests against that sub-package and confirm the violation surfaces:
func TestErrorMessageFullRepoFlag(t *testing.T) {
// fullrepopkg contains a violation not covered by the diff-gated path
analysistest.Run(t, analysistest.TestData(), errormessage.Analyzer, "errormessage/fullrepopkg")
}Without this, the two new tests only confirm the code compiles and branches correctly, not that the full-repo scope expansion actually works end-to-end.
@copilot please address this.
| // changed ones, so pre-existing violations can be tracked as a metric. | ||
| fullRepo bool | ||
| ) | ||
|
|
There was a problem hiding this comment.
[/codebase-design] The fullRepo package-level variable is a global flag that persists across test runs in the same process. Setting it via Analyzer.Flags.Set in one test will bleed into subsequent tests unless explicitly reset, which the existing t.Cleanup handles — but only if the test passes. A panic or t.Fatal before cleanup registers won't reset the flag.
💡 Suggested improvement
This is an inherent limitation of the go/analysis framework's global-flag model, but it is worth documenting in a comment so future contributors understand why tests must always call t.Cleanup to reset flags:
// NOTE: fullRepo and changedFilesCSV are global package variables bound to
// Analyzer.Flags. Tests that mutate them must reset them via t.Cleanup to
// prevent cross-test pollution.
var (
changedFilesCSV string
fullRepo bool
)Also consider registering the cleanup before any operation that could fail so the reset always runs.
@copilot please address this.
| analysistest.Run(t, analysistest.TestData(), errormessage.Analyzer, "errormessage") | ||
| } | ||
|
|
||
| func TestErrorMessageFullRepoSentinel(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] In TestErrorMessageFullRepoSentinel, the cleanup only resets changed-files but leaves full-repo at its default. However, if TestErrorMessageFullRepoFlag runs first and its cleanup fails (panic before registration), full-repo could be left as true, making this test pass for the wrong reason. Register cleanup immediately after t.Fatalf calls — before any mutating operation — to guard against this.
💡 Recommended pattern
func TestErrorMessageFullRepoSentinel(t *testing.T) {
t.Cleanup(func() { _ = errormessage.Analyzer.Flags.Set("changed-files", "") })
if err := errormessage.Analyzer.Flags.Set("changed-files", "all"); err != nil {
t.Fatalf("failed to set changed-files flag: %v", err)
}
analysistest.Run(t, analysistest.TestData(), errormessage.Analyzer, "errormessage")
}Registering cleanup first ensures it runs even if Set succeeds but a later step panics.
@copilot please address this.
| # be tracked as a metric over time. Always exits 0. | ||
| .PHONY: lint-error-messages-report | ||
| lint-error-messages-report: | ||
| @echo "Building custom linters..." |
There was a problem hiding this comment.
[/grill-with-docs] The binary is built to /tmp/gh-aw-linters, which is the same path used by the existing golint-custom target. If both targets run in the same CI job, the second build overwrites the binary and could cause non-deterministic results.
💡 Suggested fix
Use a distinct output path for this target:
lint-error-messages-report:
env -u GOOS -u GOARCH go build -o /tmp/gh-aw-linters-report ./cmd/linters
/tmp/gh-aw-linters-report -errormessage -errormessage.full-repo $(LINTER_PACKAGES) || true@copilot please address this.
|
@copilot please review the latest PR feedback, refresh this branch if it is behind
Run: https://github.com/github/gh-aw/actions/runs/31058900780
|
Investigated the failing check: JS Tests (shard 3/4) — This failure is unrelated to my changes (which only touch My Go tests all pass ( |
|
@copilot resolve the merge conflicts on this branch. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Resolved. The conflict was in |
|
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. @copilot please review the outstanding feedback and follow-up already noted on this PR, refresh the branch if needed, then run the
Run: https://github.com/github/gh-aw/actions/runs/31060975271
|
…dd-full-repo-audit-mode Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Branch is up to date with main (merged |
|
🎉 This pull request is included in a new release. Release: |
The
errormessageanalyzer only inspects files listed in-changed-filesand silently no-ops otherwise, so the ~1,129 pre-existing error-message violations acrosspkg/are invisible to tooling. This adds an opt-in full-repository mode so that debt can be measured and tracked, while diff-gating behavior stays unchanged.Changes
pkg/linters/errormessage/errormessage.go-full-repobool flag, plus an-changed-files=allsentinel for callers that only plumb the CSV flag.runsplits into a scope decision and a newrunOnFiles(pass, changed); anilchanged-set means "every file in scope", handled inshouldCheckFile.-changed-filesstill no-ops, so existing CI gating is untouched.Makefile— non-blockinglint-error-messages-reporttarget that buildscmd/lintersand audits./cmd/... ./pkg/..., always exiting 0 so it can run as a metric rather than a gate.Tests — cover both entry points (
-full-repoand-changed-files=all) against the existinganalysistestfixtures.Docs —
pkg/linters/README.mdand the analyzerDocstring note the new mode.Usage
For reference,
./pkg/parser/...alone surfaces 255 findings in this mode versus none before.Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Run: https://github.com/github/gh-aw/actions/runs/31060975271> Generated by 👨🍳 PR Sous Chef · gpt54 · 10.7 AIC · ⊞ 5.9K · ◷