gitproto: match CommandStatusErr by value so target-ref moves classify

CHANGELOG.md

@@ -9,7 +9,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/).
 
 ### Added
 
-- Typed push-rejection errors on the public API. `Sync`/`Replicate` now report a `*RefRejectedError` (carrying the rejected `Ref` and the raw server `Reason`) for per-ref receive-pack `ng` statuses, reachable with `errors.As`. Rejections that are unambiguous concurrent target-ref moves — entire-server's compare-and-swap rejection (`remote ref has changed`) and git's `--force-with-lease` lease miss (`stale info`) — additionally satisfy `errors.Is(err, ErrTargetRefMoved)`. This lets embedders distinguish a benign racing concurrent push (retryable) from a genuine push failure without substring-matching the free-form error message. Ambiguous markers (`non-fast-forward` / `fetch first`) are deliberately excluded from the move classification so a real "needs `--force`" rejection is not masked. The `ForceWithLease` lease-failure escalation (raised even under `BestEffort`) also satisfies `errors.Is(err, ErrTargetRefMoved)`, though it is not itself a `*RefRejectedError` — prefer `errors.Is` over `errors.As` when you only need the cause. The error message and the underlying `*packp.CommandStatusErr` are preserved unchanged, so existing checks keep working.
+- Typed push-rejection errors on the public API. `Sync`/`Replicate` now report a `*RefRejectedError` (carrying the rejected `Ref` and the raw server `Reason`) for per-ref receive-pack `ng` statuses, reachable with `errors.As`. Rejections that are unambiguous concurrent target-ref moves — entire-server's compare-and-swap rejection (`remote ref has changed`) and git's `--force-with-lease` lease miss (`stale info`) — additionally satisfy `errors.Is(err, ErrTargetRefMoved)`. This lets embedders distinguish a benign racing concurrent push (retryable) from a genuine push failure without substring-matching the free-form error message. Ambiguous markers (`non-fast-forward` / `fetch first`) are deliberately excluded from the move classification so a real "needs `--force`" rejection is not masked. The `ForceWithLease` lease-failure escalation (raised even under `BestEffort`) also satisfies `errors.Is(err, ErrTargetRefMoved)`, though it is not itself a `*RefRejectedError` — prefer `errors.Is` over `errors.As` when you only need the cause. The error message and the underlying value-typed `packp.CommandStatusErr` are preserved unchanged (reach it with a value `errors.As` target), so existing checks keep working.
+
+### Fixed
+
+- Concurrent target-ref rejections are now actually classified — `errors.Is(err, ErrTargetRefMoved)` and `errors.As(err, *RefRejectedError)` match on the real push path. go-git returns `packp.CommandStatusErr` **by value** from `ReportStatus.Error()`, but `asRefRejectedError` / `annotateLeaseFailure` used a `*packp.CommandStatusErr` (pointer) `errors.As` target, which never matches a value in the chain — so every live receive-pack `ng` status passed through unclassified and `ErrTargetRefMoved` was never reported. Both now use a value target, and a regression test drives a real `ReportStatus.Error()` end to end so a pointer-vs-value relapse fails CI. (Bug in the unreleased typed-rejection feature above — never shipped in a tagged release.)
 
 ## [0.6.0] - 2026-06-03
 

internal/gitproto/push.go

