diff --git a/.gitignore b/.gitignore index 0ec1a656d8..5fe882efb5 100644 --- a/.gitignore +++ b/.gitignore @@ -49,8 +49,10 @@ mise.local.toml /completions/ -# E2E test artifacts -e2e/artifacts/ +# E2E test artifacts are ignored by e2e/.gitignore, not from here. go-git's +# gitignore walk only prunes a directory when the matching pattern comes from +# that directory's own .gitignore, so a nested "e2e/artifacts/" rule here would +# leave go-git descending every artifact directory on each Worktree.Status(). # worktrees .worktrees/ diff --git a/.golangci.yaml b/.golangci.yaml index 4904850c94..c761c684e1 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -113,6 +113,9 @@ linters: - pattern: '^.*\.Checkout$' msg: "go-git Checkout deletes .gitignored dirs - use CheckoutBranch() from git_operations.go" pkg: 'github\.com/go-git/go-git' + - pattern: '^.*Worktree\.Status$' + msg: "go-git Worktree.Status() walks ignored subtrees (seconds in this repo) - use gitrepo.Status(ctx, repo)" + pkg: 'github\.com/go-git/go-git' - pattern: '^.*\.(Print|Println|Printf)$' msg: "cobra's Print* writes to OutOrStderr (stderr in production); use fmt.Fprint*(cmd.OutOrStdout(), ...)" pkg: 'github\.com/spf13/cobra' diff --git a/CLAUDE.md b/CLAUDE.md index 81fa96c652..c72f5e41a1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -492,6 +492,50 @@ 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`). +#### Reading Worktree Status - Always Use `gitrepo.Status` + +**Never call go-git's `worktree.Status()` directly.** Use +`gitrepo.Status(ctx, repo)`; a `forbidigo` rule in `.golangci.yaml` enforces +this, and `gitrepo/status.go` is the only sanctioned call site. + +`Worktree.Status()` walks the whole worktree twice — once in +`gitignore.ReadPatterns` collecting patterns, once diffing. `ReadPatterns` does +**not** thread a parent directory's patterns into its recursive walk: each +recursive call rebuilds its pattern set from that directory's own ignore files, +so the prune check only ever matches patterns declared by the directory being +scanned. + +**Consequence for `.gitignore` layout:** a rule prunes a subtree only when its +target is a *direct child* of the `.gitignore` declaring it. A root-level +`e2e/artifacts/` rule is one level too deep and never prunes, so every +`Status()` descended ~15k artifact directories and cost 5.25s (against 0.013s +for `git status --porcelain`), which timed out agent hooks. The rule therefore +lives in `e2e/.gitignore` as `/artifacts/`. When adding a new ignored directory, +declare it in a `.gitignore` in its parent directory rather than as a nested +path from the root — reviewers should flag multi-component directory patterns +added to the root `.gitignore`. + +Anchor the relocated pattern with a leading slash. Moving `a/b/` to a +`.gitignore` in `a/` as bare `b/` also drops git's root anchoring, so it would +newly match `b` at any depth below `a/`; `/b/` preserves the original scope and +prunes identically. + +`gitrepo.WithStatusCache(ctx)` memoizes the walk for callers that read status +more than once. Install it **only** across a window that neither writes tracked +files nor stages anything: the TurnStart hook qualifies (it runs before the agent +acts and writes only session metadata under `.entire/` and refs under `.git/`), +post-agent hooks such as TurnEnd do not — `DetectFileChanges` there must observe +the agent's edits. + +Staging counts as invalidation even though `.git/index` sits inside `.git/`: the +index feeds the status diff, so an index write makes a cached result stale. Entire +performs no index writes today — there are no `SetIndex` calls, the single +`Storer.Index()` use (`strategy/content_overlap.go`) is a read, and the git +subcommands on the hook paths are all index-read-only. **If you add an +index-mutating operation, check whether it lands inside a status-cache window.** +The cache is context-scoped to one short-lived hook process, so it cannot go +stale across turns. + #### 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/gitrepo/status.go b/cmd/entire/cli/gitrepo/status.go new file mode 100644 index 0000000000..fcab26ef98 --- /dev/null +++ b/cmd/entire/cli/gitrepo/status.go @@ -0,0 +1,77 @@ +package gitrepo + +import ( + "context" + "sync" + + "github.com/go-git/go-git/v6" +) + +// Status is the single entry point for reading go-git worktree status; the +// forbidigo rule in .golangci.yaml keeps callers off worktree.Status directly. +// +// go-git's Worktree.Status() is expensive: it walks the whole worktree twice +// (once collecting .gitignore patterns, once diffing), and gitignore.ReadPatterns +// does not thread a parent directory's patterns into its recursive walk, so it +// only prunes an ignored directory when the matching pattern was declared by +// that directory's own parent .gitignore. A rule one level too deep leaves the +// whole subtree walked: a single call cost 5.25s in this repo before e2e's +// artifacts rule moved into e2e/.gitignore. + +type statusCacheKey struct{} + +type statusCache struct { + mu sync.Mutex + statuses map[string]git.Status +} + +// WithStatusCache returns a context that memoizes Status results. +// +// Install it only across a window that neither writes tracked files nor stages +// anything — otherwise later callers observe a stale status. Staging counts: +// .git/index lives inside .git/, but the index feeds the status diff, so an +// index write invalidates a cached result just as a worktree write does. Entire +// performs no index writes today (no SetIndex calls; the git subcommands on the +// hook paths are all index-read-only), which is what makes turn start a valid +// window. A hook that runs after the agent has edited files is not. +func WithStatusCache(ctx context.Context) context.Context { + return context.WithValue(ctx, statusCacheKey{}, &statusCache{ + statuses: make(map[string]git.Status), + }) +} + +// Status returns the worktree status for repo, reusing a cached result when ctx +// carries a cache from WithStatusCache and the same worktree was already read. +// +// The returned map is shared with other callers holding the same cached ctx, so +// callers must treat it as read-only. +func Status(ctx context.Context, repo *git.Repository) (git.Status, error) { + worktree, err := repo.Worktree() + if err != nil { + return nil, err //nolint:wrapcheck // callers add their own context + } + + cache, ok := ctx.Value(statusCacheKey{}).(*statusCache) + if !ok { + return worktree.Status() //nolint:wrapcheck,forbidigo // the sanctioned call site + } + + // Key on the worktree root rather than the repository pointer: callers on + // the same hook path open the repository independently. + root := worktree.Filesystem().Root() + + cache.mu.Lock() + defer cache.mu.Unlock() + + if cached, hit := cache.statuses[root]; hit { + return cached, nil + } + + status, err := worktree.Status() //nolint:forbidigo // the sanctioned call site + if err != nil { + return nil, err //nolint:wrapcheck // callers add their own context + } + cache.statuses[root] = status + + return status, nil +} diff --git a/cmd/entire/cli/gitrepo/status_test.go b/cmd/entire/cli/gitrepo/status_test.go new file mode 100644 index 0000000000..885df190e7 --- /dev/null +++ b/cmd/entire/cli/gitrepo/status_test.go @@ -0,0 +1,96 @@ +package gitrepo + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/go-git/go-git/v6" +) + +func TestStatus_Cache(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + newContext func() context.Context + wantSeesWrite bool + }{ + { + name: "without cache every call re-reads the worktree", + newContext: context.Background, + wantSeesWrite: true, + }, + { + name: "with cache the first result is reused", + newContext: func() context.Context { return WithStatusCache(context.Background()) }, + wantSeesWrite: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + initRepoWithFile(t, dir, "tracked.txt", "initial") + + repo, err := OpenPath(dir) + require.NoError(t, err) + defer repo.Close() + + ctx := tt.newContext() + + _, err = Status(ctx, repo) + require.NoError(t, err) + + // Writing after the first read is what distinguishes a reused result + // from a fresh walk: only a fresh walk reports new.txt. + require.NoError(t, os.WriteFile(filepath.Join(dir, "new.txt"), []byte("hello"), 0o600)) + + status, err := Status(ctx, repo) + require.NoError(t, err) + + entry, ok := status["new.txt"] + if !tt.wantSeesWrite { + require.False(t, ok, "second call should have reused the cached result") + return + } + require.True(t, ok, "second call should have re-read the worktree") + require.Equal(t, git.Untracked, entry.Worktree) + }) + } +} + +func TestStatus_CacheKeysPerWorktree(t *testing.T) { + t.Parallel() + + dirA := t.TempDir() + initRepoWithFile(t, dirA, "tracked.txt", "initial") + + dirB := t.TempDir() + initRepoWithFile(t, dirB, "tracked.txt", "initial") + require.NoError(t, os.WriteFile(filepath.Join(dirB, "only-in-b.txt"), []byte("hello"), 0o600)) + + repoA, err := OpenPath(dirA) + require.NoError(t, err) + defer repoA.Close() + + repoB, err := OpenPath(dirB) + require.NoError(t, err) + defer repoB.Close() + + ctx := WithStatusCache(context.Background()) + + statusA, err := Status(ctx, repoA) + require.NoError(t, err) + require.NotContains(t, statusA, "only-in-b.txt") + + statusB, err := Status(ctx, repoB) + require.NoError(t, err) + require.Contains(t, statusB, "only-in-b.txt", + "a second worktree must not reuse the first worktree's cached entry") +} diff --git a/cmd/entire/cli/lifecycle.go b/cmd/entire/cli/lifecycle.go index dfef9d72da..2ed336f994 100644 --- a/cmd/entire/cli/lifecycle.go +++ b/cmd/entire/cli/lifecycle.go @@ -22,6 +22,7 @@ import ( "github.com/entireio/cli/cmd/entire/cli/agent" "github.com/entireio/cli/cmd/entire/cli/agent/codex" "github.com/entireio/cli/cmd/entire/cli/agent/types" + "github.com/entireio/cli/cmd/entire/cli/gitrepo" "github.com/entireio/cli/cmd/entire/cli/logging" "github.com/entireio/cli/cmd/entire/cli/paths" "github.com/entireio/cli/cmd/entire/cli/provenance" @@ -549,6 +550,16 @@ func handleLifecycleTurnStart(ctx context.Context, ag agent.Agent, event *agent. // background lock holder can't stall the user's prompt (see the const doc). ctx = strategy.WithSessionLockWait(ctx, turnStartSessionLockWait) + // Read the worktree status at most once for this hook. TurnStart runs before + // the agent acts, and it neither stages anything nor writes tracked files — + // only session metadata under .entire/ and refs under .git/ — so the status + // cannot change while this hook executes. Note the precondition is "no index + // writes and no tracked-file writes", not merely "nothing outside .git/": + // .git/index lives inside .git/ and staging does change reported status. + // Without the cache both CapturePrePromptState and the strategy's prompt + // attribution pay for a full go-git worktree walk. + ctx = gitrepo.WithStatusCache(ctx) + // Fill model from hint file if the agent didn't provide it on this hook if event.Model == "" { if hint := strategy.LoadModelHint(ctx, sessionID); hint != "" { diff --git a/cmd/entire/cli/rewind.go b/cmd/entire/cli/rewind.go index 79f64b54fd..c45b0c9ff2 100644 --- a/cmd/entire/cli/rewind.go +++ b/cmd/entire/cli/rewind.go @@ -1086,12 +1086,7 @@ func checkResetSafety(ctx context.Context, targetCommitHash string, uncommittedC warnings = append(warnings, uncommittedChangesWarning) } else { // Fall back to generic check - worktree, err := repo.Worktree() - if err != nil { - return nil, fmt.Errorf("failed to get worktree: %w", err) - } - - status, err := worktree.Status() + status, err := gitrepo.Status(ctx, repo) if err != nil { return nil, fmt.Errorf("failed to get status: %w", err) } diff --git a/cmd/entire/cli/state.go b/cmd/entire/cli/state.go index f345024029..a6d26de719 100644 --- a/cmd/entire/cli/state.go +++ b/cmd/entire/cli/state.go @@ -12,6 +12,7 @@ import ( "time" "github.com/entireio/cli/cmd/entire/cli/agent" + "github.com/entireio/cli/cmd/entire/cli/gitrepo" "github.com/entireio/cli/cmd/entire/cli/jsonutil" "github.com/entireio/cli/cmd/entire/cli/logging" "github.com/entireio/cli/cmd/entire/cli/osroot" @@ -269,12 +270,7 @@ func DetectFileChanges(ctx context.Context, previouslyUntracked []string) (*File } defer repo.Close() - worktree, err := repo.Worktree() - if err != nil { - return nil, fmt.Errorf("failed to get worktree: %w", err) - } - - status, err := worktree.Status() + status, err := gitrepo.Status(ctx, repo) if err != nil { return nil, fmt.Errorf("failed to get status: %w", err) } @@ -438,12 +434,7 @@ func getUntrackedFilesForState(ctx context.Context) ([]string, error) { } defer repo.Close() - worktree, err := repo.Worktree() - if err != nil { - return nil, err //nolint:wrapcheck // already present in codebase - } - - status, err := worktree.Status() + status, err := gitrepo.Status(ctx, repo) if err != nil { return nil, err //nolint:wrapcheck // already present in codebase } diff --git a/cmd/entire/cli/strategy/common.go b/cmd/entire/cli/strategy/common.go index cdce76a108..0cd4b04f16 100644 --- a/cmd/entire/cli/strategy/common.go +++ b/cmd/entire/cli/strategy/common.go @@ -1184,12 +1184,7 @@ func checkCanRewindWithWarning(ctx context.Context) (bool, string, error) { } defer repo.Close() - worktree, err := repo.Worktree() - if err != nil { - return true, "", nil - } - - status, err := worktree.Status() + status, err := gitrepo.Status(ctx, repo) if err != nil { return true, "", nil } diff --git a/cmd/entire/cli/strategy/manual_commit_hooks.go b/cmd/entire/cli/strategy/manual_commit_hooks.go index 2bf1f03360..3885b2d2b1 100644 --- a/cmd/entire/cli/strategy/manual_commit_hooks.go +++ b/cmd/entire/cli/strategy/manual_commit_hooks.go @@ -24,6 +24,7 @@ import ( "github.com/entireio/cli/cmd/entire/cli/checkpoint/remote" "github.com/entireio/cli/cmd/entire/cli/checkpointpolicy" "github.com/entireio/cli/cmd/entire/cli/gitops" + "github.com/entireio/cli/cmd/entire/cli/gitrepo" "github.com/entireio/cli/cmd/entire/cli/interactive" "github.com/entireio/cli/cmd/entire/cli/logging" "github.com/entireio/cli/cmd/entire/cli/paths" @@ -2498,8 +2499,10 @@ func (s *ManualCommitStrategy) calculatePromptAttributionAtStart( return result } - // Get worktree status to find ALL changed files - status, err := worktree.Status() + // Get worktree status to find ALL changed files. Shared with the turn-start + // pre-prompt capture via the context status cache, so the expensive go-git + // worktree walk runs once per hook rather than once per caller. + status, err := gitrepo.Status(ctx, repo) if err != nil { logging.Debug(logCtx, "prompt attribution skipped: failed to get worktree status", slog.String("error", err.Error())) diff --git a/e2e/.gitignore b/e2e/.gitignore new file mode 100644 index 0000000000..f9302a3d71 --- /dev/null +++ b/e2e/.gitignore @@ -0,0 +1,16 @@ +# E2E test artifacts (canary and real-agent run output). +# +# This rule intentionally lives here rather than as "e2e/artifacts/" in the +# repository root .gitignore. go-git's gitignore.ReadPatterns does not thread a +# parent directory's patterns into its recursive walk, so it only prunes a +# subdirectory when the matching pattern was declared by that subdirectory's own +# parent .gitignore. A root-level "e2e/artifacts/" rule therefore never pruned +# the walk, and every Worktree.Status() descended all of e2e/artifacts (~15k +# directories here), costing ~5s per call and timing out agent hooks. +# +# The leading slash anchors this to e2e/ itself, matching the scope of the +# original root-level "e2e/artifacts/" rule. Without it the pattern would also +# match a directory named "artifacts" at any depth below e2e/ (e.g. a future +# e2e/tests/artifacts/ fixture would be silently untracked). Anchoring does not +# affect pruning — go-git prunes either form. +/artifacts/