[linter-miner] feat(linters): add errstringmatch linter — flag strings.Contains(err.Error(), ...) calls#33117
Conversation
Flags strings.Contains(err.Error(), "...") calls that perform brittle substring matching on error messages instead of using errors.Is or errors.As. Evidence from codebase scan: - 7 real occurrences in pkg/cli and pkg/workflow - Issues #32751, #32752 (error predicate duplication) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
automation
cookie
There was a problem hiding this comment.
Pull request overview
Adds a new errstringmatch Go analysis linter that flags strings.Contains(err.Error(), ...) calls, which are brittle substring matches on error messages, encouraging use of errors.Is/errors.As/sentinel errors instead. The analyzer is registered in the cmd/linters multichecker and includes analysistest-based fixtures.
Changes:
- New
errstringmatchanalyzer usinggo/analysis+inspectto detect thestrings.Contains(<error>.Error(), <string>)pattern via type-checked receiver. - Test fixtures with positive and negative cases and an
analysistest-driven test. - Registers the new analyzer in
cmd/linters/main.go.
Show a summary per file
| File | Description |
|---|---|
pkg/linters/errstringmatch/errstringmatch.go |
Analyzer implementation: matches strings.Contains calls whose first arg is an Error() method call on a value implementing error and whose second arg is a string-typed expression. |
pkg/linters/errstringmatch/errstringmatch_test.go |
Wires analysistest.Run against the testdata fixture. |
pkg/linters/errstringmatch/testdata/src/errstringmatch/errstringmatch.go |
Fixture with two flagged and two unflagged cases, using // want regex annotations. |
cmd/linters/main.go |
Registers errstringmatch.Analyzer in the multichecker entry point. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 0
🧪 Test Quality Sentinel ReportTest Quality Score: 100/100✅ Excellent test quality
Test Classification Details
Language SupportTests analyzed:
Verdict
The single test uses 📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators. References: §26052123418
|
![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: 100/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). The analysistest.Run-based test covers both flagged and non-flagged cases, providing strong behavioral contract coverage.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /zoom-out — this is a new-feature PR adding a static analysis pass, so test coverage and internal code clarity are the right lenses.
Key Themes
isStringLiteralscope creep — the helper accepts anystring-typed expression (variables, function-call return values), not just literals/constants. The name and docstring claim otherwise, and there's no testdata case to pin the actual policy. This may produce false positives on non-brittle patterns.- Dead branch in
implementsError—pass.Pkg.Scope().Lookup("error")is alwaysnilin ordinary packages becauseerroris a universe builtin. The outer branch is never reached, making the function harder to read than it needs to be. - Thin testdata — the four test cases cover the happy paths, but boundary cases (non-literal second arg, chained
.Error(), wrapped errors) are absent.analysistestfixtures double as executable specifications — they're a great place to document intent.
Positive Highlights
- ✅ Solid use of
golang.org/x/tools/go/analysis— type-aware receiver check viaimplementsErroris much more robust than a naive AST name match. - ✅ Build tag, package layout, and registration in
cmd/linters/main.goall follow existing conventions perfectly. - ✅ Clear PR description with a codebase scan table — makes the motivation concrete and reviewable.
Verdict
Comments only — no blocking issues. The isStringLiteral scope question is worth settling before the linter is run on the codebase (it will fire on non-literal second args today), and implementsError is a straightforward simplification.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 4.9M
| if t == nil { | ||
| return false | ||
| } | ||
| basic, ok := t.Underlying().(*types.Basic) |
There was a problem hiding this comment.
[/tdd] isStringLiteral accepts any string-typed expression — not just literals and constants. The function name and comment claim "string literal or untyped string constant", but basic.Kind() == types.String is also true for a plain string variable.
This means the linter will flag patterns like:
strings.Contains(err.Error(), statusCode) // statusCode is a string var...which is arguably less brittle than a hardcoded literal. Consider restricting to *ast.BasicLit and constant identifiers only, or rename the function to isStringArg and document that non-literal strings are intentionally flagged.
A pinning test case would clarify intent:
// document whether non-literal is flagged or not
func checkVar(err error, msg string) bool {
return strings.Contains(err.Error(), msg)
}| // implementsError reports whether t implements the built-in error interface. | ||
| func implementsError(pass *analysis.Pass, t types.Type) bool { | ||
| errIface := pass.Pkg.Scope().Lookup("error") | ||
| if errIface == nil { |
There was a problem hiding this comment.
[/zoom-out] pass.Pkg.Scope().Lookup("error") will almost never return non-nil. error is a predeclared builtin that lives in types.Universe, not in any package scope — only a pathological package that re-declares an identifier named error would trigger the non-nil branch.
This makes the function's control flow misleading: the "happy path" is actually always the nil branch. Simplify to remove the dead outer branch:
func implementsError(pass *analysis.Pass, t types.Type) bool {
obj := types.Universe.Lookup("error")
if obj == nil {
return false
}
iface, ok := obj.Type().Underlying().(*types.Interface)
if !ok {
return false
}
return types.Implements(t, iface) || types.Implements(types.NewPointer(t), iface)
}| // not flagged: strings.Contains on a plain string, not err.Error() | ||
| func checkString(s string) bool { | ||
| return strings.Contains(s, "prefix") | ||
| } |
There was a problem hiding this comment.
[/tdd] The testdata covers only two flagged cases and two safe cases — both false-negative scenarios use structurally different call sites (errors.Is and plain string), but the test suite is missing cases that pin the boundary between flagged and non-flagged:
- Non-literal string variable — is
strings.Contains(err.Error(), msgVar)flagged or not? The currentisStringLiteralimplementation says yes (it accepts anystringtype), but that may not be the desired policy. - Chained
.Error()calls — e.g.strings.Contains(errors.Unwrap(err).Error(), "x")— does the receiver-type check handle this? - Named return / multi-assign —
strings.Contains(err.Error(), fmt.Sprintf("%d", code))where the second arg isstringbut not a literal.
Adding one or two of these as // not want or // want cases would lock in the intended contract and prevent silent scope creep when the implementation is refactored.
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (+179 new lines in Adding a new repo-specific Go static-analysis linter is exactly the kind of choice an ADR should record: it codifies a new code-quality rule, introduces a new package layout convention ( The gate auto-generated a draft ADR from the PR diff. It could not push the file directly to your branch (this PR originates from a fork, which 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. 📄 Draft ADR — copy into
|
Summary
Adds a new custom Go analysis linter
errstringmatchthat flags calls tostrings.Contains(err.Error(), "literal")— a brittle pattern that matches error messages by substring instead of using the propererrors.Is/errors.Asmechanisms.What the linter catches
The diagnostic emitted is:
Evidence
Codebase scan found 7 real occurrences in production code:
pkg/cli/project_command.go:306strings.Contains(err.Error(), "INSUFFICIENT_SCOPES")pkg/cli/audit.go:278strings.Contains(err.Error(), "403")pkg/cli/add_interactive_git.go:109strings.Contains(mergeErr.Error(), "already merged")pkg/cli/logs_download.go:320strings.Contains(err.Error(), "410")pkg/workflow/schedule_preprocessing.go:192strings.Contains(err.Error(), "syntax is not supported")Related issues:
isPermissionErrorduplicated 4× across call sitesisNotFoundErrorre-inlined instead of sharing a common predicateFiles changed
pkg/linters/errstringmatch/errstringmatch.go— analyzer implementationpkg/linters/errstringmatch/errstringmatch_test.go—analysistest-based testspkg/linters/errstringmatch/testdata/src/errstringmatch/errstringmatch.go— test fixtures with// wantannotationscmd/linters/main.go— registers the new analyzerValidation