From f8f218b9efdcd3994a2186b360d09b171bc2b824 Mon Sep 17 00:00:00 2001 From: Suhaan Thayyil <257360244+suhaanthayyil@users.noreply.github.com> Date: Sat, 11 Jul 2026 21:03:10 -0400 Subject: [PATCH 01/11] fix(gitrepo): support reftable ref storage Opening a repository that uses the reftable ref backend (git init --ref-format=reftable, or git refs migrate --ref-format=reftable) failed with "core.repositoryformatversion does not support extension: refstorage", so `entire enable` and every capture operation aborted. Root cause: the vendored go-git v6 alpha has no reftable reader. Its filesystem storer reads refs from .git/refs, .git/packed-refs and .git/HEAD (a reftable repo's HEAD is a "ref: refs/heads/.invalid" stub and the real refs live in the binary reftable stack), and its extension verification rejects extensions.refstorage=reftable outright. Upstream reftable support has not landed: go-git PR #1970 was closed unmerged and go-git issue #1827 is still open, so we cannot wait for the dependency. Fix: detect reftable repositories (presence of a reftable/ directory under the worktree or common git dir) in gitrepo.OpenPath and wrap go-git's filesystem storage with a reftableStorer. Object, config, index and shallow storage keep flowing through go-git (reftable changes only ref storage, not object storage); the ReferenceStorer methods (SetReference, CheckAndSetReference, Reference, IterReferences, RemoveReference, CountLooseRefs, PackRefs) are overridden to shell out to git plumbing (update-ref, symbolic-ref, rev-parse, for-each-ref), and the storer advertises SupportsExtension("refstorage") so go-git's open-time extension check passes. This extends the existing "prefer the git CLI where go-git is unsafe" seam already used for reset/checkout (HardResetWithProtection) and fetch rather than inventing a new architecture, and routes every ref read/write through a single point so all callers (enable, session start, shadow branches, condensation, the checkpoints branch, checkpoint list) work unchanged. Ref plumbing does not fire git hooks, so this cannot recurse back into entire. Verified end to end against reftable repos: enable, session start, file changes, stop, user commit with Entire-Checkpoint trailer, checkpoint list, and a linked worktree of a reftable repo, with no regression on the classic files backend or sha256 object format. Fixes #547 --- cmd/entire/cli/gitrepo/reftable.go | 259 ++++++++++++++++++ cmd/entire/cli/gitrepo/reftable_test.go | 112 ++++++++ cmd/entire/cli/gitrepo/repository.go | 18 +- .../integration_test/reftable_repo_test.go | 186 +++++++++++++ 4 files changed, 574 insertions(+), 1 deletion(-) create mode 100644 cmd/entire/cli/gitrepo/reftable.go create mode 100644 cmd/entire/cli/gitrepo/reftable_test.go create mode 100644 cmd/entire/cli/integration_test/reftable_repo_test.go diff --git a/cmd/entire/cli/gitrepo/reftable.go b/cmd/entire/cli/gitrepo/reftable.go new file mode 100644 index 0000000000..f001291610 --- /dev/null +++ b/cmd/entire/cli/gitrepo/reftable.go @@ -0,0 +1,259 @@ +package gitrepo + +import ( + "bufio" + "bytes" + "context" + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + "time" + + "github.com/go-git/go-git/v6/plumbing" + "github.com/go-git/go-git/v6/plumbing/storer" + gogitstorage "github.com/go-git/go-git/v6/storage" + gitfilesystem "github.com/go-git/go-git/v6/storage/filesystem" +) + +// reftableGitTimeout bounds each git plumbing invocation the reftable storer +// makes. Ref reads/writes against the local reftable stack are fast; the +// timeout only guards against a wedged git process. +const reftableGitTimeout = 30 * time.Second + +// repoUsesReftable reports whether the repository at the given git directories +// stores its references using the reftable backend rather than the classic +// loose-files + packed-refs layout. +// +// go-git (through the vendored v6 alpha) has no reftable reader: its filesystem +// storer reads refs from .git/refs, .git/packed-refs and .git/HEAD, none of +// which are authoritative in a reftable repository. Detection lets us route ref +// operations through the git CLI instead (see reftableStorer). +// +// A reftable repository is identified by the presence of a "reftable/" +// directory under either the worktree git dir or the common git dir. Git 2.45+ +// creates this directory for `git init --ref-format=reftable` and for +// `git refs migrate --ref-format=reftable`. Checking the directory avoids +// parsing config and works for linked worktrees, where the shared stack lives +// under the common git dir. +func repoUsesReftable(dotGitPath, commonGitPath string) bool { + candidates := []string{filepath.Join(dotGitPath, "reftable")} + if commonGitPath != "" && commonGitPath != dotGitPath { + candidates = append(candidates, filepath.Join(commonGitPath, "reftable")) + } + for _, dir := range candidates { + if info, err := os.Stat(dir); err == nil && info.IsDir() { + return true + } + } + return false +} + +// reftableStorer adapts a filesystem-backed go-git storage so it can open and +// operate on repositories that use the reftable ref backend. Object, config, +// index, shallow and module storage keep flowing through the embedded +// filesystem storage (reftable only changes ref storage, not object storage); +// the reference-storer methods are overridden to shell out to the git CLI, +// which is the only reftable reader/writer available to us. +// +// It also advertises reftable support via the ExtensionChecker interface so +// go-git's extension verification does not reject the repository on open. +type reftableStorer struct { + *gitfilesystem.Storage + + gitDir string +} + +var ( + _ gogitstorage.Storer = (*reftableStorer)(nil) + // The concrete methods below satisfy storer.ReferenceStorer, overriding the + // embedded filesystem implementation. + _ storer.ReferenceStorer = (*reftableStorer)(nil) +) + +// newReftableStorer wraps a filesystem storage with reftable-aware reference +// handling. gitDir must be the repository's git directory (for a linked +// worktree, the worktree's git dir); git resolves the shared reftable stack +// from its commondir automatically. +func newReftableStorer(fs *gitfilesystem.Storage, gitDir string) *reftableStorer { + return &reftableStorer{Storage: fs, gitDir: gitDir} +} + +// SupportsExtension lets go-git open a repository that declares the +// extensions.refstorage=reftable extension. Object-format and other extensions +// remain the responsibility of the embedded filesystem storage / go-git core. +func (s *reftableStorer) SupportsExtension(name, _ string) bool { + return strings.EqualFold(name, "refstorage") +} + +// runGit runs a git plumbing command scoped to this repository's git dir and +// returns trimmed stdout. Only ref plumbing (show-ref, for-each-ref, +// symbolic-ref, update-ref, rev-parse) is used, none of which trigger git +// hooks, so this cannot recurse back into entire. +func (s *reftableStorer) runGit(args ...string) (string, []byte, error) { + ctx, cancel := context.WithTimeout(context.Background(), reftableGitTimeout) + defer cancel() + + full := append([]string{"--git-dir", s.gitDir}, args...) + cmd := exec.CommandContext(ctx, "git", full...) + cmd.Env = append(os.Environ(), "GIT_TERMINAL_PROMPT=0") + + var stdout, stderr bytes.Buffer + cmd.Stdout = &stdout + cmd.Stderr = &stderr + err := cmd.Run() + return strings.TrimRight(stdout.String(), "\n"), stderr.Bytes(), err +} + +// SetReference stores a reference, dispatching to symbolic-ref for symbolic +// references and update-ref for hash references. +func (s *reftableStorer) SetReference(ref *plumbing.Reference) error { + if ref == nil { + return nil + } + if ref.Type() == plumbing.SymbolicReference { + if _, stderr, err := s.runGit("symbolic-ref", ref.Name().String(), ref.Target().String()); err != nil { + return fmt.Errorf("reftable set symbolic ref %s: %s: %w", ref.Name(), strings.TrimSpace(string(stderr)), err) + } + return nil + } + if _, stderr, err := s.runGit("update-ref", ref.Name().String(), ref.Hash().String()); err != nil { + return fmt.Errorf("reftable set ref %s: %s: %w", ref.Name(), strings.TrimSpace(string(stderr)), err) + } + return nil +} + +// CheckAndSetReference performs a compare-and-swap update. When old is non-nil +// the update is conditioned on the current value, mirroring go-git's atomic +// semantics; a mismatch is reported as storage.ErrReferenceHasChanged. +func (s *reftableStorer) CheckAndSetReference(newRef, old *plumbing.Reference) error { + if newRef == nil { + return nil + } + if newRef.Type() == plumbing.SymbolicReference { + // Symbolic refs (e.g. HEAD) have no CAS form in update-ref; set directly. + return s.SetReference(newRef) + } + if old == nil { + return s.SetReference(newRef) + } + if _, _, err := s.runGit("update-ref", newRef.Name().String(), newRef.Hash().String(), old.Hash().String()); err != nil { + // git update-ref fails when the stored value differs from the expected + // old value; surface the sentinel go-git callers check for. + return gogitstorage.ErrReferenceHasChanged + } + return nil +} + +// Reference returns the reference with the given name, preserving symbolic refs +// (such as HEAD) so go-git can resolve them itself. +func (s *reftableStorer) Reference(name plumbing.ReferenceName) (*plumbing.Reference, error) { + // Symbolic refs: symbolic-ref exits 0 and prints the target only for a + // genuine symbolic ref; -q suppresses the error for non-symbolic names. + if target, _, err := s.runGit("symbolic-ref", "-q", name.String()); err == nil && target != "" { + return plumbing.NewSymbolicReference(name, plumbing.ReferenceName(target)), nil + } + + // Hash refs: rev-parse --verify resolves the ref to the object it points at. + // "^0" would peel tags; we want the ref's direct target, so verify the name + // as-is. A non-existent ref exits non-zero. + out, _, err := s.runGit("rev-parse", "--verify", "--quiet", name.String()) + if err != nil || out == "" { + return nil, plumbing.ErrReferenceNotFound + } + h := plumbing.NewHash(out) + if h.IsZero() { + return nil, plumbing.ErrReferenceNotFound + } + return plumbing.NewHashReference(name, h), nil +} + +// IterReferences returns an iterator over every reference in the repository, +// including HEAD, matching the behaviour of the filesystem storer. +func (s *reftableStorer) IterReferences() (storer.ReferenceIter, error) { + refs := make([]*plumbing.Reference, 0, 16) + + // HEAD (symbolic on a branch, detached hash otherwise) is not emitted by + // for-each-ref, so resolve it explicitly first. + if target, _, err := s.runGit("symbolic-ref", "-q", "HEAD"); err == nil && target != "" { + refs = append(refs, plumbing.NewSymbolicReference(plumbing.HEAD, plumbing.ReferenceName(target))) + } else if out, _, err := s.runGit("rev-parse", "--verify", "--quiet", "HEAD"); err == nil && out != "" { + if h := plumbing.NewHash(out); !h.IsZero() { + refs = append(refs, plumbing.NewHashReference(plumbing.HEAD, h)) + } + } + + // All refs under refs/. %(symref) is non-empty only for symbolic refs so we + // preserve their symbolic nature (e.g. refs/remotes/origin/HEAD). + out, stderr, err := s.runGit("for-each-ref", "--format=%(objectname) %(refname) %(symref)") + if err != nil { + return nil, fmt.Errorf("reftable iterate refs: %s: %w", strings.TrimSpace(string(stderr)), err) + } + scanner := bufio.NewScanner(strings.NewReader(out)) + scanner.Buffer(make([]byte, 0, 64*1024), 1024*1024) + for scanner.Scan() { + line := scanner.Text() + if strings.TrimSpace(line) == "" { + continue + } + fields := strings.SplitN(line, " ", 3) + if len(fields) < 2 { + continue + } + objectName, refName := fields[0], fields[1] + symref := "" + if len(fields) == 3 { + symref = strings.TrimSpace(fields[2]) + } + if symref != "" { + refs = append(refs, plumbing.NewSymbolicReference(plumbing.ReferenceName(refName), plumbing.ReferenceName(symref))) + continue + } + h := plumbing.NewHash(objectName) + if h.IsZero() { + continue + } + refs = append(refs, plumbing.NewHashReference(plumbing.ReferenceName(refName), h)) + } + if err := scanner.Err(); err != nil { + return nil, fmt.Errorf("reftable scan refs: %w", err) + } + + return storer.NewReferenceSliceIter(refs), nil +} + +// RemoveReference deletes a reference. Deleting a missing ref is treated as a +// success so callers can remove idempotently, matching go-git's semantics. +func (s *reftableStorer) RemoveReference(name plumbing.ReferenceName) error { + // A symbolic ref must be deleted with symbolic-ref -d; update-ref -d on a + // symbolic ref would delete the ref it points at. + if target, _, err := s.runGit("symbolic-ref", "-q", name.String()); err == nil && target != "" { + if _, stderr, delErr := s.runGit("symbolic-ref", "-d", name.String()); delErr != nil { + return fmt.Errorf("reftable remove symbolic ref %s: %s: %w", name, strings.TrimSpace(string(stderr)), delErr) + } + return nil + } + _, stderr, err := s.runGit("update-ref", "-d", name.String()) + if err != nil { + msg := strings.ToLower(strings.TrimSpace(string(stderr))) + // Deleting an already-absent ref is not an error for our callers. + if msg == "" || strings.Contains(msg, "does not exist") || strings.Contains(msg, "not exist") { + return nil + } + return fmt.Errorf("reftable remove ref %s: %s: %w", name, strings.TrimSpace(string(stderr)), err) + } + return nil +} + +// CountLooseRefs returns 0: reftable has no loose refs, and go-git only uses +// this count to decide whether to pack loose refs, which is a no-op here. +func (s *reftableStorer) CountLooseRefs() (int, error) { + return 0, nil +} + +// PackRefs is a no-op: the reftable backend maintains its own compaction, so +// there is nothing for go-git to pack. +func (s *reftableStorer) PackRefs() error { + return nil +} diff --git a/cmd/entire/cli/gitrepo/reftable_test.go b/cmd/entire/cli/gitrepo/reftable_test.go new file mode 100644 index 0000000000..3ae62df1dd --- /dev/null +++ b/cmd/entire/cli/gitrepo/reftable_test.go @@ -0,0 +1,112 @@ +package gitrepo + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/go-git/go-git/v6/plumbing" + "github.com/stretchr/testify/require" +) + +// initReftableRepo creates a repository using the reftable ref backend via the +// git CLI, or skips the test when the installed git is too old to support it. +// It returns the repo dir and the initial commit hash. +func initReftableRepo(t *testing.T, name, content string) (string, string) { + t.Helper() + repoDir := t.TempDir() + + env := append(os.Environ(), + "GIT_CONFIG_GLOBAL="+filepath.Join(t.TempDir(), "gitconfig"), + "GIT_CONFIG_SYSTEM=/dev/null", + "GIT_TERMINAL_PROMPT=0", + ) + + initCmd := exec.Command("git", "init", "-b", "main", "--ref-format=reftable", repoDir) //nolint:noctx // test capability probe + initCmd.Env = env + if out, err := initCmd.CombinedOutput(); err != nil { + t.Skipf("git does not support reftable repositories: %v\n%s", err, out) + } + + git := func(args ...string) string { + t.Helper() + cmd := exec.Command("git", args...) //nolint:noctx // test helper + cmd.Dir = repoDir + cmd.Env = env + out, err := cmd.CombinedOutput() + require.NoErrorf(t, err, "git %s: %s", strings.Join(args, " "), out) + return strings.TrimSpace(string(out)) + } + + if got := git("rev-parse", "--show-ref-format"); got != "reftable" { + t.Skipf("git initialized ref format %q, not reftable", got) + } + git("config", "user.name", "Test User") + git("config", "user.email", "test@example.com") + git("config", "commit.gpgsign", "false") + require.NoError(t, os.WriteFile(filepath.Join(repoDir, name), []byte(content), 0o644)) + git("add", name) + git("commit", "-m", "initial") + + return repoDir, git("rev-parse", "HEAD") +} + +// TestOpenPath_ReftableRepository verifies that a reftable repository, which +// go-git's filesystem storer cannot open, is opened successfully and that ref +// read/write/list/remove all round-trip through the git-CLI-backed storer. +func TestOpenPath_ReftableRepository(t *testing.T) { + t.Parallel() + repoDir, headHash := initReftableRepo(t, "file.txt", "hello\n") + + repo, err := OpenPath(repoDir) + require.NoError(t, err, "reftable repository should open") + defer repo.Close() + + // HEAD resolves to the real branch, not the reftable .invalid stub. + head, err := repo.Head() + require.NoError(t, err) + require.Equal(t, "refs/heads/main", head.Name().String()) + require.Equal(t, headHash, head.Hash().String()) + + // Write a new ref via go-git (routed through git update-ref) and read it back. + newRef := plumbing.NewHashReference(plumbing.ReferenceName("refs/entire/test/one"), head.Hash()) + require.NoError(t, repo.Storer.SetReference(newRef)) + + got, err := repo.Storer.Reference(newRef.Name()) + require.NoError(t, err) + require.Equal(t, head.Hash(), got.Hash()) + + // The new ref appears in iteration. + iter, err := repo.Storer.IterReferences() + require.NoError(t, err) + found := false + require.NoError(t, iter.ForEach(func(r *plumbing.Reference) error { + if r.Name() == newRef.Name() { + found = true + } + return nil + })) + iter.Close() + require.True(t, found, "written ref should appear in IterReferences") + + // Removal round-trips, and removing again is a no-op. + require.NoError(t, repo.Storer.RemoveReference(newRef.Name())) + _, err = repo.Storer.Reference(newRef.Name()) + require.ErrorIs(t, err, plumbing.ErrReferenceNotFound) + require.NoError(t, repo.Storer.RemoveReference(newRef.Name())) +} + +// TestRepoUsesReftable_Detection checks that reftable detection distinguishes +// reftable repositories from classic files-backend repositories. +func TestRepoUsesReftable_Detection(t *testing.T) { + t.Parallel() + + reftableRepo, _ := initReftableRepo(t, "a.txt", "a\n") + require.True(t, repoUsesReftable(filepath.Join(reftableRepo, ".git"), filepath.Join(reftableRepo, ".git"))) + + filesRepo := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(filesRepo, ".git", "refs"), 0o755)) + require.False(t, repoUsesReftable(filepath.Join(filesRepo, ".git"), filepath.Join(filesRepo, ".git"))) +} diff --git a/cmd/entire/cli/gitrepo/repository.go b/cmd/entire/cli/gitrepo/repository.go index 74adc359f3..fbef9888fd 100644 --- a/cmd/entire/cli/gitrepo/repository.go +++ b/cmd/entire/cli/gitrepo/repository.go @@ -118,7 +118,23 @@ func openPathWithAlternates(repoRoot string) (*git.Repository, error) { AlternatesFS: newAlternatesFilesystem(), }, ) - repo, err := git.Open(storage, osfs.New(repoRoot, osfs.WithBoundOS())) + + // go-git's filesystem storer cannot read the reftable ref backend: it reads + // refs from .git/refs, packed-refs and .git/HEAD, none of which are + // authoritative in a reftable repository, and its extension check rejects + // extensions.refstorage=reftable outright. Route ref operations through the + // git CLI for such repositories while keeping object storage on go-git. + worktreeFS := osfs.New(repoRoot, osfs.WithBoundOS()) + if repoUsesReftable(dotGitPath, commonGitPath) { + repo, err := git.Open(newReftableStorer(storage, dotGitPath), worktreeFS) + if err != nil { + _ = storage.Close() + return nil, fmt.Errorf("open reftable repository storage: %w", err) + } + return repo, nil + } + + repo, err := git.Open(storage, worktreeFS) if err != nil { _ = storage.Close() return nil, fmt.Errorf("open repository storage: %w", err) diff --git a/cmd/entire/cli/integration_test/reftable_repo_test.go b/cmd/entire/cli/integration_test/reftable_repo_test.go new file mode 100644 index 0000000000..82ca2dd8b8 --- /dev/null +++ b/cmd/entire/cli/integration_test/reftable_repo_test.go @@ -0,0 +1,186 @@ +//go:build integration + +package integration + +import ( + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/entireio/cli/cmd/entire/cli/paths" + "github.com/entireio/cli/cmd/entire/cli/testutil" +) + +// TestReftableRepository_EnableAndFirstCheckpoint exercises the full capture +// flow (enable -> session start -> file changes -> stop -> user commit -> +// checkpoint) against a repository using the reftable ref backend. go-git's +// filesystem storer cannot read reftable refs, so this verifies the git-CLI +// ref routing in gitrepo.reftableStorer works end to end. Regression for #547. +func TestReftableRepository_EnableAndFirstCheckpoint(t *testing.T) { + t.Parallel() + requireGitReftableSupport(t) + + env := NewTestEnv(t) + + // Set up the reftable repo and initial commit directly via git CLI, matching + // the sha256 repo test: integration tests avoid the enable bootstrap path and + // drive hooks through getTestBinary() so they exercise the binary under test. + gitOutput(t, "", "init", "--ref-format=reftable", env.RepoDir) + gitOutput(t, env.RepoDir, "config", "user.name", "Test User") + gitOutput(t, env.RepoDir, "config", "user.email", "test@example.com") + gitOutput(t, env.RepoDir, "config", "commit.gpgsign", "false") + env.WriteFile("README.md", "# reftable repo\n") + gitOutput(t, env.RepoDir, "add", "README.md") + gitOutput(t, env.RepoDir, "commit", "-m", "Initial reftable commit") + + if got := gitOutput(t, env.RepoDir, "rev-parse", "--show-ref-format"); got != "reftable" { + t.Fatalf("repository ref format = %q, want reftable", got) + } + + output := env.RunCLI( + "enable", + "--no-github", + "--agent", "claude-code", + "--telemetry=false", + ) + if !strings.Contains(output, paths.MetadataBranchName) { + t.Fatalf("expected enable to create %s branch, got output:\n%s", paths.MetadataBranchName, output) + } + + // The metadata branch is a real ref; resolving it proves the reftable-backed + // ref write during enable succeeded. + initialHead := gitOutput(t, env.RepoDir, "rev-parse", "HEAD") + initialMetadataHead := gitOutput(t, env.RepoDir, "rev-parse", paths.MetadataBranchName) + + sess := env.NewSession() + prompt := "Create a file in the reftable repo" + if err := env.SimulateUserPromptSubmitWithPromptAndTranscriptPath(sess.ID, prompt, sess.TranscriptPath); err != nil { + t.Fatalf("user-prompt-submit failed: %v", err) + } + + const mainContent = "package main\n\nfunc main() {}\n" + env.WriteFile("main.go", mainContent) + sess.CreateTranscript(prompt, []FileChange{{Path: "main.go", Content: mainContent}}) + if err := env.SimulateStop(sess.ID, sess.TranscriptPath); err != nil { + t.Fatalf("stop hook failed creating first checkpoint: %v", err) + } + + state, err := env.GetSessionState(sess.ID) + if err != nil { + t.Fatalf("GetSessionState failed: %v", err) + } + if state == nil || state.StepCount != 1 { + t.Fatalf("session StepCount after first checkpoint = %#v, want 1", state) + } + + // The shadow branch is created and advanced via reftable ref writes. + shadowBranch := env.GetShadowBranchNameForCommit(initialHead) + if got := gitOutput(t, env.RepoDir, "rev-parse", shadowBranch); got == "" { + t.Fatalf("expected shadow branch %s to resolve", shadowBranch) + } + + env.GitCommitWithShadowHooks("Add reftable main", "main.go") + userHead := gitOutput(t, env.RepoDir, "rev-parse", "HEAD") + if userHead == initialHead { + t.Fatal("expected user commit to advance HEAD") + } + + // The condensation on user commit advances the metadata branch (a ref write) + // and links the checkpoint via a commit trailer on the user commit. + metadataHead := gitOutput(t, env.RepoDir, "rev-parse", paths.MetadataBranchName) + if metadataHead == initialMetadataHead { + t.Fatal("expected metadata branch to advance after condensing the first checkpoint") + } + + subject := gitOutput(t, env.RepoDir, "log", "-1", "--format=%s", paths.MetadataBranchName) + if !strings.HasPrefix(subject, "Checkpoint: ") { + t.Fatalf("metadata branch latest subject = %q, want Checkpoint: ", subject) + } + checkpointID := strings.TrimPrefix(subject, "Checkpoint: ") + + userBody := gitOutput(t, env.RepoDir, "log", "-1", "--format=%B", "HEAD") + if !strings.Contains(userBody, "Entire-Checkpoint: "+checkpointID) { + t.Fatalf("user commit body missing Entire-Checkpoint trailer for %s:\n%s", checkpointID, userBody) + } + + if _, found := env.ReadFileFromBranch(paths.MetadataBranchName, SessionMetadataPath(checkpointID)); !found { + t.Fatalf("expected session metadata for checkpoint %s on %s", checkpointID, paths.MetadataBranchName) + } + + // checkpoint list must work against the reftable repo (read path). + listOut := env.RunCLI("checkpoint", "list") + if !strings.Contains(listOut, checkpointID) { + t.Fatalf("checkpoint list missing checkpoint %s:\n%s", checkpointID, listOut) + } +} + +// TestReftableRepository_LinkedWorktree verifies the capture flow works inside a +// linked worktree of a reftable repository, where the shared reftable stack +// lives under the common git dir rather than the worktree git dir. +func TestReftableRepository_LinkedWorktree(t *testing.T) { + t.Parallel() + requireGitReftableSupport(t) + + env := NewTestEnv(t) + + gitOutput(t, "", "init", "--ref-format=reftable", env.RepoDir) + gitOutput(t, env.RepoDir, "config", "user.name", "Test User") + gitOutput(t, env.RepoDir, "config", "user.email", "test@example.com") + gitOutput(t, env.RepoDir, "config", "commit.gpgsign", "false") + env.WriteFile("README.md", "# reftable repo\n") + gitOutput(t, env.RepoDir, "add", "README.md") + gitOutput(t, env.RepoDir, "commit", "-m", "Initial reftable commit") + + // Create a linked worktree on a feature branch. + worktreePath := filepath.Join(t.TempDir(), "wt") + gitOutput(t, env.RepoDir, "worktree", "add", "-b", "feature/wt", worktreePath) + + // Enable and drive a checkpoint from within the worktree by pointing the CLI + // at the worktree directory. + runCLIIn(t, env, worktreePath, "enable", "--no-github", "--agent", "claude-code", "--telemetry=false") + + if got := gitOutput(t, worktreePath, "rev-parse", "--show-ref-format"); got != "reftable" { + t.Fatalf("worktree ref format = %q, want reftable", got) + } + + // A ref read against the worktree (HEAD resolution through the reftable + // storer, whose HEAD stub go-git cannot read) must return the real branch. + branch := gitOutput(t, worktreePath, "rev-parse", "--abbrev-ref", "HEAD") + if branch != "feature/wt" { + t.Fatalf("worktree branch = %q, want feature/wt", branch) + } + + metadataHead := gitOutput(t, worktreePath, "rev-parse", paths.MetadataBranchName) + if metadataHead == "" { + t.Fatalf("expected metadata branch to resolve from worktree") + } +} + +// runCLIIn runs the built entire binary in an arbitrary directory (e.g. a linked +// worktree) with the same isolated environment RunCLI uses. +func runCLIIn(t *testing.T, env *TestEnv, dir string, args ...string) string { + t.Helper() + cmd := exec.Command(getTestBinary(), args...) //nolint:noctx // test helper + cmd.Dir = dir + cmd.Env = env.cliEnv() + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("entire %s (in %s) failed: %v\n%s", strings.Join(args, " "), dir, err, out) + } + return string(out) +} + +func requireGitReftableSupport(t *testing.T) { + t.Helper() + + dir := t.TempDir() + cmd := exec.Command("git", "init", "--ref-format=reftable", dir) //nolint:noctx // test capability probe + cmd.Env = testutil.GitIsolatedEnv() + if output, err := cmd.CombinedOutput(); err != nil { + t.Skipf("git does not support reftable repositories: %v\n%s", err, output) + } + if got := gitOutput(t, dir, "rev-parse", "--show-ref-format"); got != "reftable" { + t.Skipf("git initialized ref format %q, not reftable", got) + } +} From c08e27b0d1ecae6e0a65c0a31ac13ad1da17c0cb Mon Sep 17 00:00:00 2001 From: Suhaan Thayyil <257360244+suhaanthayyil@users.noreply.github.com> Date: Sat, 11 Jul 2026 22:23:21 -0400 Subject: [PATCH 02/11] fix(gitrepo): distinguish reftable CAS conflicts from other failures The reftable ReferenceStorer mapped every git update-ref failure to storage.ErrReferenceHasChanged. strategy.atomicSetV1Ref keys a privacy-critical pre-push decision on that sentinel: it treats it as "another worktree advanced the ref" and aborts the push as a concurrency event, while wrapping every other error as a real failure. The coarse mapping therefore misreported storage errors (nonexistent object, invalid ref name, lock contention, timeout, git spawn failure) as concurrency conflicts, producing a bogus V1RefMovedError ("ref moved from X to X"). Map only a genuine compare-and-swap conflict ("is at X but expected Y", or "reference already exists" for a create race) to ErrReferenceHasChanged; surface every other failure as itself. Also pass user-controlled ref names after --end-of-options so a ref name beginning with "-" is treated as a literal argument rather than a git option (defense-in-depth; args were already argv, never a shell string, so there was no command-injection exposure). Add mutation-verified regression tests: the CAS conflict-vs-error mapping, and an argv-safety test proving ref names are never shell-evaluated. --- cmd/entire/cli/gitrepo/reftable.go | 50 +++++++--- cmd/entire/cli/gitrepo/reftable_test.go | 127 ++++++++++++++++++++++++ 2 files changed, 163 insertions(+), 14 deletions(-) diff --git a/cmd/entire/cli/gitrepo/reftable.go b/cmd/entire/cli/gitrepo/reftable.go index f001291610..91ad7b4974 100644 --- a/cmd/entire/cli/gitrepo/reftable.go +++ b/cmd/entire/cli/gitrepo/reftable.go @@ -113,12 +113,12 @@ func (s *reftableStorer) SetReference(ref *plumbing.Reference) error { return nil } if ref.Type() == plumbing.SymbolicReference { - if _, stderr, err := s.runGit("symbolic-ref", ref.Name().String(), ref.Target().String()); err != nil { + if _, stderr, err := s.runGit("symbolic-ref", "--end-of-options", ref.Name().String(), ref.Target().String()); err != nil { return fmt.Errorf("reftable set symbolic ref %s: %s: %w", ref.Name(), strings.TrimSpace(string(stderr)), err) } return nil } - if _, stderr, err := s.runGit("update-ref", ref.Name().String(), ref.Hash().String()); err != nil { + if _, stderr, err := s.runGit("update-ref", "--end-of-options", ref.Name().String(), ref.Hash().String()); err != nil { return fmt.Errorf("reftable set ref %s: %s: %w", ref.Name(), strings.TrimSpace(string(stderr)), err) } return nil @@ -126,7 +126,15 @@ func (s *reftableStorer) SetReference(ref *plumbing.Reference) error { // CheckAndSetReference performs a compare-and-swap update. When old is non-nil // the update is conditioned on the current value, mirroring go-git's atomic -// semantics; a mismatch is reported as storage.ErrReferenceHasChanged. +// semantics; a genuine mismatch is reported as storage.ErrReferenceHasChanged. +// +// Only a real compare-and-swap conflict maps to that sentinel. Callers such as +// strategy.atomicSetV1Ref treat ErrReferenceHasChanged as "another worktree +// advanced the ref" and abort the push as a concurrency event, while wrapping +// every other error as a genuine failure. Mapping an unrelated failure (bad +// object, invalid ref name, lock contention, timeout, git spawn failure) to the +// conflict sentinel would misreport a storage error as a benign race, so those +// are surfaced as themselves. func (s *reftableStorer) CheckAndSetReference(newRef, old *plumbing.Reference) error { if newRef == nil { return nil @@ -138,12 +146,26 @@ func (s *reftableStorer) CheckAndSetReference(newRef, old *plumbing.Reference) e if old == nil { return s.SetReference(newRef) } - if _, _, err := s.runGit("update-ref", newRef.Name().String(), newRef.Hash().String(), old.Hash().String()); err != nil { - // git update-ref fails when the stored value differs from the expected - // old value; surface the sentinel go-git callers check for. + _, stderr, err := s.runGit("update-ref", "--end-of-options", newRef.Name().String(), newRef.Hash().String(), old.Hash().String()) + if err == nil { + return nil + } + if isRefCASConflict(stderr) { return gogitstorage.ErrReferenceHasChanged } - return nil + return fmt.Errorf("reftable CAS ref %s: %s: %w", newRef.Name(), strings.TrimSpace(string(stderr)), err) +} + +// isRefCASConflict reports whether git update-ref stderr indicates a +// compare-and-swap conflict: the stored value was not the expected old value +// ("... is at X but expected Y"), or a create-if-absent update found the ref +// already present ("reference already exists"). These are the only failures +// that mean the reference changed concurrently. Object/name/lock/spawn errors +// carry different messages and must not be misclassified as conflicts. +func isRefCASConflict(stderr []byte) bool { + msg := strings.ToLower(string(stderr)) + return strings.Contains(msg, "but expected") || + strings.Contains(msg, "reference already exists") } // Reference returns the reference with the given name, preserving symbolic refs @@ -151,14 +173,14 @@ func (s *reftableStorer) CheckAndSetReference(newRef, old *plumbing.Reference) e func (s *reftableStorer) Reference(name plumbing.ReferenceName) (*plumbing.Reference, error) { // Symbolic refs: symbolic-ref exits 0 and prints the target only for a // genuine symbolic ref; -q suppresses the error for non-symbolic names. - if target, _, err := s.runGit("symbolic-ref", "-q", name.String()); err == nil && target != "" { + if target, _, err := s.runGit("symbolic-ref", "-q", "--end-of-options", name.String()); err == nil && target != "" { return plumbing.NewSymbolicReference(name, plumbing.ReferenceName(target)), nil } // Hash refs: rev-parse --verify resolves the ref to the object it points at. // "^0" would peel tags; we want the ref's direct target, so verify the name // as-is. A non-existent ref exits non-zero. - out, _, err := s.runGit("rev-parse", "--verify", "--quiet", name.String()) + out, _, err := s.runGit("rev-parse", "--verify", "--quiet", "--end-of-options", name.String()) if err != nil || out == "" { return nil, plumbing.ErrReferenceNotFound } @@ -176,9 +198,9 @@ func (s *reftableStorer) IterReferences() (storer.ReferenceIter, error) { // HEAD (symbolic on a branch, detached hash otherwise) is not emitted by // for-each-ref, so resolve it explicitly first. - if target, _, err := s.runGit("symbolic-ref", "-q", "HEAD"); err == nil && target != "" { + if target, _, err := s.runGit("symbolic-ref", "-q", "--end-of-options", "HEAD"); err == nil && target != "" { refs = append(refs, plumbing.NewSymbolicReference(plumbing.HEAD, plumbing.ReferenceName(target))) - } else if out, _, err := s.runGit("rev-parse", "--verify", "--quiet", "HEAD"); err == nil && out != "" { + } else if out, _, err := s.runGit("rev-parse", "--verify", "--quiet", "--end-of-options", "HEAD"); err == nil && out != "" { if h := plumbing.NewHash(out); !h.IsZero() { refs = append(refs, plumbing.NewHashReference(plumbing.HEAD, h)) } @@ -228,13 +250,13 @@ func (s *reftableStorer) IterReferences() (storer.ReferenceIter, error) { func (s *reftableStorer) RemoveReference(name plumbing.ReferenceName) error { // A symbolic ref must be deleted with symbolic-ref -d; update-ref -d on a // symbolic ref would delete the ref it points at. - if target, _, err := s.runGit("symbolic-ref", "-q", name.String()); err == nil && target != "" { - if _, stderr, delErr := s.runGit("symbolic-ref", "-d", name.String()); delErr != nil { + if target, _, err := s.runGit("symbolic-ref", "-q", "--end-of-options", name.String()); err == nil && target != "" { + if _, stderr, delErr := s.runGit("symbolic-ref", "-d", "--end-of-options", name.String()); delErr != nil { return fmt.Errorf("reftable remove symbolic ref %s: %s: %w", name, strings.TrimSpace(string(stderr)), delErr) } return nil } - _, stderr, err := s.runGit("update-ref", "-d", name.String()) + _, stderr, err := s.runGit("update-ref", "-d", "--end-of-options", name.String()) if err != nil { msg := strings.ToLower(strings.TrimSpace(string(stderr))) // Deleting an already-absent ref is not an error for our callers. diff --git a/cmd/entire/cli/gitrepo/reftable_test.go b/cmd/entire/cli/gitrepo/reftable_test.go index 3ae62df1dd..a21626fc42 100644 --- a/cmd/entire/cli/gitrepo/reftable_test.go +++ b/cmd/entire/cli/gitrepo/reftable_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/go-git/go-git/v6/plumbing" + gitstorage "github.com/go-git/go-git/v6/storage" "github.com/stretchr/testify/require" ) @@ -53,6 +54,132 @@ func initReftableRepo(t *testing.T, name, content string) (string, string) { return repoDir, git("rev-parse", "HEAD") } +// reftableCommit adds a file and commits it in an existing reftable repo, +// returning the new HEAD hash. The repo's user identity is already configured +// by initReftableRepo, so only an isolated global/system config is supplied. +func reftableCommit(t *testing.T, repoDir, name, content string) string { + t.Helper() + env := append(os.Environ(), + "GIT_CONFIG_GLOBAL="+filepath.Join(t.TempDir(), "gitconfig"), + "GIT_CONFIG_SYSTEM=/dev/null", + "GIT_TERMINAL_PROMPT=0", + ) + run := func(args ...string) string { + t.Helper() + cmd := exec.Command("git", args...) //nolint:noctx // test helper + cmd.Dir = repoDir + cmd.Env = env + out, err := cmd.CombinedOutput() + require.NoErrorf(t, err, "git %s: %s", strings.Join(args, " "), out) + return strings.TrimSpace(string(out)) + } + require.NoError(t, os.WriteFile(filepath.Join(repoDir, name), []byte(content), 0o644)) + run("add", name) + run("commit", "-m", content) + return run("rev-parse", "HEAD") +} + +// TestCheckAndSetReference_ConflictVsError verifies that CheckAndSetReference +// maps only a genuine compare-and-swap conflict to storage.ErrReferenceHasChanged +// and surfaces unrelated failures (a new value pointing at a nonexistent object) +// as themselves. Callers such as strategy.atomicSetV1Ref branch on that sentinel +// to decide whether a privacy-critical push aborts because of concurrency or a +// real storage error, so misclassifying an I/O/object failure as a conflict is a +// correctness bug. Regression for the coarse error mapping in #547. +func TestCheckAndSetReference_ConflictVsError(t *testing.T) { + t.Parallel() + repoDir, headHash := initReftableRepo(t, "file.txt", "hello\n") + + repo, err := OpenPath(repoDir) + require.NoError(t, err) + defer repo.Close() + + refName := plumbing.ReferenceName("refs/entire/cas") + head := plumbing.NewHash(headHash) + require.NoError(t, repo.Storer.SetReference(plumbing.NewHashReference(refName, head))) + + // A distinct, real object to swap the ref to. + secondHash := plumbing.NewHash(reftableCommit(t, repoDir, "second.txt", "second\n")) + require.NotEqual(t, head, secondHash) + + // Correct compare-and-swap succeeds: ref is at head, swap head -> second. + require.NoError(t, repo.Storer.CheckAndSetReference( + plumbing.NewHashReference(refName, secondHash), + plumbing.NewHashReference(refName, head), + )) + + // Genuine conflict: ref is now at second, but we claim it is still at head. + err = repo.Storer.CheckAndSetReference( + plumbing.NewHashReference(refName, head), + plumbing.NewHashReference(refName, head), + ) + require.ErrorIs(t, err, gitstorage.ErrReferenceHasChanged, + "a stale expected-old value must be reported as a CAS conflict") + + // Non-conflict failure: the expected-old value is correct (ref is at second), + // but the new value points at a nonexistent object. git rejects the object, + // not the CAS, so this must NOT be reported as a concurrency conflict. + bogus := plumbing.NewHash("1111111111111111111111111111111111111111") + err = repo.Storer.CheckAndSetReference( + plumbing.NewHashReference(refName, bogus), + plumbing.NewHashReference(refName, secondHash), + ) + require.Error(t, err) + require.NotErrorIs(t, err, gitstorage.ErrReferenceHasChanged, + "a nonexistent-object write must not be misreported as a CAS conflict") + + // The failed CAS must not have advanced the ref. + cur, err := repo.Storer.Reference(refName) + require.NoError(t, err) + require.Equal(t, secondHash, cur.Hash()) +} + +// TestReftableStorer_RefNamesAreArgvNotShell verifies that ref names are passed +// to git as argv, never interpolated into a shell command line. The injected +// name embeds a backtick command substitution with output redirection; if any +// method shelled out, the shell would create the marker file. Every method must +// instead treat the whole string as a literal ref name that round-trips. +func TestReftableStorer_RefNamesAreArgvNotShell(t *testing.T) { + t.Parallel() + repoDir, headHash := initReftableRepo(t, "file.txt", "hello\n") + + repo, err := OpenPath(repoDir) + require.NoError(t, err) + defer repo.Close() + + marker := filepath.Join(t.TempDir(), "PWNED") + require.NotContains(t, marker, " ", "test temp path must be space-free for a valid ref name") + // `>marker` is a shell redirection inside a backtick command substitution: + // it creates marker if (and only if) the name is evaluated by a shell. + injected := plumbing.ReferenceName("refs/entire/inj`>" + marker + "`tail") + head := plumbing.NewHash(headHash) + + require.NoError(t, repo.Storer.SetReference(plumbing.NewHashReference(injected, head))) + require.NoFileExists(t, marker, "ref name must not be evaluated by a shell on write") + + got, err := repo.Storer.Reference(injected) + require.NoError(t, err) + require.Equal(t, head, got.Hash()) + require.NoFileExists(t, marker, "ref name must not be evaluated by a shell on read") + + iter, err := repo.Storer.IterReferences() + require.NoError(t, err) + found := false + require.NoError(t, iter.ForEach(func(r *plumbing.Reference) error { + if r.Name() == injected { + found = true + } + return nil + })) + iter.Close() + require.True(t, found, "injected ref name must appear verbatim in iteration") + + require.NoError(t, repo.Storer.RemoveReference(injected)) + require.NoFileExists(t, marker, "ref name must not be evaluated by a shell on remove") + _, err = repo.Storer.Reference(injected) + require.ErrorIs(t, err, plumbing.ErrReferenceNotFound) +} + // TestOpenPath_ReftableRepository verifies that a reftable repository, which // go-git's filesystem storer cannot open, is opened successfully and that ref // read/write/list/remove all round-trip through the git-CLI-backed storer. From 4b528eb0b8008f41bb9d9259d87927b48faa6b15 Mon Sep 17 00:00:00 2001 From: Suhaan Thayyil <257360244+suhaanthayyil@users.noreply.github.com> Date: Sat, 11 Jul 2026 23:05:05 -0400 Subject: [PATCH 03/11] fix(gitrepo): surface transient git failures in reftable ref lookups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reference, IterReferences (HEAD resolution), and RemoveReference treated every git failure as a definitive negative: a spawn failure, our timeout, or an I/O error was reported as ErrReferenceNotFound, a silently-dropped HEAD, or a successful deletion. A transient git failure masquerading as "ref absent" can make the strategy treat a live checkpoint ref as gone (orphan reset, lost linkage) or believe a ref was deleted when it was not. Classify failures via refLookupAbsent: git genuinely reports absence only when it ran and exited non-zero with empty stderr (its --quiet not-found contract). A wrapped context error (execGit now re-wraps the timeout kill), a non-*exec.ExitError (spawn/I/O), or a non-zero exit carrying stderr is surfaced as a real error instead. RemoveReference no longer treats an empty-stderr failure as idempotent success — git exits 0 for an already-absent ref, so a non-zero exit here is a genuine failure. Add an injectable runGitFn so the classification is covered by mutation-verified unit tests: absence, timeout, spawn failure, and non-zero-with-stderr for Reference; empty-stderr and timeout not swallowed for RemoveReference; and a git failure resolving HEAD surfaced by IterReferences. --- cmd/entire/cli/gitrepo/reftable.go | 104 ++++++++++--- cmd/entire/cli/gitrepo/reftable_test.go | 196 ++++++++++++++++++++++++ 2 files changed, 282 insertions(+), 18 deletions(-) diff --git a/cmd/entire/cli/gitrepo/reftable.go b/cmd/entire/cli/gitrepo/reftable.go index 91ad7b4974..5437b6f3d3 100644 --- a/cmd/entire/cli/gitrepo/reftable.go +++ b/cmd/entire/cli/gitrepo/reftable.go @@ -4,6 +4,7 @@ import ( "bufio" "bytes" "context" + "errors" "fmt" "os" "os/exec" @@ -63,6 +64,12 @@ type reftableStorer struct { *gitfilesystem.Storage gitDir string + + // runGitFn runs a git plumbing command and returns trimmed stdout, raw + // stderr, and the exec error. It is overridable in tests to simulate + // spawn/timeout/exit failures deterministically; in production it is nil and + // runGit dispatches to execGit. + runGitFn func(args ...string) (string, []byte, error) } var ( @@ -88,10 +95,18 @@ func (s *reftableStorer) SupportsExtension(name, _ string) bool { } // runGit runs a git plumbing command scoped to this repository's git dir and -// returns trimmed stdout. Only ref plumbing (show-ref, for-each-ref, -// symbolic-ref, update-ref, rev-parse) is used, none of which trigger git -// hooks, so this cannot recurse back into entire. +// returns trimmed stdout, raw stderr, and the exec error. Only ref plumbing +// (for-each-ref, symbolic-ref, update-ref, rev-parse) is used, none of which +// trigger git hooks, so this cannot recurse back into entire. Tests may inject +// runGitFn to simulate failures; production dispatches to execGit. func (s *reftableStorer) runGit(args ...string) (string, []byte, error) { + if s.runGitFn != nil { + return s.runGitFn(args...) + } + return s.execGit(args...) +} + +func (s *reftableStorer) execGit(args ...string) (string, []byte, error) { ctx, cancel := context.WithTimeout(context.Background(), reftableGitTimeout) defer cancel() @@ -103,9 +118,42 @@ func (s *reftableStorer) runGit(args ...string) (string, []byte, error) { cmd.Stdout = &stdout cmd.Stderr = &stderr err := cmd.Run() + // On our timeout the process is killed and Run reports the kill as an + // *exec.ExitError, which would be indistinguishable from a genuine + // non-zero exit. Re-wrap with the context error so callers (refLookupAbsent) + // never mistake a wedged git for a definitive "ref not found". + if err != nil && ctx.Err() != nil { + err = fmt.Errorf("git %s timed out after %s: %w", strings.Join(args, " "), reftableGitTimeout, ctx.Err()) + } return strings.TrimRight(stdout.String(), "\n"), stderr.Bytes(), err } +// refLookupAbsent reports whether a failed ref lookup (rev-parse --verify +// --quiet, update-ref) means the reference is genuinely absent rather than a +// failure to consult git at all. Only genuine absence may map to the +// plumbing.ErrReferenceNotFound / idempotent-delete sentinels; a spawn failure, +// timeout, or I/O error must be surfaced so a transient git failure is never +// mistaken for a missing ref (which would let the strategy orphan a checkpoint +// ref or drop a link). +// +// git ran and reported absence iff it exited non-zero AND stayed silent: under +// --quiet a missing ref produces no error output, whereas a fatal/I/O failure +// exits non-zero with a "fatal: ..." message, and a spawn failure or our +// timeout is not an *exec.ExitError at all. +func refLookupAbsent(err error, stderr []byte) bool { + if err == nil { + return false + } + if errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled) { + return false + } + var exitErr *exec.ExitError + if !errors.As(err, &exitErr) { + return false + } + return len(bytes.TrimSpace(stderr)) == 0 +} + // SetReference stores a reference, dispatching to symbolic-ref for symbolic // references and update-ref for hash references. func (s *reftableStorer) SetReference(ref *plumbing.Reference) error { @@ -179,9 +227,15 @@ func (s *reftableStorer) Reference(name plumbing.ReferenceName) (*plumbing.Refer // Hash refs: rev-parse --verify resolves the ref to the object it points at. // "^0" would peel tags; we want the ref's direct target, so verify the name - // as-is. A non-existent ref exits non-zero. - out, _, err := s.runGit("rev-parse", "--verify", "--quiet", "--end-of-options", name.String()) - if err != nil || out == "" { + // as-is. A non-existent ref exits non-zero silently. + out, stderr, err := s.runGit("rev-parse", "--verify", "--quiet", "--end-of-options", name.String()) + if err != nil { + if refLookupAbsent(err, stderr) { + return nil, plumbing.ErrReferenceNotFound + } + return nil, fmt.Errorf("reftable resolve ref %s: %s: %w", name, strings.TrimSpace(string(stderr)), err) + } + if out == "" { return nil, plumbing.ErrReferenceNotFound } h := plumbing.NewHash(out) @@ -197,12 +251,22 @@ func (s *reftableStorer) IterReferences() (storer.ReferenceIter, error) { refs := make([]*plumbing.Reference, 0, 16) // HEAD (symbolic on a branch, detached hash otherwise) is not emitted by - // for-each-ref, so resolve it explicitly first. + // for-each-ref, so resolve it explicitly first. A symbolic-ref failure just + // means HEAD is detached (or the probe could not run), so fall through to + // rev-parse and classify that result: surface a real git failure rather than + // silently dropping HEAD, but omit HEAD when it is genuinely absent (an + // unborn/empty repo), matching the filesystem storer. if target, _, err := s.runGit("symbolic-ref", "-q", "--end-of-options", "HEAD"); err == nil && target != "" { refs = append(refs, plumbing.NewSymbolicReference(plumbing.HEAD, plumbing.ReferenceName(target))) - } else if out, _, err := s.runGit("rev-parse", "--verify", "--quiet", "--end-of-options", "HEAD"); err == nil && out != "" { - if h := plumbing.NewHash(out); !h.IsZero() { - refs = append(refs, plumbing.NewHashReference(plumbing.HEAD, h)) + } else { + out, stderr, headErr := s.runGit("rev-parse", "--verify", "--quiet", "--end-of-options", "HEAD") + switch { + case headErr == nil && out != "": + if h := plumbing.NewHash(out); !h.IsZero() { + refs = append(refs, plumbing.NewHashReference(plumbing.HEAD, h)) + } + case headErr != nil && !refLookupAbsent(headErr, stderr): + return nil, fmt.Errorf("reftable resolve HEAD: %s: %w", strings.TrimSpace(string(stderr)), headErr) } } @@ -257,15 +321,19 @@ func (s *reftableStorer) RemoveReference(name plumbing.ReferenceName) error { return nil } _, stderr, err := s.runGit("update-ref", "-d", "--end-of-options", name.String()) - if err != nil { - msg := strings.ToLower(strings.TrimSpace(string(stderr))) - // Deleting an already-absent ref is not an error for our callers. - if msg == "" || strings.Contains(msg, "does not exist") || strings.Contains(msg, "not exist") { - return nil - } - return fmt.Errorf("reftable remove ref %s: %s: %w", name, strings.TrimSpace(string(stderr)), err) + if err == nil { + return nil } - return nil + // git update-ref exits 0 when deleting an already-absent ref, so reaching + // here means the delete actually failed. Only an explicit "does not exist" + // is idempotent success; an empty stderr must NOT be swallowed, because a + // killed/timed-out git also produces no stderr and would otherwise be + // silently reported as a successful deletion. + msg := strings.ToLower(strings.TrimSpace(string(stderr))) + if strings.Contains(msg, "does not exist") || strings.Contains(msg, "not exist") { + return nil + } + return fmt.Errorf("reftable remove ref %s: %s: %w", name, strings.TrimSpace(string(stderr)), err) } // CountLooseRefs returns 0: reftable has no loose refs, and go-git only uses diff --git a/cmd/entire/cli/gitrepo/reftable_test.go b/cmd/entire/cli/gitrepo/reftable_test.go index a21626fc42..3a8134521f 100644 --- a/cmd/entire/cli/gitrepo/reftable_test.go +++ b/cmd/entire/cli/gitrepo/reftable_test.go @@ -1,6 +1,9 @@ package gitrepo import ( + "context" + "errors" + "fmt" "os" "os/exec" "path/filepath" @@ -180,6 +183,199 @@ func TestReftableStorer_RefNamesAreArgvNotShell(t *testing.T) { require.ErrorIs(t, err, plumbing.ErrReferenceNotFound) } +// git subcommand names used by the runGitFn fakes below (named to satisfy +// goconst, which flags the repeated literals across the injected switches). +const ( + gitSymbolicRef = "symbolic-ref" + gitRevParse = "rev-parse" +) + +// realExitError returns a genuine *exec.ExitError (git ran and exited non-zero) +// so runGitFn injections can distinguish "git reported absence" from a spawn or +// timeout failure that never produced an exit code. +func realExitError(t *testing.T) error { + t.Helper() + err := exec.Command("sh", "-c", "exit 1").Run() //nolint:noctx // test helper + var ee *exec.ExitError + require.ErrorAs(t, err, &ee) + return err +} + +// timeoutError mimics the error execGit produces when its context deadline +// fires and the git process is killed. +func timeoutError() error { + return fmt.Errorf("git rev-parse timed out: %w", context.DeadlineExceeded) +} + +// TestReference_ClassifiesLookupFailures verifies that Reference maps only a +// genuine "git ran and the ref is absent" result to ErrReferenceNotFound, and +// surfaces spawn/timeout/I-O failures instead. Reporting a transient git +// failure as "ref not found" can make the strategy treat a live checkpoint ref +// as absent (orphan reset, lost linkage), so this distinction is load-bearing. +func TestReference_ClassifiesLookupFailures(t *testing.T) { + t.Parallel() + name := plumbing.ReferenceName("refs/entire/probe") + + // symbolic-ref always fails "not symbolic" so every case exercises the + // rev-parse hash path, whose result is controlled per test. + storerWith := func(revParseOut string, revParseStderr []byte, revParseErr error) *reftableStorer { + return &reftableStorer{gitDir: "unused", runGitFn: func(args ...string) (string, []byte, error) { + switch args[0] { + case gitSymbolicRef: + return "", nil, realExitError(t) + case gitRevParse: + return revParseOut, revParseStderr, revParseErr + default: + return "", nil, nil + } + }} + } + + t.Run("genuine absence maps to ErrReferenceNotFound", func(t *testing.T) { + t.Parallel() + _, err := storerWith("", nil, realExitError(t)).Reference(name) + require.ErrorIs(t, err, plumbing.ErrReferenceNotFound) + }) + + t.Run("timeout is surfaced, not absent", func(t *testing.T) { + t.Parallel() + _, err := storerWith("", nil, timeoutError()).Reference(name) + require.Error(t, err) + require.NotErrorIs(t, err, plumbing.ErrReferenceNotFound) + }) + + t.Run("spawn failure is surfaced, not absent", func(t *testing.T) { + t.Parallel() + spawn := &exec.Error{Name: "git", Err: errors.New("executable file not found in $PATH")} + _, err := storerWith("", nil, spawn).Reference(name) + require.Error(t, err) + require.NotErrorIs(t, err, plumbing.ErrReferenceNotFound) + }) + + t.Run("non-zero exit WITH stderr is surfaced, not absent", func(t *testing.T) { + t.Parallel() + _, err := storerWith("", []byte("fatal: unable to read reftable stack\n"), realExitError(t)).Reference(name) + require.Error(t, err) + require.NotErrorIs(t, err, plumbing.ErrReferenceNotFound) + }) +} + +// TestIterReferences_HEADFailureSurfaces verifies the HEAD-resolution path in +// IterReferences surfaces a real git failure rather than silently dropping HEAD, +// while still omitting a genuinely-absent HEAD and preserving detached HEAD. +func TestIterReferences_HEADFailureSurfaces(t *testing.T) { + t.Parallel() + + t.Run("git failure resolving HEAD is surfaced", func(t *testing.T) { + t.Parallel() + s := &reftableStorer{gitDir: "unused", runGitFn: func(args ...string) (string, []byte, error) { + switch args[0] { + case gitSymbolicRef, gitRevParse: + return "", nil, timeoutError() + default: + return "", nil, nil + } + }} + _, err := s.IterReferences() + require.Error(t, err) + }) + + t.Run("detached HEAD resolves to a hash reference", func(t *testing.T) { + t.Parallel() + hash := "20b4de1033d986a83837177f961c80bb799161e6" + s := &reftableStorer{gitDir: "unused", runGitFn: func(args ...string) (string, []byte, error) { + switch args[0] { + case gitSymbolicRef: + return "", nil, realExitError(t) // detached: not symbolic + case gitRevParse: + return hash, nil, nil + case "for-each-ref": + return "", nil, nil + default: + return "", nil, nil + } + }} + iter, err := s.IterReferences() + require.NoError(t, err) + var head *plumbing.Reference + require.NoError(t, iter.ForEach(func(r *plumbing.Reference) error { + if r.Name() == plumbing.HEAD { + head = r + } + return nil + })) + iter.Close() + require.NotNil(t, head, "detached HEAD must be present in iteration") + require.Equal(t, plumbing.NewHash(hash), head.Hash()) + }) + + t.Run("genuinely absent HEAD is omitted without error", func(t *testing.T) { + t.Parallel() + s := &reftableStorer{gitDir: "unused", runGitFn: func(args ...string) (string, []byte, error) { + switch args[0] { + case gitSymbolicRef: + return "", nil, realExitError(t) // not symbolic + case gitRevParse: + return "", nil, realExitError(t) // absent: exited non-zero, silent + case "for-each-ref": + return "", nil, nil + default: + return "", nil, nil + } + }} + iter, err := s.IterReferences() + require.NoError(t, err) + count := 0 + require.NoError(t, iter.ForEach(func(_ *plumbing.Reference) error { count++; return nil })) + iter.Close() + require.Equal(t, 0, count, "an unborn/absent HEAD must yield no references, not an error") + }) +} + +// TestRemoveReference_DeleteFailureNotSwallowed verifies that a failed deletion +// is only treated as idempotent success when git ran and reported the ref +// already absent (exit 0, or an explicit "does not exist"). A non-zero exit with +// empty stderr — as produced by a killed/timed-out git — must surface as an +// error, not a phantom successful deletion. +func TestRemoveReference_DeleteFailureNotSwallowed(t *testing.T) { + t.Parallel() + name := plumbing.ReferenceName("refs/entire/rm") + + storerWith := func(updateRefStderr []byte, updateRefErr error) *reftableStorer { + return &reftableStorer{gitDir: "unused", runGitFn: func(args ...string) (string, []byte, error) { + switch args[0] { + case gitSymbolicRef: + return "", nil, realExitError(t) // not symbolic -> update-ref -d path + case "update-ref": + return "", updateRefStderr, updateRefErr + default: + return "", nil, nil + } + }} + } + + t.Run("exit 0 is idempotent success", func(t *testing.T) { + t.Parallel() + require.NoError(t, storerWith(nil, nil).RemoveReference(name)) + }) + + t.Run("explicit does-not-exist is idempotent success", func(t *testing.T) { + t.Parallel() + require.NoError(t, storerWith([]byte("error: refs/entire/rm does not exist"), realExitError(t)).RemoveReference(name)) + }) + + t.Run("non-zero exit with empty stderr is an error", func(t *testing.T) { + t.Parallel() + require.Error(t, storerWith(nil, realExitError(t)).RemoveReference(name), + "a delete failure with empty stderr must not be swallowed as success") + }) + + t.Run("timeout is an error", func(t *testing.T) { + t.Parallel() + require.Error(t, storerWith(nil, timeoutError()).RemoveReference(name)) + }) +} + // TestOpenPath_ReftableRepository verifies that a reftable repository, which // go-git's filesystem storer cannot open, is opened successfully and that ref // read/write/list/remove all round-trip through the git-CLI-backed storer. From f36c1f68d3fc016514a623e37873fcd0b5cd2dac Mon Sep 17 00:00:00 2001 From: Suhaan Thayyil <257360244+suhaanthayyil@users.noreply.github.com> Date: Sat, 11 Jul 2026 23:25:23 -0400 Subject: [PATCH 04/11] fix(gitrepo): classify reftable symbolic-ref probe failure in RemoveReference RemoveReference probes symbolic-ref -q to choose between deleting via symbolic-ref -d (symbolic) or update-ref -d (hash). It treated ANY probe error as "not symbolic" and fell through to update-ref -d. But update-ref -d on a symbolic ref deletes the ref it points at, not the symref itself (e.g. update-ref -d HEAD deletes the current branch). A transient probe failure (timeout, spawn error, I/O) on a symbolic ref would therefore be routed into a destructive delete of the wrong ref, silently losing a branch pointer. Apply the same refLookupAbsent classification used for lookups: a genuine "not a symbolic ref / not found" is exit-non-zero with empty stderr and falls through to update-ref -d; a spawn/timeout/I-O failure is surfaced instead of being assumed non-symbolic. The Reference and IterReferences symbolic-ref probes do not share this hazard: their failure falls through to a classified rev-parse that surfaces genuine failures, and the path is a non-destructive read. Mutation-verified test injects failing symbolic-ref probes and asserts the error is surfaced and never routed into update-ref -d. --- cmd/entire/cli/gitrepo/reftable.go | 16 ++++- cmd/entire/cli/gitrepo/reftable_test.go | 93 ++++++++++++++++++++++++- 2 files changed, 106 insertions(+), 3 deletions(-) diff --git a/cmd/entire/cli/gitrepo/reftable.go b/cmd/entire/cli/gitrepo/reftable.go index 5437b6f3d3..f260efd92c 100644 --- a/cmd/entire/cli/gitrepo/reftable.go +++ b/cmd/entire/cli/gitrepo/reftable.go @@ -313,13 +313,25 @@ func (s *reftableStorer) IterReferences() (storer.ReferenceIter, error) { // success so callers can remove idempotently, matching go-git's semantics. func (s *reftableStorer) RemoveReference(name plumbing.ReferenceName) error { // A symbolic ref must be deleted with symbolic-ref -d; update-ref -d on a - // symbolic ref would delete the ref it points at. - if target, _, err := s.runGit("symbolic-ref", "-q", "--end-of-options", name.String()); err == nil && target != "" { + // symbolic ref deletes the ref it points at instead (e.g. update-ref -d HEAD + // deletes the current branch). The symbolic-ref -q probe reports absence the + // same way as a lookup — exit non-zero with empty stderr — so classify its + // failure: only a genuine "not a symbolic ref / not found" may fall through + // to update-ref -d. A spawn/timeout/I-O failure must be surfaced rather than + // assumed non-symbolic, or a transient error would be routed into a + // destructive delete of the wrong ref. + target, probeStderr, probeErr := s.runGit("symbolic-ref", "-q", "--end-of-options", name.String()) + switch { + case probeErr == nil && target != "": if _, stderr, delErr := s.runGit("symbolic-ref", "-d", "--end-of-options", name.String()); delErr != nil { return fmt.Errorf("reftable remove symbolic ref %s: %s: %w", name, strings.TrimSpace(string(stderr)), delErr) } return nil + case probeErr != nil && !refLookupAbsent(probeErr, probeStderr): + return fmt.Errorf("reftable probe symbolic ref %s: %s: %w", name, strings.TrimSpace(string(probeStderr)), probeErr) } + + // name is not a symbolic ref (or does not exist): delete it as a hash ref. _, stderr, err := s.runGit("update-ref", "-d", "--end-of-options", name.String()) if err == nil { return nil diff --git a/cmd/entire/cli/gitrepo/reftable_test.go b/cmd/entire/cli/gitrepo/reftable_test.go index 3a8134521f..7a081dc978 100644 --- a/cmd/entire/cli/gitrepo/reftable_test.go +++ b/cmd/entire/cli/gitrepo/reftable_test.go @@ -188,6 +188,7 @@ func TestReftableStorer_RefNamesAreArgvNotShell(t *testing.T) { const ( gitSymbolicRef = "symbolic-ref" gitRevParse = "rev-parse" + gitUpdateRef = "update-ref" ) // realExitError returns a genuine *exec.ExitError (git ran and exited non-zero) @@ -346,7 +347,7 @@ func TestRemoveReference_DeleteFailureNotSwallowed(t *testing.T) { switch args[0] { case gitSymbolicRef: return "", nil, realExitError(t) // not symbolic -> update-ref -d path - case "update-ref": + case gitUpdateRef: return "", updateRefStderr, updateRefErr default: return "", nil, nil @@ -376,6 +377,96 @@ func TestRemoveReference_DeleteFailureNotSwallowed(t *testing.T) { }) } +// TestRemoveReference_SymbolicProbeFailureSurfaced verifies that the +// symbolic-ref -q probe classifies its failure: a genuine "not a symbolic ref" +// (exit non-zero, empty stderr) falls through to update-ref -d, but a transient +// failure (timeout/spawn/I-O) is surfaced and never routed into update-ref -d. +// Routing a transient failure into update-ref -d is destructive: on a symbolic +// ref (e.g. HEAD) update-ref -d deletes the ref it points at, silently losing a +// branch pointer. +func TestRemoveReference_SymbolicProbeFailureSurfaced(t *testing.T) { + t.Parallel() + name := plumbing.ReferenceName("refs/entire/rm") + + t.Run("transient probe failure is surfaced, never routed to update-ref -d", func(t *testing.T) { + t.Parallel() + updateRefCalled := false + s := &reftableStorer{gitDir: "unused", runGitFn: func(args ...string) (string, []byte, error) { + switch args[0] { + case gitSymbolicRef: + return "", nil, timeoutError() + case gitUpdateRef: + updateRefCalled = true + return "", nil, nil + default: + return "", nil, nil + } + }} + err := s.RemoveReference(name) + require.Error(t, err) + require.False(t, updateRefCalled, + "a transient symbolic-ref probe failure must not fall through to the destructive update-ref -d") + }) + + t.Run("fatal probe error with stderr is surfaced, not routed to update-ref -d", func(t *testing.T) { + t.Parallel() + updateRefCalled := false + s := &reftableStorer{gitDir: "unused", runGitFn: func(args ...string) (string, []byte, error) { + switch args[0] { + case gitSymbolicRef: + return "", []byte("fatal: unable to read reftable stack\n"), realExitError(t) + case gitUpdateRef: + updateRefCalled = true + return "", nil, nil + default: + return "", nil, nil + } + }} + require.Error(t, s.RemoveReference(name)) + require.False(t, updateRefCalled) + }) + + t.Run("genuine not-symbolic falls through to update-ref -d", func(t *testing.T) { + t.Parallel() + updateRefCalled := false + s := &reftableStorer{gitDir: "unused", runGitFn: func(args ...string) (string, []byte, error) { + switch args[0] { + case gitSymbolicRef: + return "", nil, realExitError(t) // exit non-zero, empty stderr = not symbolic + case gitUpdateRef: + updateRefCalled = true + return "", nil, nil + default: + return "", nil, nil + } + }} + require.NoError(t, s.RemoveReference(name)) + require.True(t, updateRefCalled, "a non-symbolic ref must be deleted via update-ref -d") + }) + + t.Run("symbolic ref is deleted via symbolic-ref -d, never update-ref -d", func(t *testing.T) { + t.Parallel() + symbolicDeleteCalled, updateRefCalled := false, false + s := &reftableStorer{gitDir: "unused", runGitFn: func(args ...string) (string, []byte, error) { + switch { + case args[0] == gitSymbolicRef && len(args) > 1 && args[1] == "-d": + symbolicDeleteCalled = true + return "", nil, nil + case args[0] == gitSymbolicRef: // the -q probe + return "refs/heads/main", nil, nil + case args[0] == "update-ref": + updateRefCalled = true + return "", nil, nil + default: + return "", nil, nil + } + }} + require.NoError(t, s.RemoveReference(name)) + require.True(t, symbolicDeleteCalled, "a symbolic ref must be deleted with symbolic-ref -d") + require.False(t, updateRefCalled, "a symbolic ref must not be deleted with update-ref -d") + }) +} + // TestOpenPath_ReftableRepository verifies that a reftable repository, which // go-git's filesystem storer cannot open, is opened successfully and that ref // read/write/list/remove all round-trip through the git-CLI-backed storer. From e93c50f7ace09ef86580261fe9895e8dc56a8d44 Mon Sep 17 00:00:00 2001 From: Suhaan Thayyil <257360244+suhaanthayyil@users.noreply.github.com> Date: Sat, 11 Jul 2026 23:53:52 -0400 Subject: [PATCH 05/11] fix(gitrepo): force C locale and classify symbolic-ref probes in reftable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two related robustness fixes to the reftable git-CLI ref storer: 1. Force LC_ALL=C / LANG=C for every reftable git plumbing invocation (new gitPlumbingEnv). git's diagnostics are i18n'd, and the reftable error classification matches English substrings ("is at X but expected Y", "reference already exists", "does not exist"). On a localized machine those messages are translated, so isRefCASConflict and RemoveReference idempotency would silently misclassify CAS conflicts and delete outcomes. Mirrors checkpoint/shadow_ref.go. refLookupAbsent already keys on exit code + empty stderr and is locale-independent. 2. Classify the symbolic-ref -q probe failure in Reference and IterReferences, matching the RemoveReference fix. Previously any probe error fell through to the hash path, so a transient failure (timeout, spawn, I/O) on a symbolic ref such as HEAD downgraded it to a Hash reference literally named "HEAD" — making go-git's ResolveReference stop there and callers that check head.Name().IsBranch() read the repo as detached. Now a genuine "not symbolic" (exit non-zero, empty stderr) falls through, but a real git failure is surfaced. Mutation-verified tests: locale env forcing overrides inherited LANG/LC_ALL, refLookupAbsent locale-independence, and transient symbolic-probe surfacing for both Reference and IterReferences. --- cmd/entire/cli/gitrepo/reftable.go | 54 ++++++++++--- cmd/entire/cli/gitrepo/reftable_test.go | 101 +++++++++++++++++++++++- 2 files changed, 141 insertions(+), 14 deletions(-) diff --git a/cmd/entire/cli/gitrepo/reftable.go b/cmd/entire/cli/gitrepo/reftable.go index f260efd92c..fa7b6e1a78 100644 --- a/cmd/entire/cli/gitrepo/reftable.go +++ b/cmd/entire/cli/gitrepo/reftable.go @@ -112,7 +112,7 @@ func (s *reftableStorer) execGit(args ...string) (string, []byte, error) { full := append([]string{"--git-dir", s.gitDir}, args...) cmd := exec.CommandContext(ctx, "git", full...) - cmd.Env = append(os.Environ(), "GIT_TERMINAL_PROMPT=0") + cmd.Env = gitPlumbingEnv() var stdout, stderr bytes.Buffer cmd.Stdout = &stdout @@ -128,6 +128,23 @@ func (s *reftableStorer) execGit(args ...string) (string, []byte, error) { return strings.TrimRight(stdout.String(), "\n"), stderr.Bytes(), err } +// gitPlumbingEnv builds the environment for a reftable git plumbing command. +// LC_ALL=C / LANG=C force untranslated (English) diagnostics so the stderr +// classification (isRefCASConflict, and RemoveReference's idempotency check) +// stays correct on a localized machine — git's error messages are i18n'd, and +// matching translated text would silently misclassify CAS conflicts and delete +// failures. GIT_TERMINAL_PROMPT=0 keeps git non-interactive. The forced values +// are appended last so they override anything the caller's environment set +// (os/exec keeps the last value for a duplicate key). Mirrors the sibling +// shell-out in checkpoint/shadow_ref.go. +func gitPlumbingEnv() []string { + return append(os.Environ(), + "GIT_TERMINAL_PROMPT=0", + "LC_ALL=C", + "LANG=C", + ) +} + // refLookupAbsent reports whether a failed ref lookup (rev-parse --verify // --quiet, update-ref) means the reference is genuinely absent rather than a // failure to consult git at all. Only genuine absence may map to the @@ -220,9 +237,18 @@ func isRefCASConflict(stderr []byte) bool { // (such as HEAD) so go-git can resolve them itself. func (s *reftableStorer) Reference(name plumbing.ReferenceName) (*plumbing.Reference, error) { // Symbolic refs: symbolic-ref exits 0 and prints the target only for a - // genuine symbolic ref; -q suppresses the error for non-symbolic names. - if target, _, err := s.runGit("symbolic-ref", "-q", "--end-of-options", name.String()); err == nil && target != "" { + // genuine symbolic ref; -q makes it exit non-zero silently for a non-symbolic + // name. Classify the probe failure the same way as elsewhere: a genuine "not + // a symbolic ref" (exit non-zero, empty stderr) falls through to the hash + // lookup, but a spawn/timeout/I-O failure is surfaced rather than silently + // downgrading a symbolic ref (e.g. HEAD on a branch) to a Hash reference + // named "HEAD", which would make callers read the repo as detached. + target, symStderr, symErr := s.runGit("symbolic-ref", "-q", "--end-of-options", name.String()) + switch { + case symErr == nil && target != "": return plumbing.NewSymbolicReference(name, plumbing.ReferenceName(target)), nil + case symErr != nil && !refLookupAbsent(symErr, symStderr): + return nil, fmt.Errorf("reftable probe symbolic ref %s: %s: %w", name, strings.TrimSpace(string(symStderr)), symErr) } // Hash refs: rev-parse --verify resolves the ref to the object it points at. @@ -251,14 +277,20 @@ func (s *reftableStorer) IterReferences() (storer.ReferenceIter, error) { refs := make([]*plumbing.Reference, 0, 16) // HEAD (symbolic on a branch, detached hash otherwise) is not emitted by - // for-each-ref, so resolve it explicitly first. A symbolic-ref failure just - // means HEAD is detached (or the probe could not run), so fall through to - // rev-parse and classify that result: surface a real git failure rather than - // silently dropping HEAD, but omit HEAD when it is genuinely absent (an - // unborn/empty repo), matching the filesystem storer. - if target, _, err := s.runGit("symbolic-ref", "-q", "--end-of-options", "HEAD"); err == nil && target != "" { - refs = append(refs, plumbing.NewSymbolicReference(plumbing.HEAD, plumbing.ReferenceName(target))) - } else { + // for-each-ref, so resolve it explicitly first, classifying each probe: + // a genuine "not symbolic" (exit non-zero, empty stderr) means HEAD is + // detached, so resolve it as a hash; a spawn/timeout/I-O failure is surfaced + // rather than silently dropping HEAD or downgrading a symbolic HEAD to a + // hash; and a genuinely absent HEAD (unborn/empty repo) is omitted, matching + // the filesystem storer. + headTarget, headSymStderr, headSymErr := s.runGit("symbolic-ref", "-q", "--end-of-options", "HEAD") + switch { + case headSymErr == nil && headTarget != "": + refs = append(refs, plumbing.NewSymbolicReference(plumbing.HEAD, plumbing.ReferenceName(headTarget))) + case headSymErr != nil && !refLookupAbsent(headSymErr, headSymStderr): + return nil, fmt.Errorf("reftable probe HEAD symbolic ref: %s: %w", strings.TrimSpace(string(headSymStderr)), headSymErr) + default: + // HEAD is detached or genuinely not symbolic: resolve it as a hash. out, stderr, headErr := s.runGit("rev-parse", "--verify", "--quiet", "--end-of-options", "HEAD") switch { case headErr == nil && out != "": diff --git a/cmd/entire/cli/gitrepo/reftable_test.go b/cmd/entire/cli/gitrepo/reftable_test.go index 7a081dc978..9066a6e8fe 100644 --- a/cmd/entire/cli/gitrepo/reftable_test.go +++ b/cmd/entire/cli/gitrepo/reftable_test.go @@ -189,6 +189,11 @@ const ( gitSymbolicRef = "symbolic-ref" gitRevParse = "rev-parse" gitUpdateRef = "update-ref" + gitForEachRef = "for-each-ref" + + // testHeadHash is an arbitrary valid-looking SHA-1 used as a fixture in the + // runGitFn fakes below. + testHeadHash = "20b4de1033d986a83837177f961c80bb799161e6" ) // realExitError returns a genuine *exec.ExitError (git ran and exited non-zero) @@ -208,6 +213,53 @@ func timeoutError() error { return fmt.Errorf("git rev-parse timed out: %w", context.DeadlineExceeded) } +// lastEnvValue returns the last value for key in an environment slice, matching +// os/exec's de-duplication (last value wins for a duplicate key). +func lastEnvValue(env []string, key string) (string, bool) { + value, found := "", false + for _, kv := range env { + if k, v, ok := strings.Cut(kv, "="); ok && k == key { + value, found = v, true + } + } + return value, found +} + +// TestGitPlumbingEnv_ForcesCLocale verifies that reftable git plumbing always +// runs under a C locale, so git's stderr is never translated and the +// English-substring classification (isRefCASConflict, RemoveReference +// idempotency) is correct even when the caller's environment is localized. The +// forced values must win over any inherited LANG/LC_ALL/LC_MESSAGES. +func TestGitPlumbingEnv_ForcesCLocale(t *testing.T) { + // Not parallel: t.Setenv is incompatible with t.Parallel. + t.Setenv("LANG", "de_DE.UTF-8") + t.Setenv("LC_ALL", "de_DE.UTF-8") + t.Setenv("LC_MESSAGES", "fr_FR.UTF-8") + + env := gitPlumbingEnv() + for key, want := range map[string]string{"LC_ALL": "C", "LANG": "C", "GIT_TERMINAL_PROMPT": "0"} { + got, found := lastEnvValue(env, key) + require.Truef(t, found, "%s must be set", key) + require.Equalf(t, want, got, "effective %s must be forced regardless of the caller's environment", key) + } +} + +// TestRefLookupAbsent_IsLocaleIndependent verifies the absence classifier keys +// on exit code + empty stderr, so it is correct in any locale: a translated, +// non-empty diagnostic is still surfaced as a real error, not absence. +func TestRefLookupAbsent_IsLocaleIndependent(t *testing.T) { + t.Parallel() + germanFatal := []byte("schwerwiegend: Referenz existiert nicht\n") + require.False(t, refLookupAbsent(realExitError(t), germanFatal), + "a non-empty (translated) diagnostic must be surfaced regardless of language") + require.True(t, refLookupAbsent(realExitError(t), nil), + "exit non-zero with empty stderr is absence in any locale") + require.True(t, refLookupAbsent(realExitError(t), []byte(" \n")), + "whitespace-only stderr counts as empty") + require.False(t, refLookupAbsent(timeoutError(), nil), + "a timeout is never absence") +} + // TestReference_ClassifiesLookupFailures verifies that Reference maps only a // genuine "git ran and the ref is absent" result to ErrReferenceNotFound, and // surfaces spawn/timeout/I-O failures instead. Reporting a transient git @@ -259,6 +311,27 @@ func TestReference_ClassifiesLookupFailures(t *testing.T) { require.Error(t, err) require.NotErrorIs(t, err, plumbing.ErrReferenceNotFound) }) + + t.Run("transient symbolic-ref probe failure is surfaced, not downgraded to a hash", func(t *testing.T) { + t.Parallel() + revParseCalled := false + s := &reftableStorer{gitDir: "unused", runGitFn: func(args ...string) (string, []byte, error) { + switch args[0] { + case gitSymbolicRef: + return "", nil, timeoutError() // probe times out on a possibly-symbolic ref + case gitRevParse: + revParseCalled = true + return testHeadHash, nil, nil // would wrongly succeed + default: + return "", nil, nil + } + }} + _, err := s.Reference(plumbing.ReferenceName("HEAD")) + require.Error(t, err) + require.NotErrorIs(t, err, plumbing.ErrReferenceNotFound) + require.False(t, revParseCalled, + "a transient symbolic-ref probe failure must be surfaced, not silently downgraded via rev-parse") + }) } // TestIterReferences_HEADFailureSurfaces verifies the HEAD-resolution path in @@ -283,14 +356,14 @@ func TestIterReferences_HEADFailureSurfaces(t *testing.T) { t.Run("detached HEAD resolves to a hash reference", func(t *testing.T) { t.Parallel() - hash := "20b4de1033d986a83837177f961c80bb799161e6" + hash := testHeadHash s := &reftableStorer{gitDir: "unused", runGitFn: func(args ...string) (string, []byte, error) { switch args[0] { case gitSymbolicRef: return "", nil, realExitError(t) // detached: not symbolic case gitRevParse: return hash, nil, nil - case "for-each-ref": + case gitForEachRef: return "", nil, nil default: return "", nil, nil @@ -318,7 +391,7 @@ func TestIterReferences_HEADFailureSurfaces(t *testing.T) { return "", nil, realExitError(t) // not symbolic case gitRevParse: return "", nil, realExitError(t) // absent: exited non-zero, silent - case "for-each-ref": + case gitForEachRef: return "", nil, nil default: return "", nil, nil @@ -331,6 +404,28 @@ func TestIterReferences_HEADFailureSurfaces(t *testing.T) { iter.Close() require.Equal(t, 0, count, "an unborn/absent HEAD must yield no references, not an error") }) + + t.Run("transient HEAD symbolic-ref probe failure is surfaced, not downgraded to a hash", func(t *testing.T) { + t.Parallel() + revParseCalled := false + s := &reftableStorer{gitDir: "unused", runGitFn: func(args ...string) (string, []byte, error) { + switch args[0] { + case gitSymbolicRef: + return "", nil, timeoutError() // HEAD probe times out on a branch + case gitRevParse: + revParseCalled = true + return testHeadHash, nil, nil // would wrongly succeed + case gitForEachRef: + return "", nil, nil + default: + return "", nil, nil + } + }} + _, err := s.IterReferences() + require.Error(t, err) + require.False(t, revParseCalled, + "a transient HEAD symbolic-ref probe failure must be surfaced, not downgraded to a hash HEAD") + }) } // TestRemoveReference_DeleteFailureNotSwallowed verifies that a failed deletion From 57860e3baef6929d5cec8d0c5572b52718b96bdb Mon Sep 17 00:00:00 2001 From: Suhaan Thayyil <257360244+suhaanthayyil@users.noreply.github.com> Date: Mon, 13 Jul 2026 08:56:18 -0400 Subject: [PATCH 06/11] fix(gitrepo): address review feedback Detach runCLIIn's spawned entire binary from any controlling TTY via execx.NonInteractive, matching TestEnv.RunCLIWithError and every other integration helper, so the linked-worktree reftable test can't hang on an interactive prompt path. --- cmd/entire/cli/integration_test/reftable_repo_test.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/cmd/entire/cli/integration_test/reftable_repo_test.go b/cmd/entire/cli/integration_test/reftable_repo_test.go index 82ca2dd8b8..5d480eae99 100644 --- a/cmd/entire/cli/integration_test/reftable_repo_test.go +++ b/cmd/entire/cli/integration_test/reftable_repo_test.go @@ -3,11 +3,13 @@ package integration import ( + "context" "os/exec" "path/filepath" "strings" "testing" + "github.com/entireio/cli/cmd/entire/cli/execx" "github.com/entireio/cli/cmd/entire/cli/paths" "github.com/entireio/cli/cmd/entire/cli/testutil" ) @@ -158,10 +160,12 @@ func TestReftableRepository_LinkedWorktree(t *testing.T) { } // runCLIIn runs the built entire binary in an arbitrary directory (e.g. a linked -// worktree) with the same isolated environment RunCLI uses. +// worktree) with the same isolated environment RunCLI uses, detached from any +// controlling TTY (matching TestEnv.RunCLIWithError) so an interactive prompt +// path can't hang the test. func runCLIIn(t *testing.T, env *TestEnv, dir string, args ...string) string { t.Helper() - cmd := exec.Command(getTestBinary(), args...) //nolint:noctx // test helper + cmd := execx.NonInteractive(context.Background(), getTestBinary(), args...) cmd.Dir = dir cmd.Env = env.cliEnv() out, err := cmd.CombinedOutput() From 8571f81255745ec5786bc9e19d77b97974c14ddd Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Tue, 28 Jul 2026 12:20:06 +0100 Subject: [PATCH 07/11] fix(gitrepo): support objectformat/worktreeconfig extensions in reftable repos reftableStorer.SupportsExtension shadows the promoted *gitfilesystem.Storage method, and go-git's extension verification only consults the storer. Approving solely refstorage meant a reftable repository that also declared extensions.objectformat=sha256 or extensions.worktreeConfig was rejected on open with ErrUnknownExtension, silently breaking sha256 reftable repositories. Delegate every non-refstorage extension to the embedded storage so its objectformat=sha1/sha256 and worktreeconfig support is preserved. Add regression tests that open real sha256 and worktreeConfig reftable repos. Assisted-by: Claude Opus 4.8 Signed-off-by: Paulo Gomes Entire-Checkpoint: 01KYM784182ZASDC9WBCKEFBNM --- cmd/entire/cli/gitrepo/reftable.go | 19 ++++- cmd/entire/cli/gitrepo/reftable_test.go | 98 ++++++++++++++++++++++++- 2 files changed, 112 insertions(+), 5 deletions(-) diff --git a/cmd/entire/cli/gitrepo/reftable.go b/cmd/entire/cli/gitrepo/reftable.go index fa7b6e1a78..32d0d7e8c2 100644 --- a/cmd/entire/cli/gitrepo/reftable.go +++ b/cmd/entire/cli/gitrepo/reftable.go @@ -88,10 +88,21 @@ func newReftableStorer(fs *gitfilesystem.Storage, gitDir string) *reftableStorer } // SupportsExtension lets go-git open a repository that declares the -// extensions.refstorage=reftable extension. Object-format and other extensions -// remain the responsibility of the embedded filesystem storage / go-git core. -func (s *reftableStorer) SupportsExtension(name, _ string) bool { - return strings.EqualFold(name, "refstorage") +// extensions.refstorage=reftable extension, and preserves the embedded +// filesystem storage's support for every other extension it recognises +// (objectformat=sha1/sha256, worktreeconfig). +// +// Defining this method here shadows the promoted *gitfilesystem.Storage +// method, so it must delegate: without the fallback, a reftable repository +// that also declares objectformat=sha256 or worktreeConfig would be rejected +// by go-git's extension verification with ErrUnknownExtension, since it only +// consults the storer's SupportsExtension. Reftable only changes ref storage, +// not object storage, so object-format support is unaffected. +func (s *reftableStorer) SupportsExtension(name, value string) bool { + if strings.EqualFold(name, "refstorage") { + return true + } + return s.Storage.SupportsExtension(name, value) } // runGit runs a git plumbing command scoped to this repository's git dir and diff --git a/cmd/entire/cli/gitrepo/reftable_test.go b/cmd/entire/cli/gitrepo/reftable_test.go index 9066a6e8fe..172bd27e45 100644 --- a/cmd/entire/cli/gitrepo/reftable_test.go +++ b/cmd/entire/cli/gitrepo/reftable_test.go @@ -19,6 +19,16 @@ import ( // git CLI, or skips the test when the installed git is too old to support it. // It returns the repo dir and the initial commit hash. func initReftableRepo(t *testing.T, name, content string) (string, string) { + t.Helper() + return initReftableRepoWithFormat(t, "", name, content) +} + +// initReftableRepoWithFormat is initReftableRepo with an explicit git object +// format. An empty objectFormat uses git's default (sha1); "sha256" exercises +// the sha256 hash, which additionally makes git write extensions.objectformat +// into the repo config. The test is skipped when the installed git cannot +// initialize the requested reftable + object-format combination. +func initReftableRepoWithFormat(t *testing.T, objectFormat, name, content string) (string, string) { t.Helper() repoDir := t.TempDir() @@ -28,7 +38,12 @@ func initReftableRepo(t *testing.T, name, content string) (string, string) { "GIT_TERMINAL_PROMPT=0", ) - initCmd := exec.Command("git", "init", "-b", "main", "--ref-format=reftable", repoDir) //nolint:noctx // test capability probe + initArgs := []string{"init", "-b", "main", "--ref-format=reftable"} + if objectFormat != "" { + initArgs = append(initArgs, "--object-format="+objectFormat) + } + initArgs = append(initArgs, repoDir) + initCmd := exec.Command("git", initArgs...) //nolint:noctx // test capability probe initCmd.Env = env if out, err := initCmd.CombinedOutput(); err != nil { t.Skipf("git does not support reftable repositories: %v\n%s", err, out) @@ -47,6 +62,11 @@ func initReftableRepo(t *testing.T, name, content string) (string, string) { if got := git("rev-parse", "--show-ref-format"); got != "reftable" { t.Skipf("git initialized ref format %q, not reftable", got) } + if objectFormat != "" { + if got := git("rev-parse", "--show-object-format"); got != objectFormat { + t.Skipf("git initialized object format %q, not %q", got, objectFormat) + } + } git("config", "user.name", "Test User") git("config", "user.email", "test@example.com") git("config", "commit.gpgsign", "false") @@ -57,6 +77,22 @@ func initReftableRepo(t *testing.T, name, content string) (string, string) { return repoDir, git("rev-parse", "HEAD") } +// setRepoConfig sets a local git config key in an existing repo, using an +// isolated global/system config so the developer's real git config is never +// read or written (matching the reftable test helpers). +func setRepoConfig(t *testing.T, repoDir, key, value string) { + t.Helper() + cmd := exec.Command("git", "config", key, value) //nolint:noctx // test helper + cmd.Dir = repoDir + cmd.Env = append(os.Environ(), + "GIT_CONFIG_GLOBAL="+filepath.Join(t.TempDir(), "gitconfig"), + "GIT_CONFIG_SYSTEM=/dev/null", + "GIT_TERMINAL_PROMPT=0", + ) + out, err := cmd.CombinedOutput() + require.NoErrorf(t, err, "git config %s %s: %s", key, value, out) +} + // reftableCommit adds a file and commits it in an existing reftable repo, // returning the new HEAD hash. The repo's user identity is already configured // by initReftableRepo, so only an isolated global/system config is supplied. @@ -607,6 +643,66 @@ func TestOpenPath_ReftableRepository(t *testing.T) { require.NoError(t, repo.Storer.RemoveReference(newRef.Name())) } +// TestOpenPath_Sha256ReftableRepository confirms that a reftable repository +// using the sha256 object format can be opened and that refs round-trip through +// the git-CLI-backed storer. +// +// Such a repository declares TWO extensions in its config: +// extensions.refstorage=reftable AND extensions.objectformat=sha256. go-git's +// verifyExtensions asks the storer's SupportsExtension whether each declared +// extension is supported. The embedded filesystem Storage approves +// objectformat=sha256, but reftableStorer defines its own SupportsExtension +// (to advertise refstorage), which shadows the embedded method by Go's +// promotion rules. As written it approves only refstorage, so objectformat is +// reported unsupported and go-git rejects the open with ErrUnknownExtension. +// The reftable backend thus silently breaks sha256 repositories. This test +// pins the correct behaviour and is the regression guard for that gap in #547. +func TestOpenPath_Sha256ReftableRepository(t *testing.T) { + t.Parallel() + repoDir, headHash := initReftableRepoWithFormat(t, "sha256", "file.txt", "hello\n") + + repo, err := OpenPath(repoDir) + require.NoError(t, err, "sha256 reftable repository should open; objectformat extension must stay supported") + defer repo.Close() + + head, err := repo.Head() + require.NoError(t, err) + require.Equal(t, "refs/heads/main", head.Name().String()) + require.Equal(t, headHash, head.Hash().String()) + + // A ref write/read round-trips through the git-CLI-backed storer, proving + // the sha256 repo is not merely openable but usable. + newRef := plumbing.NewHashReference(plumbing.ReferenceName("refs/entire/sha256"), head.Hash()) + require.NoError(t, repo.Storer.SetReference(newRef)) + got, err := repo.Storer.Reference(newRef.Name()) + require.NoError(t, err) + require.Equal(t, head.Hash(), got.Hash()) +} + +// TestOpenPath_WorktreeConfigReftableRepository confirms that a reftable +// repository that also enables the worktreeConfig extension can be opened. +// +// This is the same extension-shadowing gap as +// TestOpenPath_Sha256ReftableRepository: the embedded filesystem Storage +// approves worktreeconfig=true/false, but reftableStorer's own +// SupportsExtension shadows that method and approves only refstorage. A +// reftable repo with extensions.worktreeConfig=true therefore fails to open +// with ErrUnknownExtension. Regression guard for that gap in #547. +func TestOpenPath_WorktreeConfigReftableRepository(t *testing.T) { + t.Parallel() + repoDir, headHash := initReftableRepo(t, "file.txt", "hello\n") + setRepoConfig(t, repoDir, "extensions.worktreeConfig", "true") + + repo, err := OpenPath(repoDir) + require.NoError(t, err, "reftable repository with worktreeConfig should open; worktreeconfig extension must stay supported") + defer repo.Close() + + head, err := repo.Head() + require.NoError(t, err) + require.Equal(t, "refs/heads/main", head.Name().String()) + require.Equal(t, headHash, head.Hash().String()) +} + // TestRepoUsesReftable_Detection checks that reftable detection distinguishes // reftable repositories from classic files-backend repositories. func TestRepoUsesReftable_Detection(t *testing.T) { From 6d60593238d2ce56ad5dbba58226ef4d5f07af1a Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Tue, 28 Jul 2026 12:40:59 +0100 Subject: [PATCH 08/11] refactor(gitrepo): route all repo opens through gitrepo for reftable safety checkpoint/remote's openRepoAt opened repositories with git.PlainOpenWithOptions directly, bypassing the reftable-aware opener. On a reftable repository that call fails with 'unknown extension: refstorage', so provider checkpoint-URL resolution silently lost the ability to reuse an existing remote's scheme. Route openRepoAt through gitrepo.OpenCurrent/OpenPath (threading ctx) so every repo open goes through the single reftable- and alternates-aware entry point. Detect reftable stacks with os.Lstat instead of os.Stat so a symlink in place of the reftable/ directory is not mistaken for a genuine stack. Document in CLAUDE.md that gitrepo is the sole sanctioned opener and no code may call git.Open/PlainOpen directly. Add TODOs to drop the reftableStorer shim once go-git ships native reftable support. Assisted-by: Claude Opus 4.8 Signed-off-by: Paulo Gomes Entire-Checkpoint: 01KYM8EC609H2J2N4S0KYA95TA --- CLAUDE.md | 25 ++++++++++++++++++++++ cmd/entire/cli/checkpoint/remote/util.go | 27 ++++++++++++++++-------- cmd/entire/cli/gitrepo/reftable.go | 12 ++++++++++- cmd/entire/cli/gitrepo/repository.go | 5 +++++ 4 files changed, 59 insertions(+), 10 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index b3b9408297..ff99bc07a2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -460,6 +460,31 @@ Don't use `fmt.Print*` for operational messages (checkpoint saves, hook invocati We use github.com/go-git/go-git for most git operations, but with important exceptions: +#### Opening Repositories - Always Use `gitrepo` + +**Never call `git.Open`, `git.PlainOpen`, or `git.PlainOpenWithOptions` directly. +`cmd/entire/cli/gitrepo` is the single source of truth for opening a +repository.** Use `gitrepo.OpenCurrent(ctx)` for the current worktree or +`gitrepo.OpenPath(root)` for a specific worktree root. Both funnel through +`openPathWithAlternates`, which is the only place that opens a `*git.Repository`. + +Routing every open through `gitrepo` guarantees two behaviours no ad-hoc +`git.PlainOpen` call gets right: + +- **Object alternates** are rewritten to absolute paths so shared clones resolve + their objects (`PlainOpen` cannot follow relative/absolute alternates). +- **Reftable repositories** are detected and opened through the git-CLI-backed + reference storer (`reftableStorer`). A direct `git.PlainOpen` on a reftable + repo fails outright with `unknown extension: refstorage`, because go-git's + filesystem storer cannot read the reftable backend. The reftable storer also + re-approves the `objectformat` (sha1/sha256) and `worktreeconfig` extensions + that go-git verifies at open time. + +If a code path opens a repo with a bare go-git call, it silently breaks on +reftable and sha256 repositories. Reviewers should flag any new +`git.PlainOpen*`/`git.Open` outside `gitrepo`. Key files: `gitrepo/repository.go` +(open entry points) and `gitrepo/reftable.go` (`reftableStorer`). + #### go-git v5 Bugs - Use CLI Instead **Do NOT use go-git v5 for `checkout` or `reset --hard` operations.** diff --git a/cmd/entire/cli/checkpoint/remote/util.go b/cmd/entire/cli/checkpoint/remote/util.go index aad8668cd7..355c77dfa0 100644 --- a/cmd/entire/cli/checkpoint/remote/util.go +++ b/cmd/entire/cli/checkpoint/remote/util.go @@ -9,6 +9,7 @@ import ( "strings" "github.com/entireio/cli/cmd/entire/cli/gitremote" + "github.com/entireio/cli/cmd/entire/cli/gitrepo" "github.com/entireio/cli/cmd/entire/cli/logging" "github.com/entireio/cli/cmd/entire/cli/settings" @@ -117,7 +118,7 @@ func FetchURL(ctx context.Context, opts ...FetchURLOptions) (string, error) { // or an entire:// mirror of a different forge than the configured // provider). Honor the configured checkpoint_remote by targeting the // provider's canonical host over HTTPS rather than falling back to origin. - if providerURL, ok := resolveProviderCheckpointURL(config, opt.WorktreeRoot); ok { + if providerURL, ok := resolveProviderCheckpointURL(ctx, config, opt.WorktreeRoot); ok { return providerURL, nil } logFallback(ctx, "fetch", originURL, "derive checkpoint remote URL", err) @@ -223,7 +224,7 @@ func PushURL(ctx context.Context, pushRemoteName string) (string, bool, error) { // The checkpoint token is an HTTPS credential for the provider host; // it can't ride through the entire:// helper (which does its own // auth). Route to the provider over HTTPS instead of the mirror. - if providerURL, ok := resolveProviderCheckpointURL(config, ""); ok { + if providerURL, ok := resolveProviderCheckpointURL(ctx, config, ""); ok { return providerURL, true, nil } } @@ -235,7 +236,7 @@ func PushURL(ctx context.Context, pushRemoteName string) (string, bool, error) { // configured provider). Honor the configured checkpoint_remote by // targeting the provider's canonical host over HTTPS rather than // misrouting checkpoints to the origin remote. - if providerURL, ok := resolveProviderCheckpointURL(config, ""); ok { + if providerURL, ok := resolveProviderCheckpointURL(ctx, config, ""); ok { return providerURL, true, nil } fallbackURL, fallbackErr := resolvePushFallbackURL(ctx, pushRemoteName, originURL) @@ -340,8 +341,8 @@ func deriveCheckpointURLFromInfo(info *Info, config *settings.CheckpointRemoteCo // // Returns ok=false when no transport can be determined (unknown provider with no // usable signal), in which case the caller falls back to the origin remote. -func resolveProviderCheckpointURL(config *settings.CheckpointRemoteConfig, dir string) (string, bool) { - repo, err := openRepoAt(dir) +func resolveProviderCheckpointURL(ctx context.Context, config *settings.CheckpointRemoteConfig, dir string) (string, bool) { + repo, err := openRepoAt(ctx, dir) if err != nil { repo = nil // Fall back to env/provider-only signals. } @@ -384,12 +385,20 @@ func pickProviderTransport(repo *git.Repository, config *settings.CheckpointRemo } // openRepoAt opens the git repository at dir (current directory when dir is -// empty), walking up to the enclosing .git directory. -func openRepoAt(dir string) (*git.Repository, error) { +// empty). It routes through gitrepo, the single reftable-aware opener, so a +// reftable repository is opened via the git-CLI storer rather than rejected by +// go-git's extension check. gitrepo.OpenCurrent resolves the worktree root from +// the current directory (the walk-up equivalent of the previous DetectDotGit +// open) when no explicit root is given. +func openRepoAt(ctx context.Context, dir string) (*git.Repository, error) { if dir == "" { - dir = "." + repo, err := gitrepo.OpenCurrent(ctx) + if err != nil { + return nil, fmt.Errorf("open git repository: %w", err) + } + return repo, nil } - repo, err := git.PlainOpenWithOptions(dir, &git.PlainOpenOptions{DetectDotGit: true}) + repo, err := gitrepo.OpenPath(dir) if err != nil { return nil, fmt.Errorf("open git repository: %w", err) } diff --git a/cmd/entire/cli/gitrepo/reftable.go b/cmd/entire/cli/gitrepo/reftable.go index 32d0d7e8c2..26cdd74672 100644 --- a/cmd/entire/cli/gitrepo/reftable.go +++ b/cmd/entire/cli/gitrepo/reftable.go @@ -44,7 +44,11 @@ func repoUsesReftable(dotGitPath, commonGitPath string) bool { candidates = append(candidates, filepath.Join(commonGitPath, "reftable")) } for _, dir := range candidates { - if info, err := os.Stat(dir); err == nil && info.IsDir() { + // Lstat, not Stat: git creates reftable/ as a real directory, so a + // symlink in its place is not a genuine reftable stack. Lstat inspects + // the entry itself rather than following the link, so a symlink reports + // IsDir()==false and is correctly not treated as a reftable repository. + if info, err := os.Lstat(dir); err == nil && info.IsDir() { return true } } @@ -60,6 +64,12 @@ func repoUsesReftable(dotGitPath, commonGitPath string) bool { // // It also advertises reftable support via the ExtensionChecker interface so // go-git's extension verification does not reject the repository on open. +// +// TODO: remove this entire type (and its wiring in repository.go) once go-git +// gains a built-in reftable reader/writer. It exists only because the vendored +// go-git has no reftable backend, so ref operations must shell out to the git +// CLI. When upstream supports reftable natively, the plain filesystem storer +// handles these repositories and this shim can be deleted. type reftableStorer struct { *gitfilesystem.Storage diff --git a/cmd/entire/cli/gitrepo/repository.go b/cmd/entire/cli/gitrepo/repository.go index fbef9888fd..f05c2d267b 100644 --- a/cmd/entire/cli/gitrepo/repository.go +++ b/cmd/entire/cli/gitrepo/repository.go @@ -124,6 +124,11 @@ func openPathWithAlternates(repoRoot string) (*git.Repository, error) { // authoritative in a reftable repository, and its extension check rejects // extensions.refstorage=reftable outright. Route ref operations through the // git CLI for such repositories while keeping object storage on go-git. + // + // TODO: drop the reftable branch below and the whole reftableStorer + // (reftable.go) once go-git ships a native reftable reader/writer. At that + // point a plain git.Open(storage, worktreeFS) will handle reftable + // repositories directly and this CLI-backed shim is dead weight. worktreeFS := osfs.New(repoRoot, osfs.WithBoundOS()) if repoUsesReftable(dotGitPath, commonGitPath) { repo, err := git.Open(newReftableStorer(storage, dotGitPath), worktreeFS) From 00ed1ec492fec51556be66cce649699d53b1d1dc Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Tue, 28 Jul 2026 12:47:30 +0100 Subject: [PATCH 09/11] test(integration): extract repeated reftable ref-format literal to a constant goconst flagged the "reftable" string used in three --show-ref-format comparisons. Introduce refFormatReftable and use it in all three. Assisted-by: Claude Opus 4.8 Signed-off-by: Paulo Gomes Entire-Checkpoint: 01KYM8T9XXX11KRZH39K62HE1X --- cmd/entire/cli/integration_test/reftable_repo_test.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/cmd/entire/cli/integration_test/reftable_repo_test.go b/cmd/entire/cli/integration_test/reftable_repo_test.go index 5d480eae99..71a72e5add 100644 --- a/cmd/entire/cli/integration_test/reftable_repo_test.go +++ b/cmd/entire/cli/integration_test/reftable_repo_test.go @@ -14,6 +14,10 @@ import ( "github.com/entireio/cli/cmd/entire/cli/testutil" ) +// refFormatReftable is git's rev-parse --show-ref-format value for a repository +// using the reftable ref backend. +const refFormatReftable = "reftable" + // TestReftableRepository_EnableAndFirstCheckpoint exercises the full capture // flow (enable -> session start -> file changes -> stop -> user commit -> // checkpoint) against a repository using the reftable ref backend. go-git's @@ -36,7 +40,7 @@ func TestReftableRepository_EnableAndFirstCheckpoint(t *testing.T) { gitOutput(t, env.RepoDir, "add", "README.md") gitOutput(t, env.RepoDir, "commit", "-m", "Initial reftable commit") - if got := gitOutput(t, env.RepoDir, "rev-parse", "--show-ref-format"); got != "reftable" { + if got := gitOutput(t, env.RepoDir, "rev-parse", "--show-ref-format"); got != refFormatReftable { t.Fatalf("repository ref format = %q, want reftable", got) } @@ -142,7 +146,7 @@ func TestReftableRepository_LinkedWorktree(t *testing.T) { // at the worktree directory. runCLIIn(t, env, worktreePath, "enable", "--no-github", "--agent", "claude-code", "--telemetry=false") - if got := gitOutput(t, worktreePath, "rev-parse", "--show-ref-format"); got != "reftable" { + if got := gitOutput(t, worktreePath, "rev-parse", "--show-ref-format"); got != refFormatReftable { t.Fatalf("worktree ref format = %q, want reftable", got) } @@ -184,7 +188,7 @@ func requireGitReftableSupport(t *testing.T) { if output, err := cmd.CombinedOutput(); err != nil { t.Skipf("git does not support reftable repositories: %v\n%s", err, output) } - if got := gitOutput(t, dir, "rev-parse", "--show-ref-format"); got != "reftable" { + if got := gitOutput(t, dir, "rev-parse", "--show-ref-format"); got != refFormatReftable { t.Skipf("git initialized ref format %q, not reftable", got) } } From 7a344d97beaa7654f6ba861ff17e29c7a326e1fc Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Tue, 28 Jul 2026 13:19:40 +0100 Subject: [PATCH 10/11] test(integration): pin git-branch backend in reftable repo tests First-run enable now defaults to the git-refs checkpoint backend, so the reftable tests' entire/checkpoints/v1 assertions no longer hold. Pass --checkpoint-backend branch to both enable invocations, matching the sibling sha256 repo test. Assisted-by: Claude Opus 4.8 Signed-off-by: Paulo Gomes Entire-Checkpoint: 01KYMAN626BV0MWCMAX2023G6T --- cmd/entire/cli/integration_test/reftable_repo_test.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/cmd/entire/cli/integration_test/reftable_repo_test.go b/cmd/entire/cli/integration_test/reftable_repo_test.go index 71a72e5add..9d46e34b98 100644 --- a/cmd/entire/cli/integration_test/reftable_repo_test.go +++ b/cmd/entire/cli/integration_test/reftable_repo_test.go @@ -44,11 +44,15 @@ func TestReftableRepository_EnableAndFirstCheckpoint(t *testing.T) { t.Fatalf("repository ref format = %q, want reftable", got) } + // Pin the git-branch backend: this test asserts the v1-branch condensation + // flow in a reftable repo, and first-run enable now defaults new setups to + // git-refs. output := env.RunCLI( "enable", "--no-github", "--agent", "claude-code", "--telemetry=false", + "--checkpoint-backend", "branch", ) if !strings.Contains(output, paths.MetadataBranchName) { t.Fatalf("expected enable to create %s branch, got output:\n%s", paths.MetadataBranchName, output) @@ -144,7 +148,10 @@ func TestReftableRepository_LinkedWorktree(t *testing.T) { // Enable and drive a checkpoint from within the worktree by pointing the CLI // at the worktree directory. - runCLIIn(t, env, worktreePath, "enable", "--no-github", "--agent", "claude-code", "--telemetry=false") + // Pin the git-branch backend (see TestReftableRepository_EnableAndFirstCheckpoint): + // first-run enable now defaults to git-refs, but this test asserts the + // v1-branch metadata flow. + runCLIIn(t, env, worktreePath, "enable", "--no-github", "--agent", "claude-code", "--telemetry=false", "--checkpoint-backend", "branch") if got := gitOutput(t, worktreePath, "rev-parse", "--show-ref-format"); got != refFormatReftable { t.Fatalf("worktree ref format = %q, want reftable", got) From 6eba81b59d1fad12ea97dfb8c69a7958025a9386 Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Tue, 28 Jul 2026 13:47:53 +0100 Subject: [PATCH 11/11] test(integration): cover reftable repos under the git-refs default backend Add TestReftableRepository_GitRefsBackend and its linked-worktree variant, which drive the full capture flow on a reftable repo using the shipped default (git-refs) backend and assert per-checkpoint refs under refs/entire/checkpoints are written/read through the reftable storer, with no v1 branch. Extract bootstrapReftableRepo/readWorktreeFile helpers and drop runCLIIn's unused return. Assisted-by: Claude Opus 4.8 Signed-off-by: Paulo Gomes Entire-Checkpoint: 01KYMC8VEJVCRC9TM5J9ESR6XA --- .../integration_test/reftable_repo_test.go | 147 +++++++++++++++++- 1 file changed, 145 insertions(+), 2 deletions(-) diff --git a/cmd/entire/cli/integration_test/reftable_repo_test.go b/cmd/entire/cli/integration_test/reftable_repo_test.go index 9d46e34b98..67d8a87054 100644 --- a/cmd/entire/cli/integration_test/reftable_repo_test.go +++ b/cmd/entire/cli/integration_test/reftable_repo_test.go @@ -4,6 +4,7 @@ package integration import ( "context" + "os" "os/exec" "path/filepath" "strings" @@ -170,11 +171,154 @@ func TestReftableRepository_LinkedWorktree(t *testing.T) { } } +// TestReftableRepository_GitRefsBackend exercises the full capture flow against a +// reftable repository using the shipped default checkpoint backend (git-refs), +// where each checkpoint is condensed to its own ref under refs/entire/checkpoints +// rather than to the entire/checkpoints/v1 branch. Every ref write and read goes +// through gitrepo.reftableStorer, so this proves the reftable backend works with +// the default per-checkpoint ref layout, not just the git-branch flow. It runs +// WITHOUT --checkpoint-backend and without an ENTIRE_CHECKPOINTS_PRIMARY override +// so it pins the actual shipped default. +func TestReftableRepository_GitRefsBackend(t *testing.T) { + t.Parallel() + requireGitReftableSupport(t) + + env := NewTestEnv(t) + bootstrapReftableRepo(t, env) + + // No --checkpoint-backend flag: exercise the shipped first-run default, which + // must write the git-refs primary into settings.json. + env.RunCLI("enable", "--no-github", "--agent", "claude-code", "--telemetry=false") + if s := env.ReadFile(".entire/settings.json"); !strings.Contains(s, `"git-refs"`) { + t.Fatalf("first-run enable on a reftable repo should default to the git-refs backend, settings.json:\n%s", s) + } + + initialHead := gitOutput(t, env.RepoDir, "rev-parse", "HEAD") + + sess := env.NewSession() + prompt := "Create a file in the reftable repo" + if err := env.SimulateUserPromptSubmitWithPromptAndTranscriptPath(sess.ID, prompt, sess.TranscriptPath); err != nil { + t.Fatalf("user-prompt-submit failed: %v", err) + } + + const mainContent = "package main\n\nfunc main() {}\n" + env.WriteFile("main.go", mainContent) + sess.CreateTranscript(prompt, []FileChange{{Path: "main.go", Content: mainContent}}) + if err := env.SimulateStop(sess.ID, sess.TranscriptPath); err != nil { + t.Fatalf("stop hook failed creating first checkpoint: %v", err) + } + + state, err := env.GetSessionState(sess.ID) + if err != nil { + t.Fatalf("GetSessionState failed: %v", err) + } + if state == nil || state.StepCount != 1 { + t.Fatalf("session StepCount after first checkpoint = %#v, want 1", state) + } + + // The shadow branch is created and advanced via reftable ref writes. + shadowBranch := env.GetShadowBranchNameForCommit(initialHead) + if got := gitOutput(t, env.RepoDir, "rev-parse", shadowBranch); got == "" { + t.Fatalf("expected shadow branch %s to resolve", shadowBranch) + } + + env.GitCommitWithShadowHooks("Add reftable main", "main.go") + if userHead := gitOutput(t, env.RepoDir, "rev-parse", "HEAD"); userHead == initialHead { + t.Fatal("expected user commit to advance HEAD") + } + + // git-refs artifact: the condensed checkpoint lands on a per-checkpoint ref + // under refs/entire/checkpoints (written through the reftable storer), and the + // v1 branch is never created under this backend. + if refs := gitOutput(t, env.RepoDir, "for-each-ref", checkpointRefPrefix); refs == "" { + t.Fatalf("expected a per-checkpoint ref under %s for the git-refs default", checkpointRefPrefix) + } + if env.BranchExists(paths.MetadataBranchName) { + t.Fatalf("git-refs default must not create the %s branch", paths.MetadataBranchName) + } + + // The checkpoint's exact ref resolves through the reftable read path, and its + // ID is recoverable from the code commit's Entire-Checkpoint trailer. + checkpointID := env.GetLatestCheckpointIDFromHistory() + if !refExists(t, env.RepoDir, checkpointRefName(checkpointID)) { + t.Fatalf("expected checkpoint ref %s to resolve", checkpointRefName(checkpointID)) + } + + // checkpoint list must work against the reftable repo (read path). + if listOut := env.RunCLI("checkpoint", "list"); !strings.Contains(listOut, checkpointID) { + t.Fatalf("checkpoint list missing checkpoint %s:\n%s", checkpointID, listOut) + } +} + +// TestReftableRepository_GitRefsBackend_LinkedWorktree verifies that first-run +// enable with the default git-refs backend succeeds inside a linked worktree of a +// reftable repository (shared reftable stack under the common git dir) and that +// the reftable read paths work from the worktree. The git-branch variant is +// covered by TestReftableRepository_LinkedWorktree. +func TestReftableRepository_GitRefsBackend_LinkedWorktree(t *testing.T) { + t.Parallel() + requireGitReftableSupport(t) + + env := NewTestEnv(t) + bootstrapReftableRepo(t, env) + + worktreePath := filepath.Join(t.TempDir(), "wt") + gitOutput(t, env.RepoDir, "worktree", "add", "-b", "feature/wt", worktreePath) + + // Default backend (git-refs): no --checkpoint-backend flag. + runCLIIn(t, env, worktreePath, "enable", "--no-github", "--agent", "claude-code", "--telemetry=false") + + if s := readWorktreeFile(t, worktreePath, ".entire/settings.json"); !strings.Contains(s, `"git-refs"`) { + t.Fatalf("enable in a reftable worktree should default to git-refs, settings.json:\n%s", s) + } + if got := gitOutput(t, worktreePath, "rev-parse", "--show-ref-format"); got != refFormatReftable { + t.Fatalf("worktree ref format = %q, want reftable", got) + } + + // A ref read against the worktree (HEAD resolution through the reftable storer, + // whose HEAD stub go-git cannot read) must return the real branch. + if branch := gitOutput(t, worktreePath, "rev-parse", "--abbrev-ref", "HEAD"); branch != "feature/wt" { + t.Fatalf("worktree branch = %q, want feature/wt", branch) + } + + // The git-refs default must not bootstrap the v1 branch. + if got := gitOutput(t, worktreePath, "for-each-ref", checkpointRefPrefix); got != "" { + t.Fatalf("no checkpoint should exist yet, got refs:\n%s", got) + } +} + +// bootstrapReftableRepo initializes env.RepoDir as a reftable repository with an +// initial commit via the git CLI. Integration tests deliberately avoid the enable +// bootstrap path and drive hooks through getTestBinary(), so the repo is created +// directly here. +func bootstrapReftableRepo(t *testing.T, env *TestEnv) { + t.Helper() + gitOutput(t, "", "init", "--ref-format=reftable", env.RepoDir) + gitOutput(t, env.RepoDir, "config", "user.name", "Test User") + gitOutput(t, env.RepoDir, "config", "user.email", "test@example.com") + gitOutput(t, env.RepoDir, "config", "commit.gpgsign", "false") + env.WriteFile("README.md", "# reftable repo\n") + gitOutput(t, env.RepoDir, "add", "README.md") + gitOutput(t, env.RepoDir, "commit", "-m", "Initial reftable commit") +} + +// readWorktreeFile reads a file relative to a linked worktree root. env.ReadFile +// is scoped to env.RepoDir, so worktree-local files (e.g. a per-worktree +// .entire/settings.json) need a direct read. +func readWorktreeFile(t *testing.T, worktreePath, rel string) string { + t.Helper() + data, err := os.ReadFile(filepath.Join(worktreePath, rel)) + if err != nil { + t.Fatalf("read %s in worktree: %v", rel, err) + } + return string(data) +} + // runCLIIn runs the built entire binary in an arbitrary directory (e.g. a linked // worktree) with the same isolated environment RunCLI uses, detached from any // controlling TTY (matching TestEnv.RunCLIWithError) so an interactive prompt // path can't hang the test. -func runCLIIn(t *testing.T, env *TestEnv, dir string, args ...string) string { +func runCLIIn(t *testing.T, env *TestEnv, dir string, args ...string) { t.Helper() cmd := execx.NonInteractive(context.Background(), getTestBinary(), args...) cmd.Dir = dir @@ -183,7 +327,6 @@ func runCLIIn(t *testing.T, env *TestEnv, dir string, args ...string) string { if err != nil { t.Fatalf("entire %s (in %s) failed: %v\n%s", strings.Join(args, " "), dir, err, out) } - return string(out) } func requireGitReftableSupport(t *testing.T) {