Skip to content

fix(checkpoint): name the sync remote in the multi-remote note - #2080

Open
gtrrz-victor wants to merge 6 commits into
mainfrom
fix/checkpoint-destination-note-capture
Open

fix(checkpoint): name the sync remote in the multi-remote note#2080
gtrrz-victor wants to merge 6 commits into
mainfrom
fix/checkpoint-destination-note-capture

Conversation

@gtrrz-victor

@gtrrz-victor gtrrz-victor commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

https://entire.io/gh/entireio/cli/trails/1108

The problem

The several-remotes note printed by entire enable / entire doctor says:

Checkpoints sync to a single elected remote — not to whichever one you push to. A push to any other remote carries your code but no session history. Run entire status to see the elected destination and how many checkpoints are waiting for it.

That bolded sentence is not true of the one push that matters most. pendingCaptureCheckpointSyncRemote runs before the single-remote gate in prePush, so a push whose target agrees with the branch's declared push destination elects that remote and carries the checkpoints there in the same push (TestCheckpointSyncRemote_TrackedPushCapturesElection asserts exactly that). The note was warning about a case the CLI now handles by itself.

It also never said where checkpoints actually go — the one thing a user standing in a multi-remote repo wants to know.

The change

inspectRemoteTopology asks strategy.ResolveCheckpointSyncRemote for the election (never re-derived from settings — the same rule the existing pinned field follows, so the shown destination cannot diverge from where pushes go), and the note says the concrete thing. Election still open:

  This repo has 3 remotes (custom, origin, upstream).
    Checkpoints sync to one remote — right now "origin". A push to a different
    remote of your own can move it; `entire status` shows where they go.
    Set strategy_options.checkpoint_push_remote to pin one yourself.

"a different remote" rather than "your branch's own remote": capture requires the push target to both agree with the branch's declared push destination and differ from today's election, so on the ordinary setup where they already agree, no push moves anything — and the earlier wording promised a takeover that cannot happen.

Observed tier — destination settled, capture clause gone:

    Checkpoints sync to one remote — "custom", your branch's push destination.
    A push to any other remote carries your code but no session history.
    Set strategy_options.checkpoint_push_remote to pin a different one.

Explicit checkpoint_push_remote — no advice line, because repeating the key they already set reads as a warning that something still needs doing:

    Checkpoints sync to one remote — "custom" (set by checkpoint_push_remote).
    A push to any other remote carries your code but no session history.

The sentence this PR set out to delete survives on exactly these two tiers. It is untrue only where the election is still open, because there the electing push carries its own history to the remote it elects. Once checkpoint_push_remote is set or a capture is in force, captureEligible refuses every displacement, so no push can move the destination and the warning is exactly right — and it is the fact a multi-remote user most needs. Deleting it repo-wide traded one wrong sentence for a missing one.

Two more states name no remote at all. The first is a failed election — the resolver failing closed on a checkpoint_push_remote that names a remote which does not exist, or on a settings file it could not read. Checkpoints are going nowhere until that is fixed, so the error is kept on the topology rather than swallowed into a debug log, and reported in the wording entire status already uses:

    Checkpoints are NOT syncing: checkpoint_push_remote "gone" is not a configured git remote; checkpoint sync disabled until fixed

That is the only branch here that is a warning rather than orientation.

The second is an election that came back with no name and no error. An earlier revision of this PR ruled that out — with two or more remotes a successful election was said to always return a name — but the resolver's own git config read answers nil on a transient failure rather than caching it (cachedRemotesInConfigOrder), so an unnamed, error-free answer is reachable. Left silent it printed the remote count and nothing else, strictly less than the static text this replaces, so it falls back to the one clause true on every tier:

    Checkpoints sync to a single elected remote; `entire status` shows which.

The fan-out block above (one remote, several push URLs) is untouched — that one is still genuinely lossy.

The tier switch names the three open tiers instead of falling through to default: golangci-lint's exhaustive rule rejects the default clause. SyncRemoteSourceSole is all but unreachable at this call site — this note is written only when two or more remotes are unpinned, and the resolver reaches the sole tier only with exactly one remote configured — so it is grouped with the open tiers. Not asserted as impossible, though: the two sides count remotes with different git commands (git remote -v here, git config --local --get-regexp in the resolver), which can disagree, and the shared wording reads correctly either way.

Test

TestMultiPushURL_DestinationNoteSurfaces runs on the open-election tier, so it still asserts "no session history" absent there, alongside the two phrasings this note was already wrong with ("follow whichever remote", "always looks at origin"), so a future rewrite cannot reintroduce any of the three.

