mapdeletecheck: suppress autofix when comments overlap the replaced span by Copilot · Pull Request #46159 · github/gh-aw

Copilot · 2026-07-17T07:06:19Z

mapdeletecheck emitted SuggestedFix unconditionally, so any comment inside the if body was silently dropped when the autofix was applied. mapclearloop already guards against this; mapdeletecheck was inconsistent.

Changes

pkg/linters/internal/astutil — Added HasOverlappingComment(files, start, end), lifting the previously private helper from mapclearloop into the shared utility package.
pkg/linters/mapclearloop — Replaced local hasOverlappingComment with astutil.HasOverlappingComment.
pkg/linters/mapdeletecheck — Guards SuggestedFixes with astutil.HasOverlappingComment; diagnostic always fires, fix is suppressed when comments overlap the ifStmt span.
testdata — Added badWithComments() cases (leading and trailing comment) to assert diagnostic fires but no autofix is emitted.

Behavior

// Before: autofix would silently drop this comment
if _, ok := cache[key]; ok {
    // evict the stale entry before refetching
    delete(cache, key)
}

// After: diagnostic fires, fix is suppressed — developer applies it manually

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

…ix against comment loss Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Copilot

Pull request overview

Prevents mapdeletecheck autofixes from discarding comments by sharing comment-overlap detection with mapclearloop.

Changes:

Extracts HasOverlappingComment into shared AST utilities.
Suppresses unsafe fixes while retaining diagnostics.
Adds comment-related fixtures and refreshes workflow lock metadata.

Show a summary per file

File	Description
`pkg/linters/internal/astutil/astutil.go`	Adds shared overlap detection.
`pkg/linters/mapclearloop/mapclearloop.go`	Uses shared helper.
`pkg/linters/mapdeletecheck/mapdeletecheck.go`	Guards suggested fixes.
`pkg/linters/mapdeletecheck/testdata/src/mapdeletecheck/mapdeletecheck.go`	Adds commented test cases.
`pkg/linters/mapdeletecheck/testdata/src/mapdeletecheck/mapdeletecheck.go.golden`	Adds expected unchanged cases.
`.github/workflows/skillet.lock.yml`	Normalizes action version metadata.
`.github/workflows/release.lock.yml`	Normalizes action version metadata.
`.github/workflows/hourly-ci-cleaner.lock.yml`	Normalizes action version metadata.
`.github/workflows/avenger.lock.yml`	Normalizes action version metadata.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Files reviewed: 9/9 changed files
Comments generated: 1
Review effort level: Medium

 			Message: "redundant existence check before delete: delete(" + mText + ", " + kText + ") is already a no-op when the key is absent; remove the if statement",
