Reviewing Mirror Checkpoint Routing Logic · Entire

Review the last 2 commits. Wouldn't "if !ok || !strings.EqualFold(info.CanonicalHost(), host) {" cause a regression?

4d ago·2m·9 calls

Short answer: no, that line doesn't cause a regression. It's strictly additive for entire:// URLs.

Here's the reasoning, tracing the actual before/after behavior:

Before commit 855b8cdab, entire was not a case in deriveCheckpointURLFromInfo's switch. Every entire:// origin/push URL fell through to default: → returned an error → and both callers caught that error and fell back to resolveProviderCheckpointURL (provider host over HTTPS/SSH):

After the commit, the new case ProtocolEntire (util.go:312-321) does one of two things:

  1. Forge matches the configured provider (ok && EqualFold(info.CanonicalHost(), host)) → derives entire://<cluster>/<forge>/<repo>. This is the new happy path.
  2. Provider unknown/empty (!ok) or forge mismatch → returns an error → callers fall back to resolveProviderCheckpointURL — identical to the old behavior.

So the !ok || !EqualFold(...) guard only ever routes into the exact same provider-HTTPS fallback that ran before. Nothing that worked previously stops working.

Why the comparison is sound, not accidentally rejecting valid mirrors:

The one real behavior change (intended, not a regression): a user with an entire:// origin whose forge matches their github checkpoint provider previously had checkpoints routed direct to github.com; now they route through the mirror. That's precisely the commit's stated purpose ("Route checkpoints through entire:// push-through mirrors"), and ENTIRE_CHECKPOINT_TOKEN still forces the provider-HTTPS path (util.go:222-228) because the token can't ride through the helper.

If you were worried about a specific scenario (e.g. GitLab, or a gh mirror of GitHub Enterprise), tell me which and I'll trace it — but the gate itself preserves the prior fallback in every rejecting case.

Would this not error for forks?

4d ago·2m·4 calls

No — the forge-match line does not error for forks, and there's a test proving it (util_test.go:337-343).

Two things to separate: the forge/host check (the line you asked about) and the owner check (a separate, pre-existing guard). Forks are about owner, and the forge-match line doesn't key on owner at all.

Trace for a forkentire://app.entire.io/gh/fork/app, provider github, checkpoint repo acme/checkpoints:

A fork can't cross forges (you can't fork github.com → gitlab.com), so the forge/host always matches. The owner living under fork instead of acme is irrelevant to this check — the derived URL takes its owner from config.Repo, not from the remote.

Where forks actually diverge is the owner-mismatch guard in PushURL (util.go:213-220), which runs beforederiveCheckpointURLFromInfo:

checkpointOwner := config.Owner()            // "acme"
if pushInfo.Owner != "" && checkpointOwner != "" && !strings.EqualFold(pushInfo.Owner, checkpointOwner) {
    // pushInfo.Owner == "fork" → mismatch → fall back, enabled=false
}

So for a fork, PushURL short-circuits to the origin fallback with enabled=false (checkpoint push disabled) — it never reaches the forge line. That's exactly what the test at util_test.go:337-343 asserts (wantURL: entire://.../gh/fork/app, wantEnabled: false), and it mirrors the long-standing HTTPS fork case at line 313-319. It's a fallback, not an error, and it predates this commit.

FetchURL has no owner check, so a fork origin does reach the entire:// case — forge matches, and it derives entire://app.entire.io/gh/acme/checkpoints cleanly. No error there either.

One honest nuance worth naming (but not a regression from this change): the owner guard means a fork whose owner differs from the checkpoint-repo owner gets checkpoint push silently disabled. If supporting fork contributors pushing checkpoints to a shared acme/checkpoints is a goal, that guard — not the forge line — is what you'd need to revisit.

Add an E2E test to assert that mirrored remotes would work based on the new changes.

4d ago·4m·13 calls

Done. Both tests pass, gofmt is clean, and go vet is quiet.

What I added

Two tests in cmd/entire/cli/strategy/checkpoint_remote_test.go, exercising the new entire:// routing through the production push path that PrePush actually uses (resolvePushSettings → remote.PushURL → deriveCheckpointURLFromInfo), against a real git repo with a real entire:// origin:

  1. TestResolvePushSettings_WithCheckpointRemote_EntireMirror — the positive assertion you asked for: origin is an entire://app.entire.io/gh/org/main-repo mirror whose forge (gh) matches the github checkpoint provider, so checkpoints route through the same-cluster mirror → entire://app.entire.io/gh/org/checkpoints, not a direct github.com URL.
  2. TestResolvePushSettings_EntireMirrorForgeMismatchFallsBackToProvider — the companion that guards the regression we discussed: an entire://.../et/... mirror (forge ≠ provider) must not ride the mirror; it falls back to the provider host (git@github.com:org/checkpoints.git).

Why not a literal e2e/tests/ test

A true E2E test can't exercise this. entire:// needs the git-remote-entire helper plus a live cluster to push/fetch — the E2E and integration harnesses have neither, and remote_operations_test.go:156-161 documents exactly this, deliberately punting entire:///URL-derivation coverage to the resolvePushSettings/unit level. The tests I added are the most end-to-end assertion feasible without live infra: real repo, real remote URL, real production resolver — I just assert the derived target rather than performing a network push (v1 branch is seeded locally to skip the real fetch, matching the existing HTTPS/SSH tests).

If you do want a real-infra E2E variant (spinning up a cluster mirror), that's a larger lift and would belong behind a gated/opt-in harness — say the word and I'll scope it.

Note

The diagnostic flagged at line 985 ([]byte(fmt.Sprintf(...)) → fmt.Appendf) is pre-existing code that shifted down 82 lines from my insertion — not mine, and unrelated to this change. I left it alone. My diff is purely additive.

I haven't run mise run check in full or committed — let me know if you'd like me to.

commit changes

4d ago·2m·5 calls

Author identity matches the DCO sign-off (paulo@entire.io). Here's what I'll commit — waiting for your explicit go-ahead before running git commit (so you can unlock your GPG agent):

File (1):

Draft message:

Test entire:// mirror checkpoint routing via resolvePushSettings

Add two tests exercising the production push path (resolvePushSettings ->
remote.PushURL -> deriveCheckpointURLFromInfo) for entire:// push-through
mirrors:

- a forge-matching mirror routes checkpoints through the same-cluster
  mirror (entire://<cluster>/gh/<repo>) instead of a direct provider URL
- a forge-mismatched mirror falls back to the provider's canonical host

Assertions use a real repo and real entire:// remote and check the derived
target. entire:// can't be pushed for real without the git-remote-entire
helper and a live cluster, so this is the closest end-to-end coverage
feasible without infra.

Assisted-by: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Paulo Gomes <paulo@entire.io>

A couple of notes:

Give me the go-ahead and I'll commit.

go ahead

4d ago·22.6s·1 call

Committed as d8cd7647f on swirling-doodling-waffle — 1 file changed, +82 (test-only). GPG signing succeeded and the working tree is clean. Nothing pushed.