Guard suggested fixes against comment loss in five linters - #57860
Conversation
…oinone, stringscountcontains, and stringsindexcontains linters Co-authored-by: pelikhan <4175913+pelikhan@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
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
Lean already. Ship. 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
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Warning Firewall blocked 2 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"
- "proxy.golang.org"See Network Configuration for more information.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ 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.
🟢 Approval recommended
The guards cover every identified replacement path and the golden tests verify diagnostics remain while unsafe fixes are withheld.
Pull request overview
Prevents five Go linters from deleting comments when applying whole-expression suggested fixes, resolving #57844.
Changes:
- Suppresses fixes when replacement spans overlap comments.
- Passes parsed files into the shared contains-fix helper.
- Adds golden and unit coverage for comment preservation.
File summaries
| File | Description |
|---|---|
pkg/linters/appendoneelement/appendoneelement.go |
Guards suggested fixes against comments. |
pkg/linters/appendoneelement/testdata/src/appendoneelement/appendoneelement.go |
Adds inline-comment fixture. |
pkg/linters/appendoneelement/testdata/src/appendoneelement/appendoneelement.go.golden |
Verifies fix suppression. |
pkg/linters/timenowsub/timenowsub.go |
Guards suggested fixes against comments. |
pkg/linters/timenowsub/testdata/src/timenowsub/timenowsub.go |
Adds inline-comment fixture. |
pkg/linters/timenowsub/testdata/src/timenowsub/timenowsub.go.golden |
Verifies fix suppression. |
pkg/linters/stringsjoinone/stringsjoinone.go |
Guards suggested fixes against comments. |
pkg/linters/stringsjoinone/testdata/src/stringsjoinone/stringsjoinone.go |
Adds inline-comment fixture. |
pkg/linters/stringsjoinone/testdata/src/stringsjoinone/stringsjoinone.go.golden |
Verifies fix suppression. |
pkg/linters/stringscountcontains/stringscountcontains.go |
Supplies files to the guarded helper. |
pkg/linters/stringscountcontains/testdata/src/stringscountcontains/stringscountcontains.go |
Adds comment-overlap fixture. |
pkg/linters/stringscountcontains/testdata/src/stringscountcontains/stringscountcontains.go.golden |
Verifies fix suppression. |
pkg/linters/stringsindexcontains/stringsindexcontains.go |
Supplies files to the guarded helper. |
pkg/linters/stringsindexcontains/testdata/src/stringsindexcontains/stringsindexcontains.go |
Adds comment-overlap fixture. |
pkg/linters/stringsindexcontains/testdata/src/stringsindexcontains/stringsindexcontains.go.golden |
Verifies fix suppression. |
pkg/linters/internal/astutil/astutil.go |
Centralizes comment-overlap protection. |
pkg/linters/internal/astutil/astutil_test.go |
Tests guarded fix construction. |
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
One of the changed linters still lacks an executable regression around the exact failure mode this PR is fixing.
Blocking theme
timenowsub only adds golden fixtures for comment overlap. That does not prove the analyzer stopped emitting a SuggestedFix when comments are inside the replacement span, so the bug can regress without the tests noticing. The direct no-fix path needs a targeted analyzer-level assertion, similar in spirit to the new BuildContainsFix unit coverage.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 23.7 AIC · ⌖ 7.4 AIC · ⊞ 21.8K
Comment /review to run again
| @@ -25,6 +25,10 @@ func badIndex(starts []time.Time, i int) time.Duration { | |||
| return time.Now().Sub(starts[i]) // want `time\.Now\(\)\.Sub\(starts\[i\]\) can be simplified to time\.Since\(starts\[i\]\)` | |||
There was a problem hiding this comment.
This change fixes comment loss in four of the five linters, but timenowsub only got fixture updates and still has no executable regression test proving the analyzer suppresses autofixes when comments overlap. That leaves the core bug vulnerable to reintroduction because golden files alone do not assert whether a SuggestedFix was omitted versus simply happened to produce identical output.
💡 Add a real analyzer regression for the suppressed fix path
Please add a test that exercises timenowsub through the analyzer and verifies the diagnostic is still reported without a suggested fix when the replaced span contains a comment. Right now only BuildContainsFix has a unit test for the no-fix path; the other direct call-site guards rely on testdata that cannot distinguish "diagnostic with no fix" from "diagnostic with a fix that happened not to rewrite this file." A focused test here would lock in the behavior this PR is trying to preserve.
🏗️ ADR required — draft added for PR #57860I found that this PR requires ADR enforcement because the prefetch summary reports 107 added lines in default business-logic directories ( Evidence used
Gate resultNo existing ADR was present in the PR body or
Inferred decisionThe architectural decision in this PR is to prefer comment preservation over always emitting automatic whole-expression rewrites for these analyzers, and to enforce that policy through a shared overlap-check helper. Next actionPlease review and refine the draft ADR, especially the rationale and alternatives, and keep it with the PR as the implementation proceeds. Warning Firewall blocked 2 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"
- "proxy.golang.org"See Network Configuration for more information.
|
There was a problem hiding this comment.
Reviewed PR #57860 (backend Go linter fix, not a UI change — Impeccable modes not applicable).
Findings: No blocking issues. The guard pattern (HasOverlappingComment check before attaching SuggestedFixes, while still reporting the diagnostic) is applied consistently across all five linters (appendoneelement, timenowsub, stringsjoinone, stringscountcontains, stringsindexcontains). BuildContainsFix correctly threads pass.Files for the two Contains-based linters. Test coverage is solid: new unit test in astutil_test.go verifies both HasOverlappingComment detection and that BuildContainsFix returns no fixes when a comment overlaps, plus badWithComments/golden fixtures added per linter package.
No inline comments needed.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 17.7 AIC · ⌖ 13.2 AIC · ⊞ 8.3K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — the fix is correct, well-tested, and properly root-causes the comment-loss issue for the five targeted linters. No blocking issues found in the changed lines.
📋 Key Themes & Highlights
Positive Highlights
- ✅ Root cause addressed at the source:
HasOverlappingCommentcorrectly checks comment-group overlap against the replaced span, andBuildContainsFixcentralizes the guard for bothstrings.Count/strings.Indexlinters rather than duplicating the check per-caller. - ✅ Solid regression test coverage: a new unit test in
astutil_test.goexercisesHasOverlappingCommentdirectly plusBuildContainsFixreturning zero fixes when a comment overlaps, and each of the five linters gained abadWithCommentsgolden-file fixture proving the diagnostic still fires but the fix is suppressed. - ✅ Minimal, surgical diff —
diag := analysis.Diagnostic{...}+ conditionalSuggestedFixesassignment is a clean pattern that's easy to review and extend.
Follow-up worth tracking (non-blocking)
- Root cause vs. full remediation (
/diagnosing-bugs): this PR fixes 5 of the linters that attachSuggestedFixes, but agrepshows ~17 other linters (appendbytestring,bytesbufferstring,bytescomparestring,ctxbackground,execcommandwithoutcontext,fprintlnsprintf,lenstringsplit,lenstringzero,mapclearloop,mapdeletecheck,sprintfbool,sprintfint,stringreplaceminusone,stringsindexhasprefix,tolowerequalfold,writebytestring) still build whole-expressionTextEdits without an overlapping-comment guard, so they remain susceptible to the same class of silent comment loss. SinceHasOverlappingCommentand thediag := ...; if !overlap { diag.SuggestedFixes = ... }pattern are now established, it would be low-effort to sweep the remaining linters in a fast-follow PR (or track via the linked issue #57844 if it covers the full set).
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 Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 29.6 AIC · ⌖ 14.6 AIC · ⊞ 10.3K
Comment /matt to run again
|
🎉 This pull request is included in a new release. Release: |
Five linters (
appendoneelement,timenowsub,stringsjoinone,stringscountcontains, andstringsindexcontains) generated whole-expressionTextEditautofixes without checking for overlapping comments. Because AST printing drops comments, runninggo analysis -fixsilently deleted inline or trailing comments falling inside the replaced span.Changes
pkg/linters/internal/astutil): Addedfiles []*ast.Fileparameter toBuildContainsFixand guarded suggested fix construction withastutil.HasOverlappingComment.stringscountcontains,stringsindexcontains): Updated callers to passpass.FilesintoBuildContainsFix.appendoneelement,timenowsub,stringsjoinone): GuardedSuggestedFixesattachment with!astutil.HasOverlappingComment(pass.Files, pos, end).astutil_test.goand correspondingbadWithCommentsgolden testdata fixtures across all five linter test packages.