Enforce prod errstringmatch findings and centralize 403/410 error classification

[Copilot]

The in-tree errstringmatch analyzer was already identifying brittle strings.Contains(err.Error(), "...") checks in production code, but CI treated the results as informational. This change removes the enforceability gap by migrating the clean cases to shared helpers, explicitly documenting the unavoidable text-matching cases, and making CI block on production errstringmatch violations.

• Add shared error classification for 403/410

  • Extend pkg/errorutil with:
    • IsForbiddenError
    • IsGoneError
  • Reuse the existing case-insensitive matching pattern from IsNotFoundError
  • Add focused table-driven coverage for the new helpers

• Migrate production call sites off brittle string matching

  • Replace inline 403 checks with errorutil.IsForbiddenError in secret-check paths
  • Replace the gateway not found check with errorutil.IsNotFoundError
  • Replace the workflow-log expiry 410 check with errorutil.IsGoneError
  • Partially migrate project creation error handling by using IsNotFoundError for the NOT_FOUND branch while keeping the gh CLI scope check explicit

• Make intentional text matching explicit

  • Add //nolint:errstringmatch to the remaining cases that are still driven by upstream gh CLI or parser text:
    • gh CLI already-merged detection
    • gh CLI Windows lock-file/update-state errors
    • cron parser unsupported-syntax rejection
    • centralized auth/permission string classification in audit.go

• Align the analyzer with intended usage

  • Teach pkg/linters/errstringmatch to honor //nolint:errstringmatch
  • Skip _test.go files so enforcement stays focused on production code
  • Add analyzer testdata covering both same-line and preceding-line nolint directives

• Promote CI enforcement

  • Remove the non-blocking continue-on-error behavior from the custom-linter workflow step
  • Wire the workflow to enforce the production errstringmatch analyzer directly via make golint-custom LINTER_FLAGS="-errstringmatch -test=false"

Example of the migrated pattern:

if errorutil.IsNotFoundError(err) || errorutil.IsGoneError(err) {
	return nil
}

[Copilot]

Pull request overview

Enforces the in-tree errstringmatch analyzer in CI on production code and centralizes HTTP 403/410 error classification in pkg/errorutil so call sites can drop brittle strings.Contains(err.Error(), ...) checks.

Changes:

• Adds IsForbiddenError / IsGoneError to pkg/errorutil (refactored around a shared containsErrorSubstring helper) with table-driven tests, and migrates production callers (secret checks, gateway parsing, workflow-log download, project creation) off inline string matching.
• Teaches the errstringmatch analyzer to honor //nolint:errstringmatch (same-line or preceding line) and to skip _test.go files, plus adds testdata covering both directive placements.
• Promotes CI enforcement by removing continue-on-error and running make golint-custom LINTER_FLAGS="-errstringmatch -test=false"; annotates the remaining intentional text-matching sites (gh CLI merge/Windows-lock, cron parser, audit.go permission classifier) with explicit nolint comments.

Show a summary per file

File	Description
pkg/errorutil/errors.go	Adds IsForbiddenError/IsGoneError and factors matching into containsErrorSubstring
pkg/errorutil/errors_test.go	Table-driven tests for the new 403/410 helpers
pkg/errorutil/README.md	Documents the two new helpers and updated design note
pkg/cli/audit.go	Removes is403Error; adds nolint on the canonical permission-error string classifier
pkg/cli/audit_test.go	Removes the now-deleted is403Error test
pkg/cli/secrets.go	Uses errorutil.IsForbiddenError in place of is403Error
pkg/cli/mcp_secrets.go	Uses errorutil.IsForbiddenError in place of is403Error
pkg/cli/logs_metrics.go	Replaces inline "not found" check with errorutil.IsNotFoundError
pkg/cli/logs_download.go	Replaces inline "410" check with errorutil.IsGoneError
pkg/cli/project_command.go	Uses IsNotFoundError; keeps explicit INSUFFICIENT_SCOPES check behind a nolint
pkg/cli/add_interactive_git.go	Extracts isAlreadyMergedGHError with a justifying nolint
pkg/cli/update_extension_check.go	Adds nolint for the gh extension Windows-lock text match
pkg/workflow/schedule_preprocessing.go	Adds nolint for gronx unsupported-syntax string match
pkg/linters/errstringmatch/errstringmatch.go	Honors nolint:errstringmatch/nolint:all; skips _test.go
pkg/linters/errstringmatch/testdata/.../errstringmatch.go	Adds coverage for same-line and preceding-line nolint directives
Makefile	Adds LINTER_FLAGS passthrough for golint-custom
.github/workflows/cgo.yml	Removes continue-on-error and runs only the errstringmatch analyzer on prod code
.github/workflows/smoke-otel-backends.lock.yml	Changes the Datadog DD_APPLICATION_KEY header to read from ${DD_APP_KEY} (appears unrelated)

Copilot's findings

Tip

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

Comments suppressed due to low confidence (1)

pkg/linters/errstringmatch/errstringmatch.go:166

• The "preceding line" check accepts any comment on position.Line-1, including a trailing inline comment on the previous statement (e.g. x := f() //nolint:errstringmatch) that has nothing to do with this call. It will silently suppress a legitimate finding. Standard Go nolint conventions require the directive to either be on the same line as the offending expression or appear as a standalone comment immediately preceding it. Consider verifying that a "preceding line" comment is the only token on that line (e.g. comment column == 1 or comment line differs from the previous statement's end line) before honoring it.

				commentPos := pass.Fset.PositionFor(comment.Slash, false)
				if commentPos.Line != position.Line && commentPos.Line != position.Line-1 {
					continue
				}

				text := strings.TrimPrefix(comment.Text, "//")
				if strings.HasPrefix(text, "nolint:errstringmatch") || strings.HasPrefix(text, "nolint:all") {
					return true
				}

• Files reviewed: 18/18 changed files
• Comments generated: 3

[pelikhan]

@copilot review all comments

[Copilot]

@copilot review all comments

Addressed the actionable review items in 49885d1: tightened IsForbiddenError/IsGoneError to HTTP-shaped status patterns, updated the coupled synthetic 403 secret-check error text, and cached nolint lines per file in the analyzer instead of rescanning all package comments per finding. The lockfile concern is not present in the current branch state (DD_APPLICATION_KEY is restored).