-			SuggestedFixes: []analysis.SuggestedFix{{
+		}
+		if !astutil.HasOverlappingComment(pass.Files, ifStmt.Pos(), ifStmt.End()) {


github-actions · 2026-07-17T09:20:44Z

✅ Design Decision Gate 🏗️ completed the design decision gate check.

No ADR enforcement needed: PR #46159 does not have the 'implementation' label and has only 58 new lines of code in business logic directories (threshold: 100).

github-actions · 2026-07-17T09:20:47Z

✅ PR Code Quality Reviewer completed the code quality review.

github-actions · 2026-07-17T09:20:48Z

✅ Test Quality Sentinel completed test quality analysis.

No test files were added or modified in this PR. The PR (#46159) only modifies production code in pkg/linters/ (astutil, mapclearloop, mapdeletecheck) and test data files (.golden and testdata), but no *_test.go or *.test.js files. Test Quality Sentinel analysis skipped.

github-actions · 2026-07-17T09:20:51Z

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

github-actions

Clean refactor: extracts hasOverlappingComment into shared astutil.HasOverlappingComment, applies it consistently in both mapclearloop and mapdeletecheck, and adds test cases that confirm the diagnostic still fires while autofix is correctly suppressed. No issues found.

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 18.8 AIC · ⌖ 5.35 AIC · ⊞ 5K

github-actions

One existing unresolved bug blocks merge

The open review comment at line 124 of mapdeletecheck.go identifies a real correctness break: the // want directives on the bad() if-statements are trailing comments on the if line, falling inside [ifStmt.Pos(), ifStmt.End()). HasOverlappingComment therefore returns true for those cases too, suppressing their SuggestedFix — but the golden file expects the rewritten delete(...) form. The analysistest run will fail.

Lines 7 and 12 of the test fixture both carry // want ... as trailing comments inside the if-statement span. No fix is emitted. Golden expects the fix applied. Must be resolved before merge.

One low-severity nil-guard observation left inline on astutil.go.

🔎 Code quality review by PR Code Quality Reviewer · 49.2 AIC · ⌖ 4.41 AIC · ⊞ 5.6K
_{Comment /review to run again}

github-actions · 2026-07-17T09:29:24Z

+		if end <= file.Pos() || start >= file.End() {
+			continue
+		}
+		for _, group := range file.Comments {


Nil file entry will panic: if files contains a nil *ast.File (e.g. a buggy or test-injected pass), file.Pos(), file.End(), and file.Comments will all panic.

💡 Suggested fix

for _, file := range files { if file == nil { continue } if end <= file.Pos() || start >= file.End() {

This won't happen in normal go/analysis usage since pass.Files is always populated by the framework, but the function is exported and could be called by future linters that build a synthetic file slice. The nil check is one line and removes the footgun.

github-actions

Skills-Based Review 🧠

Applied /diagnosing-bugs and /tdd — requesting changes on two actionable issues.

📋 Key Themes & Highlights

Issues Found

Test fixture interference (critical): analysistest // want directives fall within ifStmt.Pos()..ifStmt.End(), meaning HasOverlappingComment may suppress the suggested fix even for the plain bad() cases during testing. The golden file needs verification that it reflects real auto-fix behaviour (see inline comment).
Missing unit test for extracted utility: HasOverlappingComment is a new shared function in astutil with no direct unit tests, leaving boundary conditions (adjacent comments, zero-width ranges) unexercised.

Positive Highlights

✅ Clean extraction of the private helper into the shared astutil package.
✅ Diagnostic always fires; only the autofix is suppressed — correct user experience.
✅ badWithComments() test cases are cleanly separated and well-commented.

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 · 45 AIC · ⌖ 4.49 AIC · ⊞ 6.7K
_{Comment /matt to run again}

Comments that could not be inline-anchored

pkg/linters/internal/astutil/astutil.go:136

[/tdd] HasOverlappingComment has no unit test in astutil_test.go — correctness is only exercised indirectly through linter integration tests, leaving boundary conditions (e.g. comment.End == start, zero-width range) untested.

<details>
<summary>💡 Suggested unit test sketch</summary>

func TestHasOverlappingComment(t *testing.T) {
    src := &quot;package p\n// comment\nvar x = 1\n&quot;
    fset := token.NewFileSet()
    f, _ := parser.ParseFile(fset, &quot;x.go&quot;, src, parser.ParseComments)
  …

</details>

done

pelikhan · 2026-07-17T11:26:31Z

@copilot run pr-finisher skill

gh-aw-bot · 2026-07-17T11:33:12Z

@copilot please run the pr-finisher skill, investigate the failed checks listed below, address any remaining review feedback, and rerun checks once the branch is up to date.

Failed checks:

agent: https://github.com/github/gh-aw/actions/runs/29576477781/job/87872232187

Generated by PR Sous Chef: https://github.com/github/gh-aw/actions/runs/29576656963

Generated by 👨‍🍳 PR Sous Chef · 6.7 AIC · ⌖ 7.14 AIC · ⊞ 7.5K · ◷
_{Comment /souschef to run again}

…rectives from suppressing autofixes Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

… autofixes The // want directives on the if line fell within [ifStmt.Pos(), ifStmt.End()), causing HasOverlappingComment to suppress ALL fixes including bad() cases that should have a fix. RunWithSuggestedFixes never populated allFilenames, so the golden file comparison never ran. Move // want to // want +1 on the line before each if statement so they are outside the replaced span. Update the golden file to include the // want +1 comment lines that remain after fixes are applied. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Copilot · 2026-07-17T11:43:38Z

@copilot run pr-finisher skill

Addressed in the latest commit. Moved all // want directives to // want +1 on the line before each if statement. This places the test directive before ifStmt.Pos(), keeping it outside the [ifStmt.Pos(), ifStmt.End()) span that HasOverlappingComment checks — so the bad() cases now correctly receive their suggested fix and the golden file comparison runs. Verified by deliberately corrupting the golden file: the test now fails as expected.

The function is exported and could be called with a synthetic file slice; skip nil entries to avoid a panic on file.Pos()/file.End()/file.Comments. Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>

Copilot · 2026-07-17T11:53:42Z

@copilot please run the pr-finisher skill, investigate the failed checks listed below, address any remaining review feedback, and rerun checks once the branch is up to date.

Failed checks:...

Investigated the CI failure. The Smoke CI job failed due to a runner infrastructure error — EACCES: permission denied, open '/tmp/awf-chroot-mf4XYK/hosts' — which is unrelated to the code changes in this PR. Also addressed the github-actions[bot] review thread: added a nil guard to HasOverlappingComment in commit fix(astutil): guard HasOverlappingComment against nil *ast.File entries. All local tests pass (make fmt, make test for affected packages).

github-actions · 2026-07-18T09:23:18Z

🎉 This pull request is included in a new release.

Release: v0.82.13

Initial plan

fda77a4

Copilot AI assigned Copilot and pelikhan Jul 17, 2026

Copilot started work on behalf of pelikhan July 17, 2026 07:07 View session

Copilot AI linked an issue Jul 17, 2026 that may be closed by this pull request

mapdeletecheck: autofix silently deletes comments inside the if body — no comment guard, unlike its sibling mapclearloop #46130

Closed

4 tasks

Copilot AI and others added 3 commits July 17, 2026 07:12

Initial plan for mapdeletecheck comment guard

5376003

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Add HasOverlappingComment to astutil; guard mapdeletecheck SuggestedF…

65c7291

…ix against comment loss Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Apply remaining changes

8644cae

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Copilot AI changed the title ~~[WIP] Fix mapdeletecheck to guard comments during autofix~~ mapdeletecheck: suppress autofix when comments overlap the replaced span Jul 17, 2026

Copilot finished work on behalf of pelikhan July 17, 2026 07:21

Copilot AI requested a review from pelikhan July 17, 2026 07:21

pelikhan marked this pull request as ready for review July 17, 2026 08:46

Copilot AI review requested due to automatic review settings July 17, 2026 08:46

Copilot started reviewing on behalf of pelikhan July 17, 2026 08:46 View session

Copilot AI reviewed Jul 17, 2026

View reviewed changes

github-actions Bot mentioned this pull request Jul 17, 2026

[aw] No-Op Runs #43499

Open

github-actions Bot approved these changes Jul 17, 2026

View reviewed changes

github-actions Bot requested changes Jul 17, 2026

View reviewed changes

github-actions Bot previously requested changes Jul 17, 2026

View reviewed changes

Merge branch 'main' into copilot/fix-mapdeletecheck-comment-guard

2964cea

Copilot started work on behalf of pelikhan July 17, 2026 11:27 View session

github-actions Bot mentioned this pull request Jul 17, 2026

[pr-sous-chef] Run report — 0 nudged, 0 skipped #46103

Closed

Fix mapdeletecheck test fixture: use // want +1 to prevent // want di…

2d18639

…rectives from suppressing autofixes Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Copilot finished work on behalf of pelikhan July 17, 2026 11:43

Copilot started work on behalf of gh-aw-bot July 17, 2026 11:44 View session

Copilot finished work on behalf of gh-aw-bot July 17, 2026 11:54

Copilot AI requested a review from gh-aw-bot July 17, 2026 11:54

github-actions Bot added pr-action:fast_track pr-agent:copilot pr-priority:medium pr-risk:low pr-type:bug labels Jul 17, 2026

pelikhan merged commit 53fbba0 into main Jul 18, 2026

pelikhan deleted the copilot/fix-mapdeletecheck-comment-guard branch July 18, 2026 05:29

This was referenced Jul 18, 2026

Smoke Test: MAI-Code-1-Flash - 29632319598 #46346

Closed

Smoke Test: MAI-Code-1-Flash - 29638034909 #46385

Closed

Uh oh!

Conversation

Copilot AI commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Behavior

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Review details

Uh oh!

github-actions Bot commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

One existing unresolved bug blocks merge

Uh oh!

github-actions Bot Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Skills-Based Review 🧠

Issues Found

Positive Highlights

Comments that could not be inline-anchored

Uh oh!

pelikhan commented Jul 17, 2026

Uh oh!

gh-aw-bot commented Jul 17, 2026

Uh oh!

Copilot AI commented Jul 17, 2026

Uh oh!

Copilot AI commented Jul 17, 2026

Uh oh!

github-actions Bot commented Jul 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Copilot AI commented Jul 17, 2026 •

edited

Loading

github-actions Bot commented Jul 17, 2026 •

edited

Loading

github-actions Bot commented Jul 17, 2026 •

edited

Loading

github-actions Bot commented Jul 17, 2026 •

edited

Loading

github-actions Bot commented Jul 17, 2026 •

edited

Loading