From a3136178c3089e28afad4ca34190ecddc4bc8a75 Mon Sep 17 00:00:00 2001 From: Stefan Haubold Date: Wed, 5 Aug 2026 17:36:10 +0200 Subject: [PATCH 1/3] feat(checkpoint): push checkpoint refs to one destination, and say where MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The git-refs push-discovery queue records only a ref (`{"ref": …}`) with no per-destination state, so "this ref is pushed" has to mean one place. Relying on git's fan-out across a remote's several push URLs cannot provide that: - one failing URL fails the whole `git push` invocation, so no ref unqueues even though other URLs took it — a dead mirror wedges the queue indefinitely; - and git's iteration is not even uniform. A ref REJECTION returns and git carries on to later URLs, but a transport failure (missing repo, auth, bad host) die()s — so an unreachable FIRST push URL means nothing is pushed anywhere, while the same mirror in last position is harmless. Position silently decided the outcome. So when a remote carries more than one push URL, checkpoint refs now target its first push URL directly and the rest are skipped. The recovery fetch uses the same target, which also fixes a mismatch: fan-out reconciled the remote's FETCH url while pushing to its pushurls, so with a pushurl set we reconciled a repo nothing was pushed to. A single push URL keeps the remote NAME as the target, so the overwhelmingly common topology is byte-identical to before — remote-tracking refs still update, output still says "origin", no URL-keyed promisor config appears. The git-branch backend deliberately keeps git's fan-out: its v1 branch is a single shared ref with no queue to keep coherent, and mirroring it to every push URL is what users configure those remotes for. The two backends therefore differ on multi-URL remotes; that is documented at the decision point rather than left to be discovered. Trade accepted: checkpoint refs live in exactly one repository, so cloning a different mirror of the same code will not find them, and a rejecting first URL no longer lets later URLs receive the refs. checkpoint_remote remains the way to name a checkpoint repository explicitly. Because that is invisible otherwise, it is now announced in three places: pre-push warns on stderr naming the chosen URL and how many were skipped; `entire enable` notes it; `entire doctor` reports it under "Checkpoint destination: REVIEW". All three also cover the other ambiguous topology — several remotes, where checkpoints follow whichever remote you push to while resume/explain always read via origin. All are silent on an ordinary repo and whenever checkpoint_remote already pins a destination. Two message fixes on the way past: - the batch-rejection retry claimed "Some checkpoint refs diverged" for every failure, including an unreachable destination — sending users after the wrong problem. It now says the push was rejected without asserting why. - printCheckpointRemoteHint ("a checkpoint remote is configured in Entire settings") fired for any URL target and would now fire for a URL we picked ourselves. It is gated on an actual checkpoint_remote. Co-Authored-By: Claude Opus 5 (1M context) Entire-Checkpoint: 01KZ992TB3ZNJJR9H9CHG01JCR --- cmd/entire/cli/checkpoint/remote/util.go | 11 ++ cmd/entire/cli/doctor.go | 18 ++ cmd/entire/cli/gitremote/gitremote.go | 28 +++ .../cli/integration_test/multi_pushurl.go | 26 +++ .../integration_test/multi_pushurl_test.go | 165 ++++++++++++++---- cmd/entire/cli/remote_topology.go | 157 +++++++++++++++++ cmd/entire/cli/setup.go | 3 + cmd/entire/cli/strategy/manual_commit_push.go | 24 ++- .../cli/strategy/refs_push_destination.go | 124 +++++++++++++ 9 files changed, 514 insertions(+), 42 deletions(-) create mode 100644 cmd/entire/cli/remote_topology.go create mode 100644 cmd/entire/cli/strategy/refs_push_destination.go diff --git a/cmd/entire/cli/checkpoint/remote/util.go b/cmd/entire/cli/checkpoint/remote/util.go index 78a2cf45c6..a1d4d530d6 100644 --- a/cmd/entire/cli/checkpoint/remote/util.go +++ b/cmd/entire/cli/checkpoint/remote/util.go @@ -288,6 +288,17 @@ func GetRemoteURL(ctx context.Context, remoteName string) (string, error) { return url, nil } +// GetPushURLs returns every URL a push to remoteName delivers to, in the order +// git will use them. See gitremote.GetPushURLs for why this differs from +// GetRemoteURL. +func GetPushURLs(ctx context.Context, remoteName string) ([]string, error) { + urls, err := gitremote.GetPushURLs(ctx, remoteName) + if err != nil { + return nil, fmt.Errorf("get push URLs: %w", err) + } + return urls, nil +} + // GetRemoteURLInDir returns the URL configured for the named git remote in dir. func GetRemoteURLInDir(ctx context.Context, dir, remoteName string) (string, error) { url, err := gitremote.GetRemoteURLInDir(ctx, dir, remoteName) diff --git a/cmd/entire/cli/doctor.go b/cmd/entire/cli/doctor.go index eae539fcbb..2c380d50ff 100644 --- a/cmd/entire/cli/doctor.go +++ b/cmd/entire/cli/doctor.go @@ -108,6 +108,9 @@ func runSessionsFix(cmd *cobra.Command, force bool) error { // Agent-specific: Claude Code hook config drift. checkClaudeCodeHookDrift(cmd) + // Where checkpoints land, when the repo's remotes make that ambiguous. + checkCheckpointDestination(cmd) + // Stuck sessions // Load all session states states, err := strategy.ListSessionStates(ctx) @@ -446,6 +449,21 @@ func confirmDoctorFix(ctx context.Context, w io.Writer, title string) (bool, err // Both checks are structural (file/key presence). Stays silent when // this repo doesn't have codex hooks installed or when we can't // resolve the worktree root. Warn-only. +// checkCheckpointDestination reports where checkpoints will be pushed when the +// repo's remote layout makes that ambiguous — several remotes, or one remote +// fanning out to several push URLs. Silent on the ordinary single-destination +// repo and whenever checkpoint_remote already pins one explicitly. +func checkCheckpointDestination(cmd *cobra.Command) { + w := cmd.OutOrStdout() + topology := inspectRemoteTopology(cmd.Context()) + if !topology.hasAmbiguousDestination() { + return + } + fmt.Fprintln(w, "Checkpoint destination: REVIEW") + topology.describeCheckpointDestination(w, " ") + fmt.Fprintln(w) +} + // checkClaudeCodeHookDrift warns when Entire's Claude Code hooks are installed // but out of date — e.g. an older release wrote tool matchers that no longer // fire on current Claude Code. Read-only; the fix is `entire enable --force`. diff --git a/cmd/entire/cli/gitremote/gitremote.go b/cmd/entire/cli/gitremote/gitremote.go index c731786926..d345cebd24 100644 --- a/cmd/entire/cli/gitremote/gitremote.go +++ b/cmd/entire/cli/gitremote/gitremote.go @@ -107,6 +107,34 @@ func GetRemoteURLInDir(ctx context.Context, dir, remoteName string) (string, err return strings.TrimSpace(string(output)), nil } +// GetPushURLs returns every URL a push to remoteName delivers to, in the order +// git will use them. +// +// A remote's push destinations are remote..pushurl when any is set and its +// remote..url otherwise (git's push_url_of_remote), and BOTH may repeat — +// git pushes to all of them, in config order. So this, not GetRemoteURL, +// describes where a push actually goes; GetRemoteURL reports the FETCH URL, +// which can name a different repository entirely. +// +// Returns at least one entry on success. +func GetPushURLs(ctx context.Context, remoteName string) ([]string, error) { + cmd := exec.CommandContext(ctx, "git", "remote", "get-url", "--push", "--all", remoteName) + output, err := cmd.Output() + if err != nil { + return nil, fmt.Errorf("remote %q not found", remoteName) + } + var urls []string + for _, line := range strings.Split(string(output), "\n") { + if trimmed := strings.TrimSpace(line); trimmed != "" { + urls = append(urls, trimmed) + } + } + if len(urls) == 0 { + return nil, fmt.Errorf("remote %q has no push URL", remoteName) + } + return urls, nil +} + // ParseURL parses a git remote URL (SSH SCP-style or HTTPS) into its components. func ParseURL(rawURL string) (*Info, error) { rawURL = strings.TrimSpace(rawURL) diff --git a/cmd/entire/cli/integration_test/multi_pushurl.go b/cmd/entire/cli/integration_test/multi_pushurl.go index 8eda10b5a3..bb7d16b433 100644 --- a/cmd/entire/cli/integration_test/multi_pushurl.go +++ b/cmd/entire/cli/integration_test/multi_pushurl.go @@ -126,6 +126,32 @@ func (env *TestEnv) GitPushWithHooksAllowError(remote, refSpec string) error { return err //nolint:wrapcheck // test helper: the caller asserts on presence/absence, not identity } +// AddUnreachableFirstPushURL configures remoteName to push to a nonexistent path +// FIRST and its original URL second. git iterates push URLs in order and a +// transport failure is fatal (it die()s rather than returning), so nothing after +// the bad URL is attempted — unlike a ref rejection, which lets git carry on. +// Returns the unreachable path. +func (env *TestEnv) AddUnreachableFirstPushURL(remoteName string) string { + env.T.Helper() + + ctx := env.T.Context() + missing := filepath.Join(env.T.TempDir(), "does-not-exist.git") + originalURL := env.RemoteURL(remoteName) + + for _, url := range []string{missing, originalURL} { + cmd := exec.CommandContext(ctx, "git", "remote", "set-url", "--add", "--push", remoteName, url) + cmd.Dir = env.RepoDir + cmd.Env = testutil.GitIsolatedEnv() + if output, err := cmd.CombinedOutput(); err != nil { + env.T.Fatalf("failed to add push URL %s to remote %s: %v\n%s", url, remoteName, err, output) + } + } + + env.setGitConfigBaseline() + + return missing +} + // RemoteURL returns the fetch URL configured for remoteName. func (env *TestEnv) RemoteURL(remoteName string) string { env.T.Helper() diff --git a/cmd/entire/cli/integration_test/multi_pushurl_test.go b/cmd/entire/cli/integration_test/multi_pushurl_test.go index c21bd1a639..61e6331c98 100644 --- a/cmd/entire/cli/integration_test/multi_pushurl_test.go +++ b/cmd/entire/cli/integration_test/multi_pushurl_test.go @@ -4,6 +4,7 @@ package integration import ( "slices" + "strings" "testing" "github.com/entireio/cli/cmd/entire/cli/paths" @@ -246,17 +247,22 @@ func TestMultiPushURL_Branch_DoesNotPublishV1ToEmptySecondPushURL(t *testing.T) } } -// TestMultiPushURL_Refs_FanOutToBothPushURLs is the git-refs baseline, and it -// pins down a property the backend gets for free but does not enforce: the queue -// is drained by the FIRST hook invocation and its refs removed after that push -// succeeds, so the remaining invocations are no-ops. Both URLs still receive the -// refs — again only because the CLI pushes to the remote name and git fans out. +// TestMultiPushURL_Refs_GoesToFirstPushURLOnly pins the git-refs destination +// rule: with several push URLs on one remote, checkpoint refs go to the FIRST +// push URL and the rest are deliberately skipped. // -// This is worth locking down because it is exactly what would break if -// checkpoint pushes were ever retargeted at a single resolved URL (as a -// configured checkpoint_remote already does): the queue would be emptied by the -// first invocation and the second URL would silently never receive anything. -func TestMultiPushURL_Refs_FanOutToBothPushURLs(t *testing.T) { +// The push-discovery queue records only a ref, with no per-destination state, so +// "this ref is pushed" has to mean one place. git's fan-out cannot provide that: +// one failing URL fails the whole invocation so nothing unqueues even when other +// URLs took the refs, and an unreachable FIRST URL makes git die() before +// reaching any later URL (see ..._Refs_UnreachableFirstPushURL_ReachesNothing). +// +// The trade — checkpoints live in exactly one repository, and cloning a different +// mirror of the same code will not find them — is why the user is warned on +// stderr, and why checkpoint_remote remains the way to name the repository +// explicitly. Note the git-branch backend deliberately still fans out; see +// ..._Branch_RepeatedPushesKeepBothURLsInSync. +func TestMultiPushURL_Refs_GoesToFirstPushURLOnly(t *testing.T) { t.Parallel() env := NewFeatureBranchEnv(t) @@ -278,29 +284,65 @@ func TestMultiPushURL_Refs_FanOutToBothPushURLs(t *testing.T) { if !env.CheckpointExistsOnRemote(bareA, checkpointID) { t.Errorf("checkpoint ref for %s should be on the first push URL", checkpointID) } - if !env.CheckpointExistsOnRemote(bareB, checkpointID) { - t.Errorf("checkpoint ref for %s should be on the second push URL (git fans out to every push URL)", checkpointID) + if env.CheckpointExistsOnRemote(bareB, checkpointID) { + t.Errorf("checkpoint ref for %s should NOT be on the second push URL: refs target the first push URL only", checkpointID) } + // The destination took them, so they unqueue — the property the whole rule + // exists to make possible. if queued := env.QueuedCheckpointRefs(); len(queued) != 0 { t.Errorf("queue should be empty after a confirmed push, still holds %v", queued) } } -// TestMultiPushURL_Refs_UnreachableSecondPushURL_RefsStayQueued covers the -// git-refs failure mode. Per-checkpoint refs normally only fast-forward (each -// write parents on the prior tip) and a checkpoint ID is minted once, so the -// same-ref divergence that plagues the shared v1 branch is not reachable here -// without a backfill or migration rewriting an existing checkpoint. The -// realistic partial failure is instead a mirror that cannot be reached at all. +// TestMultiPushURL_Refs_UnreachableFirstPushURL_ReachesNothing covers the case +// that motivated targeting one URL explicitly rather than leaning on git's +// fan-out. +// +// git iterates a remote's push URLs in order, and a transport failure (missing +// repo, auth, bad host) is fatal — it die()s rather than returning, so URLs after +// the failing one are never attempted at all. A rejection (non-fast-forward) by +// contrast returns, and git carries on to the later URLs. So under fan-out a dead +// mirror in FIRST position blocks checkpoint sync completely, while the same +// mirror in last position does not: order silently decided the outcome. +// +// Targeting the first push URL makes that explicit instead of emergent, and the +// refs stay queued either way, so nothing is lost. +func TestMultiPushURL_Refs_UnreachableFirstPushURL_ReachesNothing(t *testing.T) { + t.Parallel() + + env := NewFeatureBranchEnv(t) + env.CheckpointStore = StoreGitRefs + + bareA := env.SetupBareRemote() + env.AddUnreachableFirstPushURL("origin") + + checkpointID := createCheckpointedCommit(t, env, "Add auth module", "auth.go", "package auth", "Add auth module") + if len(env.QueuedCheckpointRefs()) == 0 { + t.Fatal("setup: the checkpoint write should have enqueued a ref for push") + } + + if err := env.GitPushWithHooksAllowError("origin", "HEAD"); err == nil { + t.Fatal("setup: git push should fail when the first push URL is unreachable") + } + + if env.CheckpointsPresentOnRemote(bareA) { + t.Errorf("no checkpoint should reach the reachable URL: the unreachable first push URL is the target") + } + wantRef := checkpointRefName(checkpointID) + if queued := env.QueuedCheckpointRefs(); !slices.Contains(queued, wantRef) { + t.Errorf("ref %s should stay queued when the destination is unreachable; queue holds %v", wantRef, queued) + } +} + +// TestMultiPushURL_Refs_UnreachableSecondPushURL_IsIgnored is the counterpart: +// a broken mirror in a LATER position is now simply irrelevant to checkpoint +// refs, because they target the first push URL only. // -// The refs still land on the reachable URL, and — the property that matters — -// they stay queued, so every later push retries them. That is the backend -// degrading toward "will retry" rather than toward silent loss, and it is the -// concrete behavioral difference from the git-branch path above, which drops the -// failure on the floor. The flip side is that the retry can never succeed while -// the second URL is unreachable, so the queue never drains: a persistent -// partial failure is invisible outside stderr. -func TestMultiPushURL_Refs_UnreachableSecondPushURL_RefsStayQueued(t *testing.T) { +// Under git's fan-out this same topology failed the whole push and wedged the +// queue indefinitely — the refs had reached the healthy URL but nothing could +// unqueue them, so every later push retried and failed again. Targeting one URL +// removes that failure mode entirely. +func TestMultiPushURL_Refs_UnreachableSecondPushURL_IsIgnored(t *testing.T) { t.Parallel() env := NewFeatureBranchEnv(t) @@ -310,23 +352,76 @@ func TestMultiPushURL_Refs_UnreachableSecondPushURL_RefsStayQueued(t *testing.T) env.AddUnreachableSecondPushURL("origin") checkpointID := createCheckpointedCommit(t, env, "Add auth module", "auth.go", "package auth", "Add auth module") - queuedBefore := env.QueuedCheckpointRefs() - if len(queuedBefore) == 0 { + if len(env.QueuedCheckpointRefs()) == 0 { t.Fatal("setup: the checkpoint write should have enqueued a ref for push") } - // git exits non-zero because one push URL is unreachable; the user's push - // fails as a whole, which is git's behavior and not something the CLI can - // or should mask. + // The user's own branch push still fails — that is git's fan-out over a dead + // URL and not something the CLI can or should mask. if err := env.GitPushWithHooksAllowError("origin", "HEAD"); err == nil { t.Fatal("setup: git push should fail when one of the push URLs is unreachable") } if !env.CheckpointExistsOnRemote(bareA, checkpointID) { - t.Errorf("checkpoint ref for %s should still reach the reachable push URL", checkpointID) + t.Errorf("checkpoint ref for %s should reach the first push URL", checkpointID) } - wantRef := checkpointRefName(checkpointID) - if queued := env.QueuedCheckpointRefs(); !slices.Contains(queued, wantRef) { - t.Errorf("ref %s should stay queued after a partially failed push so the next push retries it; queue holds %v", wantRef, queued) + if queued := env.QueuedCheckpointRefs(); len(queued) != 0 { + t.Errorf("refs should unqueue once the destination took them, even though a later push URL is dead; queue holds %v", queued) } } + +// TestMultiPushURL_DestinationNoteSurfaces checks that the ambiguity is +// announced rather than left for a reader of the source: `entire doctor` reports +// it, and it stays silent on an ordinary single-destination repo so the common +// output is unchanged. +func TestMultiPushURL_DestinationNoteSurfaces(t *testing.T) { + t.Parallel() + + t.Run("silent with one remote and one push URL", func(t *testing.T) { + t.Parallel() + env := NewFeatureBranchEnv(t) + env.CheckpointStore = StoreGitRefs + env.SetupBareRemote() + + out := env.RunCLI("doctor") + if strings.Contains(out, "Checkpoint destination") { + t.Errorf("doctor should not mention the checkpoint destination for an unambiguous repo:\n%s", out) + } + }) + + t.Run("doctor reports a multi-push-URL remote", func(t *testing.T) { + t.Parallel() + env := NewFeatureBranchEnv(t) + env.CheckpointStore = StoreGitRefs + env.SetupBareRemote() + env.AddSecondPushURL("origin") + + out := env.RunCLI("doctor") + for _, want := range []string{ + "Checkpoint destination", + "pushes to 2 URLs", + "first URL only", + "checkpoint_remote", + } { + if !strings.Contains(out, want) { + t.Errorf("doctor output should mention %q:\n%s", want, out) + } + } + }) + + t.Run("doctor reports several remotes", func(t *testing.T) { + t.Parallel() + env := NewFeatureBranchEnv(t) + env.CheckpointStore = StoreGitRefs + env.SetupBareRemote() + env.SetupNamedBareRemote("backup") + + out := env.RunCLI("doctor") + if !strings.Contains(out, "2 remotes") { + t.Errorf("doctor output should mention the extra remote:\n%s", out) + } + if !strings.Contains(out, "always looks at origin") { + t.Errorf("doctor output should explain that reads resolve via origin:\n%s", out) + } + }) +} diff --git a/cmd/entire/cli/remote_topology.go b/cmd/entire/cli/remote_topology.go new file mode 100644 index 0000000000..2f280097ac --- /dev/null +++ b/cmd/entire/cli/remote_topology.go @@ -0,0 +1,157 @@ +package cli + +import ( + "context" + "fmt" + "io" + "log/slog" + "sort" + "strings" + + "github.com/entireio/cli/cmd/entire/cli/checkpoint" + "github.com/entireio/cli/cmd/entire/cli/gitremote" + "github.com/entireio/cli/cmd/entire/cli/logging" + "github.com/entireio/cli/cmd/entire/cli/paths" + "github.com/entireio/cli/cmd/entire/cli/settings" +) + +// Checkpoint destinations are unambiguous in the ordinary single-remote, +// single-URL repo and stop being unambiguous in two topologies users set up +// deliberately. Neither is broken, but in both the destination is decided by +// something other than "the repo I work in", so it is worth saying out loud once +// at `entire enable` and on demand from `entire doctor` rather than letting +// someone discover it when a resume comes up empty. + +// remoteTopology summarizes the checkpoint-destination ambiguity in this repo. +type remoteTopology struct { + // Remotes is every configured remote name, sorted. + Remotes []string + // MultiURLRemote names a remote that pushes to more than one URL ("" if + // none), and PushURLs are its push URLs in the order git uses them. + MultiURLRemote string + PushURLs []string + // PrimaryIsRefs reports whether the git-refs backend is active, which + // decides what a multi-URL remote means for checkpoints. + PrimaryIsRefs bool + // CheckpointRemote is the configured checkpoint_remote repo ("" if none). + // When set, it already pins one explicit destination and there is nothing + // ambiguous left to report. + CheckpointRemote string +} + +// inspectRemoteTopology reads the repo's remotes and checkpoint configuration. +// Best-effort and local-only (no network): every failure yields an empty +// topology, which reports nothing, because this is advisory output and must +// never obstruct enable or doctor. +func inspectRemoteTopology(ctx context.Context) remoteTopology { + var t remoteTopology + + repoRoot, err := paths.WorktreeRoot(ctx) + if err != nil { + return t + } + + remotes, err := listGitRemotes(ctx, repoRoot) + if err != nil { + logging.Debug(ctx, "remote topology: could not list remotes", slog.String("error", err.Error())) + return t + } + for name := range remotes { + t.Remotes = append(t.Remotes, name) + } + sort.Strings(t.Remotes) + + if s, err := settings.Load(ctx); err == nil { + if cfg := s.GetCheckpointRemote(); cfg != nil { + t.CheckpointRemote = cfg.Repo + } + } + + if cpCfg, err := settings.LoadCheckpointsConfig(ctx); err == nil { + t.PrimaryIsRefs = checkpoint.PrimaryIsRefs(cpCfg) + } + + // Report the first remote (in sorted order) that fans out, so the message is + // stable rather than dependent on map iteration. + for _, name := range t.Remotes { + urls, err := gitremote.GetPushURLs(ctx, name) + if err != nil || len(urls) < 2 { + continue + } + t.MultiURLRemote = name + t.PushURLs = urls + break + } + + return t +} + +// hasAmbiguousDestination reports whether anything is worth telling the user. +func (t remoteTopology) hasAmbiguousDestination() bool { + if t.CheckpointRemote != "" { + return false + } + return len(t.Remotes) > 1 || t.MultiURLRemote != "" +} + +// describeCheckpointDestination writes an explanation of where checkpoints will +// go. Writes nothing when the destination is unambiguous. indent prefixes every +// line so callers can nest it under a heading. +func (t remoteTopology) describeCheckpointDestination(w io.Writer, indent string) { + if !t.hasAmbiguousDestination() { + return + } + + if t.MultiURLRemote != "" { + fmt.Fprintf(w, "%sRemote %q pushes to %d URLs:\n", indent, t.MultiURLRemote, len(t.PushURLs)) + for i, u := range t.PushURLs { + marker := " " + if i == 0 && t.PrimaryIsRefs { + marker = "→ " + } + fmt.Fprintf(w, "%s %s%s\n", indent, marker, redactForDisplay(u)) + } + if t.PrimaryIsRefs { + fmt.Fprintf(w, "%s Checkpoints go to the first URL only; the others receive your code but no\n", indent) + fmt.Fprintf(w, "%s session history. Clone that first repository to resume sessions elsewhere.\n", indent) + } else { + fmt.Fprintf(w, "%s Checkpoints are pushed to every URL. If one of them rejects or is\n", indent) + fmt.Fprintf(w, "%s unreachable, checkpoint sync reports a warning and retries on the next push.\n", indent) + } + } + + if len(t.Remotes) > 1 { + fmt.Fprintf(w, "%sThis repo has %d remotes (%s).\n", indent, len(t.Remotes), strings.Join(t.Remotes, ", ")) + fmt.Fprintf(w, "%s Checkpoints follow whichever remote you push to, while reading them back\n", indent) + fmt.Fprintf(w, "%s (resume, explain) always looks at origin — so checkpoints pushed elsewhere\n", indent) + fmt.Fprintf(w, "%s are not found again from this clone.\n", indent) + } + + fmt.Fprintf(w, "%sTo pin one repository for checkpoints, set checkpoint_remote in .entire/settings.json\n", indent) + fmt.Fprintf(w, "%s(or .entire/settings.local.json to keep it to this clone).\n", indent) +} + +// printCheckpointDestinationNote is the `entire enable` half of the explanation +// `entire doctor` prints (checkCheckpointDestination): enable is where a user is +// most likely to be looking, and the least surprising moment to learn that this +// repo's remotes make the checkpoint destination a choice. Silent on the +// ordinary repo, so it adds nothing to the common enable output. +func printCheckpointDestinationNote(ctx context.Context, w io.Writer) { + topology := inspectRemoteTopology(ctx) + if !topology.hasAmbiguousDestination() { + return + } + fmt.Fprintln(w) + fmt.Fprintln(w, "Note: this repo's remotes make the checkpoint destination ambiguous.") + topology.describeCheckpointDestination(w, " ") +} + +// redactForDisplay strips credentials from URL-shaped values and passes +// filesystem paths through unchanged (RedactURL renders a plain path as +// ":///path"). +func redactForDisplay(u string) string { + if strings.Contains(u, "://") || strings.Contains(u, "@") { + return gitremote.RedactURL(u) + } + return u +} diff --git a/cmd/entire/cli/setup.go b/cmd/entire/cli/setup.go index b3220d9f06..3e76570150 100644 --- a/cmd/entire/cli/setup.go +++ b/cmd/entire/cli/setup.go @@ -1341,6 +1341,8 @@ func runEnableInteractive(ctx context.Context, w io.Writer, agents []agent.Agent } } + printCheckpointDestinationNote(ctx, w) + return nil } @@ -1350,6 +1352,7 @@ func printEnabledStatus(ctx context.Context, w io.Writer) { fmt.Fprintf(w, "Agents: %s\n", strings.Join(displayNames, ", ")) } fmt.Fprintln(w, "\nTo add more agents, run `entire agent add `.") + printCheckpointDestinationNote(ctx, w) } // resolveFirstRunCheckpointBackend decides the checkpoint storage backend diff --git a/cmd/entire/cli/strategy/manual_commit_push.go b/cmd/entire/cli/strategy/manual_commit_push.go index 5ab8070ca6..57ba63ed02 100644 --- a/cmd/entire/cli/strategy/manual_commit_push.go +++ b/cmd/entire/cli/strategy/manual_commit_push.go @@ -278,7 +278,10 @@ func (s *ManualCommitStrategy) prePushCheckpointRefs(ctx context.Context, ps pus return nil } - if _, err := flushCheckpointRefsQueue(ctx, repo, ps.pushTarget()); err != nil { + dest := resolveRefsPushDestination(ctx, ps) + dest.warnIgnoredPushURLs(ctx, os.Stderr) + + if _, err := flushCheckpointRefsQueue(ctx, repo, dest); err != nil { // Fail-soft: a checkpoint-ref push failure must never block the user's // git push. The refs stay queued for the next pre-push. logging.Warn(ctx, "git-refs pre-push: checkpoint ref push failed; refs left queued", @@ -306,7 +309,9 @@ func PushQueuedCheckpointRefs(ctx context.Context, repo *git.Repository, remote if !checkpointPolicyAllowsGitHook(ctx, repo) { return 0, false, errors.New("checkpoint policy does not allow pushing checkpoint refs; refs stay queued") } - pushed, err = flushCheckpointRefsQueue(ctx, repo, ps.pushTarget()) + dest := resolveRefsPushDestination(ctx, ps) + dest.warnIgnoredPushURLs(ctx, os.Stderr) + pushed, err = flushCheckpointRefsQueue(ctx, repo, dest) // Clean up even on a partial/failed flush: a diverged batch can push some // refs and still return an error, and the shadow branches for the refs that // *did* land must still be cleaned up — parity with the pre-push path, which @@ -323,7 +328,8 @@ func PushQueuedCheckpointRefs(ctx context.Context, repo *git.Repository, remote // never block the user's push) and the migration command's opt-in push (which // surfaces it). Stale entries — refs no longer present locally — are pruned so // they don't block the queue forever. -func flushCheckpointRefsQueue(ctx context.Context, repo *git.Repository, pushTarget string) (int, error) { +func flushCheckpointRefsQueue(ctx context.Context, repo *git.Repository, dest refsPushDestination) (int, error) { + pushTarget := dest.target queue, err := checkpoint.PushQueueForRepo(ctx, repo) if err != nil { return 0, fmt.Errorf("resolve push queue: %w", err) @@ -354,8 +360,7 @@ func flushCheckpointRefsQueue(ctx context.Context, repo *git.Repository, pushTar // surface it (matching the v1 path's "[entire] Pushing ..." line) instead of // leaving the user's git push apparently hung. Written to stderr, which git // shows during the pre-push hook. - displayTarget := displayPushTarget(pushTarget) - fmt.Fprintf(os.Stderr, "[entire] Pushing %d checkpoint ref(s) to %s...", len(existing), displayTarget) + fmt.Fprintf(os.Stderr, "[entire] Pushing %d checkpoint ref(s) to %s...", len(existing), dest.display) stop := startProgressDots(os.Stderr) // Fast path: push all refs in one round-trip (fast-forward-only). If every @@ -377,7 +382,9 @@ func flushCheckpointRefsQueue(ctx context.Context, repo *git.Repository, pushTar if nonInteractiveSSHAuthFailure(pushCtx, batchErr) { fmt.Fprintf(os.Stderr, "[entire] Warning: couldn't push checkpoint refs: %v\n", batchErr) printNonInteractiveSSHAuthHint() - printCheckpointRemoteHint(pushTarget) + if dest.checkpointRemote { + printCheckpointRemoteHint(pushTarget) + } return 0, batchErr } @@ -386,7 +393,10 @@ func flushCheckpointRefsQueue(ctx context.Context, repo *git.Repository, pushTar // fetch+replay recovery, and remove from the queue only the refs that land // (a genuine cherry-pick conflict leaves that ref queued for a later push, // never force-overwriting the remote). - fmt.Fprintf(os.Stderr, "[entire] Some checkpoint refs diverged; syncing %d ref(s) individually...", len(existing)) + // Deliberately does not claim divergence: a rejected batch is just as often + // an unreachable or unauthorized destination, and telling a user with a dead + // remote that their refs "diverged" sends them after the wrong problem. + fmt.Fprintf(os.Stderr, "[entire] Checkpoint ref push was rejected; retrying %d ref(s) individually...", len(existing)) stop = startProgressDots(os.Stderr) pushed := make([]plumbing.ReferenceName, 0, len(existing)) var firstErr error diff --git a/cmd/entire/cli/strategy/refs_push_destination.go b/cmd/entire/cli/strategy/refs_push_destination.go new file mode 100644 index 0000000000..6eeb409536 --- /dev/null +++ b/cmd/entire/cli/strategy/refs_push_destination.go @@ -0,0 +1,124 @@ +package strategy + +import ( + "context" + "fmt" + "io" + "log/slog" + + "github.com/entireio/cli/cmd/entire/cli/checkpoint/remote" + "github.com/entireio/cli/cmd/entire/cli/logging" +) + +// refsPushDestination is where checkpoint refs are pushed, plus how to name that +// place in user-facing output. +type refsPushDestination struct { + // target is passed to git push / the recovery fetch: a remote name, or a URL. + target string + // display names the destination in progress and warning output. + display string + // checkpointRemote is true when target came from a configured + // checkpoint_remote, which gates the "a checkpoint remote is configured" + // hint — that hint must not fire for a URL we picked ourselves. + checkpointRemote bool + // ignoredPushURLs counts push URLs of a multi-URL remote that will NOT + // receive checkpoint refs. Zero in every single-destination topology. + ignoredPushURLs int +} + +// resolveRefsPushDestination picks the single destination for checkpoint-ref +// pushes. +// +// Checkpoint refs need ONE deterministic destination, because the push-discovery +// queue records only a ref (`{"ref": …}`) with no per-destination state: a ref is +// removed from the queue once "the push" succeeds, so "the push" has to mean one +// place. Relying on git's fan-out across a remote's several push URLs breaks that +// in both directions — a single failing URL fails the whole invocation and no ref +// unqueues even though some URLs took it, and an unreachable FIRST URL makes git +// die() before it reaches any later URL at all. +// +// So when a remote carries more than one push URL we target its first push URL +// directly (the one git itself would push to first) and ignore the rest. The +// recovery fetch in fetchAndRebaseRefCommon uses the same target, so — unlike the +// fan-out path, which reconciled the remote's FETCH url while pushing to its +// pushurls — the URL we reconcile is finally the URL we push to. +// +// Consequences, deliberately accepted: +// - Checkpoint refs live in exactly one repository. Cloning that repository +// resolves them (its url becomes the clone's fetch URL); cloning a different +// mirror of the same code does not. Mirroring checkpoints to several +// repositories is what checkpoint_remote is for. +// - A first push URL that REJECTS a ref (non-fast-forward) no longer lets later +// URLs receive it. git would have carried on to them; we stop. That is the +// price of a deterministic destination, and it is the case the queue can +// actually reason about. +// +// The git-branch backend deliberately keeps git's fan-out: its v1 branch is a +// single shared ref with no queue to keep coherent, and mirroring it to every +// push URL is behavior users configure their remotes for. +// +// A single push URL keeps the remote NAME as the target rather than resolving it +// to a URL, so the overwhelmingly common topology behaves exactly as before — +// remote-tracking refs still update, output still says "origin", and no +// URL-keyed promisor config appears. +func resolveRefsPushDestination(ctx context.Context, ps pushSettings) refsPushDestination { + target := ps.pushTarget() + + // A configured checkpoint_remote is already a single explicit destination. + if ps.hasCheckpointURL() { + return refsPushDestination{target: target, display: displayPushTarget(target), checkpointRemote: true} + } + + // Pushing straight to a URL (git passes a bare URL through to the hook) is + // likewise already single-destination. + if remote.IsURL(target) { + return refsPushDestination{target: target, display: displayPushTarget(target)} + } + + urls, err := remote.GetPushURLs(ctx, target) + if err != nil { + // Not a configured remote, or git could not report its URLs. Keep the + // target as given; the push itself will report any real problem. + logging.Debug(ctx, "git-refs push: could not enumerate push URLs; using target as given", + slog.String("target", target), + slog.String("error", err.Error()), + ) + return refsPushDestination{target: target, display: target} + } + if len(urls) < 2 { + return refsPushDestination{target: target, display: target} + } + + return refsPushDestination{ + target: urls[0], + display: fmt.Sprintf("%s (first of %d push URLs)", displayURL(urls[0]), len(urls)), + ignoredPushURLs: len(urls) - 1, + } +} + +// displayURL renders a push destination for humans with any credentials removed. +// RedactURL is only safe for URL-shaped values: given a plain filesystem path it +// parses to an empty scheme and host and renders as ":///path/to/repo". Paths +// carry no credentials, so pass them through unchanged. +func displayURL(u string) string { + if remote.IsURL(u) { + return remote.RedactURL(u) + } + return u +} + +// warnIgnoredPushURLs tells the user, once per push, that checkpoint refs are +// going to one URL of a multi-URL remote — otherwise the choice is invisible and +// looks like the other mirrors silently lost their checkpoints. +func (d refsPushDestination) warnIgnoredPushURLs(ctx context.Context, errOut io.Writer) { + if d.ignoredPushURLs == 0 { + return + } + fmt.Fprintf(errOut, "[entire] Checkpoints go to one repository: %s. %d other push URL(s) of this remote will not receive them.\n", + d.display, d.ignoredPushURLs) + fmt.Fprintln(errOut, "[entire] To store checkpoints in a specific repository instead, set checkpoint_remote in .entire/settings.json.") + logging.Info(ctx, "git-refs push: multi-URL remote, pushing checkpoint refs to the first push URL only", + slog.String("target", displayURL(d.target)), + slog.Int("ignored_push_urls", d.ignoredPushURLs), + ) +} From 9b02ce10490595383218a5b3462842220eff1281 Mon Sep 17 00:00:00 2001 From: Stefan Haubold Date: Wed, 5 Aug 2026 18:38:43 +0200 Subject: [PATCH 2/3] refactor: address simplify review of the refs push destination MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cleanups from a four-angle review (reuse, simplification, efficiency, altitude). No intended behavior change except where noted. Efficiency — the one that mattered: - resolveRefsPushDestination (and its `git remote get-url --push --all` spawn) ran before flushCheckpointRefsQueue's early returns, so EVERY push on the git-refs default backend paid a fork/exec for a destination that goes unused when nothing is queued — and multi-URL users got the "checkpoints go to one repository" warning on no-op pushes. It now resolves after both early returns, inside the existing push_checkpoint_refs perf span. Behavior change, and the intended one: the warning no longer fires on a push with an empty queue. - inspectRemoteTopology replaced `git remote` + one `get-url --push --all` per remote (N+1 spawns) with a single `git remote -v`, which already applies git's pushurl-replaces-url rule and lists push URLs in order. Also removes an inconsistency where remote listing ran in repoRoot while the URL lookups inherited process CWD. Correctness of what we tell the user: - inspectRemoteTopology treated a checkpoint_remote *present in settings* as a pinned destination and went silent. But PushURL only honors it when it resolves — owner mismatch, unparseable URL and non-derivable protocol all fall back — so doctor stayed quiet while pushes really did go first-URL-only. It now asks remote.PushURL per remote, the "ask the client, don't re-resolve" rule CLAUDE.md documents for CoreOrigin(). - It also reported the first *sorted* remote that fans out, so a repo with a fanning-out `backup` remote that only ever pushes to origin got warned about a rule that never applied to it. Ambiguity is now per remote, and each is named. - The git-branch explanation described the git-refs failure behavior ("retries on the next push"). Corrected: those URLs are reported and left behind, and only the fetch URL is ever reconciled. - refsPushDestination.display was set from displayPushTarget, which maps ANY URL to the literal words "checkpoint remote" — so a self-picked push URL claimed to be a checkpoint remote while the struct's own checkpointRemote said false. display is now a method that names a resolved URL by its redacted URL. Reuse and simplification: - "redact if URL-shaped, else pass the path through" existed in three copies (two added by the previous commit, one pre-existing in redactGitArgs). Now one gitremote.RedactURLOrPath, forwarded through the checkpoint/remote facade. - refsPushDestination drops its `display` field (a method), and the resolver's five branches collapse to three. - checkCheckpointDestination and printCheckpointDestinationNote were the same four-step body; one function takes the header. `indent` was speculative generality (one value at both call sites) and is gone, as is the third redundant ambiguity check. - warnIgnoredPushURLs uses the package's injectable stderrWriter instead of an io.Writer parameter both call sites hardcoded. - Dropped the `pushTarget := dest.target` alias. - Test helpers: one setPushURLs(remote, urls...) primitive under all three push-URL helpers, with position as the parameter — AddUnreachableFirstPushURL was a verbatim copy of the Second variant with the slice order flipped. The three doctor subtests became a table. Stale rationale the previous commit invalidated: both file headers still said the CLI "has no per-URL control at all", true only for git-branch now. Both carve out git-refs. Also fixed while in the file: checkCodexHookTrust's doc comment has been detached from its function since before this branch (it sat above checkClaudeCodeHookDrift); moved down to the function it documents. Co-Authored-By: Claude Opus 5 (1M context) Entire-Checkpoint: 01KZ9CNB8FYGCX05XV4V0D8NNX --- cmd/entire/cli/checkpoint/remote/util.go | 6 + cmd/entire/cli/doctor.go | 45 ++-- cmd/entire/cli/gitremote/gitremote.go | 16 ++ .../cli/integration_test/multi_pushurl.go | 84 +++---- .../integration_test/multi_pushurl_test.go | 96 ++++---- cmd/entire/cli/remote_topology.go | 208 +++++++++++------- cmd/entire/cli/repo_mirror_use.go | 10 +- cmd/entire/cli/setup.go | 4 +- cmd/entire/cli/strategy/manual_commit_push.go | 26 +-- .../cli/strategy/refs_push_destination.go | 83 ++++--- 10 files changed, 299 insertions(+), 279 deletions(-) diff --git a/cmd/entire/cli/checkpoint/remote/util.go b/cmd/entire/cli/checkpoint/remote/util.go index a1d4d530d6..84c6249b11 100644 --- a/cmd/entire/cli/checkpoint/remote/util.go +++ b/cmd/entire/cli/checkpoint/remote/util.go @@ -492,6 +492,12 @@ func RedactURL(rawURL string) string { return gitremote.RedactURL(rawURL) } +// RedactURLOrPath is RedactURL for values that may be a remote name or a local +// path rather than a URL. See gitremote.RedactURLOrPath. +func RedactURLOrPath(target string) string { + return gitremote.RedactURLOrPath(target) +} + func logFallback(ctx context.Context, operation, fallbackURL, reason string, err error, attrs ...any) { logAttrs := []any{ slog.String("operation", operation), diff --git a/cmd/entire/cli/doctor.go b/cmd/entire/cli/doctor.go index 2c380d50ff..540a1517a0 100644 --- a/cmd/entire/cli/doctor.go +++ b/cmd/entire/cli/doctor.go @@ -109,7 +109,7 @@ func runSessionsFix(cmd *cobra.Command, force bool) error { checkClaudeCodeHookDrift(cmd) // Where checkpoints land, when the repo's remotes make that ambiguous. - checkCheckpointDestination(cmd) + printCheckpointDestinationNote(ctx, cmd.OutOrStdout(), "Checkpoint destination: REVIEW") // Stuck sessions // Load all session states @@ -435,35 +435,6 @@ func confirmDoctorFix(ctx context.Context, w io.Writer, title string) (bool, err return confirmed, nil } -// checkCodexHookTrust warns about two kinds of drift in the Codex hook -// setup: -// -// 1. .codex/hooks.json is stale relative to what the CLI installs -// today (e.g. a release added PostToolUse after the user enabled -// Codex). Fix: re-run `entire enable`. -// -// 2. A declared hook lacks a `trusted_hash` entry in the user's Codex -// config — either a fresh clone or a newer hook on the file the -// user hasn't approved yet. Fix: open /hooks in Codex. -// -// Both checks are structural (file/key presence). Stays silent when -// this repo doesn't have codex hooks installed or when we can't -// resolve the worktree root. Warn-only. -// checkCheckpointDestination reports where checkpoints will be pushed when the -// repo's remote layout makes that ambiguous — several remotes, or one remote -// fanning out to several push URLs. Silent on the ordinary single-destination -// repo and whenever checkpoint_remote already pins one explicitly. -func checkCheckpointDestination(cmd *cobra.Command) { - w := cmd.OutOrStdout() - topology := inspectRemoteTopology(cmd.Context()) - if !topology.hasAmbiguousDestination() { - return - } - fmt.Fprintln(w, "Checkpoint destination: REVIEW") - topology.describeCheckpointDestination(w, " ") - fmt.Fprintln(w) -} - // checkClaudeCodeHookDrift warns when Entire's Claude Code hooks are installed // but out of date — e.g. an older release wrote tool matchers that no longer // fire on current Claude Code. Read-only; the fix is `entire enable --force`. @@ -482,6 +453,20 @@ func checkClaudeCodeHookDrift(cmd *cobra.Command) { } } +// checkCodexHookTrust warns about two kinds of drift in the Codex hook +// setup: +// +// 1. .codex/hooks.json is stale relative to what the CLI installs +// today (e.g. a release added PostToolUse after the user enabled +// Codex). Fix: re-run `entire enable`. +// +// 2. A declared hook lacks a `trusted_hash` entry in the user's Codex +// config — either a fresh clone or a newer hook on the file the +// user hasn't approved yet. Fix: open /hooks in Codex. +// +// Both checks are structural (file/key presence). Stays silent when +// this repo doesn't have codex hooks installed or when we can't +// resolve the worktree root. Warn-only. func checkCodexHookTrust(cmd *cobra.Command) { repoRoot, err := paths.WorktreeRoot(cmd.Context()) if err != nil { diff --git a/cmd/entire/cli/gitremote/gitremote.go b/cmd/entire/cli/gitremote/gitremote.go index d345cebd24..6e30bdff90 100644 --- a/cmd/entire/cli/gitremote/gitremote.go +++ b/cmd/entire/cli/gitremote/gitremote.go @@ -210,6 +210,22 @@ func RedactURL(rawURL string) string { return u.Scheme + "://" + u.Host + u.Path } +// RedactURLOrPath renders a remote for display with any credentials removed, +// accepting values that are not URLs at all. +// +// RedactURL cannot be applied blanket-fashion: it round-trips through url.Parse +// and rebuilds "scheme://host/path", so a bare filesystem path like +// /srv/repo.git comes back as ":///srv/repo.git" and a bare word like "origin" +// as "://origin". Those inputs carry no credentials, so they pass through +// unchanged. Use this wherever the value may be a remote name, a local path, or +// a URL — i.e. anywhere a push/fetch target is shown to a user. +func RedactURLOrPath(remote string) string { + if strings.Contains(remote, "://") || strings.Contains(remote, "@") { + return RedactURL(remote) + } + return remote +} + // ResolveRemoteRepo returns the forge identifier, owner, and repo name for the // given git remote. The forge is the short id used by the trails API ("gh", // "et", ...); it is derived from the hostname for direct git URLs or from the diff --git a/cmd/entire/cli/integration_test/multi_pushurl.go b/cmd/entire/cli/integration_test/multi_pushurl.go index bb7d16b433..35e80c5473 100644 --- a/cmd/entire/cli/integration_test/multi_pushurl.go +++ b/cmd/entire/cli/integration_test/multi_pushurl.go @@ -18,8 +18,10 @@ import ( // every push URL and — this is the part that matters for checkpoint sync — // invokes the pre-push hook ONCE PER PUSH URL, passing the same remote NAME as // $1 each time and the individual URL as $2. Our installed hook forwards only -// $1 (see strategy/hooks.go), so the CLI cannot tell the invocations apart and -// hands `git push ` the remote name, letting git fan out again. +// $1 (see strategy/hooks.go), so the CLI cannot tell the invocations apart. +// git-branch then hands `git push ` the remote name and lets git fan out +// again; git-refs resolves the first push URL itself and targets only that (see +// strategy.resolveRefsPushDestination). // // See multi_pushurl_test.go for what that means per checkpoint backend. @@ -56,55 +58,38 @@ func (env *TestEnv) AddSecondPushURL(remoteName string) string { env.T.Fatalf("failed to init second bare repo: %v\n%s", err, output) } - originalURL := env.RemoteURL(remoteName) - - // Re-add the original URL as an explicit push URL first, then the new one, - // so push fans out to both in that order. - for _, url := range []string{originalURL, secondBare} { - cmd = exec.CommandContext(ctx, "git", "remote", "set-url", "--add", "--push", remoteName, url) - cmd.Dir = env.RepoDir - cmd.Env = testutil.GitIsolatedEnv() - if output, err := cmd.CombinedOutput(); err != nil { - env.T.Fatalf("failed to add push URL %s to remote %s: %v\n%s", url, remoteName, err, output) - } - } + // The original URL is re-added explicitly because configuring ANY pushurl + // replaces url for push purposes — adding only the new one would silently + // redirect pushes instead of fanning out. + env.setPushURLs(remoteName, env.RemoteURL(remoteName), secondBare) // Guard the setup itself: a helper that quietly configured one push URL - // would make every fan-out assertion below vacuous. + // would make every fan-out assertion vacuous. if got := env.PushURLs(remoteName); len(got) != 2 { env.T.Fatalf("expected 2 push URLs on remote %s, got %d: %v", remoteName, len(got), got) } - // Re-baseline the .git/config guard: adding push URLs is a deliberate - // change, so it must not read as unexpected drift at cleanup. - env.setGitConfigBaseline() - return secondBare } -// AddUnreachableSecondPushURL configures remoteName to fan out to its original -// URL plus a path that does not exist, so every push to that remote partially -// fails: the real URL receives the refs, the bogus one errors, and git exits -// non-zero. Models a mirror that is down or whose credentials have expired. -// Returns the unreachable path. +// AddUnreachableSecondPushURL configures remoteName to push to its original URL +// first and a path that does not exist second. Models a mirror that is down or +// whose credentials have expired, in the position where git still reaches the +// healthy URL before failing. Returns the unreachable path. func (env *TestEnv) AddUnreachableSecondPushURL(remoteName string) string { env.T.Helper() - - ctx := env.T.Context() missing := filepath.Join(env.T.TempDir(), "does-not-exist.git") - originalURL := env.RemoteURL(remoteName) - - for _, url := range []string{originalURL, missing} { - cmd := exec.CommandContext(ctx, "git", "remote", "set-url", "--add", "--push", remoteName, url) - cmd.Dir = env.RepoDir - cmd.Env = testutil.GitIsolatedEnv() - if output, err := cmd.CombinedOutput(); err != nil { - env.T.Fatalf("failed to add push URL %s to remote %s: %v\n%s", url, remoteName, err, output) - } - } - - env.setGitConfigBaseline() + env.setPushURLs(remoteName, env.RemoteURL(remoteName), missing) + return missing +} +// AddUnreachableFirstPushURL is AddUnreachableSecondPushURL with the unreachable +// path FIRST — the position that matters, because a transport failure makes git +// die() rather than return, so no later URL is attempted at all. +func (env *TestEnv) AddUnreachableFirstPushURL(remoteName string) string { + env.T.Helper() + missing := filepath.Join(env.T.TempDir(), "does-not-exist.git") + env.setPushURLs(remoteName, missing, env.RemoteURL(remoteName)) return missing } @@ -126,20 +111,17 @@ func (env *TestEnv) GitPushWithHooksAllowError(remote, refSpec string) error { return err //nolint:wrapcheck // test helper: the caller asserts on presence/absence, not identity } -// AddUnreachableFirstPushURL configures remoteName to push to a nonexistent path -// FIRST and its original URL second. git iterates push URLs in order and a -// transport failure is fatal (it die()s rather than returning), so nothing after -// the bad URL is attempted — unlike a ref rejection, which lets git carry on. -// Returns the unreachable path. -func (env *TestEnv) AddUnreachableFirstPushURL(remoteName string) string { +// setPushURLs appends push URLs to remoteName in the given order and +// re-baselines the .git/config guard (changing push URLs is deliberate here). +// +// Order is the parameter that matters: git iterates push URLs in config order, +// and a transport failure is fatal, so a broken URL first behaves differently +// from the same URL last. +func (env *TestEnv) setPushURLs(remoteName string, urls ...string) { env.T.Helper() - ctx := env.T.Context() - missing := filepath.Join(env.T.TempDir(), "does-not-exist.git") - originalURL := env.RemoteURL(remoteName) - - for _, url := range []string{missing, originalURL} { - cmd := exec.CommandContext(ctx, "git", "remote", "set-url", "--add", "--push", remoteName, url) + for _, url := range urls { + cmd := exec.CommandContext(env.T.Context(), "git", "remote", "set-url", "--add", "--push", remoteName, url) cmd.Dir = env.RepoDir cmd.Env = testutil.GitIsolatedEnv() if output, err := cmd.CombinedOutput(); err != nil { @@ -148,8 +130,6 @@ func (env *TestEnv) AddUnreachableFirstPushURL(remoteName string) string { } env.setGitConfigBaseline() - - return missing } // RemoteURL returns the fetch URL configured for remoteName. diff --git a/cmd/entire/cli/integration_test/multi_pushurl_test.go b/cmd/entire/cli/integration_test/multi_pushurl_test.go index 61e6331c98..10d6054c03 100644 --- a/cmd/entire/cli/integration_test/multi_pushurl_test.go +++ b/cmd/entire/cli/integration_test/multi_pushurl_test.go @@ -22,10 +22,11 @@ import ( // $1, so the CLI sees N identical invocations and hands `git push ` the // name — letting git fan out a second time. // -// The CLI therefore has no per-URL control at all: it can only decide whether to -// push checkpoints for a given hook invocation, not where they land. These tests -// pin down what that means for each backend, including the places where today's -// behavior is wrong. +// The two backends diverge from there, deliberately. git-branch pushes to the +// remote NAME and inherits git's fan-out, so it has no per-URL control — it can +// only decide *whether* to push for a given hook invocation, not where. git-refs +// resolves the first push URL itself and targets that one destination, because +// its push queue records a ref with no per-destination state. // // The backends are covered by separate tests rather than ForEachBackend because // the interesting failure modes differ: the git-branch v1 branch is a single @@ -377,51 +378,48 @@ func TestMultiPushURL_Refs_UnreachableSecondPushURL_IsIgnored(t *testing.T) { func TestMultiPushURL_DestinationNoteSurfaces(t *testing.T) { t.Parallel() - t.Run("silent with one remote and one push URL", func(t *testing.T) { - t.Parallel() - env := NewFeatureBranchEnv(t) - env.CheckpointStore = StoreGitRefs - env.SetupBareRemote() + tests := []struct { + name string + setup func(env *TestEnv) + want []string + absent []string + }{ + { + name: "one remote, one push URL", + setup: func(*TestEnv) {}, + absent: []string{"Checkpoint destination"}, + }, + { + name: "remote with several push URLs", + setup: func(env *TestEnv) { env.AddSecondPushURL("origin") }, + want: []string{"Checkpoint destination", "pushes to 2 URLs", "first URL only", "checkpoint_remote"}, + }, + { + name: "several remotes", + setup: func(env *TestEnv) { env.SetupNamedBareRemote("backup") }, + want: []string{"2 remotes", "always looks at origin"}, + }, + } - out := env.RunCLI("doctor") - if strings.Contains(out, "Checkpoint destination") { - t.Errorf("doctor should not mention the checkpoint destination for an unambiguous repo:\n%s", out) - } - }) - - t.Run("doctor reports a multi-push-URL remote", func(t *testing.T) { - t.Parallel() - env := NewFeatureBranchEnv(t) - env.CheckpointStore = StoreGitRefs - env.SetupBareRemote() - env.AddSecondPushURL("origin") - - out := env.RunCLI("doctor") - for _, want := range []string{ - "Checkpoint destination", - "pushes to 2 URLs", - "first URL only", - "checkpoint_remote", - } { - if !strings.Contains(out, want) { - t.Errorf("doctor output should mention %q:\n%s", want, out) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + env := NewFeatureBranchEnv(t) + env.CheckpointStore = StoreGitRefs + env.SetupBareRemote() + tt.setup(env) + + out := env.RunCLI("doctor") + for _, want := range tt.want { + if !strings.Contains(out, want) { + t.Errorf("doctor output should mention %q:\n%s", want, out) + } } - } - }) - - t.Run("doctor reports several remotes", func(t *testing.T) { - t.Parallel() - env := NewFeatureBranchEnv(t) - env.CheckpointStore = StoreGitRefs - env.SetupBareRemote() - env.SetupNamedBareRemote("backup") - - out := env.RunCLI("doctor") - if !strings.Contains(out, "2 remotes") { - t.Errorf("doctor output should mention the extra remote:\n%s", out) - } - if !strings.Contains(out, "always looks at origin") { - t.Errorf("doctor output should explain that reads resolve via origin:\n%s", out) - } - }) + for _, absent := range tt.absent { + if strings.Contains(out, absent) { + t.Errorf("doctor output should not mention %q for this repo:\n%s", absent, out) + } + } + }) + } } diff --git a/cmd/entire/cli/remote_topology.go b/cmd/entire/cli/remote_topology.go index 2f280097ac..eb85b1076f 100644 --- a/cmd/entire/cli/remote_topology.go +++ b/cmd/entire/cli/remote_topology.go @@ -9,6 +9,7 @@ import ( "strings" "github.com/entireio/cli/cmd/entire/cli/checkpoint" + "github.com/entireio/cli/cmd/entire/cli/checkpoint/remote" "github.com/entireio/cli/cmd/entire/cli/gitremote" "github.com/entireio/cli/cmd/entire/cli/logging" "github.com/entireio/cli/cmd/entire/cli/paths" @@ -22,27 +23,40 @@ import ( // at `entire enable` and on demand from `entire doctor` rather than letting // someone discover it when a resume comes up empty. -// remoteTopology summarizes the checkpoint-destination ambiguity in this repo. +// remoteDestination is one remote and what checkpoints pushed to it would do. +type remoteDestination struct { + name string + // pushURLs are the URLs a push to this remote delivers to, in git's order. + pushURLs []string + // pinned reports that remote.PushURL resolves this remote to a configured + // checkpoint_remote, so its own push URLs are irrelevant to checkpoints. + // + // Asked of the resolver rather than derived from settings on purpose: a + // checkpoint_remote that is *present* is not necessarily *in effect* — + // PushURL falls back to the push remote on an owner mismatch, an + // unparseable URL, or a protocol it cannot map. Reading settings directly + // would report "pinned" while pushes really went elsewhere, the same class + // of bug the CoreOrigin() rule in CLAUDE.md exists to prevent. + pinned bool +} + +// fansOut reports whether checkpoints pushed to this remote face more than one +// destination. +func (d remoteDestination) fansOut() bool { return !d.pinned && len(d.pushURLs) > 1 } + +// remoteTopology summarizes checkpoint-destination ambiguity in this repo. type remoteTopology struct { - // Remotes is every configured remote name, sorted. - Remotes []string - // MultiURLRemote names a remote that pushes to more than one URL ("" if - // none), and PushURLs are its push URLs in the order git uses them. - MultiURLRemote string - PushURLs []string - // PrimaryIsRefs reports whether the git-refs backend is active, which - // decides what a multi-URL remote means for checkpoints. - PrimaryIsRefs bool - // CheckpointRemote is the configured checkpoint_remote repo ("" if none). - // When set, it already pins one explicit destination and there is nothing - // ambiguous left to report. - CheckpointRemote string + // destinations is every configured remote, sorted by name. + destinations []remoteDestination + // primaryIsRefs reports whether the git-refs backend is active, which + // decides what a fanning-out remote means for checkpoints. + primaryIsRefs bool } // inspectRemoteTopology reads the repo's remotes and checkpoint configuration. -// Best-effort and local-only (no network): every failure yields an empty -// topology, which reports nothing, because this is advisory output and must -// never obstruct enable or doctor. +// Best-effort and offline: every failure yields an empty topology, which reports +// nothing, because this is advisory output that must never obstruct enable or +// doctor. func inspectRemoteTopology(ctx context.Context) remoteTopology { var t remoteTopology @@ -51,107 +65,135 @@ func inspectRemoteTopology(ctx context.Context) remoteTopology { return t } - remotes, err := listGitRemotes(ctx, repoRoot) + // One `git remote -v` rather than `git remote` plus a get-url per remote: + // it already applies git's pushurl-replaces-url rule and lists every push + // URL in order, so N+1 subprocesses collapse to one — and all of it runs in + // repoRoot instead of mixing dir-aware and cwd-dependent lookups. + pushURLs, err := pushURLsByRemote(ctx, repoRoot) if err != nil { - logging.Debug(ctx, "remote topology: could not list remotes", slog.String("error", err.Error())) + logging.Debug(ctx, "remote topology: could not read remotes", slog.String("error", err.Error())) return t } - for name := range remotes { - t.Remotes = append(t.Remotes, name) + + names := make([]string, 0, len(pushURLs)) + for name := range pushURLs { + names = append(names, name) } - sort.Strings(t.Remotes) + sort.Strings(names) - if s, err := settings.Load(ctx); err == nil { - if cfg := s.GetCheckpointRemote(); cfg != nil { - t.CheckpointRemote = cfg.Repo + for _, name := range names { + dest := remoteDestination{name: name, pushURLs: pushURLs[name]} + if _, enabled, err := remote.PushURL(ctx, name); err == nil { + dest.pinned = enabled } + t.destinations = append(t.destinations, dest) } if cpCfg, err := settings.LoadCheckpointsConfig(ctx); err == nil { - t.PrimaryIsRefs = checkpoint.PrimaryIsRefs(cpCfg) + t.primaryIsRefs = checkpoint.PrimaryIsRefs(cpCfg) } - // Report the first remote (in sorted order) that fans out, so the message is - // stable rather than dependent on map iteration. - for _, name := range t.Remotes { - urls, err := gitremote.GetPushURLs(ctx, name) - if err != nil || len(urls) < 2 { + return t +} + +// pushURLsByRemote parses `git remote -v` into remote name -> push URLs in git's +// own order. +func pushURLsByRemote(ctx context.Context, dir string) (map[string][]string, error) { + out, err := gitRunner(ctx, dir, "remote", "-v") + if err != nil { + return nil, fmt.Errorf("list git remotes: %w", err) + } + urls := make(map[string][]string) + for _, line := range strings.Split(out, "\n") { + // "\t (push)" — fetch lines are the same shape and ignored. + name, rest, found := strings.Cut(strings.TrimSpace(line), "\t") + if !found || !strings.HasSuffix(rest, "(push)") { continue } - t.MultiURLRemote = name - t.PushURLs = urls - break + url := strings.TrimSpace(strings.TrimSuffix(rest, "(push)")) + if url != "" { + urls[name] = append(urls[name], url) + } } - - return t + // An empty map is a legitimate answer (a repo with no remotes), so it is + // returned as such rather than as an error the caller would have to classify. + return urls, nil } -// hasAmbiguousDestination reports whether anything is worth telling the user. -func (t remoteTopology) hasAmbiguousDestination() bool { - if t.CheckpointRemote != "" { - return false +// ambiguous reports whether anything is worth telling the user: a remote whose +// checkpoints face several push URLs, or several remotes to choose between. +func (t remoteTopology) ambiguous() bool { + unpinned := 0 + for _, d := range t.destinations { + if d.fansOut() { + return true + } + if !d.pinned { + unpinned++ + } } - return len(t.Remotes) > 1 || t.MultiURLRemote != "" + return unpinned > 1 } -// describeCheckpointDestination writes an explanation of where checkpoints will -// go. Writes nothing when the destination is unambiguous. indent prefixes every -// line so callers can nest it under a heading. -func (t remoteTopology) describeCheckpointDestination(w io.Writer, indent string) { - if !t.hasAmbiguousDestination() { +// describeCheckpointDestination writes an explanation of where checkpoints go, +// under the given header. Writes nothing when the destination is unambiguous. +func (t remoteTopology) describeCheckpointDestination(w io.Writer, header string) { + if !t.ambiguous() { return } - if t.MultiURLRemote != "" { - fmt.Fprintf(w, "%sRemote %q pushes to %d URLs:\n", indent, t.MultiURLRemote, len(t.PushURLs)) - for i, u := range t.PushURLs { + fmt.Fprintln(w, header) + + for _, d := range t.destinations { + if !d.fansOut() { + continue + } + fmt.Fprintf(w, " Remote %q pushes to %d URLs:\n", d.name, len(d.pushURLs)) + for i, u := range d.pushURLs { marker := " " - if i == 0 && t.PrimaryIsRefs { + if i == 0 && t.primaryIsRefs { marker = "→ " } - fmt.Fprintf(w, "%s %s%s\n", indent, marker, redactForDisplay(u)) + fmt.Fprintf(w, " %s%s\n", marker, gitremote.RedactURLOrPath(u)) } - if t.PrimaryIsRefs { - fmt.Fprintf(w, "%s Checkpoints go to the first URL only; the others receive your code but no\n", indent) - fmt.Fprintf(w, "%s session history. Clone that first repository to resume sessions elsewhere.\n", indent) + if t.primaryIsRefs { + fmt.Fprintln(w, " Checkpoints go to the first URL only; the others receive your code but") + fmt.Fprintln(w, " no session history. Clone that first repository to resume elsewhere.") } else { - fmt.Fprintf(w, "%s Checkpoints are pushed to every URL. If one of them rejects or is\n", indent) - fmt.Fprintf(w, "%s unreachable, checkpoint sync reports a warning and retries on the next push.\n", indent) + fmt.Fprintln(w, " Checkpoints are pushed to every URL. If one rejects them or is") + fmt.Fprintln(w, " unreachable it is reported and left behind, and only the fetch URL is") + fmt.Fprintln(w, " ever reconciled — so those URLs can fall permanently out of date.") } } - if len(t.Remotes) > 1 { - fmt.Fprintf(w, "%sThis repo has %d remotes (%s).\n", indent, len(t.Remotes), strings.Join(t.Remotes, ", ")) - fmt.Fprintf(w, "%s Checkpoints follow whichever remote you push to, while reading them back\n", indent) - fmt.Fprintf(w, "%s (resume, explain) always looks at origin — so checkpoints pushed elsewhere\n", indent) - fmt.Fprintf(w, "%s are not found again from this clone.\n", indent) + if names := t.unpinnedNames(); len(names) > 1 { + fmt.Fprintf(w, " This repo has %d remotes (%s).\n", len(names), strings.Join(names, ", ")) + fmt.Fprintln(w, " Checkpoints follow whichever remote you push to, while reading them back") + fmt.Fprintln(w, " (resume, explain) always looks at origin — so checkpoints pushed elsewhere") + fmt.Fprintln(w, " are not found again from this clone.") } - fmt.Fprintf(w, "%sTo pin one repository for checkpoints, set checkpoint_remote in .entire/settings.json\n", indent) - fmt.Fprintf(w, "%s(or .entire/settings.local.json to keep it to this clone).\n", indent) + fmt.Fprintln(w, " To pin one repository for checkpoints, set checkpoint_remote in") + fmt.Fprintln(w, " .entire/settings.json (or .entire/settings.local.json to keep it to this clone).") } -// printCheckpointDestinationNote is the `entire enable` half of the explanation -// `entire doctor` prints (checkCheckpointDestination): enable is where a user is -// most likely to be looking, and the least surprising moment to learn that this -// repo's remotes make the checkpoint destination a choice. Silent on the -// ordinary repo, so it adds nothing to the common enable output. -func printCheckpointDestinationNote(ctx context.Context, w io.Writer) { - topology := inspectRemoteTopology(ctx) - if !topology.hasAmbiguousDestination() { - return +// unpinnedNames lists the remotes whose checkpoint destination is not already +// pinned by a checkpoint_remote. +func (t remoteTopology) unpinnedNames() []string { + var names []string + for _, d := range t.destinations { + if !d.pinned { + names = append(names, d.name) + } } - fmt.Fprintln(w) - fmt.Fprintln(w, "Note: this repo's remotes make the checkpoint destination ambiguous.") - topology.describeCheckpointDestination(w, " ") + return names } -// redactForDisplay strips credentials from URL-shaped values and passes -// filesystem paths through unchanged (RedactURL renders a plain path as -// ":///path"). -func redactForDisplay(u string) string { - if strings.Contains(u, "://") || strings.Contains(u, "@") { - return gitremote.RedactURL(u) - } - return u +// printCheckpointDestinationNote explains where checkpoints go when this repo's +// remotes make that a choice. Shared by `entire enable` — the moment a user is +// most likely to be looking, and the least surprising place to learn it — and by +// `entire doctor`, which reports it on demand. Silent on the ordinary repo, so it +// adds nothing to the common output. +func printCheckpointDestinationNote(ctx context.Context, w io.Writer, header string) { + inspectRemoteTopology(ctx).describeCheckpointDestination(w, header) } diff --git a/cmd/entire/cli/repo_mirror_use.go b/cmd/entire/cli/repo_mirror_use.go index e5aff6a972..f6b3ab75e2 100644 --- a/cmd/entire/cli/repo_mirror_use.go +++ b/cmd/entire/cli/repo_mirror_use.go @@ -62,16 +62,12 @@ func validateGitRemoteName(name string) error { // these errors reach stderr through main.go and from there into logs and pasted // transcripts — the same reason reportMirrorRemotePlan redacts what it prints. // -// Only URL-shaped args are touched: gitremote.RedactURL would turn a bare word -// like "remote" into "://remote", so it cannot be applied blanket-fashion. +// Non-URL args (bare words like "remote", local paths) pass through untouched; +// see gitremote.RedactURLOrPath for why RedactURL cannot be applied blanket-fashion. func redactGitArgs(args []string) []string { safe := make([]string, len(args)) for i, a := range args { - if strings.Contains(a, "://") || strings.Contains(a, "@") { - safe[i] = gitremote.RedactURL(a) - continue - } - safe[i] = a + safe[i] = gitremote.RedactURLOrPath(a) } return safe } diff --git a/cmd/entire/cli/setup.go b/cmd/entire/cli/setup.go index 3e76570150..dcf53df0f8 100644 --- a/cmd/entire/cli/setup.go +++ b/cmd/entire/cli/setup.go @@ -1341,7 +1341,7 @@ func runEnableInteractive(ctx context.Context, w io.Writer, agents []agent.Agent } } - printCheckpointDestinationNote(ctx, w) + printCheckpointDestinationNote(ctx, w, "\nNote: this repo's remotes make the checkpoint destination ambiguous.") return nil } @@ -1352,7 +1352,7 @@ func printEnabledStatus(ctx context.Context, w io.Writer) { fmt.Fprintf(w, "Agents: %s\n", strings.Join(displayNames, ", ")) } fmt.Fprintln(w, "\nTo add more agents, run `entire agent add `.") - printCheckpointDestinationNote(ctx, w) + printCheckpointDestinationNote(ctx, w, "\nNote: this repo's remotes make the checkpoint destination ambiguous.") } // resolveFirstRunCheckpointBackend decides the checkpoint storage backend diff --git a/cmd/entire/cli/strategy/manual_commit_push.go b/cmd/entire/cli/strategy/manual_commit_push.go index 57ba63ed02..d8578071a0 100644 --- a/cmd/entire/cli/strategy/manual_commit_push.go +++ b/cmd/entire/cli/strategy/manual_commit_push.go @@ -278,10 +278,7 @@ func (s *ManualCommitStrategy) prePushCheckpointRefs(ctx context.Context, ps pus return nil } - dest := resolveRefsPushDestination(ctx, ps) - dest.warnIgnoredPushURLs(ctx, os.Stderr) - - if _, err := flushCheckpointRefsQueue(ctx, repo, dest); err != nil { + if _, err := flushCheckpointRefsQueue(ctx, repo, ps); err != nil { // Fail-soft: a checkpoint-ref push failure must never block the user's // git push. The refs stay queued for the next pre-push. logging.Warn(ctx, "git-refs pre-push: checkpoint ref push failed; refs left queued", @@ -309,9 +306,7 @@ func PushQueuedCheckpointRefs(ctx context.Context, repo *git.Repository, remote if !checkpointPolicyAllowsGitHook(ctx, repo) { return 0, false, errors.New("checkpoint policy does not allow pushing checkpoint refs; refs stay queued") } - dest := resolveRefsPushDestination(ctx, ps) - dest.warnIgnoredPushURLs(ctx, os.Stderr) - pushed, err = flushCheckpointRefsQueue(ctx, repo, dest) + pushed, err = flushCheckpointRefsQueue(ctx, repo, ps) // Clean up even on a partial/failed flush: a diverged batch can push some // refs and still return an error, and the shadow branches for the refs that // *did* land must still be cleaned up — parity with the pre-push path, which @@ -328,8 +323,7 @@ func PushQueuedCheckpointRefs(ctx context.Context, repo *git.Repository, remote // never block the user's push) and the migration command's opt-in push (which // surfaces it). Stale entries — refs no longer present locally — are pruned so // they don't block the queue forever. -func flushCheckpointRefsQueue(ctx context.Context, repo *git.Repository, dest refsPushDestination) (int, error) { - pushTarget := dest.target +func flushCheckpointRefsQueue(ctx context.Context, repo *git.Repository, ps pushSettings) (int, error) { queue, err := checkpoint.PushQueueForRepo(ctx, repo) if err != nil { return 0, fmt.Errorf("resolve push queue: %w", err) @@ -356,16 +350,22 @@ func flushCheckpointRefsQueue(ctx context.Context, repo *git.Repository, dest re return 0, nil } + // Resolved here, not by the caller: it spawns `git remote get-url` and its + // result is unused unless refs are actually pushed, so an ordinary push with + // an empty queue must not pay for it — nor print the multi-URL warning. + dest := resolveRefsPushDestination(pushCtx, ps) + dest.warnIgnoredPushURLs(pushCtx) + // Progress: pushing many refs over the network can take tens of seconds, so // surface it (matching the v1 path's "[entire] Pushing ..." line) instead of // leaving the user's git push apparently hung. Written to stderr, which git // shows during the pre-push hook. - fmt.Fprintf(os.Stderr, "[entire] Pushing %d checkpoint ref(s) to %s...", len(existing), dest.display) + fmt.Fprintf(os.Stderr, "[entire] Pushing %d checkpoint ref(s) to %s...", len(existing), dest.display()) stop := startProgressDots(os.Stderr) // Fast path: push all refs in one round-trip (fast-forward-only). If every // ref was up to date or fast-forwarded, we're done. - batchErr := batchPushRefs(pushCtx, pushTarget, existing) + batchErr := batchPushRefs(pushCtx, dest.target, existing) if batchErr == nil { stop(" done") if removeErr := queue.Remove(existing); removeErr != nil { @@ -383,7 +383,7 @@ func flushCheckpointRefsQueue(ctx context.Context, repo *git.Repository, dest re fmt.Fprintf(os.Stderr, "[entire] Warning: couldn't push checkpoint refs: %v\n", batchErr) printNonInteractiveSSHAuthHint() if dest.checkpointRemote { - printCheckpointRemoteHint(pushTarget) + printCheckpointRemoteHint(dest.target) } return 0, batchErr } @@ -401,7 +401,7 @@ func flushCheckpointRefsQueue(ctx context.Context, repo *git.Repository, dest re pushed := make([]plumbing.ReferenceName, 0, len(existing)) var firstErr error for _, ref := range existing { - if err := pushCheckpointRefWithRecovery(pushCtx, pushTarget, ref); err != nil { + if err := pushCheckpointRefWithRecovery(pushCtx, dest.target, ref); err != nil { logging.Warn(ctx, "git-refs push: checkpoint ref push/sync failed; left queued, not overwritten", slog.String("ref", ref.String()), slog.String("error", err.Error())) if nonInteractiveSSHAuthFailure(pushCtx, err) { diff --git a/cmd/entire/cli/strategy/refs_push_destination.go b/cmd/entire/cli/strategy/refs_push_destination.go index 6eeb409536..e48b239a0b 100644 --- a/cmd/entire/cli/strategy/refs_push_destination.go +++ b/cmd/entire/cli/strategy/refs_push_destination.go @@ -3,25 +3,23 @@ package strategy import ( "context" "fmt" - "io" "log/slog" "github.com/entireio/cli/cmd/entire/cli/checkpoint/remote" "github.com/entireio/cli/cmd/entire/cli/logging" ) -// refsPushDestination is where checkpoint refs are pushed, plus how to name that -// place in user-facing output. +// refsPushDestination is the single place checkpoint refs are pushed to. type refsPushDestination struct { - // target is passed to git push / the recovery fetch: a remote name, or a URL. + // target is passed to git push and to the recovery fetch: a remote name, or a URL. target string - // display names the destination in progress and warning output. - display string - // checkpointRemote is true when target came from a configured - // checkpoint_remote, which gates the "a checkpoint remote is configured" - // hint — that hint must not fire for a URL we picked ourselves. + // checkpointRemote records that target came from a configured + // checkpoint_remote. It cannot be recovered from target's shape — a + // resolved push URL is URL-shaped too — and it decides both how the + // destination is named and whether the "a checkpoint remote is configured" + // hint applies. checkpointRemote bool - // ignoredPushURLs counts push URLs of a multi-URL remote that will NOT + // ignoredPushURLs counts the push URLs of a multi-URL remote that will NOT // receive checkpoint refs. Zero in every single-destination topology. ignoredPushURLs int } @@ -61,18 +59,16 @@ type refsPushDestination struct { // to a URL, so the overwhelmingly common topology behaves exactly as before — // remote-tracking refs still update, output still says "origin", and no // URL-keyed promisor config appears. +// +// Call this only once there is something to push: it spawns `git remote get-url` +// and its result is unused on an empty queue. func resolveRefsPushDestination(ctx context.Context, ps pushSettings) refsPushDestination { target := ps.pushTarget() - // A configured checkpoint_remote is already a single explicit destination. - if ps.hasCheckpointURL() { - return refsPushDestination{target: target, display: displayPushTarget(target), checkpointRemote: true} - } - - // Pushing straight to a URL (git passes a bare URL through to the hook) is - // likewise already single-destination. - if remote.IsURL(target) { - return refsPushDestination{target: target, display: displayPushTarget(target)} + // A configured checkpoint_remote, or a push straight to a URL (git hands the + // hook a bare URL verbatim), is already a single explicit destination. + if ps.hasCheckpointURL() || remote.IsURL(target) { + return refsPushDestination{target: target, checkpointRemote: ps.hasCheckpointURL()} } urls, err := remote.GetPushURLs(ctx, target) @@ -83,42 +79,43 @@ func resolveRefsPushDestination(ctx context.Context, ps pushSettings) refsPushDe slog.String("target", target), slog.String("error", err.Error()), ) - return refsPushDestination{target: target, display: target} } if len(urls) < 2 { - return refsPushDestination{target: target, display: target} - } - - return refsPushDestination{ - target: urls[0], - display: fmt.Sprintf("%s (first of %d push URLs)", displayURL(urls[0]), len(urls)), - ignoredPushURLs: len(urls) - 1, + return refsPushDestination{target: target} } + return refsPushDestination{target: urls[0], ignoredPushURLs: len(urls) - 1} } -// displayURL renders a push destination for humans with any credentials removed. -// RedactURL is only safe for URL-shaped values: given a plain filesystem path it -// parses to an empty scheme and host and renders as ":///path/to/repo". Paths -// carry no credentials, so pass them through unchanged. -func displayURL(u string) string { - if remote.IsURL(u) { - return remote.RedactURL(u) +// display names the destination for progress and warning output. +// +// Deliberately not displayPushTarget: that maps ANY URL to the literal words +// "checkpoint remote", which was only ever true because a URL target implied a +// configured checkpoint_remote. A push URL we resolved ourselves is URL-shaped +// but is not a checkpoint remote, so it is named by its (redacted) URL. +func (d refsPushDestination) display() string { + switch { + case d.checkpointRemote: + return "checkpoint remote" + case d.ignoredPushURLs > 0: + return fmt.Sprintf("%s (first of %d push URLs)", remote.RedactURLOrPath(d.target), d.ignoredPushURLs+1) + default: + return remote.RedactURLOrPath(d.target) } - return u } -// warnIgnoredPushURLs tells the user, once per push, that checkpoint refs are -// going to one URL of a multi-URL remote — otherwise the choice is invisible and -// looks like the other mirrors silently lost their checkpoints. -func (d refsPushDestination) warnIgnoredPushURLs(ctx context.Context, errOut io.Writer) { +// warnIgnoredPushURLs tells the user that checkpoint refs are going to one URL of +// a multi-URL remote — otherwise the choice is invisible and looks like the other +// mirrors silently lost their checkpoints. Call it only when there are refs to +// push, so a no-op push stays quiet. +func (d refsPushDestination) warnIgnoredPushURLs(ctx context.Context) { if d.ignoredPushURLs == 0 { return } - fmt.Fprintf(errOut, "[entire] Checkpoints go to one repository: %s. %d other push URL(s) of this remote will not receive them.\n", - d.display, d.ignoredPushURLs) - fmt.Fprintln(errOut, "[entire] To store checkpoints in a specific repository instead, set checkpoint_remote in .entire/settings.json.") + fmt.Fprintf(stderrWriter, "[entire] Checkpoints go to one repository: %s. %d other push URL(s) of this remote will not receive them.\n", + d.display(), d.ignoredPushURLs) + fmt.Fprintln(stderrWriter, "[entire] To store checkpoints in a specific repository instead, set checkpoint_remote in .entire/settings.json.") logging.Info(ctx, "git-refs push: multi-URL remote, pushing checkpoint refs to the first push URL only", - slog.String("target", displayURL(d.target)), + slog.String("target", remote.RedactURLOrPath(d.target)), slog.Int("ignored_push_urls", d.ignoredPushURLs), ) } From 1210d38a1bac87110286f2a943d0f43ab7feb874 Mon Sep 17 00:00:00 2001 From: Stefan Haubold Date: Wed, 5 Aug 2026 19:36:38 +0200 Subject: [PATCH 3/3] fix(checkpoint): don't name a cause in the checkpoint-ref retry message Review feedback on #1905: "rejected" still implies the remote answered, but batchPushRefs fails on transport errors too (unreachable host, auth). Same mistake as the "diverged" wording it replaced, one step milder. Says "failed". Co-Authored-By: Claude Opus 5 (1M context) Entire-Checkpoint: 01KZ9FZCHDJD1625C8XGTEAK6F --- cmd/entire/cli/strategy/manual_commit_push.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/cmd/entire/cli/strategy/manual_commit_push.go b/cmd/entire/cli/strategy/manual_commit_push.go index d8578071a0..361aa08d98 100644 --- a/cmd/entire/cli/strategy/manual_commit_push.go +++ b/cmd/entire/cli/strategy/manual_commit_push.go @@ -393,10 +393,11 @@ func flushCheckpointRefsQueue(ctx context.Context, repo *git.Repository, ps push // fetch+replay recovery, and remove from the queue only the refs that land // (a genuine cherry-pick conflict leaves that ref queued for a later push, // never force-overwriting the remote). - // Deliberately does not claim divergence: a rejected batch is just as often - // an unreachable or unauthorized destination, and telling a user with a dead - // remote that their refs "diverged" sends them after the wrong problem. - fmt.Fprintf(os.Stderr, "[entire] Checkpoint ref push was rejected; retrying %d ref(s) individually...", len(existing)) + // Deliberately names no cause: the batch fails on divergence, but just as + // often on an unreachable or unauthorized destination. Telling a user with a + // dead remote that their refs "diverged" — or were "rejected", which equally + // implies the remote answered — sends them after the wrong problem. + fmt.Fprintf(os.Stderr, "[entire] Checkpoint ref push failed; retrying %d ref(s) individually...", len(existing)) stop = startProgressDots(os.Stderr) pushed := make([]plumbing.ReferenceName, 0, len(existing)) var firstErr error