@@ -129,8 +129,8 @@ func IsLeaseFailure(status string) bool {
 // annotateLeaseFailure wraps a lease-failure CommandStatusErr with a retry/
 // override hint. Other receive-pack errors pass through unchanged.
 func annotateLeaseFailure(err error) error {
-	var cs *packp.CommandStatusErr
-	if !errors.As(err, &cs) {
+	cs, ok := commandStatusErr(err)
+	if !ok {
 		return err
 	}
 	if !IsLeaseFailure(cs.Status) {
@@ -229,15 +229,37 @@ func isConcurrentMove(reason string) bool {
 	return false
 }
 
+// commandStatusErr extracts go-git's per-ref CommandStatusErr from err's chain.
+// errors.As is EXACT about value-vs-pointer, and the form go-git uses is not
+// part of its API contract: today report.Error() returns the error BY VALUE
+// (value receiver Error(), constructed by value in report_status.go), but go-git
+// is an alpha dependency and could switch to a *CommandStatusErr. A target that
+// only matches one form would silently stop classifying every rejection if the
+// other showed up — the exact failure a pointer-only target caused before. So we
+// try the value form first (the current reality) and fall back to the pointer
+// form. TestAsRefRejectedError_RealReportStatusPath drives go-git's real
+// report.Error() so a deeper type change still fails loud in CI.
+func commandStatusErr(err error) (packp.CommandStatusErr, bool) {
+	var byVal packp.CommandStatusErr
+	if errors.As(err, &byVal) {
+		return byVal, true
+	}
+	var byPtr *packp.CommandStatusErr
+	if errors.As(err, &byPtr) && byPtr != nil {
+		return *byPtr, true
+	}
+	return packp.CommandStatusErr{}, false
+}
+
 // asRefRejectedError wraps a target receive-pack report-status "ng" error in
 // a typed *RefRejectedError so callers can branch on errors.As /
 // errors.Is(err, ErrTargetRefMoved) instead of substring-matching the free-form
 // reason themselves. Inputs that are not a per-ref command status (e.g. an
 // unpack-status error) pass through unchanged. The input is preserved via Unwrap,
-// so the message and any errors.As(*packp.CommandStatusErr) check are unchanged.
+// so the message and the underlying packp.CommandStatusErr stay reachable.
 func asRefRejectedError(err error) error {
-	var cs *packp.CommandStatusErr
-	if !errors.As(err, &cs) {
+	cs, ok := commandStatusErr(err)
+	if !ok {
 		return err
 	}
 	return &RefRejectedError{

internal/gitproto/push_test.go

@@ -465,13 +465,16 @@ func TestAnnotateLeaseFailureWrapsStaleInfo(t *testing.T) {
 	}
 	for _, c := range cases {
 		t.Run(c.name, func(t *testing.T) {
-			in := &packp.CommandStatusErr{ReferenceName: "refs/heads/main", Status: c.status}
+			// Faithful input: go-git returns CommandStatusErr BY VALUE from
+			// report.Error(), so annotateLeaseFailure must match it with a value
+			// errors.As target. A *CommandStatusErr input would mask that.
+			in := (&packp.CommandStatus{ReferenceName: "refs/heads/main", Status: c.status}).Error()
 			out := annotateLeaseFailure(in)
 			wrapped := strings.Contains(out.Error(), "moved or differs from session start")
 			if wrapped != c.wrap {
 				t.Fatalf("wrap=%v want=%v (err=%q)", wrapped, c.wrap, out)
 			}
-			var inner *packp.CommandStatusErr
+			var inner packp.CommandStatusErr
 			if !errors.As(out, &inner) || inner.Status != c.status {
 				t.Fatalf("annotateLeaseFailure must preserve the underlying CommandStatusErr; got %#v", out)
 			}
@@ -526,7 +529,10 @@ func TestAsRefRejectedErrorClassifiesAndPreserves(t *testing.T) {
 	}
 	for _, c := range cases {
 		t.Run(c.name, func(t *testing.T) {
-			cs := &packp.CommandStatusErr{ReferenceName: "refs/heads/main", Status: c.status}
+			// Faithful input: go-git's report.Error() returns CommandStatusErr BY
+			// VALUE, not a pointer — the errors.As targets in annotateLeaseFailure /
+			// asRefRejectedError must match the value.
+			cs := (&packp.CommandStatus{ReferenceName: "refs/heads/main", Status: c.status}).Error()
 			// Mirror the production chain: lease annotation, typed wrap, then the
 			// same outer wrapping report-status / syncer / client apply with %w.
 			wrapped := fmt.Errorf("sync: %w", fmt.Errorf("report-status: %w", asRefRejectedError(annotateLeaseFailure(cs))))
@@ -543,9 +549,9 @@ func TestAsRefRejectedErrorClassifiesAndPreserves(t *testing.T) {
 			}
 			// Backward compatibility: the underlying go-git error and the raw
 			// reason substring must survive the typed wrap unchanged.
-			var cse *packp.CommandStatusErr
+			var cse packp.CommandStatusErr
 			if !errors.As(wrapped, &cse) || cse.Status != c.status {
-				t.Fatalf("underlying *packp.CommandStatusErr must be preserved; got %#v", wrapped)
+				t.Fatalf("underlying packp.CommandStatusErr must be preserved; got %#v", wrapped)
 			}
 			if !strings.Contains(wrapped.Error(), c.status) {
 				t.Fatalf("message must still contain the raw reason %q; got %q", c.status, wrapped.Error())
@@ -580,3 +586,53 @@ func TestAsRefRejectedErrorPassesNonCommandStatusErrors(t *testing.T) {
 		t.Fatalf("non-CommandStatusErr must not be wrapped as *RefRejectedError")
 	}
 }
+
+// TestAsRefRejectedError_RealReportStatusPath drives the EXACT error go-git
+// hands sendReceivePack — a value-typed packp.CommandStatusErr produced by
+// ReportStatus.Error() — through the production wrap (annotateLeaseFailure →
+// asRefRejectedError → "report-status: %w"). The hand-built table tests above
+// can construct the input however they like; this one pins the classification
+// to go-git's real return type. If a pointer-vs-value regression ever creeps
+// back into the errors.As targets, asRefRejectedError stops matching, errors.Is
+// goes false, and this fails — which is exactly the bug it guards against.
+func TestAsRefRejectedError_RealReportStatusPath(t *testing.T) {
+	rs := &packp.ReportStatus{
+		UnpackStatus: "ok",
+		CommandStatuses: []*packp.CommandStatus{
+			{ReferenceName: "refs/heads/main", Status: "remote ref has changed"},
+		},
+	}
+	reportErr := rs.Error() // value packp.CommandStatusErr, exactly as in sendReceivePack
+	if reportErr == nil {
+		t.Fatal("expected a rejection error from report-status")
+	}
+
+	wrapped := fmt.Errorf("report-status: %w", asRefRejectedError(annotateLeaseFailure(reportErr)))
+
+	if !errors.Is(wrapped, ErrTargetRefMoved) {
+		t.Fatalf("a real go-git report-status CAS rejection must satisfy errors.Is(ErrTargetRefMoved); got %v (underlying %T)", wrapped, reportErr)
+	}
+	var rej *RefRejectedError
+	if !errors.As(wrapped, &rej) || rej.Reason != "remote ref has changed" {
+		t.Fatalf("must classify as *RefRejectedError carrying the raw reason; got %#v", wrapped)
+	}
+}
+
+// TestAsRefRejectedError_ToleratesPointerCommandStatusErr guards the value/
+// pointer robustness in commandStatusErr. go-git returns CommandStatusErr by
+// VALUE today (covered by the real-report-status test above); this pins the
+// other form so that if go-git ever hands the error over as a *CommandStatusErr,
+// classification keeps working instead of silently regressing to "every
+// rejection unclassified".
+func TestAsRefRejectedError_ToleratesPointerCommandStatusErr(t *testing.T) {
+	ptr := &packp.CommandStatusErr{ReferenceName: "refs/heads/main", Status: "remote ref has changed"}
+	wrapped := fmt.Errorf("report-status: %w", asRefRejectedError(annotateLeaseFailure(ptr)))
+
+	if !errors.Is(wrapped, ErrTargetRefMoved) {
+		t.Fatalf("a *CommandStatusErr rejection must also classify as ErrTargetRefMoved; got %v", wrapped)
+	}
+	var rej *RefRejectedError
+	if !errors.As(wrapped, &rej) || rej.Reason != "remote ref has changed" {
+		t.Fatalf("must classify the pointer form as *RefRejectedError; got %#v", wrapped)
+	}
+}