Go Best Practices Review and Hardening

I've reviewed the full branch diff, verified the referenced symbols exist, and confirmed everything compiles and vets cleanly under the integration and e2e build tags. Here's the review from a Go best-practices angle.

Overall

This is high-quality test infrastructure. The backend-abstraction approach (ForEachBackend + backend-aware assertion helpers) is the right design — it parameterizes the existing suite over both checkpoint topologies without duplicating test bodies, and the t.Parallel()/closure-constructs-env pattern is correctly applied throughout. Comments are unusually thorough and explain why (the hermeticity rationale, the insteadOf-vs-proxy tradeoff, the git-refs t.Skip divergence). Repo conventions (t.Helper, execx.NonInteractive, testutil.GitIsolatedEnv, //go:build integration) are respected.

A few things worth flagging, none blocking.

1. Error-swallowing is inconsistent with the branch's own guard (minor, real)

RemoteCheckpointState deliberately t.Fatalfs on a git for-each-ref failure, with a comment explaining the reasoning:

But the sibling helpers anyRefUnderPrefix and refExists (backend.go:117, 129) return false on git error. Those feed the negative assertions:

A broken for-each-ref (corrupt/missing repo dir, wrong path) → false → the "should NOT exist" test passes vacuously — exactly the failure mode you guarded against one function up. for-each-ref on a healthy repo essentially never errors, so this is low-probability, but for consistency anyRefUnderPrefix should distinguish "git failed" from "zero refs" and fatal on the former. refExists is fine as-is (show-ref --verify --quiet legitimately exits non-zero for a missing ref — that is the signal).

2. e2e/testutil reimplements id.ShardFor by hand

integration_test/backend.go correctly uses the production id.CheckpointID(...).ShardFor(). The e2e mirror instead hand-rolls it:

id is an ordinary (non-tag-gated) package, so e2e/testutil/backend.go could import it and drop the copy — one fewer "keep these in sync by hand" comment, one fewer place for the two implementations to drift. The broader integration↔e2e duplication is harder to eliminate (different build tags), and the comments acknowledge it, so I'd leave that — but the shard reimplementation is avoidable.

3. Minor / cosmetic

• defaultPrePushStdin opens the repo twice — GetCurrentBranch() then gitrepo.OpenPath(). Trivial in tests; not worth changing unless you touch it.
• InstallRealPrePushHook doc says git passes remote name ($1) and URL ($2), but the generated script forwards only $1. Functionally correct (the CLI hook only needs the remote name), but the comment reads as if both are used — a half-line clarification would prevent a future reader from thinking $2 is dropped by mistake.
• LatestCheckpointID (no Get prefix) is more idiomatic Go than its GetLatestCheckpointID* siblings — the inconsistency is pre-existing and I'd leave the new one as-is rather than regress toward the older naming.

The git-refs t.Skip in TestGracefulDegradation_...OnCloneIsSilent is handled the right way: it documents a genuine backend divergence, keeps the degradation-path coverage running under git-refs, and preserves the local-presence check under git-branch, rather than asserting a known-broken behavior.

Want me to apply #1 and #2? They're small, self-contained edits.

Both fixes are in and clean — fmt made no changes and lint reports 0 issues.

Summary

Fix 1 — anyRefUnderPrefix no longer swallows git errors (integration_test/backend.go)

It now t.Fatalfs on a for-each-ref failure instead of returning false, matching the deliberate guard in RemoteCheckpointState. An absent prefix still correctly reports false (git exits 0 with empty output), so only a genuinely broken invocation fails — closing the vacuous-pass hole in the "should NOT exist" assertions. refExists was left as-is since its non-zero exit is the legitimate "ref missing" signal.

Fix 2 — dropped the hand-rolled shard in e2e (e2e/testutil/backend.go)

Deleted the checkpointShard copy and pointed checkpointRefName at the production id.CheckpointID(...).ShardFor() — same computation the integration mirror already uses. One fewer keep-in-sync-by-hand surface. Verified id.ShardFor is byte-identical (last two chars, whole ID if <2) before swapping.

Both packages vet clean under their build tags, and mise run fmt && mise run lint passes with 0 issues. I didn't run the integration/e2e suites themselves (integration needs mise run test:integration; e2e makes real calls) — say the word if you want me to kick off mise run test:integration.

Committed and pushed to soph/git-remote-tests-1-infra (c6636c4ab..7a45cf4f3). Both review fixes are on the remote.

Here's my assessment of PR #913.

Verdict: still merge-worthy in substance — but stale, and it must be re-validated before merging

The fix is still needed and not superseded. On current origin/main, workingTreeMatchesCommit is still the raw-blob-hash-only version:

So the autocrlf phantom-carry-forward bug the PR targets still exists, and nobody fixed it another way. The fix is real work, not a no-op.

It still applies cleanly. GitHub reports MERGEABLE, and I confirmed why: the call site (line 496) and signature (line 524) on main are byte-identical to the PR's diff context, and every helper the new test leans on — setupGitRepo, createShadowBranchWithContent, checkpoint.ShadowBranchNameForCommit, object.Signature — is still present.

The change itself is sound and idiomatic. Asking git first (git diff --exit-code --quiet -- <path>) so clean/smudge filters don't produce phantom diffs is the right approach; the errors.As exit-code-1-vs-other handling is correct, the 5s timeout is derived from ctx, and it keeps the raw-hash path as a fallback when git is absent/fails. Post-commit the index equals HEAD, so git's working-tree-vs-index cleanliness check correctly answers "does the file still differ from the committed blob."

The catch — don't merge on the green checkmark you see

• CI is 3 months old. The PR head is 3315 commits behind main (3 ahead). All checks passed, but against an April base. MERGEABLE only means no textual conflict — it says nothing about semantic drift or whether tests still pass on today's main.
• It's BLOCKED / REVIEW_REQUIRED — no approving review yet.

Two smaller notes on the change (non-blocking)

• Per-file subprocess cost: it now spawns a git process per candidate file instead of in-process hashing. Fine for typical small carry-forward sets, but a regression for large ones. This is what Cursor's "Medium Risk" flag is about (git availability/config dependence).
• The behavior correctly degrades to the old hash path when git errors (128/timeout), so no environment regresses to broken — worst case is back to the old behavior.

Recommendation

Rebase onto current main, push, and let CI re-run against today's tree — then get one approving review. If it's green after rebase, it's good to merge.

Want me to rebase soph/codex-e2e-fix onto latest main and push (re-triggering CI)? I'd do it in a worktree so it doesn't disturb your current branch.

Done. Opened PR #1654: https://github.com/entireio/cli/pull/1654

What it does

Branched fresh off origin/main and applied four of the five bumps from Dependabot #1652, holding betterleaks at v1.5.0:

betterleaks stays at 1.5.0 — its 1.6.x release swaps the expression runtime from CEL to Expr and reworks pattern loading, which is what's breaking the redact/ path. (charmbracelet/ultraviolet moved as a transitive dep of the charm packages.)

Verified locally

• go build ./... — clean
• mise run fmt && mise run lint — 0 issues
• mise run test — 7587 passed, 5 skipped, including the redact package that actually uses betterleaks

Suggested follow-up on #1652

Since #1654 covers the four safe updates, you'll want to keep #1652 from re-introducing the betterleaks bump. Cleanest path: close #1652 and tell Dependabot to hold betterleaks until the redaction breakage is fixed, e.g. comment on #1652:

Want me to post that comment and/or close #1652? I held off since closing a PR is your call.