# PR #135 Review — `worker: classify concurrent-ref pushes as raced`

## Verdict: 🔴 Request changes — one blocker, everything else optional polish

The worker-side design is sound — the `raced` disposition, the Nak→Term escalation, the `nak+term`-numerator carve-out, the fail-open parity, the log-level split. **But the typed sentinel the entire feature hinges on never actually attaches on the real receive-pack path, so in production this PR changes nothing** — every real CAS miss still classifies `nak`, still logs ERROR "Sync failed", still feeds the alert. CI is green only because every test fabricates the error.

## 🔴 Blocker — the feature is dead code in production

**`errors.As` pointer-vs-value mismatch in vendored git-sync** — `vendor/entire.io/entire/git-sync/internal/gitproto/push.go:238` (`asRefRejectedError`), consumed at `push.go:302`.

`asRefRejectedError` (and the pre-existing `annotateLeaseFailure` at `push.go:133`) do:

```go
var cs *packp.CommandStatusErr      // target type: *CommandStatusErr
if !errors.As(err, &cs) { return err }   // never matches → returns early
```

But go-git v6 returns `CommandStatusErr` **by value** — value receiver `Error()` (`report_status.go:35`), constructed by value at `report_status.go:185`. `errors.As` against a `**CommandStatusErr` target can't match a value-typed `CommandStatusErr` in the chain, so it returns early, `*RefRejectedError` is never built, `moved` is never set, and `errors.Is(err, ErrTargetRefMoved)` is always `false`. `isConcurrentRefError` → always `false` in prod.

**I verified this empirically** — a throwaway test driving the exact error go-git hands git-sync:

```go
dynamic type of report.Error()                = packp.CommandStatusErr   (value)
errors.As(err, **CommandStatusErr)            = false   ← git-sync's actual target
errors.As(err, *CommandStatusErr)             = true    ← the fix
errors.Is(wrapped, gitsync.ErrTargetRefMoved) = false   ← real path, sentinel lost
```

And confirmed the worker uses neither `BestEffort` nor `OnRejection` nor force flags, so `asRefRejectedError` at `push.go:302` is the **sole** attachment path for it — and it's the broken one. (The only other wrap, `syncer.go:475`, needs `BestEffort + ForceWithLease`, which the worker never sets.)

**Why it's a true blocker:** the PR's stated goal is killing ~304 spurious ERROR/nak ticks per 7d. With the sentinel never attaching, that number doesn't move. The new `outcome:raced` metric, `entire.sync.raced` tag, and `shouldMarkSpanError` carve-out are all inert. The same bug also means the lease-hint annotation has silently never fired.

**Fix (in git-sync, then re-vendor — you already control the pin):**

1. `asRefRejectedError` 
2. `annotateLeaseFailure`: `var cs packp.CommandStatusErr` (value target).
3. Add a git-sync test that drives a real `report.Error()` through `asRefRejectedError` and asserts `errors.Is(ErrTargetRefMoved)` — not a pre-wrapped sentinel.
4. In the worker, add one test feeding a **value** `packp.CommandStatusErr{Status:"remote ref has changed"}` through the chain, so this exact regression is caught instead of papered over by `fmt.Errorf("...%w", ErrTargetRefMoved)`.