testify: migrate assert.Contains(err.Error()) → ErrorContains, fix require-error violations

cmd/gh-aw/main_help_text_test.go

@@ -77,7 +77,7 @@ func TestCompileGhAwRefMutuallyExclusiveFlags(t *testing.T) {
 
 			err := rootCmd.Execute()
 			require.Error(t, err)
-			assert.Contains(t, err.Error(), "if any flags in the group", "expected mutually exclusive flag-group error")
+			require.ErrorContains(t, err, "if any flags in the group", "expected mutually exclusive flag-group error")
 		})
 	}
 }

docs/adr/47252-testify-error-assertion-conventions.md

@@ -0,0 +1,55 @@
+# ADR-47252: Testify Error-Assertion Conventions — ErrorContains and require.*
+
+**Date**: 2026-07-22
+**Status**: Draft
+**Deciders**: pelikhan (PR author), go-fan module review recommendations
+
+---
+
+### Context
+
+The Go test suite used `assert.Contains(t, err.Error(), "…")` as a widespread pattern for checking error message substrings. This pattern has a latent defect: if `err` is `nil`, calling `.Error()` on it panics and crashes the entire test binary rather than recording a clean test failure. Separately, several error assertions used `assert.*` even when preceded by a `require.Error` precondition — meaning a failure in the preceding check would still allow subsequent assertions to execute, leading to confusing secondary failures. `.golangci.yml` already enables `testifylint` with `enable-all: true`, which flags both of these patterns.
+
+### Decision
+
+We will standardise on two rules for error assertions in Go tests:
+
+1. **Use `ErrorContains(t, err, "…")` instead of `assert.Contains(t, err.Error(), "…")`** — `ErrorContains` validates that `err != nil` before inspecting the message, eliminating the nil-deref crash risk.
+2. **Use `require.*` error assertions (not `assert.*`) after a `require.Error` precondition, and for any assertion whose failure would invalidate immediately following code** — `require` stops the test on first failure rather than allowing cascading false positives.
+
+This decision is enforced mechanically across all `*_test.go` files; no production code was changed.
+
+### Alternatives Considered
+
+#### Alternative 1: Guard each call site with an explicit nil check
+
+Add `if err != nil { assert.Contains(t, err.Error(), "…") }` before every affected assertion. This eliminates the panic without changing the assertion library API but is highly verbose (600+ sites), increases maintenance burden, and still does not address the `assert` vs `require` semantics issue.
+
+#### Alternative 2: Keep `assert.Contains(t, err.Error(), "…")` and rely on test author discipline
+
+Accept the current pattern and document that callers must ensure `err != nil` before calling `.Error()`. This has zero migration cost but preserves the crash risk and contradicts the existing `testifylint enable-all: true` policy, which was already configured to flag these patterns.
+
+#### Alternative 3: Wrap error assertions in a custom helper function
+
+Introduce a project-local `assertErrorContains(t, err, substr)` helper that handles the nil check internally. This is technically valid but duplicates functionality already provided by `testify` (`ErrorContains` was added in testify v1.7.1), adds an unnecessary abstraction, and reduces newcomer familiarity with the standard library.
+
+### Consequences
+
+#### Positive
+- Nil-deref crash risk in error assertion paths is eliminated; a nil `err` now produces a clean `FAIL` line rather than a test binary crash.
+- Test failures are reported at the first meaningful assertion (`require` stops execution), removing cascading false positives that obscure root causes.
+- The codebase is now fully compliant with `testifylint enable-all: true`; no suppression flags or `//nolint` directives are needed.
+- The pattern is idiomatic and familiar to any Go developer using testify — no custom DSL to learn.
+
+#### Negative
+- This is a 600+ file, 624-line mechanical change. Even though it is purely additive/substitutive in `*_test.go` files, the large diff creates merge conflicts for any concurrent branches that touch the same test files.
+- Once merged, any contributor who reverts to the old `assert.Contains(t, err.Error(), "…")` pattern will see a CI lint failure — the stricter convention is now enforced by CI, not just recommended.
+
+#### Neutral
+- No change to test behaviour for passing tests; only the failure mode changes.
+- No CI configuration changes required (`testifylint enable-all: true` was already present).
+- Five files had their `assert` import removed as a side effect of the migration; unrelated `assert.*` calls in those files were also migrated as part of the sweep.
+
+---
+
+*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*

pkg/actionpins/spec_test.go

