diff --git a/cmd/entire/cli/checkpoint/remote/util.go b/cmd/entire/cli/checkpoint/remote/util.go index 78a2cf45c6..84c6249b11 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) @@ -481,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 eae539fcbb..540a1517a0 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. + printCheckpointDestinationNote(ctx, cmd.OutOrStdout(), "Checkpoint destination: REVIEW") + // Stuck sessions // Load all session states states, err := strategy.ListSessionStates(ctx) @@ -432,20 +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. // 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`. @@ -464,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 c731786926..6e30bdff90 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) @@ -182,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 8eda10b5a3..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,6 +111,27 @@ func (env *TestEnv) GitPushWithHooksAllowError(remote, refSpec string) error { return err //nolint:wrapcheck // test helper: the caller asserts on presence/absence, not identity } +// 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() + + 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 { + env.T.Fatalf("failed to add push URL %s to remote %s: %v\n%s", url, remoteName, err, output) + } + } + + env.setGitConfigBaseline() +} + // 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..10d6054c03 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" @@ -21,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 @@ -246,17 +248,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 +285,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 +353,73 @@ 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() + + 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"}, + }, + } + + 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) + } + } + 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 new file mode 100644 index 0000000000..eb85b1076f --- /dev/null +++ b/cmd/entire/cli/remote_topology.go @@ -0,0 +1,199 @@ +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/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" + "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. + +// 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 { + // 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 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 + + repoRoot, err := paths.WorktreeRoot(ctx) + if err != nil { + return t + } + + // 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 read remotes", slog.String("error", err.Error())) + return t + } + + names := make([]string, 0, len(pushURLs)) + for name := range pushURLs { + names = append(names, name) + } + sort.Strings(names) + + 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) + } + + 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 + } + url := strings.TrimSpace(strings.TrimSuffix(rest, "(push)")) + if url != "" { + urls[name] = append(urls[name], url) + } + } + // 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 +} + +// 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 unpinned > 1 +} + +// 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 + } + + 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 { + marker = "→ " + } + fmt.Fprintf(w, " %s%s\n", marker, gitremote.RedactURLOrPath(u)) + } + 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.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 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.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).") +} + +// 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) + } + } + return names +} + +// 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 b3220d9f06..dcf53df0f8 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, "\nNote: this repo's remotes make the checkpoint destination ambiguous.") + 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, "\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 5ab8070ca6..361aa08d98 100644 --- a/cmd/entire/cli/strategy/manual_commit_push.go +++ b/cmd/entire/cli/strategy/manual_commit_push.go @@ -278,7 +278,7 @@ func (s *ManualCommitStrategy) prePushCheckpointRefs(ctx context.Context, ps pus return nil } - if _, err := flushCheckpointRefsQueue(ctx, repo, ps.pushTarget()); 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", @@ -306,7 +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") } - pushed, err = flushCheckpointRefsQueue(ctx, repo, ps.pushTarget()) + 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 @@ -323,7 +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, pushTarget string) (int, error) { +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) @@ -350,17 +350,22 @@ func flushCheckpointRefsQueue(ctx context.Context, repo *git.Repository, pushTar 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. - 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 // 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 { @@ -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(dest.target) + } return 0, batchErr } @@ -386,12 +393,16 @@ 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 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 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 new file mode 100644 index 0000000000..e48b239a0b --- /dev/null +++ b/cmd/entire/cli/strategy/refs_push_destination.go @@ -0,0 +1,121 @@ +package strategy + +import ( + "context" + "fmt" + "log/slog" + + "github.com/entireio/cli/cmd/entire/cli/checkpoint/remote" + "github.com/entireio/cli/cmd/entire/cli/logging" +) + +// refsPushDestination is the single place checkpoint refs are pushed to. +type refsPushDestination struct { + // target is passed to git push and to the recovery fetch: a remote name, or a URL. + target string + // 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 the 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. +// +// 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, 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) + 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()), + ) + } + if len(urls) < 2 { + return refsPushDestination{target: target} + } + return refsPushDestination{target: urls[0], ignoredPushURLs: len(urls) - 1} +} + +// 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) + } +} + +// 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(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", remote.RedactURLOrPath(d.target)), + slog.Int("ignored_push_urls", d.ignoredPushURLs), + ) +}