test-quality-sentinel: fix TestMain misclassification, add goleak detection and goroutine-leak guards#47891
Conversation
…ection, improve scoring - Pre-fetch: exclude TestMain from go-new-test-funcs.txt, add go-testmain-funcs.txt and go-goleak-entries.txt - Pre-fetch awk: skip TestMain entries from stats so they don't pollute test counts - Prompt Step 1: reference new pre-fetched files (testmain-funcs, goleak-entries) - Prompt Step 2: distinguish behavioral tests from TestMain infrastructure - Prompt Step 4: classify goleak.VerifyTestMain as design invariant (high_value, behavioral_contract) - Prompt Step 5: skip inflation scoring for TestMain-only files - Prompt Step 6: add infrastructure-only PR path (no numeric score, approve on no violations) - Prompt Step 8: add APPROVE verdict for infrastructure-only PRs - Guidelines: add goroutine-leak guard and infrastructure-only PR calibration rules - Add go.uber.org/goleak v1.3.0 dependency - pkg/parser: upgrade TestMain to use goleak.VerifyTestMain - pkg/workflow: add test_main_test.go with goleak.VerifyTestMain Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer failed during the skills-based review. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #47891 does not have the 'implementation' label and has only 18 new lines of code in business logic directories (threshold: 100). |
There was a problem hiding this comment.
Pull request overview
Improves test-quality classification for TestMain infrastructure and adds package-wide goroutine leak detection.
Changes:
- Separates
TestMainand goleak signals from behavioral tests. - Adds infrastructure-only scoring and approval guidance.
- Enables goleak checks for parser and workflow tests.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/test-quality-sentinel.md |
Adds infrastructure detection and scoring rules. |
.github/workflows/test-quality-sentinel.lock.yml |
Recompiles the workflow. |
pkg/parser/frontmatter_hash_test_main_test.go |
Enables parser goleak detection. |
pkg/workflow/test_main_test.go |
Enables workflow goleak detection. |
go.mod |
Adds the goleak dependency. |
go.sum |
Adds goleak checksums. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
.github/workflows/test-quality-sentinel.md:72
- This unanchored search also records removed lines and unchanged diff context. A PR that removes
goleak.VerifyTestMaincan therefore be reported as adding a positive leak guard. Restrict the signal to added diff lines.
grep -E "goleak\.VerifyTestMain" /tmp/gh-aw/agent/test-diff.txt \
.github/workflows/test-quality-sentinel.md:344
- “Approve without hesitation” applies to every PR containing a goleak guard, including mixed PRs with failing behavioral tests or hard violations, and conflicts with the guarded approval rule in Step 8. Treat goleak as a positive signal, but only approve infrastructure-only PRs when no hard violations exist; mixed PRs must still use normal scoring.
- **Goroutine-leak guards**: `TestMain` with `goleak.VerifyTestMain` is an unconditional design invariant — approve without hesitation; note it as a positive quality signal
- Files reviewed: 5/6 changed files
- Comments generated: 4
- Review effort level: Medium
| grep -E "^\+func TestMain\b" /tmp/gh-aw/agent/test-diff.txt \ | ||
| > /tmp/gh-aw/agent/go-testmain-funcs.txt || true |
|
|
||
| **Infrastructure vs. behavioral**: `TestMain(m *testing.M)` entries in `go-testmain-funcs.txt` are test infrastructure, not behavioral test cases. Do not score them as test functions. Instead, note them separately in the report and credit any `goleak.VerifyTestMain` usage (see `go-goleak-entries.txt`) as a goroutine-leak design invariant. | ||
|
|
||
| If `go-new-test-funcs.txt` is empty and `go-testmain-funcs.txt` is non-empty, this is an **infrastructure-only PR** — see the scoring guidance in Step 6. |
|
|
||
| Thresholds: `>=80 ✅ Excellent`, `60-79 ⚠️ Acceptable`, `40-59 🔶 Needs improvement`, `<40 ❌ Poor`. | ||
|
|
||
| **Infrastructure-only PRs**: If `go-new-test-funcs.txt` is empty (no behavioral `func Test*` functions) but `go-testmain-funcs.txt` is non-empty (only `TestMain` infrastructure added), skip the numeric score formula and assign **Score: N/A — Infrastructure**. Do not fail the PR on implementation ratio (there are no behavioral tests to evaluate). Still flag hard violations (missing build tags, go mock library usage). If `go-goleak-entries.txt` is non-empty, report it as a quality improvement and approve. |
| go.opentelemetry.io/otel v1.44.0 // indirect | ||
| go.opentelemetry.io/otel/metric v1.44.0 // indirect | ||
| go.opentelemetry.io/otel/trace v1.44.0 // indirect | ||
| go.uber.org/goleak v1.3.0 // indirect |
🧪 Test Quality Sentinel Report🏗️ Test Quality Score: N/A — Infrastructure
📊 Metrics
Infrastructure added:
🟢 Goroutine-Leak Guards
Verdict
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Test Quality Sentinel PR Review
The workflow fixes and goroutine-leak guard additions are well-structured and correct. One blocking issue found:
go.uber.org/goleak listed as // indirect in go.mod — this package is directly imported in test files; // indirect is incorrect and should be removed by running go mod tidy.
All other changes look good:
TestMainexclusion from behavioral test counters is correctgoleak.VerifyTestMainintegration is clean- New
pkg/workflow/test_main_test.gohas proper build tag and minimal, correct implementation - Workflow prompt updates are clear and well-scoped
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 20.3 AIC · ⌖ 4.56 AIC · ⊞ 5K
| go.opentelemetry.io/otel v1.44.0 // indirect | ||
| go.opentelemetry.io/otel/metric v1.44.0 // indirect | ||
| go.opentelemetry.io/otel/trace v1.44.0 // indirect | ||
| go.uber.org/goleak v1.3.0 // indirect |
There was a problem hiding this comment.
`go.uber.org/goleak` is marked `// indirect` but is a direct dependency.
This package is directly imported in `pkg/parser/frontmatter_hash_test_main_test.go` and `pkg/workflow/test_main_test.go`. Running `go mod tidy` will remove the `// indirect` annotation.
Please run `go mod tidy` to fix this.
@copilot please address this.
There was a problem hiding this comment.
Two issues need fixing before merge
1. not run — is directly imported in test files but marked in go.mod. CI tidy-enforcement will fail.
2. Goleak detection grep has no anchor (correctness bug) — grep -E "goleak\.VerifyTestMain" matches both added and removed diff lines. A PR that deletes goleak will still get approved as a quality improvement. Fix: anchor to ^+.
Other observations (non-blocking)
pkg/workflow/test_main_test.gousespackage workflow_test(external test package). This is valid — Go compiles both into one binary and there can only be oneTestMain— but it means theTestMaincontrols the whole binary regardless. No compile error, just worth confirming intentionally.- No
goleak.IgnoreTopFunctionoptions are set. If any third-party library (otel, etc.) leaves goroutines running, every test run will start failing. This is a future maintenance risk, not a blocker now.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 · 59.7 AIC · ⌖ 4.71 AIC · ⊞ 5.7K
Comment /review to run again
Comments that could not be inline-anchored
go.mod:105
goleak is marked // indirect but is directly imported: the package is explicitly used in pkg/parser and pkg/workflow test files, so go mod tidy was not run after adding the import.
<details>
<summary>💡 Suggested fix</summary>
Remove the // indirect comment from the entry:
go.uber.org/goleak v1.3.0Then run go mod tidy. Any CI that enforces tidy will fail on this mismatch.
</details>
.github/workflows/test-quality-sentinel.md:36
Goleak detection grep matches removed lines too: the pattern grep -E "goleak\.VerifyTestMain" has no ^+ anchor, so it will match both added (+) and removed (-) lines from the diff, causing false positive quality credit when goleak is being deleted from a file.
<details>
<summary>💡 Suggested fix</summary>
Anchor to added lines only:
grep -E "^\+.*goleak\.VerifyTestMain" /tmp/gh-aw/agent/test-diff.txt > /tmp/gh-aw/agent/go-goleak-entries.txt || trueWithout this, …
|
@copilot please run the skill, confirm the branch is up to date, address unresolved review feedback, and rerun or retrigger checks as needed on the latest HEAD. Unresolved review feedback:
Failed checks on the latest head: Run: https://github.com/github/gh-aw/actions/runs/30143146258
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…in checks Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in commit I ran the requested workflow skill, confirmed the branch is up to date (includes merge from Latest HEAD checks were retriggered by the push, but current runs for |
|
🎉 This pull request is included in a new release. Release: |
The workflow incorrectly treated
func TestMain(m *testing.M)as a behavioral test function, pollutinggo-new-test-funcs.txtand the awk stats counters. Infrastructure-only PRs (e.g. addinggoleak.VerifyTestMain) had no defined scoring path, forcing the agent to improvise a 70/100 score by judgment each time.Workflow fixes (
test-quality-sentinel.md)Pre-fetch step:
TestMainfromgo-new-test-funcs.txtviagrep -v "^\+func TestMain\b"go-testmain-funcs.txt— infrastructure entries captured separatelygo-goleak-entries.txt— detectsgoleak.VerifyTestMainas a quality signalif (test_name == "TestMain") test_name=""to skip infrastructure from countsPrompt:
TestMaininfrastructure from behavioralfunc Test*functions; defines "infrastructure-only PR"goleak.VerifyTestMainclassified asdesign_test/behavioral_contract/high_value— no traditional assertions requiredTestMain-only filesAPPROVEverdict for infrastructure-only PRsRepository test quality
go.uber.org/goleak v1.3.0pkg/parser: upgrades no-opTestMaintogoleak.VerifyTestMainpkg/workflow: newtest_main_test.gowithgoleak.VerifyTestMain, covering all ~100 unit testsRun: https://github.com/github/gh-aw/actions/runs/30143146258