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