feat(errs): export Git command classification policy

platform/errs/README.md

@@ -145,7 +145,7 @@ Classifiers are not installed globally. A host that wants YARPC statuses classif
 
 The YARPC classifier reads the typed status code rather than matching its rendered message. Cancellation is retryable caller-side infrastructure; transient or ambiguous server codes (`Unknown`, `DeadlineExceeded`, `ResourceExhausted`, `Aborted`, `Internal`, and `Unavailable`) are retryable dependency failures; request verdicts and permanent server failures are non-retryable dependency failures. A deadline may expire after a mutating RPC succeeded, so this classification relies on the repository-wide requirement that queue-driven operations are idempotent.
 
-The Git classifier reads `gitexec.CommandError`, which preserves the Git subcommand and the underlying `os/exec` error through contextual wrapping. Git has no typed status to read — a connection reset and a deleted branch both leave `fetch` at a non-zero exit — so the classifier pairs the subcommand with the diagnostic git printed: a transport fragment counts only against a command that talks to the remote, and a lock fragment counts against any command that writes to the checkout. Only a recognised pair is retryable; every other Git failure, including a diagnostic the package has never seen, is a permanent infrastructure failure attributed to the remote or to this service. The direction is deliberate — an unlisted transient failure costs one lost retry, while a permanent failure defaulting to retryable would replay a deterministic error through the whole retry budget before dead-lettering anyway — and it is what makes the fragment lists safe to extend as Git's wording drifts between versions. Cancellation is not the Git classifier's to report: `os/exec` kills a context-cancelled child and reports only `signal: killed`, so `gitexec.CommandFailure` puts `context.Canceled` back in the chain and the generic classifier recognises it there.
+The Git classifier reads the `giterrs.CommandFailure` contract: the Git subcommand and the diagnostic Git printed. The local execution package's `gitexec.CommandError` satisfies that contract, and remote execution backends can satisfy it with their own native error type instead of reconstructing a local process error. Backends that need the policy without the classifier adapter can call `giterrs.ClassifyCommand` directly. Git has no typed status to read — a connection reset and a deleted branch both leave `fetch` at a non-zero exit — so the policy pairs the subcommand with the diagnostic: a transport fragment counts only against a command that talks to the remote, and a lock fragment counts against any command that writes to the checkout. Only a recognised pair is retryable; every other Git failure, including a diagnostic the package has never seen, is a permanent infrastructure failure attributed to the remote or to this service. The direction is deliberate — an unlisted transient failure costs one lost retry, while a permanent failure defaulting to retryable would replay a deterministic error through the whole retry budget before dead-lettering anyway — and it is what makes the fragment lists safe to extend as Git's wording drifts between versions. Cancellation is not the Git classifier's to report: `os/exec` kills a context-cancelled child and reports only `signal: killed`, so `gitexec.CommandFailure` puts `context.Canceled` back in the chain and the generic classifier recognises it there.
 
 Tests follow the same shape: assert per-node behaviour against `Classifier.Classify(node)` directly, and assert end-to-end behaviour by running `errs.NewClassifierProcessor(Classifier).Process(err)` and checking the helpers (`IsRetryable`, `IsUserError`, …) on the result. See `platform/errs/mysql/mysql_test.go`, `platform/errs/git/git_test.go`, `platform/errs/yarpc/yarpc_test.go`, and `platform/errs/generic/generic_test.go`.
 

platform/errs/git/BUILD.bazel