TestDescribeSyncRemote (new, cmd/entire/cli/remote_topology_test.go) covers every tier the note words differently — config, observed, open election by default and by first, fail-closed, and unresolved — asserting both the text that must appear and the text that must not, since the integration test only ever reaches the open-election tier. The settled tiers assert "no session history" present and the open ones assert it absent, which is where that split is pinned. describeSyncRemote is a pure function of two fields and an io.Writer, so the table is cheap.

Verification

  • golangci-lint 2.11.3 ./cmd/entire/cli/... → 0 issues, gofmt clean. (An earlier revision of this PR claimed lint was clean when it was not: exhaustive was failing on the tier switch. Fixed here.)
  • mise run test → 9313 tests, 4 skipped, 0 failures (186s), as of f9eb943's parent; re-run after f9eb943 was scoped to go test ./cmd/entire/cli/..., which passes.
  • TestMultiPushURL (integration, all cases) and TestDescribeSyncRemote pass.
  • The four states that existed at 6c71f73 were verified by hand against a 3-remote repo. The two added since — the fail-closed warning and the unresolved fallback — are covered by TestDescribeSyncRemote only, since the unresolved one needs a transient git failure to reproduce by hand.

Trail findings

One finding on trail #1108, dismissed as not applicable: it asked for separate wording for SyncRemoteSourceSole, which this call site does not reach. Recorded in a code comment instead of a fourth message.

Copilot's inline review caught the silent-return case above before the fail-closed branch landed; its second half — an unnamed election with no error — outlived that fix and is closed by f9eb943.

🤖 Generated with Claude Code

The several-remotes note told users that a push to any remote other than
the elected one "carries your code but no session history". That is not
true of the one push that matters most: capture runs before the
single-remote gate, so a push whose target agrees with the branch's
declared push destination elects that remote AND carries the checkpoints
there in the same push. The note was warning about a case the CLI now
handles by itself.

It also never said where checkpoints actually go, which is the one thing
a user standing in a multi-remote repo wants to know.

Ask the resolver for the election (never re-derive it from settings -
the same rule the `pinned` field follows) and say the concrete thing:

  Checkpoints sync to one remote - right now "origin", and your first push to
  your branch's own remote takes it over. `entire status` shows where.
  Set strategy_options.checkpoint_push_remote to pin one yourself.

Once the destination is settled the note shrinks: the observed tier drops
the capture clause, and a repo that set checkpoint_push_remote is told
nothing further, because repeating the key it already set reads as a
warning that something still needs doing.

The test asserts "no session history" absent alongside the two phrasings
this note was previously wrong with, so the next rewrite cannot
reintroduce any of the three.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 20, 2026 08:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the multi-remote checkpoint destination note (shown by entire enable / entire doctor) to name the currently elected checkpoint sync remote by querying the same resolver used by push logic, so the note reflects where checkpoints actually sync and avoids previously incorrect wording.

Changes:

  • Add checkpoint sync remote + source to remoteTopology, populated via strategy.ResolveCheckpointSyncRemote.
  • Replace the prior generic “no session history on other remotes” warning with tier-aware, concrete messaging that names the elected remote.
  • Tighten the integration test to assert previously incorrect phrasings (including “no session history”) stay absent.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
cmd/entire/cli/remote_topology.go Resolve and print the elected checkpoint sync remote in the multi-remote note.
cmd/entire/cli/integration_test/multi_pushurl_test.go Update assertions to cover the new phrasing and prevent regressions to prior incorrect wording.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread cmd/entire/cli/remote_topology.go Outdated
gtrrz-victor and others added 2 commits August 20, 2026 11:56
Where this note is written the election has at least two remotes to
choose between, and a successful election over two or more remotes always
returns a name — so an empty name there means one thing only: the
resolver failed closed because checkpoint_push_remote names a remote that
does not exist, and checkpoints are going nowhere at all. Swallowing that
error into a debug log left the reader of a multi-remote repo with "This
repo has 2 remotes (...)" and no word about checkpoints, which is the one
case that is not orientation but a thing to go and fix. Keep the error on
the topology and say it, in the wording `entire status` already uses.

The switch also has to name the three open tiers instead of falling to
default: golangci-lint's exhaustive rule rejects the default clause, so
the previous commit did not lint clean. Sole is unreachable at this call
site and carries a comment saying so, since "your first push takes it
over" is meaningless with one remote and the next reader should not have
to re-derive that it cannot happen.

Unit-test describeSyncRemote directly: it is a pure function of two
fields, and the integration test only ever reaches the open-election
tier, leaving the two branches whose whole purpose is different wording
uncovered.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01M0F9JTNJB0C0S7PK9EEFC1FC
Three gaps found reviewing the note against the resolver it now asks.

