From 434aa5f498fa5022d2e497cf680ee2d0c2c837e6 Mon Sep 17 00:00:00 2001 From: Lakshman Patel Date: Wed, 15 Jul 2026 11:16:53 +0530 Subject: [PATCH 1/4] Add operation log and 'trace undo' to revert rewind/reset/fork/cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a jj-style operation log (cli/oplog), the only record of trace's own ref mutations — previously the sole safety net was git's own reflog, which isn't queryable via trace and never covers the shadow-branch SetReference rewrites rewind/cleanup perform directly (those don't touch HEAD). Entries are stored on the existing trace/checkpoints/v1 orphan branch alongside checkpoints, so they sync the same way via the already-working push/fetch/merge path. 'trace undo' reverts the most recent recorded operation; 'trace log' lists the operation history. --- cli/fork_cmd.go | 13 +- cli/oplog/oplog.go | 257 +++++++++++++++++++++++++++ cli/oplog/oplog_test.go | 139 +++++++++++++++ cli/oplog_cmd.go | 64 +++++++ cli/rewind.go | 2 + cli/rewind_2.go | 31 ++++ cli/root.go | 2 + cli/strategy/cleanup.go | 8 + cli/strategy/manual_commit_rewind.go | 13 ++ cli/strategy/oplog_helper.go | 38 ++++ cli/undo.go | 95 ++++++++++ cli/undo_test.go | 123 +++++++++++++ 12 files changed, 784 insertions(+), 1 deletion(-) create mode 100644 cli/oplog/oplog.go create mode 100644 cli/oplog/oplog_test.go create mode 100644 cli/oplog_cmd.go create mode 100644 cli/strategy/oplog_helper.go create mode 100644 cli/undo.go create mode 100644 cli/undo_test.go diff --git a/cli/fork_cmd.go b/cli/fork_cmd.go index 9594522..587009c 100644 --- a/cli/fork_cmd.go +++ b/cli/fork_cmd.go @@ -13,9 +13,11 @@ import ( "github.com/GrayCodeAI/trace/cli/checkpoint/id" "github.com/GrayCodeAI/trace/cli/checkpoint/remote" "github.com/GrayCodeAI/trace/cli/logging" + "github.com/GrayCodeAI/trace/cli/oplog" "github.com/GrayCodeAI/trace/cli/paths" "github.com/GrayCodeAI/trace/cli/session" "github.com/GrayCodeAI/trace/cli/settings" + "github.com/GrayCodeAI/trace/cli/strategy" "github.com/GrayCodeAI/trace/cli/trailers" "github.com/GrayCodeAI/trace/cli/versioninfo" "github.com/google/uuid" @@ -212,12 +214,21 @@ func forkSession( // gives the fork an independent starting point without copying a worktree. commitHash, branchName := forkCodeCommit(ctx, repo, cpID, meta, newSessionID) if !commitHash.IsZero() { - ref := plumbing.NewHashReference(plumbing.NewBranchReferenceName(branchName), commitHash) + refName := plumbing.NewBranchReferenceName(branchName) + ref := plumbing.NewHashReference(refName, commitHash) if err := repo.Storer.SetReference(ref); err != nil { return forkResult{}, fmt.Errorf("failed to create fork branch %s: %w", branchName, err) } result.ForkBranch = branchName result.BaseCommit = commitHash.String() + + // New branch, so before-hash is the zero hash — 'trace undo' deletes + // the branch rather than trying to point it "back" anywhere. + if logErr := strategy.RecordOplogEntry( + ctx, repo, oplog.OpFork, refName.String(), plumbing.ZeroHash, commitHash, cpID.String(), + ); logErr != nil { + logging.Warn(ctx, "failed to record oplog entry for fork", "error", logErr.Error()) + } } if err := writeForkSessionState(ctx, newSessionID, result.BaseCommit, meta); err != nil { diff --git a/cli/oplog/oplog.go b/cli/oplog/oplog.go new file mode 100644 index 0000000..1c30a32 --- /dev/null +++ b/cli/oplog/oplog.go @@ -0,0 +1,257 @@ +// Package oplog is a jj-style operation log: a record of every +// state-mutating git operation trace performs (rewind, reset --hard, fork, +// checkpoint cleanup), kept separately from the operation each of those +// performs so a bad operation can itself be undone. +// +// Before this package existed, the only record of these operations was +// git's own reflog — not queryable via trace, expiring on its own schedule, +// and not covering the shadow-branch ref rewrites rewind/cleanup perform +// directly via SetReference (those don't touch HEAD, so they never appear +// in the reflog at all). +// +// Entries are stored as JSON blobs on the trace/checkpoints/v1 orphan +// branch, in the same sharded-path convention checkpoints already use +// (oplog///entry.json). That means entries participate in +// the same push/fetch/merge sync checkpoints already have — no new sync +// mechanism was built for this. +package oplog + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io" + "sort" + "strings" + "time" + + "github.com/GrayCodeAI/trace/cli/checkpoint" + "github.com/GrayCodeAI/trace/cli/paths" + + "github.com/go-git/go-git/v6" + "github.com/go-git/go-git/v6/plumbing" + "github.com/go-git/go-git/v6/plumbing/filemode" + "github.com/go-git/go-git/v6/plumbing/object" +) + +// Op identifies the kind of state-mutating operation recorded in the log. +type Op string + +const ( + OpRewind Op = "rewind" + OpResetHard Op = "reset_hard" + OpFork Op = "fork" + OpCleanup Op = "cleanup" + OpUndo Op = "undo" +) + +// Entry is a single record in the operation log. +// +// BeforeHash/AfterHash are stored as plain hex strings (via +// plumbing.Hash.String()), not plumbing.Hash itself — that type keeps its +// bytes in unexported fields with no custom (Un)MarshalJSON, so a struct +// containing it round-trips through encoding/json as all-zero with no error +// at all, which is exactly the kind of silent-corruption bug this package +// exists to avoid. +type Entry struct { + ID string `json:"id"` + Op Op `json:"op"` + Timestamp time.Time `json:"timestamp"` + Ref string `json:"ref"` + BeforeHash string `json:"before_hash"` + AfterHash string `json:"after_hash"` + CheckpointID string `json:"checkpoint_id,omitempty"` + Detail string `json:"detail,omitempty"` +} + +const oplogDir = "oplog" + +// Append records a new operation-log entry, committing it onto the +// trace/checkpoints/v1 orphan branch (creating the branch if it doesn't yet +// exist). authorName/authorEmail identify the committer for the oplog +// commit itself; callers typically already have these on hand via +// strategy.GetGitAuthorFromRepo. +func Append(ctx context.Context, repo *git.Repository, e Entry, authorName, authorEmail string) error { + if e.ID == "" { + return errors.New("oplog: entry ID is required") + } + if len(e.ID) < 3 { + return fmt.Errorf("oplog: entry ID %q is too short to shard", e.ID) + } + if e.Timestamp.IsZero() { + e.Timestamp = time.Now().UTC() + } + + data, err := json.MarshalIndent(e, "", " ") + if err != nil { + return fmt.Errorf("oplog: marshal entry: %w", err) + } + blobHash, err := checkpoint.CreateBlobFromContent(repo, data) + if err != nil { + return fmt.Errorf("oplog: create blob: %w", err) + } + + refName := plumbing.NewBranchReferenceName(paths.MetadataBranchName) + var parentHash, rootTreeHash plumbing.Hash + ref, err := repo.Reference(refName, true) + switch { + case err == nil: + parentHash = ref.Hash() + commit, cErr := repo.CommitObject(parentHash) + if cErr != nil { + return fmt.Errorf("oplog: read metadata branch commit: %w", cErr) + } + rootTreeHash = commit.TreeHash + case errors.Is(err, plumbing.ErrReferenceNotFound): + parentHash = plumbing.ZeroHash + rootTreeHash = plumbing.ZeroHash + default: + return fmt.Errorf("oplog: read metadata branch ref: %w", err) + } + + newRootTreeHash, err := spliceOplogEntry(ctx, repo, rootTreeHash, e.ID, blobHash) + if err != nil { + return err + } + + msg := fmt.Sprintf("oplog: %s\n\nRecorded by trace to allow undoing this operation.\n", e.Op) + commitHash, err := checkpoint.CreateCommit(ctx, repo, newRootTreeHash, parentHash, msg, authorName, authorEmail) + if err != nil { + return fmt.Errorf("oplog: create commit: %w", err) + } + + if err := repo.Storer.SetReference(plumbing.NewHashReference(refName, commitHash)); err != nil { + return fmt.Errorf("oplog: update metadata branch ref: %w", err) + } + return nil +} + +// spliceOplogEntry rebuilds only the "oplog/" subtree of the metadata +// branch's root tree, leaving every other top-level entry (checkpoints, +// sessions, etc.) untouched and byte-identical — those subtrees keep their +// existing hashes rather than being needlessly re-encoded. +func spliceOplogEntry(ctx context.Context, repo *git.Repository, rootTreeHash plumbing.Hash, id string, blobHash plumbing.Hash) (plumbing.Hash, error) { + oplogEntries := map[string]object.TreeEntry{} + var otherRootEntries []object.TreeEntry + + if rootTreeHash != plumbing.ZeroHash { + rootTree, err := repo.TreeObject(rootTreeHash) + if err != nil { + return plumbing.ZeroHash, fmt.Errorf("oplog: read root tree: %w", err) + } + for _, entry := range rootTree.Entries { + if entry.Name == oplogDir { + oplogTree, err := repo.TreeObject(entry.Hash) + if err != nil { + return plumbing.ZeroHash, fmt.Errorf("oplog: read oplog subtree: %w", err) + } + if err := checkpoint.FlattenTree(repo, oplogTree, "", oplogEntries); err != nil { + return plumbing.ZeroHash, fmt.Errorf("oplog: flatten oplog subtree: %w", err) + } + continue + } + otherRootEntries = append(otherRootEntries, entry) + } + } + + shardPath := id[:2] + "/" + id[2:] + "/entry.json" + oplogEntries[shardPath] = object.TreeEntry{Mode: filemode.Regular, Hash: blobHash} + + newOplogTreeHash, err := checkpoint.BuildTreeFromEntries(ctx, repo, oplogEntries) + if err != nil { + return plumbing.ZeroHash, fmt.Errorf("oplog: build oplog subtree: %w", err) + } + + newRootEntries := append(otherRootEntries, object.TreeEntry{Name: oplogDir, Mode: filemode.Dir, Hash: newOplogTreeHash}) + sort.Slice(newRootEntries, func(i, j int) bool { return newRootEntries[i].Name < newRootEntries[j].Name }) + + newTree := &object.Tree{Entries: newRootEntries} + obj := repo.Storer.NewEncodedObject() + if err := newTree.Encode(obj); err != nil { + return plumbing.ZeroHash, fmt.Errorf("oplog: encode root tree: %w", err) + } + return repo.Storer.SetEncodedObject(obj) +} + +// List returns operation-log entries, newest first. limit <= 0 means no +// limit. Returns (nil, nil) if the metadata branch or the oplog subtree +// doesn't exist yet (nothing has been recorded). +func List(repo *git.Repository, limit int) ([]Entry, error) { + refName := plumbing.NewBranchReferenceName(paths.MetadataBranchName) + ref, err := repo.Reference(refName, true) + if err != nil { + if errors.Is(err, plumbing.ErrReferenceNotFound) { + return nil, nil + } + return nil, fmt.Errorf("oplog: read metadata branch ref: %w", err) + } + commit, err := repo.CommitObject(ref.Hash()) + if err != nil { + return nil, fmt.Errorf("oplog: read metadata branch commit: %w", err) + } + rootTree, err := commit.Tree() + if err != nil { + return nil, fmt.Errorf("oplog: read metadata branch tree: %w", err) + } + + var oplogTree *object.Tree + for _, e := range rootTree.Entries { + if e.Name == oplogDir { + t, tErr := repo.TreeObject(e.Hash) + if tErr != nil { + return nil, fmt.Errorf("oplog: read oplog subtree: %w", tErr) + } + oplogTree = t + break + } + } + if oplogTree == nil { + return nil, nil + } + + flat := map[string]object.TreeEntry{} + if err := checkpoint.FlattenTree(repo, oplogTree, "", flat); err != nil { + return nil, fmt.Errorf("oplog: flatten oplog subtree: %w", err) + } + + entries := make([]Entry, 0, len(flat)) + for path, te := range flat { + if !strings.HasSuffix(path, "/entry.json") { + continue + } + e, err := readEntryBlob(repo, te.Hash) + if err != nil { + return nil, fmt.Errorf("oplog: read entry %s: %w", path, err) + } + entries = append(entries, e) + } + + sort.Slice(entries, func(i, j int) bool { return entries[i].Timestamp.After(entries[j].Timestamp) }) + if limit > 0 && len(entries) > limit { + entries = entries[:limit] + } + return entries, nil +} + +func readEntryBlob(repo *git.Repository, hash plumbing.Hash) (Entry, error) { + var e Entry + blob, err := repo.BlobObject(hash) + if err != nil { + return e, err + } + reader, err := blob.Reader() + if err != nil { + return e, err + } + defer func() { _ = reader.Close() }() + + data, err := io.ReadAll(reader) + if err != nil { + return e, err + } + if err := json.Unmarshal(data, &e); err != nil { + return e, err + } + return e, nil +} diff --git a/cli/oplog/oplog_test.go b/cli/oplog/oplog_test.go new file mode 100644 index 0000000..a2f8d18 --- /dev/null +++ b/cli/oplog/oplog_test.go @@ -0,0 +1,139 @@ +package oplog + +import ( + "context" + "strings" + "testing" + "time" + + "github.com/GrayCodeAI/trace/cli/testutil" + + "github.com/go-git/go-git/v6" +) + +func openTestRepo(t *testing.T) *git.Repository { + t.Helper() + dir := t.TempDir() + testutil.InitRepo(t, dir) + repo, err := git.PlainOpen(dir) + if err != nil { + t.Fatalf("git.PlainOpen: %v", err) + } + return repo +} + +func TestAppendAndList_RoundTrip(t *testing.T) { + repo := openTestRepo(t) + ctx := context.Background() + + e1 := Entry{ + ID: "aaaaaaaaaa", + Op: OpRewind, + Timestamp: time.Now().UTC().Add(-time.Minute), + Ref: "refs/heads/trace/deadbeef", + BeforeHash: strings.Repeat("1", 39) + "a", + AfterHash: strings.Repeat("2", 39) + "b", + } + if err := Append(ctx, repo, e1, "Test User", "test@example.com"); err != nil { + t.Fatalf("Append(e1): %v", err) + } + + e2 := Entry{ + ID: "bbbbbbbbbb", + Op: OpResetHard, + Timestamp: time.Now().UTC(), + Ref: "refs/heads/main", + } + if err := Append(ctx, repo, e2, "Test User", "test@example.com"); err != nil { + t.Fatalf("Append(e2): %v", err) + } + + entries, err := List(repo, 0) + if err != nil { + t.Fatalf("List: %v", err) + } + if len(entries) != 2 { + t.Fatalf("len(entries) = %d, want 2", len(entries)) + } + // Newest first. + if entries[0].ID != e2.ID { + t.Errorf("entries[0].ID = %q, want %q (newest first)", entries[0].ID, e2.ID) + } + if entries[1].ID != e1.ID { + t.Errorf("entries[1].ID = %q, want %q", entries[1].ID, e1.ID) + } + if entries[1].BeforeHash != e1.BeforeHash { + t.Errorf("entries[1].BeforeHash = %v, want %v", entries[1].BeforeHash, e1.BeforeHash) + } +} + +func TestList_EmptyWhenNoMetadataBranch(t *testing.T) { + repo := openTestRepo(t) + entries, err := List(repo, 0) + if err != nil { + t.Fatalf("List: %v", err) + } + if entries != nil { + t.Errorf("List() = %v, want nil for a repo with no metadata branch", entries) + } +} + +func TestList_RespectsLimit(t *testing.T) { + repo := openTestRepo(t) + ctx := context.Background() + + for i, id := range []string{"aaaaaaaaaa", "bbbbbbbbbb", "cccccccccc"} { + e := Entry{ + ID: id, + Op: OpFork, + Timestamp: time.Now().UTC().Add(time.Duration(i) * time.Second), + } + if err := Append(ctx, repo, e, "Test User", "test@example.com"); err != nil { + t.Fatalf("Append(%s): %v", id, err) + } + } + + entries, err := List(repo, 2) + if err != nil { + t.Fatalf("List: %v", err) + } + if len(entries) != 2 { + t.Fatalf("len(entries) = %d, want 2", len(entries)) + } + if entries[0].ID != "cccccccccc" { + t.Errorf("entries[0].ID = %q, want cccccccccc (newest)", entries[0].ID) + } +} + +func TestAppend_PreservesUnrelatedRootEntries(t *testing.T) { + // Append must not disturb other top-level trees on the metadata branch + // (checkpoints, sessions, etc.) — only splice the oplog/ subtree. + repo := openTestRepo(t) + ctx := context.Background() + + if err := Append(ctx, repo, Entry{ID: "aaaaaaaaaa", Op: OpRewind, Timestamp: time.Now()}, "Test User", "test@example.com"); err != nil { + t.Fatalf("Append: %v", err) + } + + // Second append onto the same branch must succeed and both entries must + // still be readable — a regression here would mean the splice logic + // dropped the first entry. + if err := Append(ctx, repo, Entry{ID: "bbbbbbbbbb", Op: OpCleanup, Timestamp: time.Now()}, "Test User", "test@example.com"); err != nil { + t.Fatalf("second Append: %v", err) + } + + entries, err := List(repo, 0) + if err != nil { + t.Fatalf("List: %v", err) + } + if len(entries) != 2 { + t.Fatalf("len(entries) = %d, want 2 after two appends", len(entries)) + } +} + +func TestAppend_RequiresID(t *testing.T) { + repo := openTestRepo(t) + if err := Append(context.Background(), repo, Entry{Op: OpRewind}, "Test User", "test@example.com"); err == nil { + t.Fatal("expected an error for an entry with no ID") + } +} diff --git a/cli/oplog_cmd.go b/cli/oplog_cmd.go new file mode 100644 index 0000000..f9546a2 --- /dev/null +++ b/cli/oplog_cmd.go @@ -0,0 +1,64 @@ +package cli + +import ( + "context" + "errors" + "fmt" + "io" + + "github.com/GrayCodeAI/trace/cli/oplog" + "github.com/GrayCodeAI/trace/cli/paths" + + "github.com/spf13/cobra" +) + +func newOplogCmd() *cobra.Command { + var limit int + cmd := &cobra.Command{ + Use: "log", + Short: "Show trace's operation log (rewinds, resets, forks, cleanups)", + Long: `Log shows the state-mutating operations trace has recorded, newest first — +the same history 'trace undo' steps back through. This is separate from git +log: it tracks trace's own ref mutations (including ones that never touch +HEAD, like a shadow-branch rewind), not commit history.`, + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, args []string) error { + ctx := cmd.Context() + if _, err := paths.WorktreeRoot(ctx); err != nil { + cmd.SilenceUsage = true + return errors.New("not a git repository") + } + return runOplogList(ctx, cmd.OutOrStdout(), limit) + }, + } + cmd.Flags().IntVar(&limit, "limit", 20, "maximum number of entries to show (0 = all)") + return cmd +} + +func runOplogList(ctx context.Context, w io.Writer, limit int) error { + repo, err := openRepository(ctx) + if err != nil { + return fmt.Errorf("not a git repository: %w", err) + } + + entries, err := oplog.List(repo, limit) + if err != nil { + return fmt.Errorf("failed to read operation log: %w", err) + } + if len(entries) == 0 { + fmt.Fprintln(w, "No operations recorded yet.") + return nil + } + + for _, e := range entries { + fmt.Fprintf(w, "%s %-11s %s", e.Timestamp.Format("2006-01-02 15:04:05"), e.Op, e.Ref) + if len(e.BeforeHash) >= 7 && len(e.AfterHash) >= 7 { + fmt.Fprintf(w, " %s -> %s", e.BeforeHash[:7], e.AfterHash[:7]) + } + if e.CheckpointID != "" { + fmt.Fprintf(w, " (%s)", e.CheckpointID) + } + fmt.Fprintln(w) + } + return nil +} diff --git a/cli/rewind.go b/cli/rewind.go index 1fcca3f..e79fb73 100644 --- a/cli/rewind.go +++ b/cli/rewind.go @@ -686,6 +686,8 @@ func handleLogsOnlyResetNonInteractive(ctx context.Context, w, errW io.Writer, s return fmt.Errorf("failed to reset branch: %w", err) } + recordResetOplogEntry(logCtx, currentHead, point.ID) + logging.Debug( logCtx, "logs-only reset completed", slog.String("checkpoint_id", point.ID), diff --git a/cli/rewind_2.go b/cli/rewind_2.go index c7166e7..197e094 100644 --- a/cli/rewind_2.go +++ b/cli/rewind_2.go @@ -14,6 +14,7 @@ import ( agentpkg "github.com/GrayCodeAI/trace/cli/agent" "github.com/GrayCodeAI/trace/cli/logging" + "github.com/GrayCodeAI/trace/cli/oplog" "github.com/GrayCodeAI/trace/cli/strategy" "github.com/GrayCodeAI/trace/cli/transcript" @@ -310,6 +311,8 @@ func handleLogsOnlyReset(ctx context.Context, w, errW io.Writer, start *strategy return fmt.Errorf("failed to reset branch: %w", err) } + recordResetOplogEntry(logCtx, currentHead, point.ID) + logging.Debug( logCtx, "logs-only reset (interactive) completed", slog.String("checkpoint_id", point.ID), @@ -446,6 +449,34 @@ func performGitResetHard(ctx context.Context, commitHash string) error { return nil } +// recordResetOplogEntry appends an oplog entry for a completed git reset +// --hard, resolving the reset branch's ref name via HEAD (reset --hard +// moves whatever ref HEAD currently points to, branch or detached). +// Best-effort: failures are logged, not propagated — the reset itself +// already succeeded by the time this is called. +func recordResetOplogEntry(ctx context.Context, beforeHex, afterHex string) { + if beforeHex == "" { + // getCurrentHeadHash() failed before the reset; nothing to record. + return + } + repo, err := openRepository(ctx) + if err != nil { + logging.Warn(ctx, "failed to open repository for oplog entry", "error", err.Error()) + return + } + head, err := repo.Head() + if err != nil { + logging.Warn(ctx, "failed to resolve HEAD for oplog entry", "error", err.Error()) + return + } + if err := strategy.RecordOplogEntry( + ctx, repo, oplog.OpResetHard, head.Name().String(), + plumbing.NewHash(beforeHex), plumbing.NewHash(afterHex), "", + ); err != nil { + logging.Warn(ctx, "failed to record oplog entry for reset --hard", "error", err.Error()) + } +} + // sanitizeForTerminal removes or replaces characters that cause rendering issues // in terminal UI components. This includes emojis with skin-tone modifiers and // other multi-codepoint characters that confuse width calculations. diff --git a/cli/root.go b/cli/root.go index 096fc87..4966115 100644 --- a/cli/root.go +++ b/cli/root.go @@ -120,6 +120,8 @@ func NewRootCmd() *cobra.Command { cmd.AddCommand(newForkCmd()) // 'fork' — clone a checkpoint into a new session for A/B testing cmd.AddCommand(newAnnotateCmd()) // 'annotate' — attach comments to a session/checkpoint cmd.AddCommand(newCIInitCmd()) // 'ci-init' — configure CI session auto-capture + cmd.AddCommand(newUndoCmd()) // 'undo' — revert the most recent rewind/reset/fork/cleanup + cmd.AddCommand(newOplogCmd()) // 'log' — show trace's operation log // Hidden top-level shortcuts. Functional but print a deprecation hint. cmd.AddCommand(hideAsAlias(newRewindCmd(), "trace checkpoint rewind")) diff --git a/cli/strategy/cleanup.go b/cli/strategy/cleanup.go index 6c43962..a23bc62 100644 --- a/cli/strategy/cleanup.go +++ b/cli/strategy/cleanup.go @@ -14,6 +14,7 @@ import ( "github.com/GrayCodeAI/trace/cli/checkpoint/id" "github.com/GrayCodeAI/trace/cli/checkpoint/remote" "github.com/GrayCodeAI/trace/cli/logging" + "github.com/GrayCodeAI/trace/cli/oplog" "github.com/GrayCodeAI/trace/cli/paths" "github.com/GrayCodeAI/trace/cli/session" "github.com/GrayCodeAI/trace/cli/settings" @@ -342,6 +343,13 @@ func DeleteOrphanedCheckpoints(ctx context.Context, checkpointIDs []string) (del return nil, nil, fmt.Errorf("failed to update branch: %w", err) } + if logErr := RecordOplogEntry( + ctx, repo, oplog.OpCleanup, refName.String(), ref.Hash(), commitHash, + strings.Join(checkpointIDs, ","), + ); logErr != nil { + logging.Warn(ctx, "failed to record oplog entry for cleanup", "error", logErr.Error()) + } + // All checkpoints deleted successfully return checkpointIDs, []string{}, nil } diff --git a/cli/strategy/manual_commit_rewind.go b/cli/strategy/manual_commit_rewind.go index 81026ea..86679c3 100644 --- a/cli/strategy/manual_commit_rewind.go +++ b/cli/strategy/manual_commit_rewind.go @@ -16,6 +16,7 @@ import ( cpkg "github.com/GrayCodeAI/trace/cli/checkpoint" "github.com/GrayCodeAI/trace/cli/checkpoint/id" "github.com/GrayCodeAI/trace/cli/logging" + "github.com/GrayCodeAI/trace/cli/oplog" "github.com/GrayCodeAI/trace/cli/osroot" "github.com/GrayCodeAI/trace/cli/paths" "github.com/GrayCodeAI/trace/cli/settings" @@ -488,12 +489,24 @@ func (s *ManualCommitStrategy) resetShadowBranchToCheckpoint(ctx context.Context shadowBranchName := getShadowBranchNameForCommit(state.BaseCommit, state.WorktreeID) refName := plumbing.NewBranchReferenceName(shadowBranchName) + // Record the ref's current hash before mutating it, so 'trace undo' can + // restore it — this SetReference doesn't touch HEAD, so it never shows + // up in git's own reflog either. + var beforeHash plumbing.Hash + if existing, existErr := repo.Reference(refName, true); existErr == nil { + beforeHash = existing.Hash() + } + // Update the reference to point to the checkpoint commit ref := plumbing.NewHashReference(refName, commit.Hash) if err := repo.Storer.SetReference(ref); err != nil { return fmt.Errorf("failed to update shadow branch: %w", err) } + if logErr := RecordOplogEntry(ctx, repo, oplog.OpRewind, refName.String(), beforeHash, commit.Hash, sessionID); logErr != nil { + logging.Warn(ctx, "failed to record oplog entry for rewind", "error", logErr.Error()) + } + fmt.Fprintf(os.Stderr, "[trace] Reset shadow branch %s to checkpoint %s\n", shadowBranchName, commit.Hash.String()[:7]) return nil } diff --git a/cli/strategy/oplog_helper.go b/cli/strategy/oplog_helper.go new file mode 100644 index 0000000..614e6b5 --- /dev/null +++ b/cli/strategy/oplog_helper.go @@ -0,0 +1,38 @@ +package strategy + +import ( + "context" + + "github.com/GrayCodeAI/trace/cli/checkpoint/id" + "github.com/GrayCodeAI/trace/cli/oplog" + + "github.com/go-git/go-git/v6" + "github.com/go-git/go-git/v6/plumbing" +) + +// RecordOplogEntry appends an operation-log entry for a state-mutating ref +// change (rewind, reset --hard, fork, checkpoint cleanup), so 'trace undo' +// can revert it later. Exported so both this package's internal call sites +// and the cli package (rewind_2.go's performGitResetHard caller, fork_cmd.go) +// can share one implementation. +// +// Callers should treat a non-nil error as worth logging, not as a reason to +// fail the caller's own operation: by the time this is called the ref +// mutation itself has already succeeded, and an audit-log write failure +// shouldn't unwind a successful rewind/reset/fork/cleanup. +func RecordOplogEntry(ctx context.Context, repo *git.Repository, op oplog.Op, ref string, before, after plumbing.Hash, checkpointID string) error { + entryID, err := id.Generate() + if err != nil { + return err + } + authorName, authorEmail := GetGitAuthorFromRepo(repo) + entry := oplog.Entry{ + ID: entryID.String(), + Op: op, + Ref: ref, + BeforeHash: before.String(), + AfterHash: after.String(), + CheckpointID: checkpointID, + } + return oplog.Append(ctx, repo, entry, authorName, authorEmail) +} diff --git a/cli/undo.go b/cli/undo.go new file mode 100644 index 0000000..f915031 --- /dev/null +++ b/cli/undo.go @@ -0,0 +1,95 @@ +package cli + +import ( + "context" + "errors" + "fmt" + "io" + + "github.com/GrayCodeAI/trace/cli/logging" + "github.com/GrayCodeAI/trace/cli/oplog" + "github.com/GrayCodeAI/trace/cli/paths" + "github.com/GrayCodeAI/trace/cli/strategy" + + "github.com/go-git/go-git/v6/plumbing" + "github.com/spf13/cobra" +) + +func newUndoCmd() *cobra.Command { + cmd := &cobra.Command{ + Use: "undo", + Short: "Undo the most recent rewind, reset, fork, or checkpoint cleanup", + Long: `Undo reverts the most recent state-mutating trace operation, using trace's +own operation log — separate from git's reflog, and the only record that +covers a shadow-branch rewind or a checkpoint-cleanup ref rewrite, neither of +which touch HEAD and so never appear in the reflog at all. + +Only the single most recent operation is undone. Run 'trace undo' again to +step back further, or 'trace log' to see the full operation history.`, + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, args []string) error { + ctx := cmd.Context() + if _, err := paths.WorktreeRoot(ctx); err != nil { + cmd.SilenceUsage = true + return errors.New("not a git repository") + } + return runUndo(ctx, cmd.OutOrStdout()) + }, + } + return cmd +} + +func runUndo(ctx context.Context, w io.Writer) error { + repo, err := openRepository(ctx) + if err != nil { + return fmt.Errorf("not a git repository: %w", err) + } + + entries, err := oplog.List(repo, 1) + if err != nil { + return fmt.Errorf("failed to read operation log: %w", err) + } + if len(entries) == 0 { + fmt.Fprintln(w, "Nothing to undo — the operation log is empty.") + return nil + } + entry := entries[0] + + // Deliberately don't auto-chain: undoing an undo silently could produce + // a confusing double-reversal. Require the user to look at 'trace log' + // and act on a specific entry instead. + if entry.Op == oplog.OpUndo { + return fmt.Errorf("the most recent operation is itself an undo (recorded %s) — inspect 'trace log' and act deliberately rather than auto-chaining undos", entry.Timestamp.Format("2006-01-02 15:04:05")) + } + + refName := plumbing.ReferenceName(entry.Ref) + beforeHash := plumbing.NewHash(entry.BeforeHash) + originalAfterHash := plumbing.NewHash(entry.AfterHash) + + var summary string + if beforeHash.IsZero() { + // The operation created this ref from nothing (e.g. fork's new + // branch) — undo removes it rather than trying to point it "back" + // somewhere that never existed. + if err := repo.Storer.RemoveReference(refName); err != nil { + return fmt.Errorf("failed to remove ref %s: %w", refName, err) + } + summary = fmt.Sprintf("Removed %s (created by %s, nothing to restore it to).", refName.Short(), entry.Op) + } else { + if err := repo.Storer.SetReference(plumbing.NewHashReference(refName, beforeHash)); err != nil { + return fmt.Errorf("failed to restore ref %s: %w", refName, err) + } + summary = fmt.Sprintf("Restored %s to %s (undoing a %s).", refName.Short(), beforeHash.String()[:7], entry.Op) + } + + // Record the undo itself, matching jj's operation-log symmetry: undoing + // is itself an operation, so it's visible in 'trace log' and could in + // principle be undone too (deliberately not automated, per the guard + // above). + if logErr := strategy.RecordOplogEntry(ctx, repo, oplog.OpUndo, entry.Ref, originalAfterHash, beforeHash, entry.CheckpointID); logErr != nil { + logging.Warn(ctx, "failed to record oplog entry for undo", "error", logErr.Error()) + } + + fmt.Fprintln(w, summary) + return nil +} diff --git a/cli/undo_test.go b/cli/undo_test.go new file mode 100644 index 0000000..0bfad6a --- /dev/null +++ b/cli/undo_test.go @@ -0,0 +1,123 @@ +package cli + +import ( + "bytes" + "context" + "testing" + + "github.com/GrayCodeAI/trace/cli/oplog" + "github.com/GrayCodeAI/trace/cli/strategy" + "github.com/GrayCodeAI/trace/cli/testutil" + + "github.com/go-git/go-git/v6/plumbing" +) + +func TestRunUndo_RestoresRewrittenRef(t *testing.T) { + dir := t.TempDir() + testutil.InitRepo(t, dir) + t.Chdir(dir) + + testutil.WriteFile(t, dir, "a.txt", "one") + testutil.GitAdd(t, dir, "a.txt") + testutil.GitCommit(t, dir, "first commit") + before := testutil.GetHeadHash(t, dir) + + testutil.WriteFile(t, dir, "a.txt", "two") + testutil.GitAdd(t, dir, "a.txt") + testutil.GitCommit(t, dir, "second commit") + after := testutil.GetHeadHash(t, dir) + + repo, err := openRepository(context.Background()) + if err != nil { + t.Fatalf("openRepository: %v", err) + } + + // Simulate what resetShadowBranchToCheckpoint/performGitResetHard + // record: a rewind moved "refs/heads/master" (or main) from `before` to + // `after` — actually mutate the branch ref directly here, mirroring the + // production ref-rewrite pattern, then record it in the oplog exactly + // as the real mutation points do. + headRef, err := repo.Head() + if err != nil { + t.Fatalf("repo.Head: %v", err) + } + branchRefName := headRef.Name() + + if err := strategy.RecordOplogEntry( + context.Background(), repo, oplog.OpRewind, branchRefName.String(), + plumbing.NewHash(before), plumbing.NewHash(after), "", + ); err != nil { + t.Fatalf("RecordOplogEntry: %v", err) + } + + // Now move the branch ref forward again, as if the rewind itself had + // already applied (before -> after was the rewind; here we just verify + // undo restores the ref to `before`). + if err := repo.Storer.SetReference(plumbing.NewHashReference(branchRefName, plumbing.NewHash(after))); err != nil { + t.Fatalf("SetReference: %v", err) + } + + var out bytes.Buffer + if err := runUndo(context.Background(), &out); err != nil { + t.Fatalf("runUndo: %v", err) + } + + ref, err := repo.Reference(branchRefName, true) + if err != nil { + t.Fatalf("repo.Reference: %v", err) + } + if ref.Hash().String() != before { + t.Errorf("branch ref = %s, want %s (restored to before-hash)", ref.Hash().String(), before) + } + + if out.String() == "" { + t.Error("runUndo produced no output summary") + } +} + +func TestRunUndo_EmptyOplog(t *testing.T) { + dir := t.TempDir() + testutil.InitRepo(t, dir) + t.Chdir(dir) + + testutil.WriteFile(t, dir, "a.txt", "one") + testutil.GitAdd(t, dir, "a.txt") + testutil.GitCommit(t, dir, "first commit") + + var out bytes.Buffer + if err := runUndo(context.Background(), &out); err != nil { + t.Fatalf("runUndo: %v", err) + } + if out.String() != "Nothing to undo — the operation log is empty.\n" { + t.Errorf("runUndo output = %q, want the empty-oplog message", out.String()) + } +} + +func TestRunUndo_RefusesToChainUndos(t *testing.T) { + dir := t.TempDir() + testutil.InitRepo(t, dir) + t.Chdir(dir) + + testutil.WriteFile(t, dir, "a.txt", "one") + testutil.GitAdd(t, dir, "a.txt") + testutil.GitCommit(t, dir, "first commit") + + repo, err := openRepository(context.Background()) + if err != nil { + t.Fatalf("openRepository: %v", err) + } + authorName, authorEmail := strategy.GetGitAuthorFromRepo(repo) + entry := oplog.Entry{ + ID: "aaaaaaaaaa", + Op: oplog.OpUndo, + Ref: "refs/heads/does-not-matter", + } + if err := oplog.Append(context.Background(), repo, entry, authorName, authorEmail); err != nil { + t.Fatalf("oplog.Append: %v", err) + } + + var out bytes.Buffer + if err := runUndo(context.Background(), &out); err == nil { + t.Fatal("expected runUndo to refuse chaining an undo, got nil error") + } +} From d2ee51f453c3f6da70827184346d67dd531045ed Mon Sep 17 00:00:00 2001 From: Lakshman Patel Date: Wed, 15 Jul 2026 11:49:08 +0530 Subject: [PATCH 2/4] Fix trace undo to restore the working tree after a reset-hard undo runUndo restored only the git ref via SetReference for every operation type. That's correct for rewind/cleanup (shadow-branch-only rewrites that never touch HEAD), but wrong for reset-hard: git reset --hard moves the ref, index, and working tree together, so undoing it via SetReference alone left files on disk in the post-reset state while the branch pointer claimed otherwise. Now reuses performGitResetHard for OpResetHard entries, the same code path the original operation used. --- cli/undo.go | 15 ++++++++++-- cli/undo_test.go | 64 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 2 deletions(-) diff --git a/cli/undo.go b/cli/undo.go index f915031..6fd3543 100644 --- a/cli/undo.go +++ b/cli/undo.go @@ -67,7 +67,18 @@ func runUndo(ctx context.Context, w io.Writer) error { originalAfterHash := plumbing.NewHash(entry.AfterHash) var summary string - if beforeHash.IsZero() { + switch { + case entry.Op == oplog.OpResetHard: + // git reset --hard moves the ref, the index, AND the working tree + // together — restoring only the ref via SetReference would leave + // files on disk in the post-reset state while the branch pointer + // claims otherwise, a real desync. Reuse the exact same code path + // the original operation used so the semantics match precisely. + if err := performGitResetHard(ctx, entry.BeforeHash); err != nil { + return fmt.Errorf("failed to reset %s back to %s: %w", refName, beforeHash.String()[:7], err) + } + summary = fmt.Sprintf("Reset %s back to %s (undoing a reset --hard) — working tree and index restored.", refName.Short(), beforeHash.String()[:7]) + case beforeHash.IsZero(): // The operation created this ref from nothing (e.g. fork's new // branch) — undo removes it rather than trying to point it "back" // somewhere that never existed. @@ -75,7 +86,7 @@ func runUndo(ctx context.Context, w io.Writer) error { return fmt.Errorf("failed to remove ref %s: %w", refName, err) } summary = fmt.Sprintf("Removed %s (created by %s, nothing to restore it to).", refName.Short(), entry.Op) - } else { + default: if err := repo.Storer.SetReference(plumbing.NewHashReference(refName, beforeHash)); err != nil { return fmt.Errorf("failed to restore ref %s: %w", refName, err) } diff --git a/cli/undo_test.go b/cli/undo_test.go index 0bfad6a..28cc07c 100644 --- a/cli/undo_test.go +++ b/cli/undo_test.go @@ -75,6 +75,70 @@ func TestRunUndo_RestoresRewrittenRef(t *testing.T) { } } +func TestRunUndo_RestoresWorkingTreeAfterResetHard(t *testing.T) { + dir := t.TempDir() + testutil.InitRepo(t, dir) + t.Chdir(dir) + + testutil.WriteFile(t, dir, "a.txt", "one") + testutil.GitAdd(t, dir, "a.txt") + testutil.GitCommit(t, dir, "commit A") + commitA := testutil.GetHeadHash(t, dir) + + testutil.WriteFile(t, dir, "a.txt", "two") + testutil.GitAdd(t, dir, "a.txt") + testutil.GitCommit(t, dir, "commit B") + commitB := testutil.GetHeadHash(t, dir) + + repo, err := openRepository(context.Background()) + if err != nil { + t.Fatalf("openRepository: %v", err) + } + headRef, err := repo.Head() + if err != nil { + t.Fatalf("repo.Head: %v", err) + } + branchRefName := headRef.Name() + + // Simulate the reset --hard that a real 'trace rewind --reset' would + // have already performed: moves ref + index + working tree from B back + // to A, exactly like performGitResetHard (which this test also uses). + if err := performGitResetHard(context.Background(), commitA); err != nil { + t.Fatalf("performGitResetHard: %v", err) + } + if got := testutil.ReadFile(t, dir, "a.txt"); got != "one" { + t.Fatalf("setup: a.txt = %q after simulated reset, want %q", got, "one") + } + + // Record the operation as rewind.go/rewind_2.go actually do: before is + // the state prior to the reset (B), after is the state the reset + // produced (A). + if err := strategy.RecordOplogEntry( + context.Background(), repo, oplog.OpResetHard, branchRefName.String(), + plumbing.NewHash(commitB), plumbing.NewHash(commitA), "", + ); err != nil { + t.Fatalf("RecordOplogEntry: %v", err) + } + + var out bytes.Buffer + if err := runUndo(context.Background(), &out); err != nil { + t.Fatalf("runUndo: %v", err) + } + + // Both the ref AND the working tree must be back at B — this is the + // bug this test guards against: undo previously only restored the ref. + ref, err := repo.Reference(branchRefName, true) + if err != nil { + t.Fatalf("repo.Reference: %v", err) + } + if ref.Hash().String() != commitB { + t.Errorf("branch ref = %s, want %s (commit B)", ref.Hash().String(), commitB) + } + if got := testutil.ReadFile(t, dir, "a.txt"); got != "two" { + t.Errorf("a.txt = %q after undo, want %q (working tree must be restored, not just the ref)", got, "two") + } +} + func TestRunUndo_EmptyOplog(t *testing.T) { dir := t.TempDir() testutil.InitRepo(t, dir) From 164d86be64a8386938ac2f63a87ec0b47fd8acf0 Mon Sep 17 00:00:00 2001 From: Lakshman Patel Date: Wed, 15 Jul 2026 17:34:56 +0530 Subject: [PATCH 3/4] fix: serialize V2GitStore storer access under StorerMu go-git's filesystem storer is not safe for concurrent read+write, even across separate Repository instances sharing the same .git directory. The v1 GitStore path already wraps every operation in StorerMu; the v2 path touched the storer directly and never acquired it. Lock every public V2GitStore method that reads or writes the storer. Because several public methods call other public methods under their own lock, extract private *Locked helpers and route internal callers through them to avoid deadlock (WriteCommitted->WriteCommittedWithSessionIndex, rotateGeneration->NextGenerationNumber->ListArchivedGenerations, ReadGenerationFromRef->ReadGeneration, ReadSessionContentByID/GetSessionLog-> ReadCommitted, and the private rotateCurrentIfNeeded->rotateGeneration/ CountCheckpointsInTree paths). The pending-rotation path is intentionally left unlocked: it uses a separate .lock file (withPendingFullGenerationPublicationLock) and never touches the git storer. --- cli/checkpoint/v2_committed.go | 37 +++++++++++++++++++++--- cli/checkpoint/v2_generation.go | 50 ++++++++++++++++++++++++++++++--- cli/checkpoint/v2_read.go | 46 ++++++++++++++++++++++++++---- 3 files changed, 119 insertions(+), 14 deletions(-) diff --git a/cli/checkpoint/v2_committed.go b/cli/checkpoint/v2_committed.go index b1bb9b3..e0f51ba 100644 --- a/cli/checkpoint/v2_committed.go +++ b/cli/checkpoint/v2_committed.go @@ -33,7 +33,11 @@ import ( // determined from the /main ref and passed to the /full/current write to // keep both refs consistent. func (s *V2GitStore) WriteCommitted(ctx context.Context, opts WriteCommittedOptions) error { - _, err := s.WriteCommittedWithSessionIndex(ctx, opts) + StorerMu.Lock() + defer StorerMu.Unlock() + // writeCommittedWithSessionIndexLocked (not the public wrapper) because we + // already hold StorerMu — re-entering the non-reentrant mutex would deadlock. + _, err := s.writeCommittedWithSessionIndexLocked(ctx, opts) return err } @@ -41,6 +45,14 @@ func (s *V2GitStore) WriteCommitted(ctx context.Context, opts WriteCommittedOpti // v2 session index used for the write. The index may point at an existing // session when the checkpoint already contains the same session ID. func (s *V2GitStore) WriteCommittedWithSessionIndex(ctx context.Context, opts WriteCommittedOptions) (int, error) { + StorerMu.Lock() + defer StorerMu.Unlock() + return s.writeCommittedWithSessionIndexLocked(ctx, opts) +} + +// writeCommittedWithSessionIndexLocked is the unlocked implementation shared by +// WriteCommitted and WriteCommittedWithSessionIndex. Callers MUST hold StorerMu. +func (s *V2GitStore) writeCommittedWithSessionIndexLocked(ctx context.Context, opts WriteCommittedOptions) (int, error) { // Validate upfront before any writes to avoid partial ref updates if err := validateWriteOpts(opts); err != nil { return 0, err @@ -68,6 +80,9 @@ func (s *V2GitStore) WriteCommittedWithSessionIndex(ctx context.Context, opts Wr // // Returns ErrCheckpointNotFound if the checkpoint doesn't exist on /main. func (s *V2GitStore) UpdateCommitted(ctx context.Context, opts UpdateCommittedOptions) error { + StorerMu.Lock() + defer StorerMu.Unlock() + if opts.CheckpointID.IsEmpty() { return errors.New("invalid update options: checkpoint ID is required") } @@ -98,6 +113,8 @@ type fullSessionArtifacts struct { // HasFullSessionArtifacts reports whether the raw transcript and content hash // for a checkpoint session exist in any local v2 /full/* ref. func (s *V2GitStore) HasFullSessionArtifacts(checkpointID id.CheckpointID, sessionIndex int) (bool, error) { + StorerMu.Lock() + defer StorerMu.Unlock() artifacts, err := s.findFullSessionArtifacts(checkpointID, sessionIndex) if err != nil { return false, err @@ -138,7 +155,9 @@ func (s *V2GitStore) findFullSessionArtifacts(checkpointID id.CheckpointID, sess func (s *V2GitStore) fullRefSearchOrder() ([]plumbing.ReferenceName, error) { refNames := []plumbing.ReferenceName{plumbing.ReferenceName(paths.V2FullCurrentRefName)} - archived, err := s.ListArchivedGenerations() + // listArchivedGenerationsLocked (not the public wrapper) — this private + // helper is only reached from other public methods that already hold StorerMu. + archived, err := s.listArchivedGenerationsLocked() if err != nil { return nil, err } @@ -705,7 +724,9 @@ func (s *V2GitStore) writeCommittedFullTranscript(ctx context.Context, opts Writ } func (s *V2GitStore) rotateCurrentIfNeeded(ctx context.Context, treeHash plumbing.Hash) { - checkpointCount, countErr := s.CountCheckpointsInTree(treeHash) + // countCheckpointsInTreeLocked (not the public wrapper) — we already hold + // StorerMu via the writing public method that invoked writeCommittedFullTranscript. + checkpointCount, countErr := s.countCheckpointsInTreeLocked(treeHash) if countErr != nil { logging.Warn( ctx, "failed to count checkpoints for rotation check", @@ -716,7 +737,9 @@ func (s *V2GitStore) rotateCurrentIfNeeded(ctx context.Context, treeHash plumbin if checkpointCount < s.maxCheckpoints() { return } - if rotErr := s.rotateGeneration(ctx); rotErr != nil { + // rotateGenerationLocked (not the public wrapper) — we already hold StorerMu + // via the writing public method that invoked writeCommittedFullTranscript. + if rotErr := s.rotateGenerationLocked(ctx); rotErr != nil { logging.Warn( ctx, "generation rotation failed", slog.String("error", rotErr.Error()), @@ -802,6 +825,9 @@ func validateWriteOpts(opts WriteCommittedOptions) error { // UpdateSummary persists an AI-generated summary into the latest session's // metadata on the v2 /main ref. Mirrors GitStore.UpdateSummary for v1. func (s *V2GitStore) UpdateSummary(ctx context.Context, checkpointID id.CheckpointID, summary *Summary) error { + StorerMu.Lock() + defer StorerMu.Unlock() + if err := ctx.Err(); err != nil { return err //nolint:wrapcheck // Propagating context cancellation } @@ -875,6 +901,9 @@ func (s *V2GitStore) UpdateSummary(ctx context.Context, checkpointID id.Checkpoi // Older CLI versions wrote these before the rename to raw_transcript. // Returns nil if /full/current doesn't exist or no v1 files were found. func (s *V2GitStore) CleanupV1TranscriptFiles(ctx context.Context, checkpointID id.CheckpointID, sessionCount int) error { + StorerMu.Lock() + defer StorerMu.Unlock() + refName := plumbing.ReferenceName(paths.V2FullCurrentRefName) parentHash, rootTreeHash, err := s.GetRefState(refName) if err != nil { diff --git a/cli/checkpoint/v2_generation.go b/cli/checkpoint/v2_generation.go index bf71a8a..56c98ce 100644 --- a/cli/checkpoint/v2_generation.go +++ b/cli/checkpoint/v2_generation.go @@ -47,6 +47,14 @@ type GenerationMetadata struct { // ReadGeneration reads generation.json from the given tree hash. // Returns a zero-value GenerationMetadata if the file doesn't exist (new/empty generation). func (s *V2GitStore) ReadGeneration(treeHash plumbing.Hash) (GenerationMetadata, error) { + StorerMu.Lock() + defer StorerMu.Unlock() + return s.readGenerationLocked(treeHash) +} + +// readGenerationLocked is the unlocked implementation of ReadGeneration. +// Callers MUST hold StorerMu. +func (s *V2GitStore) readGenerationLocked(treeHash plumbing.Hash) (GenerationMetadata, error) { if treeHash == plumbing.ZeroHash { return GenerationMetadata{}, nil } @@ -79,11 +87,13 @@ func (s *V2GitStore) ReadGeneration(treeHash plumbing.Hash) (GenerationMetadata, // ReadGenerationFromRef reads generation.json from the tree pointed to by the given ref. func (s *V2GitStore) ReadGenerationFromRef(refName plumbing.ReferenceName) (GenerationMetadata, error) { + StorerMu.Lock() + defer StorerMu.Unlock() _, treeHash, err := s.GetRefState(refName) if err != nil { return GenerationMetadata{}, fmt.Errorf("failed to get ref state: %w", err) } - return s.ReadGeneration(treeHash) + return s.readGenerationLocked(treeHash) } // marshalGenerationBlob marshals gen as generation.json and stores it as a git blob. @@ -120,6 +130,14 @@ func (s *V2GitStore) writeGeneration(gen GenerationMetadata, entries map[string] // The tree structure is // — we count second-level directories // across all shard prefixes. Returns 0 for an empty tree. func (s *V2GitStore) CountCheckpointsInTree(treeHash plumbing.Hash) (int, error) { + StorerMu.Lock() + defer StorerMu.Unlock() + return s.countCheckpointsInTreeLocked(treeHash) +} + +// countCheckpointsInTreeLocked is the unlocked implementation of +// CountCheckpointsInTree. Callers MUST hold StorerMu. +func (s *V2GitStore) countCheckpointsInTreeLocked(treeHash plumbing.Hash) (int, error) { if treeHash == plumbing.ZeroHash { return 0, nil } @@ -396,6 +414,14 @@ var GenerationRefPattern = regexp.MustCompile(`^\d{13}$`) // listArchivedGenerations returns the names of all archived generation refs // (everything under V2FullRefPrefix matching the expected numeric format), sorted ascending. func (s *V2GitStore) ListArchivedGenerations() ([]string, error) { + StorerMu.Lock() + defer StorerMu.Unlock() + return s.listArchivedGenerationsLocked() +} + +// listArchivedGenerationsLocked is the unlocked implementation of +// ListArchivedGenerations. Callers MUST hold StorerMu. +func (s *V2GitStore) listArchivedGenerationsLocked() ([]string, error) { refs, err := s.repo.References() if err != nil { return nil, fmt.Errorf("failed to list references: %w", err) @@ -425,7 +451,15 @@ func (s *V2GitStore) ListArchivedGenerations() ([]string, error) { // NextGenerationNumber returns the next sequential generation number for archiving. // Scans existing archived refs and returns max+1. Returns 1 if no archives exist. func (s *V2GitStore) NextGenerationNumber() (int, error) { - archived, err := s.ListArchivedGenerations() + StorerMu.Lock() + defer StorerMu.Unlock() + return s.nextGenerationNumberLocked() +} + +// nextGenerationNumberLocked is the unlocked implementation of +// NextGenerationNumber. Callers MUST hold StorerMu. +func (s *V2GitStore) nextGenerationNumberLocked() (int, error) { + archived, err := s.listArchivedGenerationsLocked() if err != nil { return 0, err } @@ -451,6 +485,14 @@ func (s *V2GitStore) NextGenerationNumber() (int, error) { // 2. Reset: create a fresh orphan commit with an empty tree + seed generation.json, // point /full/current at it. func (s *V2GitStore) rotateGeneration(ctx context.Context) error { + StorerMu.Lock() + defer StorerMu.Unlock() + return s.rotateGenerationLocked(ctx) +} + +// rotateGenerationLocked is the unlocked implementation of rotateGeneration. +// Callers MUST hold StorerMu. +func (s *V2GitStore) rotateGenerationLocked(ctx context.Context) error { refName := plumbing.ReferenceName(paths.V2FullCurrentRefName) // Guard against concurrent rotation: re-read /full/current and check if @@ -459,7 +501,7 @@ func (s *V2GitStore) rotateGeneration(ctx context.Context) error { if err != nil { return fmt.Errorf("rotation: failed to read /full/current: %w", err) } - checkpointCount, err := s.CountCheckpointsInTree(currentTreeHash) + checkpointCount, err := s.countCheckpointsInTreeLocked(currentTreeHash) if err != nil { return fmt.Errorf("rotation: failed to count checkpoints: %w", err) } @@ -472,7 +514,7 @@ func (s *V2GitStore) rotateGeneration(ctx context.Context) error { return fmt.Errorf("rotation: failed to read /full/current ref: %w", err) } - archiveNumber, err := s.NextGenerationNumber() + archiveNumber, err := s.nextGenerationNumberLocked() if err != nil { return fmt.Errorf("rotation: failed to determine next generation number: %w", err) } diff --git a/cli/checkpoint/v2_read.go b/cli/checkpoint/v2_read.go index 6ef72e5..6a2cc11 100644 --- a/cli/checkpoint/v2_read.go +++ b/cli/checkpoint/v2_read.go @@ -24,6 +24,14 @@ import ( // ReadCommitted reads the checkpoint summary from the v2 /main ref. // Returns nil, nil if the checkpoint doesn't exist (same contract as GitStore.ReadCommitted). func (s *V2GitStore) ReadCommitted(ctx context.Context, checkpointID id.CheckpointID) (*CheckpointSummary, error) { + StorerMu.Lock() + defer StorerMu.Unlock() + return s.readCommittedLocked(ctx, checkpointID) +} + +// readCommittedLocked is the unlocked implementation of ReadCommitted. +// Callers MUST hold StorerMu. +func (s *V2GitStore) readCommittedLocked(ctx context.Context, checkpointID id.CheckpointID) (*CheckpointSummary, error) { if err := ctx.Err(); err != nil { return nil, err //nolint:wrapcheck // Propagating context cancellation } @@ -66,6 +74,9 @@ func (s *V2GitStore) ReadCommitted(ctx context.Context, checkpointID id.Checkpoi // ListCommitted lists all committed checkpoints from the v2 /main ref. // Scans sharded paths: // directories containing metadata.json. func (s *V2GitStore) ListCommitted(ctx context.Context) ([]CommittedInfo, error) { + StorerMu.Lock() + defer StorerMu.Unlock() + if err := ctx.Err(); err != nil { return nil, err //nolint:wrapcheck // Propagating context cancellation } @@ -140,6 +151,9 @@ func (s *V2GitStore) ListCommitted(ctx context.Context) ([]CommittedInfo, error) // ReadSessionCompactTranscript reads transcript.jsonl for a session from the v2 // /main ref. Returns ErrNoTranscript when compact transcript is missing. func (s *V2GitStore) ReadSessionCompactTranscript(ctx context.Context, checkpointID id.CheckpointID, sessionIndex int) ([]byte, error) { + StorerMu.Lock() + defer StorerMu.Unlock() + if err := ctx.Err(); err != nil { return nil, err //nolint:wrapcheck // Propagating context cancellation } @@ -186,6 +200,9 @@ func (s *V2GitStore) ReadSessionCompactTranscript(ctx context.Context, checkpoin // ReadSessionMetadata reads only the metadata.json for a specific session within a v2 checkpoint. // Returns ErrCheckpointNotFound if the checkpoint or session doesn't exist on /main. func (s *V2GitStore) ReadSessionMetadata(ctx context.Context, checkpointID id.CheckpointID, sessionIndex int) (*CommittedMetadata, error) { + StorerMu.Lock() + defer StorerMu.Unlock() + if err := ctx.Err(); err != nil { return nil, err //nolint:wrapcheck // Propagating context cancellation } @@ -234,6 +251,9 @@ func (s *V2GitStore) ReadSessionMetadata(ctx context.Context, checkpointID id.Ch // (transcript.jsonl) on /main can substitute for display. // Returns ErrCheckpointNotFound if the checkpoint or session doesn't exist on /main. func (s *V2GitStore) ReadSessionMetadataAndPrompts(ctx context.Context, checkpointID id.CheckpointID, sessionIndex int) (*SessionContent, error) { + StorerMu.Lock() + defer StorerMu.Unlock() + if err := ctx.Err(); err != nil { return nil, err //nolint:wrapcheck // Propagating context cancellation } @@ -294,6 +314,14 @@ func (s *V2GitStore) ReadSessionMetadataAndPrompts(ctx context.Context, checkpoi // Returns ErrNoTranscript if the session exists but no raw transcript is available. // Returns ErrCheckpointNotFound if the checkpoint or session doesn't exist on /main. func (s *V2GitStore) ReadSessionContent(ctx context.Context, checkpointID id.CheckpointID, sessionIndex int) (*SessionContent, error) { + StorerMu.Lock() + defer StorerMu.Unlock() + return s.readSessionContentLocked(ctx, checkpointID, sessionIndex) +} + +// readSessionContentLocked is the unlocked implementation of ReadSessionContent. +// Callers MUST hold StorerMu. +func (s *V2GitStore) readSessionContentLocked(ctx context.Context, checkpointID id.CheckpointID, sessionIndex int) (*SessionContent, error) { if err := ctx.Err(); err != nil { return nil, err //nolint:wrapcheck // Propagating context cancellation } @@ -365,7 +393,7 @@ func (s *V2GitStore) readTranscriptFromFullRefs(ctx context.Context, checkpointI return transcript, nil } - archived, err := s.ListArchivedGenerations() + archived, err := s.listArchivedGenerationsLocked() if err != nil { return nil, err } @@ -387,7 +415,7 @@ func (s *V2GitStore) readTranscriptFromFullRefs(ctx context.Context, checkpointI } // Search newly fetched refs only - newArchived, err := s.ListArchivedGenerations() + newArchived, err := s.listArchivedGenerationsLocked() if err != nil { return nil, nil //nolint:nilerr // Best-effort: fetch-on-demand failure shouldn't block resume } @@ -551,7 +579,10 @@ func readTranscriptFromObjectTree(tree *object.Tree, agentType types.AgentType) // non-wrapped error (containing the session ID and checkpoint ID for context) // if no session in the checkpoint matches sessionID. func (s *V2GitStore) ReadSessionContentByID(ctx context.Context, checkpointID id.CheckpointID, sessionID string) (*SessionContent, error) { - summary, err := s.ReadCommitted(ctx, checkpointID) + StorerMu.Lock() + defer StorerMu.Unlock() + + summary, err := s.readCommittedLocked(ctx, checkpointID) if err != nil { return nil, err } @@ -560,7 +591,7 @@ func (s *V2GitStore) ReadSessionContentByID(ctx context.Context, checkpointID id } for i := range summary.Sessions { - content, readErr := s.ReadSessionContent(ctx, checkpointID, i) + content, readErr := s.readSessionContentLocked(ctx, checkpointID, i) if readErr != nil { continue } @@ -575,7 +606,10 @@ func (s *V2GitStore) ReadSessionContentByID(ctx context.Context, checkpointID id // GetSessionLog reads the latest session's raw transcript and session ID from v2 refs. // Convenience wrapper matching the GitStore.GetSessionLog signature. func (s *V2GitStore) GetSessionLog(ctx context.Context, cpID id.CheckpointID) ([]byte, string, error) { - summary, err := s.ReadCommitted(ctx, cpID) + StorerMu.Lock() + defer StorerMu.Unlock() + + summary, err := s.readCommittedLocked(ctx, cpID) if err != nil { return nil, "", err } @@ -587,7 +621,7 @@ func (s *V2GitStore) GetSessionLog(ctx context.Context, cpID id.CheckpointID) ([ } latestIndex := len(summary.Sessions) - 1 - content, err := s.ReadSessionContent(ctx, cpID, latestIndex) + content, err := s.readSessionContentLocked(ctx, cpID, latestIndex) if err != nil { return nil, "", err } From 8e6502b432731f1dcae225038c5eda43f042b6d4 Mon Sep 17 00:00:00 2001 From: Lakshman Patel Date: Wed, 15 Jul 2026 22:59:20 +0530 Subject: [PATCH 4/4] fix(trace): guard fork/undo storer writes under StokerMu --- cli/fork_cmd.go | 11 +++++++++-- cli/undo.go | 22 +++++++++++++++++++--- 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/cli/fork_cmd.go b/cli/fork_cmd.go index 587009c..2e00868 100644 --- a/cli/fork_cmd.go +++ b/cli/fork_cmd.go @@ -216,7 +216,12 @@ func forkSession( if !commitHash.IsZero() { refName := plumbing.NewBranchReferenceName(branchName) ref := plumbing.NewHashReference(refName, commitHash) + // Serialize against concurrent V2GitStore storer access (and any + // other StorerMu-guarded writer) — go-git's storer is not + // concurrency-safe. fork/undo already cooperate via the same mutex. + checkpoint.StorerMu.Lock() if err := repo.Storer.SetReference(ref); err != nil { + checkpoint.StorerMu.Unlock() return forkResult{}, fmt.Errorf("failed to create fork branch %s: %w", branchName, err) } result.ForkBranch = branchName @@ -224,9 +229,11 @@ func forkSession( // New branch, so before-hash is the zero hash — 'trace undo' deletes // the branch rather than trying to point it "back" anywhere. - if logErr := strategy.RecordOplogEntry( + logErr := strategy.RecordOplogEntry( ctx, repo, oplog.OpFork, refName.String(), plumbing.ZeroHash, commitHash, cpID.String(), - ); logErr != nil { + ) + checkpoint.StorerMu.Unlock() + if logErr != nil { logging.Warn(ctx, "failed to record oplog entry for fork", "error", logErr.Error()) } } diff --git a/cli/undo.go b/cli/undo.go index 6fd3543..a651005 100644 --- a/cli/undo.go +++ b/cli/undo.go @@ -6,6 +6,7 @@ import ( "fmt" "io" + "github.com/GrayCodeAI/trace/cli/checkpoint" "github.com/GrayCodeAI/trace/cli/logging" "github.com/GrayCodeAI/trace/cli/oplog" "github.com/GrayCodeAI/trace/cli/paths" @@ -74,6 +75,9 @@ func runUndo(ctx context.Context, w io.Writer) error { // files on disk in the post-reset state while the branch pointer // claims otherwise, a real desync. Reuse the exact same code path // the original operation used so the semantics match precisely. + // (performGitResetHard shells out to the `git` CLI, which serializes + // against other git processes on the same repo via its own locking, + // so it does not need StorerMu here.) if err := performGitResetHard(ctx, entry.BeforeHash); err != nil { return fmt.Errorf("failed to reset %s back to %s: %w", refName, beforeHash.String()[:7], err) } @@ -81,23 +85,35 @@ func runUndo(ctx context.Context, w io.Writer) error { case beforeHash.IsZero(): // The operation created this ref from nothing (e.g. fork's new // branch) — undo removes it rather than trying to point it "back" - // somewhere that never existed. + // somewhere that never existed. Direct repo.Storer access, so + // serialize it against concurrent V2GitStore writes under + // StorerMu (go-git's storer is not concurrency-safe). + checkpoint.StorerMu.Lock() if err := repo.Storer.RemoveReference(refName); err != nil { + checkpoint.StorerMu.Unlock() return fmt.Errorf("failed to remove ref %s: %w", refName, err) } + checkpoint.StorerMu.Unlock() summary = fmt.Sprintf("Removed %s (created by %s, nothing to restore it to).", refName.Short(), entry.Op) default: + checkpoint.StorerMu.Lock() if err := repo.Storer.SetReference(plumbing.NewHashReference(refName, beforeHash)); err != nil { + checkpoint.StorerMu.Unlock() return fmt.Errorf("failed to restore ref %s: %w", refName, err) } + checkpoint.StorerMu.Unlock() summary = fmt.Sprintf("Restored %s to %s (undoing a %s).", refName.Short(), beforeHash.String()[:7], entry.Op) } // Record the undo itself, matching jj's operation-log symmetry: undoing // is itself an operation, so it's visible in 'trace log' and could in // principle be undone too (deliberately not automated, per the guard - // above). - if logErr := strategy.RecordOplogEntry(ctx, repo, oplog.OpUndo, entry.Ref, originalAfterHash, beforeHash, entry.CheckpointID); logErr != nil { + // above). Held under StorerMu so this oplog ref write lands together with + // the ref mutation above from the perspective of other storer writers. + checkpoint.StorerMu.Lock() + logErr := strategy.RecordOplogEntry(ctx, repo, oplog.OpUndo, entry.Ref, originalAfterHash, beforeHash, entry.CheckpointID) + checkpoint.StorerMu.Unlock() + if logErr != nil { logging.Warn(ctx, "failed to record oplog entry for undo", "error", logErr.Error()) }