Repository navigation
Conversation
|
✅ PR Specification Reviewer completed successfully! PR Specification ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Ponytail Reviewer completed successfully! Ponytail ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Design Decision Gate 🏗️Outcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
🧠 Matt Pocock Skills Reviewer failed to deliver outputs during the skills-based review.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues are reported for the linter’s dot-imported receiver fix.
No open issues 🎉
What changed in this PR
This PR fixes a missed diagnostic in gh-aw’s ioutildeprecated linter for dot-imported Discard used as a selector receiver, such as Discard.Write(p).
Changes:
- Skip only selector member identifiers, allowing dot-imported receivers to be checked.
- Add regression fixtures for dot-imported values and method calls.
- Add fixtures guarding against duplicate diagnostics on qualified calls and false positives on fields named
Discard.
| File | Description |
|---|---|
pkg/linters/ioutildeprecated/testdata/src/ioutildeprecated/ioutildeprecated.go |
Adds qualified-call and field-name regression cases. |
pkg/linters/ioutildeprecated/testdata/src/ioutildeprecated/dot_import.go |
Adds dot-imported Discard value and receiver cases. |
pkg/linters/ioutildeprecated/ioutildeprecated.go |
Narrows selector filtering to retain receiver checks. |
Review effort: Balanced (auto)
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review summary
No actionable issues found in the changed lines. The selector-side check correctly preserves dot-imported receiver diagnostics while avoiding duplicate reports for qualified selectors; added fixtures cover the key cases.
Verification
Static review completed using the pre-fetched diff. Tests were not run because this review is source-only and the PR notes sandbox denial.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · ~2 AIC · ⌖ ~1 AIC · ⊞ 1.6K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd. Approving: the fix is correct and well-tested. Two minor nits are left as inline comments.
📋 Verification and key themes
Verification
- Ran
go test ./pkg/linters/ioutildeprecated/locally with disposable caches. It passes. - Mutation check: with the
Selskip removed,TestIoutilDeprecatedfails with unexpected double reports on every qualifiedioutil.*use (includingioutil.Discard.Writeat line 49). So theSelskip is load-bearing and the newBadDiscardMethodfixture guards it. - Reverting only the new condition makes
BadDotDiscardMethod(dot_import.go:30) fail with a missing diagnostic. The fixture guards the false negative. gofmt -lis clean. No production code dot-importsio/ioutil.
Key themes
- Root cause, not symptom: the over-broad parent check is narrowed to the exact case that was wrong.
- Double-report guard: the qualified
ioutil.Discard.Writecase is covered with an exact one-diagnostic expectation. - Stale comment: the fixture comment describes history rather than intent (inline).
- Negative control:
GoodFieldNamedDiscardpasses regardless of theSelchange, so it is not what guards the fix (inline).
Positive highlights
- ✅ The inline comment explaining why
Selis skipped butXis kept is accurate and prevents a future regression. - ✅ Good use of
// wantfixtures for both the positive and the double-report cases.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · routed: haiku55 xhigh · ~6 AIC · ⌖ ~1 AIC · ⊞ 5.4K
Comment /matt to run again
| _ = Discard // want `ioutil\.Discard is deprecated; use io\.Discard instead` | ||
| } | ||
|
|
||
| // Discard used as a selector receiver was previously skipped because the |
There was a problem hiding this comment.
[/diagnosing-bugs] Nit: this comment records history ("was previously skipped"), which will read as stale once the fix lands. Describing what the case guards is more durable, for example: // Discard as a selector receiver must be reported.
@copilot please address this.
| Discard int | ||
| } | ||
|
|
||
| // A field that shares a deprecated name is not reported. |
There was a problem hiding this comment.
[/tdd] GoodFieldNamedDiscard does not guard the Sel change. It passes even with the Sel skip removed, because a struct field is rejected by the io/ioutil package check, not the selector check. BadDiscardMethod is the test that guards the Sel skip, so keep this as a negative control but do not rely on it as a regression guard.
💡 Verification
I removed the Sel skip entirely and re-ran go test ./pkg/linters/ioutildeprecated/. The new failures were the qualified ioutil.* double reports, and h.Discard at line 58 stayed clean.
@copilot please address this.
|
Copilot assignment requested for the following outstanding work:
Once you start work, post this work summary as a PR comment without the leading Copilot mention, including the Sous-chef head, work, and state lines below, to record that the assignment is running. Apply the necessary fixes on this PR's existing branch, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved; stop when only human review or CI remains. Do not merge or trigger CI. Sous-chef requested-head: 6034f1e
|
…d-dot-import-receiver-r121-377a88d4d227732a Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Summary
ioutildeprecated's dot-import pass (checkDotImportIoutilUsage) skipped every identifier whose parent is an*ast.SelectorExpr. The intent was to skip theSelside (ioutil.Xis handled bycheckQualifiedIoutilUsage, andx.Discardcould be a field). But the check also skipped theXside, so a dot-imported receiver was never reported:Discardis the only entry inreplacementsthat is a value with methods, so it's the only name that can appear as a selector receiver.Fix
The
Xside of a qualifiedioutil.Discardis theioutilidentifier. It resolves to a*types.PkgNameowned by the importing package, notio/ioutil, so this change can't double-report qualified uses.Tests
dot_import.go: newBadDotDiscardMethod(Discard.Write(...), which was the false negative) andBadDotDiscard(plain dot-imported value) as a baseline.ioutildeprecated.go:BadDiscardMethod(ioutil.Discard.Write(...)) expects exactly one diagnostic, guarding against double reports.GoodFieldNamedDiscardchecks that a struct field namedDiscardis still not reported.Evidence and validation
cursor.Parent()uses inpkg/linters(Sergo run 121).osisnotexistandastutil.IsGoOrDeferClosurealready handle intermediate nodes correctly. This was the only over-broad parent check.get_diagnostics_for_file(gopls via Serena) reports 0 diagnostics onioutildeprecated.go. Both fixtures type-check with no errors. gopls'sdeprecatedanalyzer also reportsioutil.Discard.Writeonce, which matches the new expectation.io/ioutil, so this CI-enforced linter won't report anything new on existing code.go testwas denied by the sandbox in this run, soanalysistesthasn't run. Please let CI rungo test ./pkg/linters/ioutildeprecated/.🤖 Generated with [Claude Code]((claude.com/redacted)