From dbb0fe3a0b59a489ce2c8187ffcfc74a350dea32 Mon Sep 17 00:00:00 2001
From: Ethan <ethan@viam.com>
Date: Tue, 25 Feb 2025 15:21:57 -0500
Subject: [PATCH] RSDK-9755 - CLI lint enforce createCommandWithT usage (#4803)

---
 .github/CODEOWNERS                  |   1 +
 .github/workflows/test.yml          |   6 ++
 cli/internal/cli_lint/.gitignore    |   1 +
 cli/internal/cli_lint/cli_linter.go | 101 ++++++++++++++++++++++++++++
 4 files changed, 109 insertions(+)
 create mode 100644 cli/internal/cli_lint/.gitignore
 create mode 100644 cli/internal/cli_lint/cli_linter.go

diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS
index e3f01c162fc..5de33797719 100644
--- a/.github/CODEOWNERS
+++ b/.github/CODEOWNERS
@@ -3,4 +3,5 @@
 /cli/app.go @viamrobotics/sdk-netcode
 /cli/CONTRIBUTING.md @viamrobotics/sdk-netcode
 /cli/STYLEGUIDE.md @viamrobotics/sdk-netcode
+/cli/internal/ @viamrobotics/sdk-netcode
 
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
index 1ff39f1d512..ebcf58593fe 100644
--- a/.github/workflows/test.yml
+++ b/.github/workflows/test.yml
@@ -58,6 +58,12 @@ jobs:
       - name: Chown
         run: chown -R testbot:testbot .
 
+      - name: Run CLI linter tests
+        run: |
+          cd cli/internal/cli_lint
+          go build .
+          ./cli_lint ../../
+
       - name: Verify no uncommitted changes from "make build-go lint-go generate-go"
         run: |
           sudo -Hu testbot bash -lc 'git init && git add . && make build-go lint-go generate-go'
diff --git a/cli/internal/cli_lint/.gitignore b/cli/internal/cli_lint/.gitignore
new file mode 100644
index 00000000000..5671fc2d764
--- /dev/null
+++ b/cli/internal/cli_lint/.gitignore
@@ -0,0 +1 @@
+cli_lint
diff --git a/cli/internal/cli_lint/cli_linter.go b/cli/internal/cli_lint/cli_linter.go
new file mode 100644
index 00000000000..224eaa9a3a1
--- /dev/null
+++ b/cli/internal/cli_lint/cli_linter.go
@@ -0,0 +1,101 @@
+// Package main is the CLI-specific linter itself.
+package main
+
+import (
+	"go/ast"
+	"go/types"
+	"strings"
+
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/singlechecker"
+)
+
+var enforceCreateCommandWithT = &analysis.Analyzer{
+	Name: "createcommandwitht",
+	Doc:  "Use CreateCommandWithT",
+	Run:  enforceCreateCommandWithTRun,
+}
+
+func enforceCreateCommandWithTRun(pass *analysis.Pass) (interface{}, error) {
+	var commandType types.Type
+
+	for _, pkg := range pass.Pkg.Imports() {
+		// we want to differentiate the upstream `cli` package and our own `cli` package
+		if pkg.Name() == "cli" && !strings.Contains(pkg.Path(), "viam") {
+			commandType = pkg.Scope().Lookup("Command").Type()
+			commandType = types.NewPointer(commandType) // actual `Commands` in the app are pointers
+		}
+	}
+	if commandType == nil { // no CLI imports so we can skip
+		return nil, nil
+	}
+
+	for _, file := range pass.Files {
+		ast.Inspect(file, func(node ast.Node) bool {
+			composite, isComposite := node.(*ast.CompositeLit)
+
+			// we aren't looking at a struct, so no need to evaluate further
+			if !isComposite {
+				return true
+			}
+			compositeType := pass.TypesInfo.TypeOf(composite)
+
+			// we aren't looking at a CLI command, so no need to evaluate further
+			if compositeType.String() != commandType.String() {
+				return true
+			}
+
+			for _, elt := range composite.Elts {
+				keyValue, isKeyValue := elt.(*ast.KeyValueExpr)
+
+				// we aren't looking at assignment of a struct param, so no need to evaluate further
+				if !isKeyValue {
+					return true
+				}
+				key := keyValue.Key.(*ast.Ident)
+
+				// "Action", "Before", and "After" are the three types of CLI actions for which
+				// `createCommandWithT` was designed
+				if key.Name == "Action" || key.Name == "Before" ||
+					key.Name == "After" {
+					callExpr, isCallExpr := keyValue.Value.(*ast.CallExpr)
+
+					// `Action` was assigned a literal value, rather than a function call.
+					if !isCallExpr {
+						pass.Report(analysis.Diagnostic{
+							Pos:     keyValue.Pos(),
+							End:     keyValue.End(),
+							Message: "must use createCommandWithT when constructing a CLI action",
+						})
+						return true
+					}
+
+					callExprFunc, isCallExprFunc := callExpr.Fun.(*ast.IndexExpr)
+					if !isCallExprFunc { // this should never happen, but just for explicitness
+						return true
+					}
+					funcIdent, isFuncIdent := callExprFunc.X.(*ast.Ident)
+					if !isFuncIdent { // as above, this should never happen
+						return true
+					}
+
+					if funcIdent.Name != "createCommandWithT" {
+						// some other func was used to generate the action
+						pass.Report(analysis.Diagnostic{
+							Pos:     funcIdent.Pos(),
+							End:     funcIdent.End(),
+							Message: "must use createCommandWithT when constructing a CLI action",
+						})
+					}
+				}
+			}
+			return true
+		})
+	}
+
+	return nil, nil
+}
+
+func main() {
+	singlechecker.Main(enforceCreateCommandWithT)
+}