Add a cop that looks at nil checks against an interface

cmd/root/debug.go

@@ -10,6 +10,7 @@ import (
 
 	"github.com/docker/docker-agent/pkg/cli"
 	"github.com/docker/docker-agent/pkg/config"
+	"github.com/docker/docker-agent/pkg/reflectx"
 	"github.com/docker/docker-agent/pkg/sessiontitle"
 	"github.com/docker/docker-agent/pkg/team"
 	"github.com/docker/docker-agent/pkg/teamloader"
@@ -158,7 +159,7 @@ func (f *debugFlags) runDebugTitleCommand(cmd *cobra.Command, args []string) (co
 	}
 
 	model := agent.Model(ctx)
-	if model == nil {
+	if reflectx.IsNil(model) {
 		return fmt.Errorf("agent %q has no model configured", agent.Name())
 	}
 

cmd/root/run.go

@@ -24,6 +24,7 @@ import (
 	"github.com/docker/docker-agent/pkg/paths"
 	"github.com/docker/docker-agent/pkg/permissions"
 	"github.com/docker/docker-agent/pkg/profiling"
+	"github.com/docker/docker-agent/pkg/reflectx"
 	"github.com/docker/docker-agent/pkg/runtime"
 	"github.com/docker/docker-agent/pkg/session"
 	"github.com/docker/docker-agent/pkg/teamloader"
@@ -514,7 +515,7 @@ func (f *runExecFlags) buildAppOpts(args []string) ([]app.Opt, error) {
 	if f.exitAfterResponse {
 		opts = append(opts, app.WithExitAfterFirstResponse())
 	}
-	if f.snapshotController != nil {
+	if !reflectx.IsNil(f.snapshotController) {
 		opts = append(opts, app.WithSnapshotController(f.snapshotController))
 	}
 	return opts, nil
@@ -586,7 +587,7 @@ func (f *runExecFlags) createSessionSpawner(agentSource config.Source, sessStore
 		if gen := localRt.TitleGenerator(); gen != nil {
 			appOpts = append(appOpts, app.WithTitleGenerator(gen))
 		}
-		if ctrl != nil {
+		if !reflectx.IsNil(ctrl) {
 			appOpts = append(appOpts, app.WithSnapshotController(ctrl))
 		}
 

go.mod

@@ -71,6 +71,7 @@ require (
 	golang.org/x/sync v0.20.0
 	golang.org/x/sys v0.44.0
 	golang.org/x/term v0.43.0
+	golang.org/x/tools v0.45.0
 	google.golang.org/adk v1.2.0
 	google.golang.org/genai v1.57.0
 	gopkg.in/dnaeon/go-vcr.v4 v4.0.6
@@ -99,7 +100,6 @@ require (
 	github.com/standard-webhooks/standard-webhooks/libraries v0.0.1 // indirect
 	go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc v0.63.0 // indirect
 	golang.org/x/mod v0.36.0 // indirect
-	golang.org/x/tools v0.45.0 // indirect
 	google.golang.org/api v0.272.0 // indirect
 )
 

lint/interface_nil_comparison.go

@@ -0,0 +1,261 @@
+package main
+
+import (
+	"go/ast"
+	"go/token"
+	"go/types"
+	"os"
+	"path/filepath"
+	"strings"
+	"sync"
+
+	"github.com/dgageot/rubocop-go/cop"
+	"golang.org/x/tools/go/packages"
+)
+
+// InterfaceNilComparison rejects nil comparisons against Docker-owned
+// non-empty interface types. Interface values can hold typed nil pointers, so
+// `x == nil` and `x != nil` only test the interface header and can miss a nil
+// implementation value. Use reflectx.IsNil instead.
+//
+// The cop intentionally ignores error, any/interface{}, and interfaces owned by
+// external packages to keep the rule focused on project abstractions we control.
+var InterfaceNilComparison = &cop.Func{
+	Meta: cop.Meta{
+		Name:        "Lint/InterfaceNilComparison",
+		Description: "use reflectx.IsNil instead of comparing project interface values to nil",
+		Severity:    cop.Warning,
+	},
+	Run: func(p *cop.Pass) {
+		if typed, ok := typedFileForInterfaceNilComparison(p); ok {
+			checkInterfaceNilComparisons(p, typed.file, typed.fset, typed.info, typed.pkg, typed.modulePath)
+			return
+		}
+		if p.Info != nil {
+			checkInterfaceNilComparisons(p, p.File, p.FileSet, p.Info, p.Package, "")
+		}
+	},
+}
+
+type interfaceNilTypedFile struct {
+	file       *ast.File
+	fset       *token.FileSet
+	info       *types.Info
+	pkg        *types.Package
+	modulePath string
+}
+
+type interfaceNilCache struct {
+	files map[string][]interfaceNilTypedFile
+}
+
+var (
+	interfaceNilMu     sync.Mutex
+	interfaceNilByRoot = map[string]*interfaceNilCache{}
+)
+
+func typedFileForInterfaceNilComparison(p *cop.Pass) (interfaceNilTypedFile, bool) {
+	filename, err := filepath.Abs(p.Filename())
+	if err != nil {
+		return interfaceNilTypedFile{}, false
+	}
+	root, ok := moduleRoot(filepath.Dir(filename))
+	if !ok {
+		return interfaceNilTypedFile{}, false
+	}
+
+	cache := loadInterfaceNilCache(root)
+	matches := cache.files[filepath.Clean(filename)]
+	for _, match := range matches {
+		if match.file.Name != nil && match.file.Name.Name == p.PackageName() {
+			return match, true
+		}
+	}
+	if len(matches) > 0 {
+		return matches[0], true
+	}
+	return interfaceNilTypedFile{}, false
+}
+
+func loadInterfaceNilCache(root string) *interfaceNilCache {
+	root = filepath.Clean(root)
+
+	interfaceNilMu.Lock()
+	cached := interfaceNilByRoot[root]
+	interfaceNilMu.Unlock()
+	if cached != nil {
+		return cached
+	}
+
+	modulePath := readModulePath(root)
+	cache := &interfaceNilCache{files: map[string][]interfaceNilTypedFile{}}
+	cfg := &packages.Config{
+		Dir:   root,
+		Tests: true,
+		Mode: packages.NeedName |
+			packages.NeedFiles |
+			packages.NeedCompiledGoFiles |
+			packages.NeedSyntax |
+			packages.NeedTypes |
+			packages.NeedTypesInfo,
+	}
+	pkgs, err := packages.Load(cfg, "./...")
+	if err != nil && len(pkgs) == 0 {
+		return storeInterfaceNilCache(root, cache)
+	}
+	for _, pkg := range pkgs {
+		if pkg.Fset == nil || pkg.TypesInfo == nil || pkg.Types == nil {
+			continue
+		}
+		for _, file := range pkg.Syntax {
+			pos := pkg.Fset.Position(file.Package)
+			filename, absErr := filepath.Abs(pos.Filename)
+			if absErr != nil {
+				continue
+			}
+			filename = filepath.Clean(filename)
+			cache.files[filename] = append(cache.files[filename], interfaceNilTypedFile{
+				file:       file,
+				fset:       pkg.Fset,
+				info:       pkg.TypesInfo,
+				pkg:        pkg.Types,
+				modulePath: modulePath,
+			})
+		}
+	}
+
+	return storeInterfaceNilCache(root, cache)
+}
+
+func storeInterfaceNilCache(root string, cache *interfaceNilCache) *interfaceNilCache {
+	interfaceNilMu.Lock()
+	defer interfaceNilMu.Unlock()
+	if existing := interfaceNilByRoot[root]; existing != nil {
+		return existing
+	}
+	interfaceNilByRoot[root] = cache
+	return cache
+}
+
+func checkInterfaceNilComparisons(
+	p *cop.Pass,
+	file *ast.File,
+	fset *token.FileSet,
+	info *types.Info,
+	pkg *types.Package,
+	modulePath string,
+) {
+	ast.Inspect(file, func(n ast.Node) bool {
+		binary, ok := n.(*ast.BinaryExpr)
+		if !ok || (binary.Op != token.EQL && binary.Op != token.NEQ) {
+			return true
+		}
+
+		expr, ok := nilComparisonOperand(binary)
+		if !ok {
+			return true
+		}
+		typ := info.TypeOf(expr)
+		if !forbiddenInterfaceNilType(typ, pkg, modulePath) {
+			return true
+		}
+
+		start, end, ok := equivalentSpan(p, fset, binary.Pos(), binary.End())
+		if !ok {
+			return true
+		}
+		p.ReportAtf(start, end,
+			"do not compare interface value of type %s to nil; use reflectx.IsNil so typed nil implementations are treated as nil",
+			typ.String())
+		return true
+	})
+}
+
+func nilComparisonOperand(binary *ast.BinaryExpr) (ast.Expr, bool) {
+	if isNilIdent(binary.X) {
+		return binary.Y, true
+	}
+	if isNilIdent(binary.Y) {
+		return binary.X, true
+	}
+	return nil, false
+}
+
+func isNilIdent(expr ast.Expr) bool {
+	id, ok := expr.(*ast.Ident)
+	return ok && id.Name == "nil"
+}
+
+func forbiddenInterfaceNilType(typ types.Type, pkg *types.Package, modulePath string) bool {
+	if typ == nil || isErrorType(typ) {
+		return false
+	}
+	iface, ok := types.Unalias(typ).Underlying().(*types.Interface)
+	if !ok || iface.NumMethods() == 0 {
+		return false
+	}
+	if modulePath == "" {
+		return true
+	}
+	if path := namedTypePackagePath(typ); path != "" {
+		return path == modulePath || strings.HasPrefix(path, modulePath+"/")
+	}
+	return pkg != nil && (pkg.Path() == modulePath || strings.HasPrefix(pkg.Path(), modulePath+"/"))
+}
+
+func isErrorType(typ types.Type) bool {
+	errType := types.Universe.Lookup("error").Type()
+	return types.Identical(typ, errType) || types.Identical(types.Unalias(typ).Underlying(), errType.Underlying())
+}
+
+func namedTypePackagePath(typ types.Type) string {
+	named, ok := types.Unalias(typ).(*types.Named)
+	if !ok || named.Obj() == nil || named.Obj().Pkg() == nil {
+		return ""
+	}
+	return named.Obj().Pkg().Path()
+}
+
+func equivalentSpan(p *cop.Pass, sourceFSet *token.FileSet, pos, end token.Pos) (token.Pos, token.Pos, bool) {
+	if sourceFSet == p.FileSet {
+		return pos, end, true
+	}
+	sourceFile := sourceFSet.File(pos)
+	destFile := p.FileSet.File(p.File.Package)
+	if sourceFile == nil || destFile == nil {
+		return token.NoPos, token.NoPos, false
+	}
+	startOffset := sourceFile.Offset(pos)
+	endOffset := sourceFile.Offset(end)
+	if startOffset < 0 || endOffset < startOffset || endOffset > destFile.Size() {
+		return token.NoPos, token.NoPos, false
+	}
+	return destFile.Pos(startOffset), destFile.Pos(endOffset), true
+}
+
+func moduleRoot(dir string) (string, bool) {
+	for {
+		if _, err := os.Stat(filepath.Join(dir, "go.mod")); err == nil {
+			return dir, true
+		}
+		parent := filepath.Dir(dir)
+		if parent == dir {
+			return "", false
+		}
+		dir = parent
+	}
+}
+
+func readModulePath(root string) string {
+	data, err := os.ReadFile(filepath.Join(root, "go.mod"))
+	if err != nil {
+		return ""
+	}
+	for line := range strings.SplitSeq(string(data), "\n") {
+		fields := strings.Fields(line)
+		if len(fields) == 2 && fields[0] == "module" {
+			return fields[1]
+		}
+	}
+	return ""
+}

lint/interface_nil_comparison_test.go

@@ -0,0 +1,43 @@
+package main
+
+import (
+	"testing"
+
+	"github.com/dgageot/rubocop-go/coptest"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestInterfaceNilComparisonReportsInterfaceNilChecks(t *testing.T) {
+	offenses := coptest.RunTyped(t, InterfaceNilComparison, `package sample
+
+type Worker interface { Work() }
+
+func f(w Worker) {
+	if w == nil {
+	}
+	if nil != w {
+	}
+}
+`)
+
+	require.Len(t, offenses, 2)
+	assert.Equal(t, "Lint/InterfaceNilComparison", offenses[0].CopName)
+	assert.Contains(t, offenses[0].Message, "reflectx.IsNil")
+}
+
+func TestInterfaceNilComparisonIgnoresErrorAnyAndConcreteTypes(t *testing.T) {
+	offenses := coptest.RunTyped(t, InterfaceNilComparison, `package sample
+
+func f(err error, value any, ptr *int) {
+	if err == nil {
+	}
+	if value == nil {
+	}
+	if ptr == nil {
+	}
+}
+`)
+
+	assert.Empty(t, offenses)
+}

lint/main.go

@@ -29,6 +29,7 @@ var cops = []cop.Cop{
 	HookConfigSync,
 	HookBuiltinsRegistered,
 	SlogContextual,
+	InterfaceNilComparison,
 }
 
 func main() {

pkg/agent/agent.go

@@ -13,6 +13,7 @@ import (
 	"github.com/docker/docker-agent/pkg/config/latest"
 	"github.com/docker/docker-agent/pkg/config/types"
 	"github.com/docker/docker-agent/pkg/model/provider"
+	"github.com/docker/docker-agent/pkg/reflectx"
 	"github.com/docker/docker-agent/pkg/tools"
 )
 
@@ -185,7 +186,7 @@ func (a *Agent) SetModelOverride(models ...provider.Provider) ModelOverrideSnaps
 	// Filter out nil providers
 	var validModels []provider.Provider
 	for _, m := range models {
-		if m != nil {
+		if !reflectx.IsNil(m) {
 			validModels = append(validModels, m)
 		}
 	}