The election can come back unnamed without an error: the resolver's own
`git config` read answers nil on a transient failure rather than caching
it, so a repo with two remotes could print its remote count and then say
nothing at all about where checkpoints go — strictly less than the static
text this replaced. Fall back to the one clause true on every tier.

"A push to any other remote carries your code but no session history" was
dropped repo-wide because the electing push disproves it. It only does so
on the open tiers; with checkpoint_push_remote set or a capture in force,
captureEligible refuses every displacement, so the sentence is exactly
right there — and it is the fact a multi-remote user most needs. Restore
it on those two tiers only.

"your first push to your branch's own remote takes it over" promises a
move that cannot happen in the ordinary setup: capture needs the target to
both agree with the declared push destination and differ from today's
election, so when they already agree, no push moves anything.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01M0FAV2BKGMVDTCWRRPAFED5P
@gtrrz-victor
gtrrz-victor marked this pull request as ready for review August 20, 2026 10:23
@gtrrz-victor
gtrrz-victor requested a review from a team as a code owner August 20, 2026 10:23
gtrrz-victor and others added 3 commits August 21, 2026 14:28
The note makes three claims — a push-URL breakdown, a remote list and the
sync destination — and only the last consulted the election, from its own
copy of the resolver rather than the computation `entire status` uses. Three
states came out wrong, all the same defect:

- A dedicated checkpoint_remote store: the note named the pinned elected
  remote, which its own list omitted and which is not where checkpoints go,
  while `entire status` named the store.
- A fanning-out remote that is not the elected one: "checkpoints go to the
  first URL only" of a remote whose pushes are gated out entirely.
- A fail-closed election: a URL breakdown claiming delivery above the line
  saying nothing is syncing.

inspectRemoteTopology now shares resolveCheckpointSyncDestination, split out
of computeCheckpointSyncInfo so the note pays for the destination without the
unpushed counts. The URL block prints only for the elected remote, the remote
list counts every remote, and the closing advice is dropped where a
checkpoint_remote already exists or nothing is syncing.

A fourth state was reported nowhere: a checkpoint_remote rejected as inherited
is a logging.Warn inside remote.PushURL, so the setting is silently ignored and
the fallback looks like an ordinary election. status, its --json, and the note
now say so, naming the likely cause rather than asserting it — PushURL returns
a bare enabled=false for ownership, an unparseable push URL and an unmappable
protocol alike.

describeCheckpointDestination had no test coverage at all; it gets a table plus
a real-repo test that would have caught the original bug.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01M0J4PVN4P5SA96W6E8JRFCWD
Quality pass over bd53c7c, no behaviour change beyond two wording details.

quoteNames was a second prose-list joiner in the same package, and the two
disagreed on the Oxford comma — so `entire enable` and `entire checkpoint
tokens` punctuated lists differently. Collapsed onto the existing one, renamed
formatProseList since it is no longer about token classes.

The note grew a third hand-rolled loop collecting names off t.destinations, and
bound unpinnedNames() only to measure it; allNames() joins its two siblings and
the dead binding is gone. The fan-out loop, once conditioned on the elected
remote, could only ever act on one destination, so electedDestination() says
that instead of a scan with a triple-negative guard. storeConfigured() collapses
the three-part advice guard and names what it is really asking: dedicated and
IgnoredStore are the in-effect and not-in-effect halves of "a store exists".
The remoteTopology.dedicated() forwarder and computeCheckpointSyncInfo's
non-parallel switch are gone, and checkpointSyncInfo's doc no longer claims to
be the shared computation — it is a result type, and there are three consumers
now, which was the point of the commit that made this doc wrong.

Wording: the ignored-store paragraph moves to a two-space indent, since a lone
fanning-out remote reaches it with no remote-list line above to hang off.

Tests: oneURL/twoURLs become package-level fixtures the sibling test can use,
one assertNote helper replaces three verbatim want/absent loops, and the table
uses one literal style throughout.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01M0J60J4X8WA5DP51GM3NKRTN
The dedicated-store note read "Pushes to no remote carry them there." when
pinnedNames() came back empty. Normally it cannot: being pinned is what puts the
elected remote into dedicated mode. But inspectRemoteTopology drops the error
from each per-remote PushURL probe while the election runs its own, so a
transient git failure in the loop leaves every remote unpinned with dedicated
mode still on.

Says nothing rather than naming none of them: in that state the pushes really do
reach the store, so "no remote carries them" is the one claim that would be
false. The clause is now conditional and quoteNames returns empty for empty,
leaving the sentence to its caller.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01M0J6QBPF1D18PYNV1S40HYBT
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants