errstringmatch: extend brittle error-string detection to HasPrefix/HasSuffix/EqualFold/Index/LastIndex/Compare

[Copilot]

The errstringmatch linter only flagged strings.Contains(err.Error(), ...), leaving equally brittle patterns using HasPrefix, HasSuffix, EqualFold, Index, LastIndex, and Compare silently unchecked.

Changes

• errstringmatch.go: Replaces the narrow isStringsContains predicate with brittleErrStringFuncName, which matches the full set of brittle strings functions via a map lookup and returns the matched name. The diagnostic message now names the matched function:

  avoid strings.HasPrefix(err.Error(), ...) — use errors.Is, errors.As, or a sentinel error instead
  

• brittleErrStringFuncs map: Contains, HasPrefix, HasSuffix, EqualFold, Index, LastIndex, Compare — all share the same two-argument shape (err.Error(), literal), so the existing arg-guard logic transfers unchanged.
• Testdata: Adds positive cases for each newly covered function and negative cases confirming non-err.Error() calls to the same functions are not flagged.
• Docs: Updates package comment and Analyzer.Doc to reflect the expanded function set.

[github-actions]

✅ Test Quality Sentinel completed test quality analysis.

No test files (as defined: Go *_test.go, JS *.test.cjs/test.js) were added or modified in PR #40248. The PR extends pkg/linters/errstringmatch to detect HasPrefix/HasSuffix/EqualFold/Index/LastIndex/Compare on err.Error(), and adds corresponding analysistest testdata cases (with // want directives) for all new functions. The existing TestErrStringMatch test function (errstringmatch_test.go) exercises these new testdata cases via analysistest.Run — but the test file itself was not modified. Test Quality Sentinel skipped per scope rules.

[github-actions]

✅ PR Code Quality Reviewer completed the code quality review.

[github-actions]

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

[github-actions]

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

No ADR enforcement needed: PR #40248 does not have the 'implementation' label (has_implementation_label=false) and has only 73 new lines in business logic directories (≤100 threshold). Neither Condition A nor Condition B is met.

[Copilot]

Pull request overview

This PR broadens the errstringmatch Go analysis linter so it no longer only flags strings.Contains(err.Error(), ...), but also detects the same brittle error-string matching pattern when implemented via other strings helpers (e.g., HasPrefix, Index, Compare), and updates testdata accordingly.

Changes:

• Expand detection from only strings.Contains to a broader set of strings.<Func>(err.Error(), <string>) calls via a centralized function-name matcher.
• Improve the diagnostic to name the specific matched strings function.
• Add new analysistest testdata cases to exercise the newly detected functions.

Show a summary per file

File	Description
pkg/linters/errstringmatch/errstringmatch.go	Expands function matching to additional brittle strings APIs and updates the reported diagnostic.
pkg/linters/errstringmatch/testdata/src/errstringmatch/errstringmatch.go	Adds positive/negative testdata cases for newly covered strings functions.

Copilot's findings

Tip

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

• Files reviewed: 2/2 changed files
• Comments generated: 2

[github-actions]

Skills-Based Review 🧠

Applied /tdd and /zoom-out — two minor observations, no blocking issues.

📋 Key Themes & Highlights

Key Themes

• Test coverage gap: 4 of the 6 newly covered functions (HasSuffix, Index, LastIndex, Compare) are missing negative test cases confirming that plain-string callers are not flagged. See inline comment on testdata.
• Known second-arg gap: EqualFold and Compare are symmetric functions; strings.EqualFold(s, err.Error()) is equally brittle but would not be flagged (consistent with the pre-existing behaviour for Contains). Worth a brief acknowledgement comment.

Positive Highlights

• ✅ Clean extension: replacing the single isStringsContains boolean predicate with a map-based brittleErrStringFuncName returning (string, bool) is the right abstraction — easy to extend, easy to read.
• ✅ Diagnostic message now names the matched function (avoid strings.HasPrefix(err.Error(), ...)), making the lint output actionable.
• ✅ All six new positive test cases are present with correctly escaped want directives that verify the function name in the message.
• ✅ map[string]bool set pattern is consistent with rawloginlib and other linters in this package.
• ✅ Package comment and Analyzer.Doc are updated in sync with the implementation.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 70.3 AIC · ⌖ 7.71 AIC · ⊞ 6.9K