@@ -5,10 +5,7 @@ go_library(
     srcs = ["git.go"],
     importpath = "github.com/uber/submitqueue/platform/errs/git",
     visibility = ["//visibility:public"],
-    deps = [
-        "//platform/errs:go_default_library",
-        "//platform/git/exec:go_default_library",
-    ],
+    deps = ["//platform/errs:go_default_library"],
 )
 
 go_test(

platform/errs/git/git.go

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-// Package git provides an errs.Classifier for failures from Git processes.
+// Package git classifies failures reported by Git commands.
 //
 // Git has no typed status to read: it reports almost everything as a non-zero
 // exit and a line of prose, so a connection reset and a deleted branch both
@@ -47,7 +47,6 @@ import (
 	"strings"
 
 	"github.com/uber/submitqueue/platform/errs"
-	gitexec "github.com/uber/submitqueue/platform/git/exec"
 )
 
 // Classifier recognises Git process failures, reporting a known transient
@@ -63,6 +62,25 @@ var Classifier errs.Classifier = classifier{}
 
 type classifier struct{}
 
+// CommandFailure is the Git failure information needed by Classifier.
+//
+// Git does not expose a structured failure protocol: unrelated failures
+// collapse to generic non-zero exit statuses, leaving the command and its text
+// output as the only useful classification signals.
+//
+// Execution backends may satisfy this contract with their native error type;
+// they do not need to reconstruct an error from another Git implementation.
+type CommandFailure interface {
+	error
+	// Operation returns the Git subcommand that failed, such as fetch, commit,
+	// or push. It excludes the Git executable and later arguments.
+	Operation() string
+	// Diagnostic returns the complete failure output reported by Git. It must
+	// retain all stderr or stdout lines because a classifying fragment may
+	// appear after introductory advice or hints.
+	Diagnostic() string
+}
+
 // remoteOperations are the Git subcommands that exchange data with the
 // configured remote. They attribute their failures to that remote, and they
 // are the only operations a transport fragment can legitimately describe.
@@ -106,21 +124,13 @@ var transientCheckoutFragments = []string{
 	"resource temporarily unavailable",
 }
 
-// Classify inspects a single node. Per the errs.Classifier contract, this must
-// not call errors.Is / errors.As — the classifier-processor owns the chain
-// walk.
-func (classifier) Classify(err error) errs.Verdict {
-	commandErr, ok := err.(*gitexec.CommandError)
-	if !ok {
-		// The only Unknown this classifier returns, and it means "not my
-		// node" rather than "no opinion on this failure". Returning a verdict
-		// here would claim every error the walk passes — a MySQL driver error
-		// among them — before its own classifier were asked.
-		return errs.Unknown
-	}
-
-	diagnostic := strings.ToLower(commandErr.Diagnostic())
-	remote := remoteOperations[commandErr.Operation()]
+// ClassifyCommand classifies a Git subcommand and its rendered diagnostic.
+//
+// It is the shared policy for local processes, remote Git execution services,
+// and any other backend that can report those two values.
+func ClassifyCommand(operation, diagnostic string) errs.Verdict {
+	diagnostic = strings.ToLower(diagnostic)
+	remote := remoteOperations[operation]
 
 	transient := containsAny(diagnostic, transientCheckoutFragments) ||
 		(remote && containsAny(diagnostic, transientTransportFragments))
@@ -137,6 +147,17 @@ func (classifier) Classify(err error) errs.Verdict {
 	}
 }
 
+// Classify inspects a single node. Per the errs.Classifier contract, this must
+// not call errors.Is / errors.As — the classifier-processor owns the chain
+// walk.
+func (classifier) Classify(err error) errs.Verdict {
+	commandFailure, ok := err.(CommandFailure)
+	if !ok {
+		return errs.Unknown
+	}
+	return ClassifyCommand(commandFailure.Operation(), commandFailure.Diagnostic())
+}
+
 func containsAny(diagnostic string, fragments []string) bool {
 	for _, fragment := range fragments {
 		if strings.Contains(diagnostic, fragment) {

platform/errs/git/git_test.go

@@ -32,79 +32,137 @@ func gitError(operation, diagnostic string) error {
 	return gitexec.NewCommandError(operation, diagnostic, errors.New("exit status 128"))
 }
 
-func TestClassifier(t *testing.T) {
+type remoteCommandFailure struct {
+	operation  string
+	diagnostic string
+}
+
+func (e remoteCommandFailure) Error() string {
+	return e.diagnostic
+}
+
+func (e remoteCommandFailure) Operation() string {
+	return e.operation
+}
+
+func (e remoteCommandFailure) Diagnostic() string {
+	return e.diagnostic
+}
+
+func TestClassifyCommand(t *testing.T) {
 	tests := []struct {
-		name string
-		err  error
-		want errs.Verdict
+		name       string
+		operation  string
+		diagnostic string
+		want       errs.Verdict
 	}{
 		{
-			name: "transport fault on fetch is a retryable dependency failure",
-			err:  gitError("fetch", "fatal: unable to access 'https://host/r.git/': Connection reset by peer"),
-			want: errs.InfraDependencyRetryable,
+			name:       "transport fault on fetch is a retryable dependency failure",
+			operation:  "fetch",
+			diagnostic: "fatal: unable to access 'https://host/r.git/': Connection reset by peer",
+			want:       errs.InfraDependencyRetryable,
 		},
 		{
-			name: "unresolvable host on ls-remote is a retryable dependency failure",
-			err:  gitError("ls-remote", "fatal: Could not resolve host: github.example.com"),
-			want: errs.InfraDependencyRetryable,
+			name:       "unresolvable host on ls-remote is a retryable dependency failure",
+			operation:  "ls-remote",
+			diagnostic: "fatal: Could not resolve host: github.example.com",
+			want:       errs.InfraDependencyRetryable,
 		},
 		{
-			name: "checkout contention on a local commit is a retryable local failure",
-			err:  gitError("commit", "fatal: Unable to create '/checkout/.git/index.lock': File exists."),
-			want: errs.InfraRetryable,
+			name:       "checkout contention on a local commit is a retryable local failure",
+			operation:  "commit",
+			diagnostic: "fatal: Unable to create '/checkout/.git/index.lock': File exists.",
+			want:       errs.InfraRetryable,
 		},
 		{
-			name: "checkout contention during fetch is attributed to the remote it ran against",
-			err:  gitError("fetch", "error: cannot lock ref 'refs/remotes/origin/main'"),
-			want: errs.InfraDependencyRetryable,
+			name:       "checkout contention during fetch is attributed to the remote it ran against",
+			operation:  "fetch",
+			diagnostic: "error: cannot lock ref 'refs/remotes/origin/main'",
+			want:       errs.InfraDependencyRetryable,
 		},
 		{
-			name: "transport fragment on a local operation is not evidence of a transient failure",
-			err:  gitError("merge", "error: could not resolve host mentioned in a commit message"),
-			want: errs.Infra,
+			name:       "transport fragment on a local operation is not evidence of a transient failure",
+			operation:  "merge",
+			diagnostic: "error: could not resolve host mentioned in a commit message",
+			want:       errs.Infra,
 		},
 		{
-			name: "unknown revision is a permanent local failure",
-			err:  gitError("rev-parse", "fatal: ambiguous argument 'origin/main': unknown revision or path not in the working tree."),
-			want: errs.Infra,
+			name:       "unknown revision is a permanent local failure",
+			operation:  "rev-parse",
+			diagnostic: "fatal: ambiguous argument 'origin/main': unknown revision or path not in the working tree.",
+			want:       errs.Infra,
 		},
 		{
-			name: "empty squash commit is a permanent local failure",
-			err:  gitError("commit", "exit status 1"),
-			want: errs.Infra,
+			name:       "empty squash commit is a permanent local failure",
+			operation:  "commit",
+			diagnostic: "exit status 1",
+			want:       errs.Infra,
 		},
 		{
-			name: "failure with no diagnostic at all is a permanent local failure",
-			err:  gitError("cat-file", ""),
-			want: errs.Infra,
+			name:      "failure with no diagnostic at all is a permanent local failure",
+			operation: "cat-file",
+			want:      errs.Infra,
 		},
 		{
-			name: "path outside the repository is a permanent local failure",
-			err:  gitError("clean", "fatal: '/etc': '/etc' is outside repository at '/checkout'"),
-			want: errs.Infra,
+			name:       "path outside the repository is a permanent local failure",
+			operation:  "clean",
+			diagnostic: "fatal: '/etc': '/etc' is outside repository at '/checkout'",
+			want:       errs.Infra,
 		},
 		{
-			name: "non-fast-forward push is a permanent dependency failure",
-			err:  gitError("push", "! [rejected]        main -> main (fetch first)"),
-			want: errs.InfraDependency,
+			name:       "non-fast-forward push is a permanent dependency failure",
+			operation:  "push",
+			diagnostic: "! [rejected]        main -> main (fetch first)",
+			want:       errs.InfraDependency,
 		},
 		{
-			name: "authentication failure is a permanent dependency failure",
-			err:  gitError("fetch", "fatal: Authentication failed for 'https://host/r.git/'"),
-			want: errs.InfraDependency,
+			name:       "authentication failure is a permanent dependency failure",
+			operation:  "fetch",
+			diagnostic: "fatal: Authentication failed for 'https://host/r.git/'",
+			want:       errs.InfraDependency,
 		},
 		{
 			// Git prints this trailer under permanent failures too — a missing
 			// remote, absent access rights — so it is not evidence of a
 			// transient one.
-			name: "generic remote trailer is a permanent dependency failure",
-			err:  gitError("fetch", "fatal: 'origin' does not appear to be a git repository\nfatal: Could not read from remote repository."),
-			want: errs.InfraDependency,
+			name:       "generic remote trailer is a permanent dependency failure",
+			operation:  "fetch",
+			diagnostic: "fatal: 'origin' does not appear to be a git repository\nfatal: Could not read from remote repository.",
+			want:       errs.InfraDependency,
+		},
+		{
+			name:       "unknown subcommand is a permanent local failure",
+			operation:  "bisect",
+			diagnostic: "fatal: something went wrong",
+			want:       errs.Infra,
 		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.want, ClassifyCommand(tt.operation, tt.diagnostic))
+		})
+	}
+}
+
+func TestClassifier(t *testing.T) {
+	tests := []struct {
+		name string
+		err  error
+		want errs.Verdict
+	}{
 		{
-			name: "unknown subcommand is a permanent local failure",
-			err:  gitError("bisect", "fatal: something went wrong"),
-			want: errs.Infra,
+			name: "local process failure",
+			err:  gitError("fetch", "fatal: Connection reset by peer"),
+			want: errs.InfraDependencyRetryable,
+		},
+		{
+			name: "remote execution failure",
+			err: remoteCommandFailure{
+				operation:  "commit",
+				diagnostic: "fatal: Unable to create '/checkout/.git/index.lock': File exists.",
+			},
+			want: errs.InfraRetryable,
 		},
 		{
 			name: "non-Git error is not this classifier's node",
@@ -113,7 +171,6 @@ func TestClassifier(t *testing.T) {
 		},
 		{
 			name: "nil is not this classifier's node",
-			err:  nil,
 			want: errs.Unknown,
 		},
 	}
@@ -129,13 +186,13 @@ func TestClassifier_FragmentsMatchRegardlessOfCase(t *testing.T) {
 	for _, fragment := range transientTransportFragments {
 		t.Run(fragment, func(t *testing.T) {
 			shouted := "fatal: " + strings.ToUpper(fragment)
-			assert.Equal(t, errs.InfraDependencyRetryable, Classifier.Classify(gitError("fetch", shouted)))
+			assert.Equal(t, errs.InfraDependencyRetryable, ClassifyCommand("fetch", shouted))
 		})
 	}
 	for _, fragment := range transientCheckoutFragments {
 		t.Run(fragment, func(t *testing.T) {
 			shouted := "fatal: " + strings.ToUpper(fragment)
-			assert.Equal(t, errs.InfraRetryable, Classifier.Classify(gitError("commit", shouted)))
+			assert.Equal(t, errs.InfraRetryable, ClassifyCommand("commit", shouted))
 		})
 	}
 }