From 06be8477483f32592a5219185d44768589c87eb3 Mon Sep 17 00:00:00 2001 From: Andrea Nodari Date: Sat, 13 Jun 2026 11:26:45 +0900 Subject: [PATCH] gitproto: classify target-ref moves by extracting CommandStatusErr robustly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit asRefRejectedError and annotateLeaseFailure used errors.As against a *packp.CommandStatusErr (pointer) target, but go-git returns CommandStatusErr BY VALUE from ReportStatus.Error() (value receiver, constructed by value in report_status.go). A pointer target never matches a value in the error chain, so on the real receive-pack path the errors.As fell through, no *RefRejectedError was built, moved was never set, and errors.Is(err, ErrTargetRefMoved) was always false. Every live concurrent target-ref rejection ("remote ref has changed") passed through unclassified — the typed-rejection feature was inert in production. The same bug silently disabled the lease-failure hint. Extract the error through a shared commandStatusErr helper that accepts BOTH the value form (today's go-git) and a *CommandStatusErr, since errors.As is exact about value-vs-pointer and the form is not part of go-git's (alpha) API contract — so a future switch can't silently regress classification to "every rejection unclassified". The existing unit tests masked the original bug by feeding a pointer &packp.CommandStatusErr{} as input, which the pointer target did match — not how go-git hands the error over. Make the table tests build input via go-git's real (&CommandStatus{}).Error() (value), add TestAsRefRejectedError_RealReportStatusPath (drives a real ReportStatus.Error() end to end; fails loud on a deeper type change) and TestAsRefRejectedError_ToleratesPointerCommandStatusErr (pins the pointer form). Bug was in the unreleased typed-rejection feature (#71); never shipped tagged. Co-Authored-By: Claude Opus 4.8 (1M context) Entire-Checkpoint: f226bb2085bf --- CHANGELOG.md | 6 +++- internal/gitproto/push.go | 32 ++++++++++++++--- internal/gitproto/push_test.go | 66 +++++++++++++++++++++++++++++++--- 3 files changed, 93 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b3768cb7..af99d786 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/internal/gitproto/push.go b/internal/gitproto/push.go index b3d94491..3663ec0c 100644 --- a/internal/gitproto/push.go +++ b/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{ diff --git a/internal/gitproto/push_test.go b/internal/gitproto/push_test.go index b3a29f8f..aea68244 100644 --- a/internal/gitproto/push_test.go +++ b/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) + } +}