feat(linters): add errortypeassertion analyzer for error-to-concrete assertions

cmd/linters/main.go

@@ -21,6 +21,7 @@ import (
 	"github.com/github/gh-aw/pkg/linters/deferinloop"
 	"github.com/github/gh-aw/pkg/linters/errorfwrapv"
 	"github.com/github/gh-aw/pkg/linters/errormessage"
+	"github.com/github/gh-aw/pkg/linters/errortypeassertion"
 	"github.com/github/gh-aw/pkg/linters/errstringmatch"
 	"github.com/github/gh-aw/pkg/linters/excessivefuncparams"
 	"github.com/github/gh-aw/pkg/linters/execcommandwithoutcontext"
@@ -61,6 +62,7 @@ func main() {
 		ctxbackground.Analyzer,
 		deferinloop.Analyzer,
 		errormessage.Analyzer,
+		errortypeassertion.Analyzer,
 		fprintlnsprintf.Analyzer,
 		errstringmatch.Analyzer,
 		errorfwrapv.Analyzer,

docs/adr/42323-add-errortypeassertion-linter.md

@@ -0,0 +1,44 @@
+# ADR-42323: Add errortypeassertion Custom Go Analyzer
+
+**Date**: 2026-06-29
+**Status**: Draft
+**Deciders**: Unknown
+
+---
+
+### Context
+
+Go 1.13 introduced error wrapping via `fmt.Errorf("...: %w", err)` and the `errors.As` / `errors.Is` traversal API. However, callsites that use direct type assertions — `err.(*os.PathError)` — silently fail when the error value is wrapped, because a type assertion checks the concrete dynamic type of the outermost value and does not unwrap the chain. This creates correctness bugs that are easy to miss in code review: the assertion compiles and runs without panic, but always produces a zero value or `ok == false` for wrapped errors. The `gh-aw` codebase already has a suite of custom `go/analysis` analyzers in `pkg/linters/` that enforce similar error-handling patterns (e.g., `errorfwrapv`, `errstringmatch`), and adding enforcement here follows the same pattern of static analysis at build time rather than relying solely on reviewer attention.
+
+### Decision
+
+We will add a new custom `go/analysis` analyzer, `errortypeassertion`, that flags any `TypeAssertExpr` where the asserted-from expression has the built-in `error` type and the asserted-to type is a concrete (non-interface) type, and emits a diagnostic recommending `errors.As`. Interface assertions (e.g., `err.(interface{ Timeout() bool })`) and type-switch guards are intentionally excluded because they represent valid behavior checks, not wrapped-error traversal. The analyzer is registered in `cmd/linters/main.go` and in the spec test, consistent with all other analyzers in this suite.
+
+### Alternatives Considered
+
+#### Alternative 1: Code Review and Documentation Only
+
+Rely on PR reviewers and developer documentation to catch direct error type assertions. This costs nothing to implement but provides no automated enforcement, so violations persist whenever reviewers miss them. In a large codebase with many contributors, manual review is insufficient for systematic enforcement of a subtle correctness invariant.
+
+#### Alternative 2: Use an Existing Third-Party Linter (e.g., `errorlint --errorlint-assertion`)
+
+The `errorlint` linter from `golangci-lint` has an `--errorlint-assertion` flag that flags exactly this pattern. This avoids building and maintaining a custom analyzer. However, it introduces an external dependency outside the existing custom analyzer framework, would not integrate with the project's internal `nolint`, `filecheck`, and `astutil` helpers, and may flag patterns the team intentionally wants to allow — requiring either upstream configuration or wrapper logic that approaches the complexity of a custom analyzer.
+
+### Consequences
+
+#### Positive
+- Correctness bugs caused by direct error type assertions bypassing wrapped error chains are caught at static analysis time, before runtime.
+- The new analyzer reuses all existing internal infrastructure (`astutil.Inspector`, `nolint.BuildLineIndex`, `filecheck.IsTestFile`), keeping enforcement uniform and the implementation small (72 lines).
+- Developers receive an actionable diagnostic pointing them to `errors.As`, reducing the learning curve for the error wrapping pattern.
+
+#### Negative
+- Adds a custom analyzer that must be maintained alongside the internal helper packages; if the shared helpers change their API, this analyzer must be updated.
+- Any new legitimate error assertion pattern not covered by the current exclusion rules (e.g., future patterns that are not interface assertions) would produce false positives until the analyzer is updated.
+
+#### Neutral
+- The analyzer is suppressed by `//nolint:errortypeassertion` for cases where the caller knowingly uses direct assertion (e.g., in code that cannot use `errors.As` due to interface constraints). This matches the nolint convention used by all other analyzers in the suite.
+- Test files are excluded from analysis by the `filecheck.IsTestFile` helper, consistent with the suite's test-exclusion policy.
+
+---
+
+*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*

pkg/linters/README.md

@@ -12,6 +12,7 @@ This package currently provides custom Go analyzers in the following subpackages
 - `errorfwrapv` — reports `fmt.Errorf` calls that format error arguments with `%v` instead of `%w`.
 - `excessivefuncparams` — reports function declarations that exceed a configurable parameter-count threshold.
 - `errormessage` — reports non-actionable error-message patterns in changed files.
+- `errortypeassertion` — reports type assertions from `error` to concrete types and recommends `errors.As`.
 - `errstringmatch` — reports `strings.Contains(err.Error(), "...")` patterns and recommends `errors.Is` / `errors.As`.
 - `fileclosenotdeferred` — reports non-deferred file `Close()` calls that can leak resources.
 - `execcommandwithoutcontext` — reports `exec.Command(...)` calls inside functions that already receive `context.Context` and should use `exec.CommandContext(...)`.
@@ -57,6 +58,7 @@ This package currently provides custom Go analyzers in the following subpackages
 | `errorfwrapv` | Custom `go/analysis` analyzer that flags `fmt.Errorf` calls that format error arguments with `%v` instead of `%w` |
 | `excessivefuncparams` | Custom `go/analysis` analyzer that flags function declarations with too many positional parameters |
 | `errormessage` | Custom `go/analysis` analyzer that flags non-actionable error message patterns in changed files |
+| `errortypeassertion` | Custom `go/analysis` analyzer that flags type assertions from `error` to concrete types and recommends `errors.As` |
 | `errstringmatch` | Custom `go/analysis` analyzer that flags brittle `strings.Contains(err.Error(), "...")` checks |
 | `execcommandwithoutcontext` | Custom `go/analysis` analyzer that flags `exec.Command(...)` calls that should use `exec.CommandContext(...)` in context-receiving functions |
 | `fileclosenotdeferred` | Custom `go/analysis` analyzer that flags file `Close()` calls that are not deferred immediately |
@@ -147,6 +149,7 @@ _ = ssljson.Analyzer
 - `github.com/github/gh-aw/pkg/linters/contextcancelnotdeferred` — context-cancel-not-deferred analyzer subpackage
 - `github.com/github/gh-aw/pkg/linters/ctxbackground` — context-background analyzer subpackage
 - `github.com/github/gh-aw/pkg/linters/errormessage` — error-message analyzer subpackage (also re-exported as `ErrorMessageAnalyzer`)
+- `github.com/github/gh-aw/pkg/linters/errortypeassertion` — error-type-assertion analyzer subpackage
 - `github.com/github/gh-aw/pkg/linters/errstringmatch` — err-string-match analyzer subpackage
 - `github.com/github/gh-aw/pkg/linters/execcommandwithoutcontext` — exec-command-without-context analyzer subpackage
 - `github.com/github/gh-aw/pkg/linters/excessivefuncparams` — excessive-func-params analyzer subpackage

pkg/linters/errortypeassertion/errortypeassertion.go

@@ -0,0 +1,148 @@
+// Package errortypeassertion implements a Go analysis linter that flags type
+// assertions on values typed as the built-in error interface when asserting to
+// concrete types, and recommends errors.As for wrapped error traversal.
+package errortypeassertion
+
+import (
+	"go/ast"
+	"go/types"
+
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/passes/inspect"
+
+	"github.com/github/gh-aw/pkg/linters/internal/astutil"
+	"github.com/github/gh-aw/pkg/linters/internal/filecheck"
+	"github.com/github/gh-aw/pkg/linters/internal/nolint"
+)
+
+// Analyzer is the error-type-assertion analysis pass.
+var Analyzer = &analysis.Analyzer{
+	Name:     "errortypeassertion",
+	Doc:      "reports type assertions from error to concrete types; use errors.As for wrapped errors",
+	URL:      "https://github.com/github/gh-aw/tree/main/pkg/linters/errortypeassertion",
+	Requires: []*analysis.Analyzer{inspect.Analyzer},
+	Run:      run,
+}
+
+func run(pass *analysis.Pass) (any, error) {
+	insp, err := astutil.Inspector(pass)
+	if err != nil {
+		return nil, err
+	}
+	noLintLinesByFile := nolint.BuildLineIndex(pass, "errortypeassertion")
+
+	builtinErrorObj := types.Universe.Lookup("error")
+	if builtinErrorObj == nil {
+		// types.Universe always contains "error"; this branch indicates a broken
+		// Go toolchain setup and should never be reached in practice.
+		panic("errortypeassertion: types.Universe does not contain built-in error type")
+	}
+	builtinErrorType := builtinErrorObj.Type()
+
+	insp.Preorder([]ast.Node{(*ast.TypeAssertExpr)(nil), (*ast.TypeSwitchStmt)(nil)}, func(n ast.Node) {
+		switch node := n.(type) {
+		case *ast.TypeAssertExpr:
+			checkTypeAssertExpr(pass, noLintLinesByFile, builtinErrorType, node)
+		case *ast.TypeSwitchStmt:
+			checkTypeSwitchStmt(pass, noLintLinesByFile, builtinErrorType, node)
+		}
+	})
+
+	return nil, nil
+}
+
+// checkTypeAssertExpr flags direct type assertions of the form err.(ConcreteType).
+func checkTypeAssertExpr(pass *analysis.Pass, noLintLinesByFile map[string]map[int]struct{}, builtinErrorType types.Type, typeAssert *ast.TypeAssertExpr) {
+	// Type-switch guards have nil Type; skip them (handled by checkTypeSwitchStmt).
+	if typeAssert.Type == nil {
+		return
+	}
+
+	pos := pass.Fset.PositionFor(typeAssert.Pos(), false)
+	if filecheck.IsTestFile(pos.Filename) || nolint.HasDirective(pos, noLintLinesByFile) {
+		return
+	}
+
+	// types.Identical matches only the exact built-in error type. Named interface
+	// types that embed error (e.g. "type MyErr interface { error }") are
+	// intentionally excluded: they carry additional methods that may justify a
+	// direct assertion.
+	assertedFrom := pass.TypesInfo.TypeOf(typeAssert.X)
+	if assertedFrom == nil || !types.Identical(assertedFrom, builtinErrorType) {
+		return
+	}
+
+	assertedTo := pass.TypesInfo.TypeOf(typeAssert.Type)
+	if assertedTo == nil {
+		return
+	}
+	if _, isInterface := assertedTo.Underlying().(*types.Interface); isInterface {
+		return
+	}
+
+	pass.ReportRangef(
+		typeAssert,
+		"type assertion on error to %s bypasses wrapped errors; use errors.As instead",
+		assertedTo,
+	)
+}
+
+// checkTypeSwitchStmt flags concrete-type case arms in type switches on error,
+// e.g. "case *os.PathError:" inside "switch err.(type)".
+func checkTypeSwitchStmt(pass *analysis.Pass, noLintLinesByFile map[string]map[int]struct{}, builtinErrorType types.Type, stmt *ast.TypeSwitchStmt) {
+	x := typeSwitchX(stmt)
+	if x == nil {
+		return
+	}
+
+	// types.Identical matches only the exact built-in error type (see
+	// checkTypeAssertExpr for rationale).
+	assertedFrom := pass.TypesInfo.TypeOf(x)
+	if assertedFrom == nil || !types.Identical(assertedFrom, builtinErrorType) {
+		return
+	}
+
+	for _, clause := range stmt.Body.List {
+		cc, ok := clause.(*ast.CaseClause)
+		if !ok {
+			continue
+		}
+		for _, typeExpr := range cc.List {
+			pos := pass.Fset.PositionFor(typeExpr.Pos(), false)
+			if filecheck.IsTestFile(pos.Filename) || nolint.HasDirective(pos, noLintLinesByFile) {
+				continue
+			}
+
+			assertedTo := pass.TypesInfo.TypeOf(typeExpr)
+			if assertedTo == nil {
+				continue
+			}
+			if _, isInterface := assertedTo.Underlying().(*types.Interface); isInterface {
+				continue
+			}
+
+			pass.ReportRangef(
+				typeExpr,
+				"type assertion on error to %s bypasses wrapped errors; use errors.As instead",
+				assertedTo,
+			)
+		}
+	}
+}
+
+// typeSwitchX returns the expression being switched on in a TypeSwitchStmt.
+func typeSwitchX(stmt *ast.TypeSwitchStmt) ast.Expr {
+	switch a := stmt.Assign.(type) {
+	case *ast.AssignStmt:
+		if len(a.Rhs) == 1 {
+			if ta, ok := a.Rhs[0].(*ast.TypeAssertExpr); ok {
+				return ta.X
+			}
+		}
+	case *ast.ExprStmt:
+		if ta, ok := a.X.(*ast.TypeAssertExpr); ok {
+			return ta.X
+		}
+	}
+	return nil
+}

pkg/linters/errortypeassertion/errortypeassertion_test.go

@@ -0,0 +1,16 @@
+//go:build !integration
+
+package errortypeassertion_test
+
+import (
+	"testing"
+
+	"golang.org/x/tools/go/analysis/analysistest"
+
+	"github.com/github/gh-aw/pkg/linters/errortypeassertion"
+)
+
+func TestAnalyzer(t *testing.T) {
+	testdata := analysistest.TestData()
+	analysistest.Run(t, testdata, errortypeassertion.Analyzer, "errortypeassertion")
+}

pkg/linters/errortypeassertion/testdata/src/errortypeassertion/errortypeassertion.go

@@ -0,0 +1,59 @@
+package errortypeassertion
+
+import (
+	"errors"
+	"fmt"
+	"os"
+)
+
+func GoodErrorsAs(err error) {
+	var pathErr *os.PathError
+	if errors.As(err, &pathErr) {
+		fmt.Println(pathErr.Path)
+	}
+}
+
+func GoodInterfaceAssertion(err error) {
+	_, _ = err.(interface{ Timeout() bool })
+}
+
+func GoodTypeSwitch(err error) {
+	switch e := err.(type) {
+	case interface{ Temporary() bool }:
+		fmt.Println(e.Temporary())
+	}
+}
+
+func BadTypeSwitch(err error) {
+	switch e := err.(type) {
+	case *os.PathError: // want `type assertion on error to \*os\.PathError bypasses wrapped errors; use errors\.As instead`
+		fmt.Println(e.Path)
+	case interface{ Temporary() bool }:
+		fmt.Println(e.Temporary())
+	}
+}
+
+func BadSingleValue(err error) {
+	_ = err.(*os.PathError) // want `type assertion on error to \*os\.PathError bypasses wrapped errors; use errors\.As instead`
+}
+
+func BadTwoValue(err error) {
+	if pathErr, ok := err.(*os.PathError); ok { // want `type assertion on error to \*os\.PathError bypasses wrapped errors; use errors\.As instead`
+		fmt.Println(pathErr.Path)
+	}
+}
+
+type errorAlias = error
+
+func BadAlias(err errorAlias) {
+	_ = err.(*os.PathError) // want `type assertion on error to \*os\.PathError bypasses wrapped errors; use errors\.As instead`
+}
+
+func SuppressedPreviousLine(err error) {
+	//nolint:errortypeassertion
+	_ = err.(*os.PathError)
+}
+
+func SuppressedSameLine(err error) {
+	_ = err.(*os.PathError) //nolint:errortypeassertion
+}

pkg/linters/spec_test.go

@@ -16,6 +16,7 @@ import (
 	"github.com/github/gh-aw/pkg/linters/deferinloop"
 	"github.com/github/gh-aw/pkg/linters/errorfwrapv"
 	"github.com/github/gh-aw/pkg/linters/errormessage"
+	"github.com/github/gh-aw/pkg/linters/errortypeassertion"
 	"github.com/github/gh-aw/pkg/linters/errstringmatch"
 	"github.com/github/gh-aw/pkg/linters/excessivefuncparams"
 	"github.com/github/gh-aw/pkg/linters/execcommandwithoutcontext"
@@ -61,14 +62,14 @@ type docAnalyzer struct {
 }
 
 // documentedAnalyzers returns the analyzer subpackages documented in the README
-// "Public API > Subpackages" table. The README documents 35 analyzers
+// "Public API > Subpackages" table. The README documents 36 analyzers
 // subpackages (the non-analyzer `internal` helper subpackage is excluded because
 // it exposes no Analyzer).
 //
 // Spec (README "Public API > Subpackages"):
 //
 //	contextcancelnotdeferred, ctxbackground, deferinloop, errorfwrapv, excessivefuncparams, errormessage,
-//	errstringmatch, execcommandwithoutcontext, fileclosenotdeferred, fmterrorfnoverbs, fprintlnsprintf,
+//	errortypeassertion, errstringmatch, execcommandwithoutcontext, fileclosenotdeferred, fmterrorfnoverbs, fprintlnsprintf,
 //	hardcodedfilepath, httpnoctx, jsonmarshalignoredeerror, largefunc, lenstringsplit, lenstringzero,
 //	manualmutexunlock, osexitinlibrary, ossetenvlibrary, panic-in-library-code, rawloginlib,
 //	regexpcompileinfunction, seenmapbool, sortslice, sprintferrdot, sprintferrorsnew, ssljson,
@@ -82,6 +83,7 @@ func documentedAnalyzers() []docAnalyzer {
 		{"errorfwrapv", errorfwrapv.Analyzer},
 		{"excessivefuncparams", excessivefuncparams.Analyzer},
 		{"errormessage", errormessage.Analyzer},
+		{"errortypeassertion", errortypeassertion.Analyzer},
 		{"errstringmatch", errstringmatch.Analyzer},
 		{"execcommandwithoutcontext", execcommandwithoutcontext.Analyzer},
 		{"fileclosenotdeferred", fileclosenotdeferred.Analyzer},