[github-actions]

[/tdd] Only HasPrefix and EqualFold have negative ("not flagged") test cases; HasSuffix, Index, LastIndex, and Compare are missing equivalent coverage. If isErrDotError were inadvertently broken for those functions, no test would catch it.

💡 Suggested additions

// not flagged: strings.HasSuffix on a plain string, not err.Error()
func checkHasSuffixString(s string) bool {
	return strings.HasSuffix(s, "suffix")
}

// not flagged: strings.Index on a plain string, not err.Error()
func checkIndexString(s string) bool {
	return strings.Index(s, "sub") >= 0
}

// not flagged: strings.LastIndex on a plain string, not err.Error()
func checkLastIndexString(s string) bool {
	return strings.LastIndex(s, "sub") >= 0
}

// not flagged: strings.Compare on a plain string, not err.Error()
func checkCompareString(s string) bool {
	return strings.Compare(s, "value") == 0
}

One negative test per function makes the testdata file a self-documenting specification of what is and is not flagged.

[github-actions]

[/zoom-out] EqualFold and Compare are semantically symmetric — strings.EqualFold(someString, err.Error()) and strings.Compare(someString, err.Error()) == 0 are equally brittle patterns, but they would not be flagged because only outer.Args[0] is checked for err.Error(). This is consistent with the existing behaviour for Contains, so it is not a regression introduced here.

Worth adding a brief comment (or a TODO) to the package doc or isErrDotError acknowledging this known gap, so future contributors do not confuse the absence of second-arg detection with intentional design.

[github-actions]

🔎 Code quality review by PR Code Quality Reviewer · 71.5 AIC · ⌖ 7.71 AIC · ⊞ 5.1K

[github-actions]

EqualFold and Compare with swapped arguments silently bypass the linter, creating a false-confidence gap for the two symmetric functions this PR introduces.

💡 Details

strings.EqualFold and strings.Compare are symmetric (argument order does not change the equality result). The linter only checks outer.Args[0] against err.Error(), so:

strings.EqualFold("timeout", err.Error())   // not flagged — false negative
strings.Compare("EOF", err.Error()) == 0    // not flagged — false negative

For the non-symmetric functions (HasPrefix, HasSuffix, Index, LastIndex) the reversed order has a different semantic, so skipping it is defensible. But EqualFold and Compare are genuinely equivalent in either order when used for equality testing.

Consider extending the check for these two specifically:

// for EqualFold and Compare, either arg can be err.Error()
isSymmetric := funcName == "EqualFold" || funcName == "Compare"
arg0Err := isErrDotError(pass, outer.Args[0])
arg1Err := isErrDotError(pass, outer.Args[1])
if !arg0Err && !(isSymmetric && arg1Err) {
    return
}

[github-actions]

No nolint:errstringmatch suppression test covers any of the six newly added functions, leaving the nolint path untested for the expanded diagnostic message format.

💡 Details

The existing nolint tests at lines 30–37 verify suppression only for strings.Contains. None of the new functions (HasPrefix, HasSuffix, EqualFold, Index, LastIndex, Compare) have a corresponding suppression test. The diagnostic message changed from a hardcoded string to a fmt.Sprintf-formatted string ("avoid strings.%s(err.Error(), ..."). If the format regresses, the nolint directive lookup could silently break for any of these six new functions without a test catching it.

Add at least one suppression test for a representative new function:

func checkHasPrefixNolint(err error) bool {
    (nolint/redacted):errstringmatch
    return strings.HasPrefix(err.Error(), "connection refused")
}

[github-actions]

Negative test coverage is incomplete: Index, LastIndex, and Compare have no "not flagged" test cases for non-error string arguments.

💡 Details

Lines 69–77 add negative cases only for HasPrefix and EqualFold with non-error string arguments. Index, LastIndex, and Compare have no corresponding negative tests. A regression where the linter incorrectly flags strings.Index(someString, "x") would go undetected. Add:

func checkIndexString(s string) bool {
    return strings.Index(s, "denied") >= 0
}
func checkLastIndexString(s string) bool {
    return strings.LastIndex(s, "denied") >= 0
}
func checkCompareString(s string) bool {
    return strings.Compare(s, "timeout") == 0
}