Repository navigation
Conversation
Import writes read-only checkpoints and creates no commit, so the pre-push git hook -- the only sync trigger -- never fires for it. Combined with idempotency keyed on local existence, history imported while logged out was stranded permanently: a post-login re-run reported "0 turn(s) (N already imported)" and made no attempt to sync, so the sessions never reached the dashboard. The git-refs store already enqueues every checkpoint ref as it writes it (gitRefsStore.setRef -> enqueueForPush) and nothing drains that queue but a push, so those turns are still queued afterwards. Draining the queue after a logged-in import is therefore enough to recover them, with no change to the import idempotency logic. syncImportedCheckpoints runs on every logged-in import, deliberately NOT gated on this run having imported anything new -- that is what makes a post-login re-run the recovery path. It is best-effort: the import already succeeded locally, so a push failure is reported and swallowed rather than failing the command, and Ctrl-C stays silent. Scoped to the git-refs backend, the sole `entire enable` default since entireio#1900. git-branch has no push queue -- its checkpoints ride the entire/checkpoints/v1 branch that the pre-push hook already ships -- so it is skipped rather than paying for a checkpoint-policy sync to drain a queue it never fills. Those repos keep syncing exactly as they do today. resolveMigratePushRemote is renamed resolveCheckpointPushRemote and shared with the migration command: both are non-hook drain paths and must resolve the elected checkpoint sync remote the same fail-closed way. `entire import` gains --remote to override it, matching `entire doctor migrate-checkpoints`. Verified against a real remote: on 0.9.0 the checkpoint stays local after login + re-import (0 refs on the remote); with this change the same sequence pushes it and prints "Synced 1 checkpoint(s) to origin." Refs entireio#1773 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 01KZH4HXC95ET9V176H4SS6NTZ
Soph
left a comment
There was a problem hiding this comment.
Thanks for this! The diagnosis is right, the queue insight is the right one, and I appreciate that the PR body pre-empts the "why not PrePush" question. I verified the load-bearing claim independently: enqueueForPush has no auth gating, so the refs really are still queued. Two things I'd want fixed before merge.
Blockers
--remotecan route checkpoint data off the elected sync remote.PushQueuedCheckpointRefshas nocheckpointSyncAllowedForRemotegate and the flag isn't validated, soimport --remote <anything>pushes transcripts there. CLAUDE.md states the opposite invariant, so this is a contract break.- The reported count is the whole repo queue, not the import's refs — an import that finds nothing can print
Synced 12 checkpoint(s).
One reframe, not a blocker
For git-refs the next ordinary git push already drains the queue regardless of login state, so imported history is pending rather than stranded. That doesn't undercut the PR - syncing promptly instead of at the next push is worth having - but the docstring and the notice copy should say that, and it's why (2) matters: if prompt sync is the value, the count has to be about the import.
Relatedly, warnIfImportNotSynced still says "Log in with 'entire login' before importing", which reads as "too late for this history". Worth fixing in the same PR.
Also
- The four existing
TestRunSelectedImports_*tests weren't stubbed, so they now run the real sync, which readsENTIRE_TOKEN— not covered by the testdirs net. AddstubImportSync(or forceimportLoggedInfalse) to them. - No docs changes:
ref-checkpoint-backend.mdstill namesPrePushas the only drain, and CLAUDE.md needs the--remotebehavior reflected once it's gated.
On your question regarding the push. The enable call is one step in the onboarding flow, if actual checkpoints are pushed then it does not matter if the enable call came before.
Again thanks for opening this!
| // | ||
| // Import writes read-only checkpoints and creates no commit, so the pre-push git | ||
| // hook — the only sync trigger — never fires for it. Before this, history | ||
| // imported while logged out stayed local-only forever: import's idempotency is |
There was a problem hiding this comment.
For git-refs — the only backend this touches — the next ordinary git push already drains the whole queue regardless of login state (prePushCheckpointRefs), which is what doctor migrate-checkpoints tells users: "Refs are queued; they push on the next git push." So it isn't stranded forever, it's pending until the next push. Worth correcting here, since as written this will convince the next maintainer the queue has no other drain trigger.
That makes the value of this PR "imported history syncs promptly" rather than "recovers otherwise-unrecoverable history" — still worth having, just a different claim.
| // | ||
| // Best-effort by design: the import itself already succeeded locally, so a push | ||
| // failure is reported and swallowed rather than failing the command. | ||
| func syncImportedCheckpoints(ctx context.Context, w io.Writer, repo *git.Repository, explicitRemote string) { |
There was a problem hiding this comment.
No WithNonInteractiveSSH and no timeout on the batch push, so a passphrase-protected key prompts mid-enable and a hung remote blocks it. The only other caller (doctor migrate-checkpoints) gates its drain behind CanPromptInteractively() + a confirm; this has neither.
| // entire/checkpoints/v1 branch, which the pre-push hook ships alongside the | ||
| // user's own push. Draining a queue it never fills would be a no-op that | ||
| // still paid for a checkpoint-policy sync, so skip the backend entirely. | ||
| cpCfg, cfgErr := settings.LoadCheckpointsConfig(ctx) |
There was a problem hiding this comment.
cfgErr is dropped with no log and no output, so an invalid checkpoints block silently disables the whole fix — the same silent stranding #1773 is about. Both other error paths here log.
| remote, err := importResolvePushRemote(ctx, explicitRemote) | ||
| if err != nil { | ||
| logging.Debug(ctx, "import: skipping checkpoint sync", "error", err.Error()) | ||
| fmt.Fprintf(w, "Note: imported history is not synced yet: %v\n", err) |
There was a problem hiding this comment.
Surfaces resolveCheckpointPushRemote's "pass --remote explicitly" — a flag entire enable doesn't have — and fires on the ordinary git init && entire enable no-remote-yet case, not only on errors.
| return | ||
| } | ||
|
|
||
| pushed, pushDisabled, err := importPushQueuedRefs(ctx, repo, remote) |
There was a problem hiding this comment.
Blocker: PushQueuedCheckpointRefs has no checkpointSyncAllowedForRemote gate (only prePush does, manual_commit_push.go:84), and --remote is returned verbatim without checking it is a configured remote. So import --remote fork (or a pasted URL) pushes refs/entire/checkpoints/*, transcripts included, there. CLAUDE.md: "Pushes to any other remote or to a raw URL never carry checkpoint data."
Separately: the policy fetch inside runs before the queue-empty check, so every logged-in import pays a remote round-trip with nothing to push — queue.Peek() (pushqueue.go:117) makes that cheap to skip.
| case pushed == 0: | ||
| // Nothing queued — everything this import produced was already synced. | ||
| default: | ||
| fmt.Fprintf(w, "Synced %d checkpoint(s) to %s.\n", pushed, remote) |
There was a problem hiding this comment.
Blocker: pushed is the whole repo-wide queue, not this import's refs, so 12 unpushed session checkpoints give Imported 0 turn(s)… Synced 12 checkpoint(s). Matters more if prompt sync is the point of the PR. And under checkpoint_remote the refs go to that URL, not to remote — stderr and stdout then name different destinations.
| warnIfImportNotSynced(w, importedLocalHistory) | ||
| // When it runs logged in, push what was just imported: enable creates no | ||
| // commit either, so nothing else would trigger the pre-push hook (#1773). | ||
| syncImportedCheckpoints(ctx, w, repo, "") |
There was a problem hiding this comment.
Ctrl-C breaks the loop above and lands here with a cancelled ctx: git config fails instantly and the swallow at checkpoint_sync_remote.go:120 prints no git remotes configured in a repo that has them (or a false checkpoint_push_remote misconfiguration). import_cmd.go:98 guards this; this path doesn't.
| cmd.Flags().StringVar(&pathFlag, "path", "", "Override the transcript directory to import from") | ||
| cmd.Flags().BoolVar(&dryRun, "dry-run", false, "Report what would be imported without writing") | ||
| cmd.Flags().StringSliceVar(&sessions, "session", nil, "Import only these session IDs (repeatable)") | ||
| cmd.Flags().StringVar(&remoteFlag, "remote", "", "Git remote to sync imported checkpoints to (default: the elected checkpoint sync remote)") |
There was a problem hiding this comment.
Worth validating this against the elected sync remote before use — see the gate note on import_sync.go:65.
| require.NoError(t, os.WriteFile(filepath.Join(claudeDir, "s.jsonl"), | ||
| []byte(`{"type":"user","uuid":"u1","message":{"role":"user","content":"hi"}}`+"\n"), 0o644)) | ||
|
|
||
| calls := stubImportSync(t, true, func(context.Context, *git.Repository, string) (int, bool, error) { |
There was a problem hiding this comment.
Doesn't reproduce #1773: both runs are loggedIn=true, so the logged-out precondition never exists, and the push is stubbed — so the claim the PR rests on (refs written while logged out are still queued afterwards) is never exercised. Breaking enqueue would leave this green.
| t.Chdir(tmpDir) | ||
|
|
||
| got, err := resolveMigratePushRemote(context.Background(), "explicit-remote") | ||
| got, err := resolveCheckpointPushRemote(context.Background(), "explicit-remote") |
There was a problem hiding this comment.
Rename the five TestResolveMigratePushRemote_* functions too — they now point at a symbol that no longer exists.
Fixes #1773 (fix 2 — sync after login). @karthik-rameshkumar handed this over on the issue after landing fix 1 (#1774).
The problem
entire importwrites read-only checkpoints and creates no commit, so the pre-push git hook — the only sync trigger — never fires for it. Because idempotency is keyed on local existence, history imported while logged out is stranded permanently: the post-login re-run reports0 turn(s) (N already imported)and makes no attempt to sync.The insight that keeps this small
The git-refs store already enqueues every checkpoint ref as it writes it (
gitRefsStore.setRef→enqueueForPush,checkpoint/refs_store.go:209), and nothing drains that queue but a push. So turns imported while logged out are still queued afterwards — the data is packed and addressed, nobody mails it.Draining the queue after a logged-in import is therefore enough to recover them, with no change to the import idempotency logic and no new push machinery.
What this adds
syncImportedCheckpoints(newimport_sync.go, ~70 lines), called from the two existingwarnIfImportNotSyncedsites —import_cmd.goandsetup_import.go. It reusesstrategy.PushQueuedCheckpointRefs, whose only current caller isdoctor migrate-checkpoints' opt-in "push now"; this is the same foreground-drain shape.Deliberately not gated on
TurnsImported > 0. That is the fix: a logged-in re-run that imports nothing new still pushes, which is what makes it the recovery path.Best-effort by design — the import already succeeded locally, so a push failure prints a
Note:and returns rather than failing the command;context.Canceledstays silent.Scope: git-refs only
Since #1900 made git-refs the sole
entire enabledefault, this covers every repo newly hitting the bug. git-branch has no push queue — its checkpoints ride theentire/checkpoints/v1branch the pre-push hook already ships — so it is skipped rather than paying for a checkpoint-policy sync to drain a queue it never fills. Those repos keep syncing exactly as they do today; no regression, just not fixed here. Happy to extend if you'd prefer one PR covering both.I looked at
ManualCommitStrategy.PrePushfirst (it covers both backends and has no production callers — onlyPrePushFromGitHookis wired, athooks_git_cmd.go:329). I went withPushQueuedCheckpointRefsinstead because it returns(pushed, pushDisabled, err)so the user can be told accurately what happened,PrePushpassesprotectFirstUserBranch: falseand so skips the empty-remote guard, and it forces non-interactive SSH — right for a hook, wrong for a foreground command.Remote resolution
resolveMigratePushRemote→resolveCheckpointPushRemote, now shared with the migration command: both are non-hook drain paths and must resolve the elected checkpoint sync remote the same fail-closed way (a misconfiguredcheckpoint_push_remoteis an error, never a fallback to origin).entire importgains--remoteto override, matchingdoctor migrate-checkpoints.Verification
End-to-end against a real bare remote, identical sequence on both binaries:
0 turn(s) (1 already imported)Synced 1 checkpoint(s) to origin.Tests in
import_sync_test.gocover: logged out never pushes; git-branch never pushes; success/disabled/empty-queue/failure/cancelled messaging;--remoteoverride; remote-resolution failure reported not fatal; dry-run never pushes. The regression test isTestImportSyncsAlreadyImportedTurns— imports twice and asserts the second run still syncs despite importing nothing new.mise run checkgreen: lint 0 issues, unit + integration pass, e2e canary 60/60 (vogon) and 4/4 (roger-roger).Open question for a maintainer
The issue's own closing note is still unanswered, and it affects this PR:
Relevant because
reportRepoEnabled(setup.go:976) is silently skipped when logged out, so a repo enabled before login is never reported to the backend. If pushing alone is not sufficient, this PR would print "Synced" while the dashboard stays empty, and the notice wording should probably also point at connecting the repo. Happy to follow up either way.First contribution here — glad to adjust any of the above.