Repository navigation
fix(linters): reconcile deferred Close error guidance - #67631
Conversation
Preserve complementary analyzer scopes and verify shared cleanup patterns without duplicate findings or weaker enforcement. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft.
|
There was a problem hiding this comment.
🟢 Approval recommended
The documentation, diagnostics, analyzer scopes, and regression coverage are consistent with the stated policy.
0 open findings
What changed in this PR
Aligns complementary Close() error analyzers around a unified deferred-cleanup policy.
Changes:
- Clarifies analyzer scopes and remediation guidance.
- Documents safe deferred cleanup using
errors.Join. - Adds combined regression coverage for diagnostics and suppressions.
| File | Description |
|---|---|
pkg/linters/unchecked-deferred-close/unchecked_deferredclose.go |
Improves scope and diagnostic guidance. |
pkg/linters/README.md |
Documents unified close-error policy. |
pkg/linters/closeerrorunchecked/testdata/src/combined/combined.go |
Adds cross-analyzer regression fixtures. |
pkg/linters/closeerrorunchecked/testdata/src/closeerrorunchecked/closeerrorunchecked.go |
Removes misleading defer guidance. |
pkg/linters/closeerrorunchecked/closeerrorunchecked.go |
Clarifies analyzer scope. |
pkg/linters/closeerrorunchecked/closeerrorunchecked_test.go |
Runs both analyzers against shared fixtures. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
No actionable blocking issues found in the changed lines.
Review notes
I checked the scope split between closeerrorunchecked and unchecked_deferredclose, the new combined regression test, and the updated fixture guidance. The patch keeps analyzer behavior consistent with the documented AST split and adds coverage for the previously misleading defer examples without weakening either analyzer or introducing obvious duplicate-report paths.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 14.2 AIC · ⌖ 5.34 AIC · ⊞ 21.2K
Comment /review to run again
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (1 test)
Verdict
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 100/100. 0% implementation tests (threshold: 30%).
🧪 Test quality analysis by Test Quality Sentinel · copilot · haiku45 · 16.8 AIC · ⌖ 6.77 AIC · ⊞ 8.2K
Comment /review to run again
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (146 new lines in 📄 Draft ADR committed:
🔎 Evidence used
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd (via pr-triage: bug_fix). The root cause (stale, misleading fixture comments implying bare defer Close() was approved) is fixed consistently across both analyzer docs, the README, and renamed fixture functions — confirmed by running the test suite locally.
📋 Key Themes & Highlights
Verification performed
go test ./pkg/linters/closeerrorunchecked/... ./pkg/linters/unchecked-deferred-close/...— all pass.go veton both packages — clean.- Doc-sync tests (
TestDocGo_CountMatchesBullets,TestDocGo_AnalyzersMatchREADME,TestDocSurfacesMatchRegistryAndSpecList) still pass after the README edit. - Confirmed by probe test that the new
combined.gofixture genuinely requires both analyzers running together (single-analyzer run fails with unmatched diagnostics), so the regression coverage is real, not incidental.
One actionable suggestion
- The new
TestComplementaryCloseErrorAnalyzershand-rolls a mini multi-analyzer runner (shallow-copies*pass, swapsAnalyzer/Reportper iteration) directly in the test file. This works today, but the technique is non-obvious and likely to be needed again for other complementary-analyzer pairs hinted at by the new README policy section. Extracting it into a smallanalyzerutilhelper would make it reusable and reduce the risk of someone copy-pasting the*passplumbing incorrectly. See inline comment.
Positive highlights
- ✅ Root cause addressed at the source (fixture comments + analyzer
Docstrings + package comments), not just symptom-patched. - ✅
combined.gofixture is comprehensive: covers files, custom closers, interfaces, closures, suppressions, and the no-error-method case — good boundary coverage per/tdd. - ✅ Clear, consistent cross-references between the two analyzers' descriptions prevent the original confusion from recurring.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 123.9 AIC · ⌖ 13.6 AIC · ⊞ 10.3K
Comment /matt to run again
Extract diagnostic prefixing and isolated dependency results into a tested regression helper with explicit unsupported-configuration errors. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Summary
Fixes #67608.
The issue correctly identifies stale fixture guidance, but the analyzers have complementary AST scopes rather than conflicting enforcement:
closeerroruncheckedhandles expression statements and assignments, whileunchecked_deferredclosehandles direct deferred calls. Not reporting an out-of-scope statement is not approval of silently discarding its error.errors.Joinexample that preserves operation and cleanup errors.Existing enforcement, registry entries, and CI rollout exclusions are unchanged; neither analyzer is weakened or made to emit duplicate findings.
Validation
go test ./pkg/linters/closeerrorunchecked ./pkg/linters/unchecked-deferred-close -count=1passed.PATH="/opt/homebrew/bin:$PATH" make agent-report-progresspassed, including build, changed-file lint, impacted Go tests, schema freshness, and workflow drift checks (337 workflows in sync).make fmtcompleted Go formatting but its unrelated JavaScript formatting step failed ontestdata/copilot_sdk_web_fetch_contract.jsonwithReferenceError: convertToJson is not defined; incidental JavaScript formatting changes were removed. The final change-scoped gate formatted all changed Go files successfully.