From c97efe2028da503bab827deba79e612d12752bd9 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Mon, 5 Oct 2026 18:45:39 -0700 Subject: [PATCH] fix(opf): push the v1 commit the OPF rewrite verified, not the branch tip After the pre-push OPF rewrite, the git-branch path pushed entire/checkpoints/v1 by name, so a checkpoint another session appended between the rewrite and the push shipped without being scanned. v1 now ships as the exact commit the rewrite verified; a later append stays local and is rewritten on the next push. If the remote rejects the pinned push, v1 is still synced with the remote but not retried, since the rebased tip holds unverified commits. OPF skipped or off keeps pushing by name. Co-Authored-By: Claude Opus 5.5 Entire-Checkpoint: 01M47E6JTDJH7F4C49AM56YT9B --- .../manual_commit_opf_rewrite_test.go | 43 ++++++++++++++ cmd/entire/cli/strategy/manual_commit_push.go | 23 +++++++- cmd/entire/cli/strategy/push_common.go | 50 +++++++++++++++- cmd/entire/cli/strategy/push_common_test.go | 58 +++++++++++++++---- docs/security-and-privacy.md | 2 +- 5 files changed, 161 insertions(+), 15 deletions(-) diff --git a/cmd/entire/cli/strategy/manual_commit_opf_rewrite_test.go b/cmd/entire/cli/strategy/manual_commit_opf_rewrite_test.go index 66ca818a07..3cb4e5b4ff 100644 --- a/cmd/entire/cli/strategy/manual_commit_opf_rewrite_test.go +++ b/cmd/entire/cli/strategy/manual_commit_opf_rewrite_test.go @@ -312,6 +312,49 @@ func TestPrePushFromGitHook_DeferralStillRunsOPF(t *testing.T) { require.Equal(t, 1, fake.batchCallCount()) } +// A checkpoint another session appends to v1 after the OPF rewrite but before +// the push has not been scanned. The push ships the commit the rewrite verified +// and leaves the newer one local for the next push to rewrite. +func TestPrePush_V1PushesTheVerifiedCommitNotALaterAppend(t *testing.T) { + configureFakeOPF(t, &fakeOPFForRewrite{}) + + dir, repo, _ := setupV1RepoInDir(t) + remoteDir := filepath.Join(t.TempDir(), "origin.git") + _, err := git.PlainInit(remoteDir, true) + require.NoError(t, err) + _, err = repo.CreateRemote(&gitconfig.RemoteConfig{Name: "origin", URLs: []string{remoteDir}}) + require.NoError(t, err) + + t.Chdir(dir) + paths.ClearWorktreeRootCache() + t.Cleanup(paths.ClearWorktreeRootCache) + + var verified, appended plumbing.Hash + old := rewriteUnpushedV1ForPush + rewriteUnpushedV1ForPush = func(ctx context.Context, r *git.Repository, target string) (plumbing.Hash, error) { + h, rewriteErr := old(ctx, r, target) + verified = h + appended = addV1Checkpoint(t, repo, "b2c3d4e5f6a1", "sess-late", "PERSONABC arrived late", "late prompt") + return h, rewriteErr + } + t.Cleanup(func() { rewriteUnpushedV1ForPush = old }) + + // PrePush, not the hook entry point: the empty-remote deferral would + // otherwise skip the automatic v1 push this test is about. + require.NoError(t, NewManualCommitStrategy().PrePush(t.Context(), "origin")) + + require.False(t, verified.IsZero()) + require.NotEqual(t, verified, appended) + assert.Equal(t, verified.String(), remoteRefHash(t, remoteDir, plumbing.NewBranchReferenceName(paths.MetadataBranchName)), + "the remote must receive the verified commit, not the later unscanned append") + local, err := repo.Reference(plumbing.NewBranchReferenceName(paths.MetadataBranchName), true) + require.NoError(t, err) + assert.Equal(t, appended, local.Hash(), "the unscanned append stays local") + late, err := repo.CommitObject(appended) + require.NoError(t, err) + assert.False(t, trailers.HasOPFApplied(late.Message)) +} + // Regression: the no-categories abort must reach the git hook boundary. // PrePush deliberately swallows transient checkpoint-push failures, so // without this pin a refactor could downgrade the rewrite's fail-closed diff --git a/cmd/entire/cli/strategy/manual_commit_push.go b/cmd/entire/cli/strategy/manual_commit_push.go index 004737379f..5d9faf48bd 100644 --- a/cmd/entire/cli/strategy/manual_commit_push.go +++ b/cmd/entire/cli/strategy/manual_commit_push.go @@ -16,6 +16,7 @@ import ( "github.com/entireio/cli/cmd/entire/cli/checkpoint" checkpointremote "github.com/entireio/cli/cmd/entire/cli/checkpoint/remote" "github.com/entireio/cli/cmd/entire/cli/logging" + "github.com/entireio/cli/cmd/entire/cli/paths" "github.com/entireio/cli/cmd/entire/cli/settings" "github.com/entireio/cli/perf" "github.com/entireio/cli/redact" @@ -33,6 +34,10 @@ var ErrOPFAbortedByUser = errors.New("OPF prompt aborted by user; push cancelled var opfPrePushProgressWriter io.Writer = os.Stderr +// rewriteUnpushedV1ForPush lets a test append to v1 between the rewrite and +// the push. +var rewriteUnpushedV1ForPush = RewriteUnpushedV1WithOPF //nolint:gochecknoglobals // concurrent-append test seam + // PrePush is called by the git pre-push hook before pushing to a remote. // It pushes each ref in refs.Push alongside the user's push. // @@ -126,6 +131,11 @@ func (s *ManualCommitStrategy) prePush(ctx context.Context, remote string, prote // re-redact unpushed v1 commits with OPF (producing the OPF-applied, // 9-layer pipeline) before pushing. Skipped entirely when OPF is off, // so the common-case fast path is unchanged. + // + // After a rewrite, v1 ships as the exact commit the rewrite verified + // (opfVerifiedV1), never by name: a checkpoint another session appends to + // the local branch before the push runs has not been scanned. + opfPinnedV1, opfVerifiedV1 := false, plumbing.ZeroHash if redact.OPFEnabled() { decision, decisionErr := opfPrePushDecision(ctx) if decisionErr != nil { @@ -160,7 +170,8 @@ func (s *ManualCommitStrategy) prePush(ctx context.Context, remote string, prote openSpan.End() defer repo.Close() _, opfSpan := perf.Start(ctx, "opf_pre_push_rewrite") - if _, rewriteErr := RewriteUnpushedV1WithOPF(ctx, repo, ps.pushTarget()); rewriteErr != nil { + verified, rewriteErr := rewriteUnpushedV1ForPush(ctx, repo, ps.pushTarget()) + if rewriteErr != nil { opfSpan.RecordError(rewriteErr) opfSpan.End() logging.Warn(ctx, "OPF pre-push rewrite failed; aborting push", @@ -169,6 +180,7 @@ func (s *ManualCommitStrategy) prePush(ctx context.Context, remote string, prote return rewriteErr } opfSpan.End() + opfPinnedV1, opfVerifiedV1 = true, verified } } @@ -191,8 +203,15 @@ func (s *ManualCommitStrategy) prePush(ctx context.Context, remote string, prote // fail-soft and nil even when the remote refused the ref. deliveredCount, anyFailed := 0, false refs := checkpoint.ResolveRefs(ctx) + v1Ref := plumbing.NewBranchReferenceName(paths.MetadataBranchName) for _, ref := range refs.Push { - delivered, err := pushRefIfNeeded(pushCtx, ps.pushTarget(), ref) + var delivered bool + var err error + if opfPinnedV1 && ref == v1Ref { + delivered, err = pushVerifiedRefIfNeeded(pushCtx, ps.pushTarget(), ref, opfVerifiedV1) + } else { + delivered, err = pushRefIfNeeded(pushCtx, ps.pushTarget(), ref) + } if err != nil { pushCheckpointsSpan.RecordError(err) pushCheckpointsSpan.End() diff --git a/cmd/entire/cli/strategy/push_common.go b/cmd/entire/cli/strategy/push_common.go index 79b832fb0a..1fd5bccf02 100644 --- a/cmd/entire/cli/strategy/push_common.go +++ b/cmd/entire/cli/strategy/push_common.go @@ -173,6 +173,29 @@ func pushRefIfNeeded(ctx context.Context, target string, ref plumbing.ReferenceN return doPushRef(ctx, target, ref) } +// pushVerifiedRefIfNeeded is pushRefIfNeeded for content OPF has verified: it +// pushes exactly verified and never re-reads ref, because anything appended to +// the local ref since verification has not been scanned. A zero verified hash +// means nothing was verified, so nothing is pushed. +func pushVerifiedRefIfNeeded(ctx context.Context, target string, ref plumbing.ReferenceName, verified plumbing.Hash) (delivered bool, err error) { + if verified.IsZero() { + return false, nil + } + repo, err := OpenRepository(ctx) + if err != nil { + logging.Debug(ctx, "push skipped: open repository failed", + slog.String("ref", ref.String()), + slog.String("error", err.Error())) + return false, nil + } + defer repo.Close() + + if ref.IsBranch() && !remote.IsURL(target) && !hasUnpushedBranchRef(repo, target, verified, ref.Short()) { + return true, nil + } + return doPushRefAt(ctx, target, ref, verified) +} + // hasUnpushedBranchRef checks if the local branch differs from the remote. // Returns true if there's any difference that needs syncing (local ahead, remote ahead, or diverged). func hasUnpushedBranchRef(repo *git.Repository, remoteName string, localHash plumbing.Hash, branchName string) bool { @@ -257,6 +280,15 @@ func flushAbortReason(ctx context.Context, consecutiveFailures int) string { // delivery, such as latching the captured checkpoint sync remote, must read // delivered and not err. func doPushRef(ctx context.Context, target string, ref plumbing.ReferenceName) (delivered bool, err error) { + return doPushRefAt(ctx, target, ref, plumbing.ZeroHash) +} + +// doPushRefAt is doPushRef pushing src instead of ref's current local tip when +// src is non-zero. A pinned push still syncs ref after a rejection, so the +// next push starts from the remote's tip, but does not retry: the rebased tip +// holds whatever the local ref held at sync time, which src was chosen to +// exclude. +func doPushRefAt(ctx context.Context, target string, ref plumbing.ReferenceName, src plumbing.Hash) (delivered bool, err error) { ctx, cancel := context.WithTimeout(ctx, checkpointPushBudget) defer cancel() @@ -267,7 +299,7 @@ func doPushRef(ctx context.Context, target string, ref plumbing.ReferenceName) ( stop := startProgressDots(os.Stderr) // Try pushing first - result, err := tryPushRefCommon(ctx, target, ref) + result, err := tryPushRefAt(ctx, target, ref, src) if err == nil { finishPush(ctx, stop, result, target) return true, nil @@ -312,6 +344,11 @@ func doPushRef(ctx context.Context, target string, ref plumbing.ReferenceName) ( } stop(" done") + if !src.IsZero() { + fmt.Fprintf(os.Stderr, "[entire] %s will be pushed on your next push\n", refLabel) + return false, nil + } + // Try pushing again after rebase fmt.Fprintf(os.Stderr, "[entire] Pushing %s to %s...", refLabel, displayTarget) stop = startProgressDots(os.Stderr) @@ -453,8 +490,17 @@ func finishPush(ctx context.Context, stop func(string), result pushResult, targe // server-side ref protection). This keeps one consistent non-force policy for // every checkpoint ref, branch or per-checkpoint. func tryPushRefCommon(ctx context.Context, remoteName string, ref plumbing.ReferenceName) (pushResult, error) { + return tryPushRefAt(ctx, remoteName, ref, plumbing.ZeroHash) +} + +// tryPushRefAt is tryPushRefCommon pushing src to ref when src is non-zero. +// git still updates the remote-tracking ref for a ":refs/heads/..." push. +func tryPushRefAt(ctx context.Context, remoteName string, ref plumbing.ReferenceName, src plumbing.Hash) (pushResult, error) { refSpec := ref.Short() - if !ref.IsBranch() { + switch { + case !src.IsZero(): + refSpec = src.String() + ":" + ref.String() + case !ref.IsBranch(): refSpec = ref.String() + ":" + ref.String() } diff --git a/cmd/entire/cli/strategy/push_common_test.go b/cmd/entire/cli/strategy/push_common_test.go index 9b9a8ebb86..527d8d88ff 100644 --- a/cmd/entire/cli/strategy/push_common_test.go +++ b/cmd/entire/cli/strategy/push_common_test.go @@ -336,19 +336,15 @@ func TestFetchAndRebase_NonBranchRefDisconnected(t *testing.T) { assert.ErrorIs(t, err, plumbing.ErrReferenceNotFound, "non-branch reconciliation must not create the primary ref") } -// TestFetchAndRebase_DivergedBranches verifies that when local and remote -// metadata branches have diverged (shared ancestor, different commits on each), -// fetchAndRebaseRefCommon produces a linear history (no merge commits) -// with all data from both sides preserved. -// -// Not parallel: uses t.Chdir() (required for OpenRepository). -func TestFetchAndRebase_DivergedBranches(t *testing.T) { - ctx := context.Background() +// setupDivergedMetadataClones returns a bare origin and clone A, whose local +// metadata branch holds checkpoint bb while origin's holds cc on the same base. +func setupDivergedMetadataClones(t *testing.T) (bareDir, cloneA string) { + t.Helper() testutil.IsolateGitConfigEnv(t) branchName := paths.MetadataBranchName // 1. Create bare origin with a metadata branch containing a base checkpoint - bareDir := t.TempDir() + bareDir = t.TempDir() workDir := t.TempDir() gitRun := func(dir string, args ...string) { testutil.RunGit(t, dir, args...) @@ -378,7 +374,7 @@ func TestFetchAndRebase_DivergedBranches(t *testing.T) { gitRun(workDir, "checkout", "main") // 2. Clone into two separate working directories - cloneA := filepath.Join(t.TempDir(), "cloneA") + cloneA = filepath.Join(t.TempDir(), "cloneA") cloneB := filepath.Join(t.TempDir(), "cloneB") require.NoError(t, os.MkdirAll(cloneA, 0o755)) require.NoError(t, os.MkdirAll(cloneB, 0o755)) @@ -419,6 +415,20 @@ func TestFetchAndRebase_DivergedBranches(t *testing.T) { gitRun(cloneB, "push", "origin", branchName) gitRun(cloneB, "checkout", "main") + return bareDir, cloneA +} + +// TestFetchAndRebase_DivergedBranches verifies that when local and remote +// metadata branches have diverged (shared ancestor, different commits on each), +// fetchAndRebaseRefCommon produces a linear history (no merge commits) +// with all data from both sides preserved. +// +// Not parallel: uses t.Chdir() (required for OpenRepository). +func TestFetchAndRebase_DivergedBranches(t *testing.T) { + ctx := context.Background() + branchName := paths.MetadataBranchName + _, cloneA := setupDivergedMetadataClones(t) + // 5. Run fetchAndRebaseRefCommon on clone A (diverged: local has bb, remote has cc) t.Chdir(cloneA) @@ -459,6 +469,34 @@ func TestFetchAndRebase_DivergedBranches(t *testing.T) { assert.Contains(t, entries, "cc/cccccccccc/metadata.json", "remote checkpoint should be preserved") } +// TestDoPushRefAt_PinnedRejectionSyncsWithoutRetry pins the pinned-push +// recovery: after a rejection the ref is still synced, so the next push starts +// from the remote's tip, but the rebased tip is not pushed, because it holds +// whatever the local ref held at sync time rather than the verified commit. +// +// Not parallel: uses t.Chdir() (required for OpenRepository). +func TestDoPushRefAt_PinnedRejectionSyncsWithoutRetry(t *testing.T) { + ctx := context.Background() + refName := plumbing.NewBranchReferenceName(paths.MetadataBranchName) + bareDir, cloneA := setupDivergedMetadataClones(t) + t.Chdir(cloneA) + + repo, err := git.PlainOpen(cloneA) + require.NoError(t, err) + verified, err := repo.Reference(refName, true) + require.NoError(t, err) + remoteBefore := remoteRefHash(t, bareDir, refName) + + delivered, err := doPushRefAt(ctx, "origin", refName, verified.Hash()) + require.NoError(t, err) + assert.False(t, delivered, "a pinned push that was rejected must not report delivery") + assert.Equal(t, remoteBefore, remoteRefHash(t, bareDir, refName), "the rebased tip must not be pushed") + + synced, err := repo.Reference(refName, true) + require.NoError(t, err) + assert.NotEqual(t, verified.Hash(), synced.Hash(), "the local ref is still synced onto the remote") +} + // TestFetchAndRebase_SharedCloneLocalCommitInAlternate verifies that the // metadata branch replay path can read local-only commits that are present via // .git/objects/info/alternates. Git CLI can see these objects, but go-git may diff --git a/docs/security-and-privacy.md b/docs/security-and-privacy.md index a5e2af7a8d..0296b2d0e2 100644 --- a/docs/security-and-privacy.md +++ b/docs/security-and-privacy.md @@ -303,7 +303,7 @@ set -x ENTIRE_OPF_BATCH_LIMIT unlimited; git push The three caps protect different failure modes: the commit cap stops "100 throwaway commits", the batch cap stops "one commit with 50 MB of prose", and the raw-byte cap stops the loader exhausting memory before either of the others can be evaluated. On `git-refs` the two byte caps are cumulative across the whole flush (all queued refs together) while the commit cap is per ref. -**Concurrent push** from another worktree: both backends compare-and-swap the local ref. If another process moved it while OPF was running, `git-branch` exits with `entire/checkpoints/v1 moved during OPF rewrite …; re-run 'git push' (no fetch needed; the move was local)` and aborts. On `git-refs` the affected ref simply stays queued and the next push picks it up. Note that the `git-refs` rewrite rebuilds every commit before touching any ref, but the ref updates themselves are not atomic *across* refs: a conflict partway through leaves the earlier refs already rewritten. They stay queued and push OPF-applied next time, so this is safe — just not "nothing moved". +**Concurrent push** from another worktree: both backends compare-and-swap the local ref. If another process moved it while OPF was running, `git-branch` exits with `entire/checkpoints/v1 moved during OPF rewrite …; re-run 'git push' (no fetch needed; the move was local)` and aborts. A checkpoint appended after the rewrite finishes is not pushed: `git-branch` pushes the exact commit the rewrite verified, and the newer one is rewritten on the next push. On `git-refs` the affected ref simply stays queued and the next push picks it up. Note that the `git-refs` rewrite rebuilds every commit before touching any ref, but the ref updates themselves are not atomic *across* refs: a conflict partway through leaves the earlier refs already rewritten. They stay queued and push OPF-applied next time, so this is safe — just not "nothing moved". #### Persistence of un-redacted-by-OPF content