@@ -322,7 +322,7 @@ func TestSpec_PublicAPI_ResolveActionPin_EnforcePinned(t *testing.T) {
 			result, err := actionpins.ResolveActionPin("does-not-exist/x", "v1", ctx)
 			if tt.wantErr {
 				require.Error(t, err, "enforce mode should return an error for this scenario")
-				assert.Contains(t, err.Error(), tt.wantErrContains)
+				require.ErrorContains(t, err, tt.wantErrContains)
 				assert.Empty(t, result, "erroring enforce mode should not return a pinned reference")
 			} else {
 				require.NoError(t, err, "non-error scenario should not return an error")
@@ -895,7 +895,7 @@ func TestSpec_PublicAPI_ResolveActionPin_MappingTargetUnknown(t *testing.T) {
 
 	require.NotPanics(t, func() {
 		result, err := actionpins.ResolveActionPin("actions/checkout", "v4", ctx)
-		require.NoError(t, err)
+		assert.NoError(t, err)
 		assert.Empty(t, result, "mapping to unknown repo should produce unresolved empty result")
 	})
 }

pkg/agentdrain/anomaly_test.go

@@ -299,7 +299,7 @@ func TestNewAnomalyDetector_ThresholdBoundaries(t *testing.T) {
 			detector, err := NewAnomalyDetector(tt.simThreshold, tt.rareThreshold)
 			if tt.wantErr != "" {
 				require.Error(t, err, "NewAnomalyDetector should reject invalid thresholds")
-				assert.Contains(t, err.Error(), tt.wantErr, "error should describe invalid threshold")
+				require.ErrorContains(t, err, tt.wantErr, "error should describe invalid threshold")
 				require.Nil(t, detector, "NewAnomalyDetector should return nil detector on validation error")
 				return
 			}
@@ -529,7 +529,7 @@ func TestAnalyzeEvent_Variants(t *testing.T) {
 			result, report, err := m.AnalyzeEvent(tt.evt)
 			if tt.wantErr {
 				require.Error(t, err, "AnalyzeEvent should return an error")
-				assert.Contains(t, err.Error(), tt.wantErrMsg, "error message mismatch")
+				require.ErrorContains(t, err, tt.wantErrMsg, "error message mismatch")
 				assert.Nil(t, result, "AnalyzeEvent should return nil result on error")
 				assert.Nil(t, report, "AnalyzeEvent should return nil report on error")
 				return

pkg/agentdrain/miner_test.go

@@ -264,7 +264,7 @@ func TestTrainEmptyLine(t *testing.T) {
 	result, err := m.Train(" \t\n ")
 	assert.Nil(t, result, "Train should return nil result for whitespace-only input")
 	require.Error(t, err, "Train should return an error for whitespace-only input")
-	assert.Contains(t, err.Error(), "empty line after masking", "Train error should explain empty line after masking")
+	require.ErrorContains(t, err, "empty line after masking", "Train error should explain empty line after masking")
 }
 
 func TestNewMaskerInvalidPattern(t *testing.T) {
@@ -278,7 +278,7 @@ func TestNewMaskerInvalidPattern(t *testing.T) {
 
 	assert.Nil(t, masker, "NewMasker should return nil masker for invalid regex pattern")
 	require.Error(t, err, "NewMasker should fail when a regex pattern is invalid")
-	assert.Contains(t, err.Error(), `mask rule "invalid"`, "NewMasker error should identify the failing rule")
+	require.ErrorContains(t, err, `mask rule "invalid"`, "NewMasker error should identify the failing rule")
 }
 
 func TestConcurrency(t *testing.T) {

pkg/cli/actions_build_command_test.go

@@ -24,7 +24,7 @@ func TestActionsBuildCommand_NoActionsDir(t *testing.T) {
 	// Test with non-existent actions directory
 	err = ActionsBuildCommand()
 	require.Error(t, err, "Should error when actions/ directory does not exist")
-	assert.Contains(t, err.Error(), "actions/ directory does not exist", "Error should mention missing directory")
+	require.ErrorContains(t, err, "actions/ directory does not exist", "Error should mention missing directory")
 }
 
 func TestActionsValidateCommand_NoActionsDir(t *testing.T) {
@@ -233,7 +233,7 @@ runs:
 			if tt.expectError {
 				require.Error(t, err, "Expected an error")
 				if tt.errorContains != "" {
-					assert.Contains(t, err.Error(), tt.errorContains, "Error should contain expected message")
+					require.ErrorContains(t, err, tt.errorContains, "Error should contain expected message")
 				}
 			} else {
 				require.NoError(t, err, "Should not error for valid action.yml")

pkg/cli/add_command_test.go

@@ -130,7 +130,7 @@ func TestAddWorkflows(t *testing.T) {
 			if tt.expectError {
 				require.Error(t, err, "Expected error for test case: %s", tt.name)
 				if tt.errorContains != "" {
-					assert.Contains(t, err.Error(), tt.errorContains, "Error should contain expected message")
+					require.ErrorContains(t, err, tt.errorContains, "Error should contain expected message")
 				}
 			} else {
 				assert.NoError(t, err, "Should not error for test case: %s", tt.name)
@@ -381,14 +381,14 @@ func TestRejectBootstrapProfileForRegularAdd(t *testing.T) {
 	t.Run("rejects regular add for packages with manifest config", func(t *testing.T) {
 		err := rejectBootstrapProfileForRegularAdd([]string{"githubnext/central-agentic-ops"}, profileWithConfig)
 		require.Error(t, err)
-		assert.Contains(t, err.Error(), "package githubnext/central-agentic-ops declares aw.yml config")
-		assert.Contains(t, err.Error(), "gh aw add-wizard githubnext/central-agentic-ops")
+		require.ErrorContains(t, err, "package githubnext/central-agentic-ops declares aw.yml config")
+		require.ErrorContains(t, err, "gh aw add-wizard githubnext/central-agentic-ops")
 	})
 
 	t.Run("uses requested sources in the add-wizard guidance", func(t *testing.T) {
 		err := rejectBootstrapProfileForRegularAdd([]string{"githubnext/central-agentic-ops", "./local-workflow.md"}, profileWithConfig)
 		require.Error(t, err)
-		assert.Contains(t, err.Error(), "gh aw add-wizard githubnext/central-agentic-ops ./local-workflow.md")
+		require.ErrorContains(t, err, "gh aw add-wizard githubnext/central-agentic-ops ./local-workflow.md")
 	})
 
 	t.Run("allows packages without manifest config", func(t *testing.T) {
@@ -558,7 +558,7 @@ source: octo/other/.github/workflows/dependabot.md@main
 	skip, err := validateWorkflowDestination(workflowsDir, "dependabot", "githubnext/central-agentic-ops", AddOptions{})
 	require.Error(t, err)
 	assert.False(t, skip)
-	assert.Contains(t, err.Error(), "workflow 'dependabot' already exists")
+	require.ErrorContains(t, err, "workflow 'dependabot' already exists")
 }
 
 // TestAddMultipleWorkflowsNameFlag verifies that --name is not allowed when multiple workflows are specified.
@@ -570,7 +570,7 @@ func TestAddMultipleWorkflowsNameFlag(t *testing.T) {
 
 	err := cmd.Execute()
 	require.Error(t, err, "Should error when --name is used with multiple workflows")
-	assert.Contains(t, err.Error(), "--name flag cannot be used when adding multiple workflows", "Error should mention --name restriction")
+	require.ErrorContains(t, err, "--name flag cannot be used when adding multiple workflows", "Error should mention --name restriction")
 }
 
 // setupMinimalGitRepo initialises a bare-minimum git repo in dir and returns the
@@ -817,7 +817,7 @@ func TestAddWorkflowWithTracking_ActionWorkflow_Force(t *testing.T) {
 	// Without --force: should fail
 	err := addWorkflowWithTracking(context.Background(), resolved, nil, AddOptions{})
 	require.Error(t, err)
-	assert.Contains(t, err.Error(), "already exists")
+	require.ErrorContains(t, err, "already exists")
 
 	// With --force: should overwrite
 	err = addWorkflowWithTracking(context.Background(), resolved, nil, AddOptions{Force: true})
@@ -863,7 +863,7 @@ func TestAddWorkflowsWithTracking_RollsBackWrittenFilesOnWriteFailure(t *testing
 		Quiet:                  true,
 	})
 	require.Error(t, err)
-	assert.Contains(t, err.Error(), "failed to write destination file")
+	require.ErrorContains(t, err, "failed to write destination file")
 
 	_, statErr := os.Stat(filepath.Join(workflowsDir, "ok.md"))
 	assert.True(t, os.IsNotExist(statErr), "successful writes from this operation should be rolled back on later write failure")
@@ -911,7 +911,7 @@ func TestAddSkillFileWithTracking_RejectsInvalidPaths(t *testing.T) {
 
 		err := addSkillFileWithTracking(resolved, nil, AddOptions{Quiet: true}, gitRoot)
 		require.Error(t, err)
-		assert.Contains(t, err.Error(), "escapes destination skill directory")
+		require.ErrorContains(t, err, "escapes destination skill directory")
 	})
 
 	t.Run("rejects source path when skill root cannot be determined", func(t *testing.T) {
@@ -926,7 +926,7 @@ func TestAddSkillFileWithTracking_RejectsInvalidPaths(t *testing.T) {
 
 		err := addSkillFileWithTracking(resolved, nil, AddOptions{Quiet: true}, gitRoot)
 		require.Error(t, err)
-		assert.Contains(t, err.Error(), "failed to determine relative path")
+		require.ErrorContains(t, err, "failed to determine relative path")
 	})
 }
 
@@ -959,7 +959,7 @@ func TestAddCopilotRequestsPermissionToContent(t *testing.T) {
 		content := "---\nengine: copilot\npermissions: read-all\n---\nDo the thing.\n"
 		_, err := addCopilotRequestsPermissionToContent(content)
 		require.Error(t, err)
-		assert.Contains(t, err.Error(), "non-mapping scalar")
+		require.ErrorContains(t, err, "non-mapping scalar")
 	})
 }
 

pkg/cli/add_package_manifest_test.go

@@ -328,7 +328,7 @@ files:
 
 		_, err := resolveRepositoryPackage(t.Context(), &RepoSpec{RepoSlug: "owner/repo"}, "")
 		require.Error(t, err)
-		assert.Contains(t, err.Error(), `name must be a non-empty string`)
+		require.ErrorContains(t, err, `name must be a non-empty string`)
 	})
 
 	t.Run("requires aw manifest when only legacy alias exists", func(t *testing.T) {
@@ -348,7 +348,7 @@ files:
 		_, err := resolveRepositoryPackage(t.Context(), &RepoSpec{RepoSlug: "owner/repo"}, "")
 		require.Error(t, err)
 		assert.Equal(t, []string{"aw.yml"}, requestedPaths)
-		assert.Contains(t, err.Error(), `no aw.yml manifest found`)
+		require.ErrorContains(t, err, `no aw.yml manifest found`)
 	})
 
 	t.Run("accepts manifest-version and compatible min-version", func(t *testing.T) {
@@ -444,7 +444,7 @@ name: Repo Assist
 
 		_, err := resolveRepositoryPackage(t.Context(), &RepoSpec{RepoSlug: "owner/repo"}, "")
 		require.Error(t, err)
-		assert.Contains(t, err.Error(), `manifest-version`)
+		require.ErrorContains(t, err, `manifest-version`)
 	})
 
 	t.Run("accepts branding field", func(t *testing.T) {
@@ -539,7 +539,7 @@ bootstrap:
 
 		_, err := resolveRepositoryPackage(t.Context(), &RepoSpec{RepoSlug: "owner/repo"}, "")
 		require.Error(t, err, "old bootstrap key must produce an error, not be silently ignored")
-		assert.Contains(t, err.Error(), "bootstrap")
+		require.ErrorContains(t, err, "bootstrap")
 	})
 
 	t.Run("rejects unsupported branding icon", func(t *testing.T) {
@@ -556,7 +556,7 @@ branding:
 
 		_, err := resolveRepositoryPackage(t.Context(), &RepoSpec{RepoSlug: "owner/repo"}, "")
 		require.Error(t, err)
-		assert.Contains(t, err.Error(), `icon`)
+		require.ErrorContains(t, err, `icon`)
 	})
 
 	t.Run("rejects docs field", func(t *testing.T) {
@@ -571,7 +571,7 @@ docs: docs/overview.md
 
 		_, err := resolveRepositoryPackage(t.Context(), &RepoSpec{RepoSlug: "owner/repo"}, "")
 		require.Error(t, err)
-		assert.Contains(t, err.Error(), `docs`)
+		require.ErrorContains(t, err, `docs`)
 	})
 
 	t.Run("rejects non-string emoji field", func(t *testing.T) {
@@ -587,7 +587,7 @@ emoji:
 
 		_, err := resolveRepositoryPackage(t.Context(), &RepoSpec{RepoSlug: "owner/repo"}, "")
 		require.Error(t, err)
-		assert.Contains(t, err.Error(), `emoji`)
+		require.ErrorContains(t, err, `emoji`)
 	})
 
 	t.Run("rejects non-string license field", func(t *testing.T) {
@@ -603,7 +603,7 @@ license:
 
 		_, err := resolveRepositoryPackage(t.Context(), &RepoSpec{RepoSlug: "owner/repo"}, "")
 		require.Error(t, err)
-		assert.Contains(t, err.Error(), `license`)
+		require.ErrorContains(t, err, `license`)
 	})
 
 	t.Run("rejects incompatible min-version", func(t *testing.T) {
@@ -618,7 +618,7 @@ name: Repo Assist
 
 		_, err := resolveRepositoryPackage(t.Context(), &RepoSpec{RepoSlug: "owner/repo"}, "")
 		require.Error(t, err)
-		assert.Contains(t, err.Error(), `requires gh-aw`)
+		require.ErrorContains(t, err, `requires gh-aw`)
 	})
 
 	t.Run("requires package README", func(t *testing.T) {
@@ -638,7 +638,7 @@ files:
 
 		_, err := resolveRepositoryPackage(t.Context(), &RepoSpec{RepoSlug: "owner/repo"}, "")
 		require.Error(t, err)
-		assert.Contains(t, err.Error(), `missing required README.md`)
+		require.ErrorContains(t, err, `missing required README.md`)
 	})
 
 	t.Run("reports nested package path when README is missing", func(t *testing.T) {
@@ -658,8 +658,8 @@ files:
 
 		_, err := resolveRepositoryPackage(t.Context(), &RepoSpec{RepoSlug: "owner/repo", PackagePath: "packages/repo-assist"}, "")
 		require.Error(t, err)
-		assert.Contains(t, err.Error(), `owner/repo/packages/repo-assist`)
-		assert.Contains(t, err.Error(), `packages/repo-assist/README.md`)
+		require.ErrorContains(t, err, `owner/repo/packages/repo-assist`)
+		require.ErrorContains(t, err, `packages/repo-assist/README.md`)
 	})
 
 	t.Run("rejects unknown manifest fields", func(t *testing.T) {
@@ -674,7 +674,7 @@ unknown-field: true
 
 		_, err := resolveRepositoryPackage(t.Context(), &RepoSpec{RepoSlug: "owner/repo"}, "")
 		require.Error(t, err)
-		assert.Contains(t, err.Error(), `unknown-field`)
+		require.ErrorContains(t, err, `unknown-field`)
 	})
 
 	t.Run("resolves nested package manifests", func(t *testing.T) {
@@ -845,7 +845,7 @@ func TestResolveWorkflows_RepositoryPackageRejectsPrivateTrue(t *testing.T) {
 
 	_, err := ResolveWorkflows(context.Background(), []string{"owner/repo"}, false)
 	require.Error(t, err)
-	assert.Contains(t, err.Error(), `workflow "workflows/review.md" sets private: true`)
+	require.ErrorContains(t, err, `workflow "workflows/review.md" sets private: true`)
 }
 
 func TestResolveWorkflows_NestedRepositoryPackage(t *testing.T) {
@@ -1145,7 +1145,7 @@ func TestParseRepositoryPackageSpec(t *testing.T) {
 			assert.Equal(t, tt.wantOK, ok)
 			if tt.wantErr != "" {
 				require.Error(t, err)
-				assert.Contains(t, err.Error(), tt.wantErr)
+				require.ErrorContains(t, err, tt.wantErr)
 				return
 			}
 			require.NoError(t, err)
@@ -1332,7 +1332,7 @@ files:
 
 		_, err := resolveRepositoryPackage(t.Context(), &RepoSpec{RepoSlug: "owner/repo"}, "")
 		require.Error(t, err)
-		assert.Contains(t, err.Error(), "duplicate workflow filename")
+		require.ErrorContains(t, err, "duplicate workflow filename")
 	})
 }
 

pkg/cli/add_wildcard_test.go

@@ -266,7 +266,7 @@ func TestExpandLocalWildcardWorkflows_NoMatches(t *testing.T) {
 	_, err = expandLocalWildcardWorkflows(specs, false)
 	// Should error because no workflows found after expansion
 	require.Error(t, err, "Should error when no workflows match")
-	assert.Contains(t, err.Error(), "no workflows to add after expansion")
+	require.ErrorContains(t, err, "no workflows to add after expansion")
 }
 
 // TestAddWorkflowWithTracking_WildcardDuplicateHandling tests that when adding workflows from wildcard,

pkg/cli/add_workflow_resolution_redirect_test.go

@@ -71,7 +71,7 @@ func TestResolveAddWorkflowSpecAndContent(t *testing.T) {
 			WorkflowName: "a",
 		}, false)
 		require.Error(t, err, "redirect loop should fail")
-		assert.Contains(t, err.Error(), "redirect loop detected", "error should mention loop detection")
+		require.ErrorContains(t, err, "redirect loop detected", "error should mention loop detection")
 	})
 
 	t.Run("local workflows are not redirected", func(t *testing.T) {