Skip to content

Commit df06e90

Browse files
committed
feat(errs): export Git command classification policy
Summary: Intent: - Let local Git and remote Git execution backends share one retryability policy. - Avoid requiring non-process backends to manufacture gitexec.CommandError values. Changes: - Export a backend-neutral CommandFailure contract accepted by the Git classifier. - Export direct command classification and decouple the policy from platform/git/exec. - Cover local and remote error implementations and document the extension point. --- <sub>Generated by the 🪄 [pr-create](https://sg.uberinternal.com/code.uber.internal/uber-code/devexp-agent-marketplace/-/blob/claude-code/plugins/dev/uber-dev/skills/pr-create/SKILL.md) skill in devexp-agent-marketplace</sub>
1 parent 40b0609 commit df06e90

4 files changed

Lines changed: 134 additions & 68 deletions

File tree

platform/errs/README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,7 @@ Classifiers are not installed globally. A host that wants YARPC statuses classif
145145

146146
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.
147147

148-
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.
148+
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.
149149

150150
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`.
151151

platform/errs/git/BUILD.bazel

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,7 @@ go_library(
55
srcs = ["git.go"],
66
importpath = "github.com/uber/submitqueue/platform/errs/git",
77
visibility = ["//visibility:public"],
8-
deps = [
9-
"//platform/errs:go_default_library",
10-
"//platform/git/exec:go_default_library",
11-
],
8+
deps = ["//platform/errs:go_default_library"],
129
)
1310

1411
go_test(

platform/errs/git/git.go

Lines changed: 29 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
// See the License for the specific language governing permissions and
1313
// limitations under the License.
1414

15-
// Package git provides an errs.Classifier for failures from Git processes.
15+
// Package git classifies failures reported by Git commands.
1616
//
1717
// Git has no typed status to read: it reports almost everything as a non-zero
1818
// exit and a line of prose, so a connection reset and a deleted branch both
@@ -47,7 +47,6 @@ import (
4747
"strings"
4848

4949
"github.com/uber/submitqueue/platform/errs"
50-
gitexec "github.com/uber/submitqueue/platform/git/exec"
5150
)
5251

5352
// Classifier recognises Git process failures, reporting a known transient
@@ -63,6 +62,16 @@ var Classifier errs.Classifier = classifier{}
6362

6463
type classifier struct{}
6564

65+
// CommandFailure is the Git failure information needed by Classifier.
66+
//
67+
// Execution backends may satisfy this contract with their native error type;
68+
// they do not need to reconstruct an error from another Git implementation.
69+
type CommandFailure interface {
70+
error
71+
Operation() string
72+
Diagnostic() string
73+
}
74+
6675
// remoteOperations are the Git subcommands that exchange data with the
6776
// configured remote. They attribute their failures to that remote, and they
6877
// are the only operations a transport fragment can legitimately describe.
@@ -106,21 +115,13 @@ var transientCheckoutFragments = []string{
106115
"resource temporarily unavailable",
107116
}
108117

109-
// Classify inspects a single node. Per the errs.Classifier contract, this must
110-
// not call errors.Is / errors.As — the classifier-processor owns the chain
111-
// walk.
112-
func (classifier) Classify(err error) errs.Verdict {
113-
commandErr, ok := err.(*gitexec.CommandError)
114-
if !ok {
115-
// The only Unknown this classifier returns, and it means "not my
116-
// node" rather than "no opinion on this failure". Returning a verdict
117-
// here would claim every error the walk passes — a MySQL driver error
118-
// among them — before its own classifier were asked.
119-
return errs.Unknown
120-
}
121-
122-
diagnostic := strings.ToLower(commandErr.Diagnostic())
123-
remote := remoteOperations[commandErr.Operation()]
118+
// ClassifyCommand classifies a Git subcommand and its rendered diagnostic.
119+
//
120+
// It is the shared policy for local processes, remote Git execution services,
121+
// and any other backend that can report those two values.
122+
func ClassifyCommand(operation, diagnostic string) errs.Verdict {
123+
diagnostic = strings.ToLower(diagnostic)
124+
remote := remoteOperations[operation]
124125

125126
transient := containsAny(diagnostic, transientCheckoutFragments) ||
126127
(remote && containsAny(diagnostic, transientTransportFragments))
@@ -137,6 +138,17 @@ func (classifier) Classify(err error) errs.Verdict {
137138
}
138139
}
139140

141+
// Classify inspects a single node. Per the errs.Classifier contract, this must
142+
// not call errors.Is / errors.As — the classifier-processor owns the chain
143+
// walk.
144+
func (classifier) Classify(err error) errs.Verdict {
145+
commandFailure, ok := err.(CommandFailure)
146+
if !ok {
147+
return errs.Unknown
148+
}
149+
return ClassifyCommand(commandFailure.Operation(), commandFailure.Diagnostic())
150+
}
151+
140152
func containsAny(diagnostic string, fragments []string) bool {
141153
for _, fragment := range fragments {
142154
if strings.Contains(diagnostic, fragment) {

platform/errs/git/git_test.go

Lines changed: 103 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -32,79 +32,137 @@ func gitError(operation, diagnostic string) error {
3232
return gitexec.NewCommandError(operation, diagnostic, errors.New("exit status 128"))
3333
}
3434

35-
func TestClassifier(t *testing.T) {
35+
type remoteCommandFailure struct {
36+
operation string
37+
diagnostic string
38+
}
39+
40+
func (e remoteCommandFailure) Error() string {
41+
return e.diagnostic
42+
}
43+
44+
func (e remoteCommandFailure) Operation() string {
45+
return e.operation
46+
}
47+
48+
func (e remoteCommandFailure) Diagnostic() string {
49+
return e.diagnostic
50+
}
51+
52+
func TestClassifyCommand(t *testing.T) {
3653
tests := []struct {
37-
name string
38-
err error
39-
want errs.Verdict
54+
name string
55+
operation string
56+
diagnostic string
57+
want errs.Verdict
4058
}{
4159
{
42-
name: "transport fault on fetch is a retryable dependency failure",
43-
err: gitError("fetch", "fatal: unable to access 'https://host/r.git/': Connection reset by peer"),
44-
want: errs.InfraDependencyRetryable,
60+
name: "transport fault on fetch is a retryable dependency failure",
61+
operation: "fetch",
62+
diagnostic: "fatal: unable to access 'https://host/r.git/': Connection reset by peer",
63+
want: errs.InfraDependencyRetryable,
4564
},
4665
{
47-
name: "unresolvable host on ls-remote is a retryable dependency failure",
48-
err: gitError("ls-remote", "fatal: Could not resolve host: github.example.com"),
49-
want: errs.InfraDependencyRetryable,
66+
name: "unresolvable host on ls-remote is a retryable dependency failure",
67+
operation: "ls-remote",
68+
diagnostic: "fatal: Could not resolve host: github.example.com",
69+
want: errs.InfraDependencyRetryable,
5070
},
5171
{
52-
name: "checkout contention on a local commit is a retryable local failure",
53-
err: gitError("commit", "fatal: Unable to create '/checkout/.git/index.lock': File exists."),
54-
want: errs.InfraRetryable,
72+
name: "checkout contention on a local commit is a retryable local failure",
73+
operation: "commit",
74+
diagnostic: "fatal: Unable to create '/checkout/.git/index.lock': File exists.",
75+
want: errs.InfraRetryable,
5576
},
5677
{
57-
name: "checkout contention during fetch is attributed to the remote it ran against",
58-
err: gitError("fetch", "error: cannot lock ref 'refs/remotes/origin/main'"),
59-
want: errs.InfraDependencyRetryable,
78+
name: "checkout contention during fetch is attributed to the remote it ran against",
79+
operation: "fetch",
80+
diagnostic: "error: cannot lock ref 'refs/remotes/origin/main'",
81+
want: errs.InfraDependencyRetryable,
6082
},
6183
{
62-
name: "transport fragment on a local operation is not evidence of a transient failure",
63-
err: gitError("merge", "error: could not resolve host mentioned in a commit message"),
64-
want: errs.Infra,
84+
name: "transport fragment on a local operation is not evidence of a transient failure",
85+
operation: "merge",
86+
diagnostic: "error: could not resolve host mentioned in a commit message",
87+
want: errs.Infra,
6588
},
6689
{
67-
name: "unknown revision is a permanent local failure",
68-
err: gitError("rev-parse", "fatal: ambiguous argument 'origin/main': unknown revision or path not in the working tree."),
69-
want: errs.Infra,
90+
name: "unknown revision is a permanent local failure",
91+
operation: "rev-parse",
92+
diagnostic: "fatal: ambiguous argument 'origin/main': unknown revision or path not in the working tree.",
93+
want: errs.Infra,
7094
},
7195
{
72-
name: "empty squash commit is a permanent local failure",
73-
err: gitError("commit", "exit status 1"),
74-
want: errs.Infra,
96+
name: "empty squash commit is a permanent local failure",
97+
operation: "commit",
98+
diagnostic: "exit status 1",
99+
want: errs.Infra,
75100
},
76101
{
77-
name: "failure with no diagnostic at all is a permanent local failure",
78-
err: gitError("cat-file", ""),
79-
want: errs.Infra,
102+
name: "failure with no diagnostic at all is a permanent local failure",
103+
operation: "cat-file",
104+
want: errs.Infra,
80105
},
81106
{
82-
name: "path outside the repository is a permanent local failure",
83-
err: gitError("clean", "fatal: '/etc': '/etc' is outside repository at '/checkout'"),
84-
want: errs.Infra,
107+
name: "path outside the repository is a permanent local failure",
108+
operation: "clean",
109+
diagnostic: "fatal: '/etc': '/etc' is outside repository at '/checkout'",
110+
want: errs.Infra,
85111
},
86112
{
87-
name: "non-fast-forward push is a permanent dependency failure",
88-
err: gitError("push", "! [rejected] main -> main (fetch first)"),
89-
want: errs.InfraDependency,
113+
name: "non-fast-forward push is a permanent dependency failure",
114+
operation: "push",
115+
diagnostic: "! [rejected] main -> main (fetch first)",
116+
want: errs.InfraDependency,
90117
},
91118
{
92-
name: "authentication failure is a permanent dependency failure",
93-
err: gitError("fetch", "fatal: Authentication failed for 'https://host/r.git/'"),
94-
want: errs.InfraDependency,
119+
name: "authentication failure is a permanent dependency failure",
120+
operation: "fetch",
121+
diagnostic: "fatal: Authentication failed for 'https://host/r.git/'",
122+
want: errs.InfraDependency,
95123
},
96124
{
97125
// Git prints this trailer under permanent failures too — a missing
98126
// remote, absent access rights — so it is not evidence of a
99127
// transient one.
100-
name: "generic remote trailer is a permanent dependency failure",
101-
err: gitError("fetch", "fatal: 'origin' does not appear to be a git repository\nfatal: Could not read from remote repository."),
102-
want: errs.InfraDependency,
128+
name: "generic remote trailer is a permanent dependency failure",
129+
operation: "fetch",
130+
diagnostic: "fatal: 'origin' does not appear to be a git repository\nfatal: Could not read from remote repository.",
131+
want: errs.InfraDependency,
132+
},
133+
{
134+
name: "unknown subcommand is a permanent local failure",
135+
operation: "bisect",
136+
diagnostic: "fatal: something went wrong",
137+
want: errs.Infra,
103138
},
139+
}
140+
141+
for _, tt := range tests {
142+
t.Run(tt.name, func(t *testing.T) {
143+
assert.Equal(t, tt.want, ClassifyCommand(tt.operation, tt.diagnostic))
144+
})
145+
}
146+
}
147+
148+
func TestClassifier(t *testing.T) {
149+
tests := []struct {
150+
name string
151+
err error
152+
want errs.Verdict
153+
}{
104154
{
105-
name: "unknown subcommand is a permanent local failure",
106-
err: gitError("bisect", "fatal: something went wrong"),
107-
want: errs.Infra,
155+
name: "local process failure",
156+
err: gitError("fetch", "fatal: Connection reset by peer"),
157+
want: errs.InfraDependencyRetryable,
158+
},
159+
{
160+
name: "remote execution failure",
161+
err: remoteCommandFailure{
162+
operation: "commit",
163+
diagnostic: "fatal: Unable to create '/checkout/.git/index.lock': File exists.",
164+
},
165+
want: errs.InfraRetryable,
108166
},
109167
{
110168
name: "non-Git error is not this classifier's node",
@@ -113,7 +171,6 @@ func TestClassifier(t *testing.T) {
113171
},
114172
{
115173
name: "nil is not this classifier's node",
116-
err: nil,
117174
want: errs.Unknown,
118175
},
119176
}
@@ -129,13 +186,13 @@ func TestClassifier_FragmentsMatchRegardlessOfCase(t *testing.T) {
129186
for _, fragment := range transientTransportFragments {
130187
t.Run(fragment, func(t *testing.T) {
131188
shouted := "fatal: " + strings.ToUpper(fragment)
132-
assert.Equal(t, errs.InfraDependencyRetryable, Classifier.Classify(gitError("fetch", shouted)))
189+
assert.Equal(t, errs.InfraDependencyRetryable, ClassifyCommand("fetch", shouted))
133190
})
134191
}
135192
for _, fragment := range transientCheckoutFragments {
136193
t.Run(fragment, func(t *testing.T) {
137194
shouted := "fatal: " + strings.ToUpper(fragment)
138-
assert.Equal(t, errs.InfraRetryable, Classifier.Classify(gitError("commit", shouted)))
195+
assert.Equal(t, errs.InfraRetryable, ClassifyCommand("commit", shouted))
139196
})
140197
}
141198
}

0 commit comments

Comments
 (0)