From 6ddce4747dafe98984d4b0930b4ce6d97bf84bc5 Mon Sep 17 00:00:00 2001 From: Stefan Haubold Date: Thu, 6 Aug 2026 13:47:57 +0200 Subject: [PATCH 1/4] perf: stop go-git walking ignored e2e/artifacts on every hook The UserPromptSubmit hook took ~11.4s in this repo (9.8s of it system time), overshooting Claude Code's 30s hook timeout once the Go build cache was cold. Both seconds-scale costs were go-git's Worktree.Status(), measured at 5.25s here against 0.013s for `git status --porcelain`. Cause: gitignore.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. A rule prunes a subtree only when its target is a direct child of the .gitignore that declares it. The root-level "e2e/artifacts/" rule was one level too deep to ever prune, so every Status() descended all 15k artifact directories. Verified against go-git main: with the same 3000-directory tree, a root "big/" rule prunes in 0ms while a root "e2e/artifacts/" rule costs a full descent. Two changes: - Move the rule to e2e/.gitignore as "artifacts/", making it a direct child of its declaring .gitignore. Identical semantics to git, which reports the directory as ignored either way. - Add gitrepo.Status, which memoizes the walk when the context carries a cache from gitrepo.WithStatusCache. TurnStart installs one: it runs before the agent acts and only writes under .entire/ and .git/, so the status cannot change mid-hook. CapturePrePromptState and the strategy's prompt attribution previously paid for one full walk each. Hook wall time: 11.36s -> 0.91s. The second Status() call is now a cache hit at 0.015s. Artifacts were left in place and grew to 15,742 subdirectories during the test run with no regression. go-git#2284 (merged, unreleased; we are on v6.0.0-alpha.5) cuts the blind ignore-file opens and would take Status() from 5.24s to 2.47s here, but does not address the pattern-inheritance gap. Co-Authored-By: Claude Opus 5 (1M context) Entire-Checkpoint: 01KZBEDNJDD7WST4YWWQ3A1HQB --- .gitignore | 6 +- cmd/entire/cli/gitrepo/status.go | 75 ++++++++ .../cli/gitrepo/status_external_test.go | 162 ++++++++++++++++++ cmd/entire/cli/lifecycle.go | 8 + cmd/entire/cli/state.go | 8 +- .../cli/strategy/manual_commit_hooks.go | 7 +- e2e/.gitignore | 10 ++ 7 files changed, 266 insertions(+), 10 deletions(-) create mode 100644 cmd/entire/cli/gitrepo/status.go create mode 100644 cmd/entire/cli/gitrepo/status_external_test.go create mode 100644 e2e/.gitignore 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/cmd/entire/cli/gitrepo/status.go b/cmd/entire/cli/gitrepo/status.go new file mode 100644 index 0000000000..203e60d4ab --- /dev/null +++ b/cmd/entire/cli/gitrepo/status.go @@ -0,0 +1,75 @@ +package gitrepo + +import ( + "context" + "sync" + + "github.com/go-git/go-git/v6" +) + +// go-git's Worktree.Status() is expensive: it walks the whole worktree twice +// (once collecting .gitignore patterns, once diffing), and it does not prune +// ignored subtrees whose pattern was declared by an ancestor .gitignore. A +// single call costs seconds in a repo with a large ignored directory, so hooks +// that need the status more than once must not recompute it. +// +// Status is the single entry point for reading worktree status. When ctx +// carries a cache installed by WithStatusCache, the first call for a worktree +// computes the status and later calls with that ctx reuse it. + +type statusCacheKey struct{} + +type statusResult struct { + status git.Status + err error +} + +type statusCache struct { + mu sync.Mutex + results map[string]statusResult +} + +// WithStatusCache returns a context that memoizes Status results. +// +// Install it only across a window in which the worktree cannot change — +// otherwise later callers observe a stale status. A hook that runs before the +// agent acts (such as turn start) is such a 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{ + results: make(map[string]statusResult), + }) +} + +// 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 || cache == nil { + return worktree.Status() //nolint:wrapcheck // callers add their own context + } + + // 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, ok := cache.results[root]; ok { + return cached.status, cached.err + } + + status, statusErr := worktree.Status() + cache.results[root] = statusResult{status: status, err: statusErr} + + return status, statusErr //nolint:wrapcheck // callers add their own context +} diff --git a/cmd/entire/cli/gitrepo/status_external_test.go b/cmd/entire/cli/gitrepo/status_external_test.go new file mode 100644 index 0000000000..567101648c --- /dev/null +++ b/cmd/entire/cli/gitrepo/status_external_test.go @@ -0,0 +1,162 @@ +// Package gitrepo_test holds gitrepo tests that need testutil. testutil imports +// gitrepo, so these cannot live in the gitrepo package itself. +package gitrepo_test + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/entireio/cli/cmd/entire/cli/gitrepo" + "github.com/entireio/cli/cmd/entire/cli/testutil" + + "github.com/go-git/go-git/v6" +) + +// newRepoWithCommit returns a repo directory holding one committed file. +func newRepoWithCommit(t *testing.T) string { + t.Helper() + + dir := t.TempDir() + testutil.InitRepo(t, dir) + testutil.WriteFile(t, dir, "tracked.txt", "initial") + testutil.GitAdd(t, dir, "tracked.txt") + testutil.GitCommit(t, dir, "initial commit") + + return dir +} + +func TestStatus_WithoutCacheSeesWorktreeChanges(t *testing.T) { + t.Parallel() + + dir := newRepoWithCommit(t) + repo, err := gitrepo.OpenPath(dir) + if err != nil { + t.Fatalf("OpenPath(%q) error = %v", dir, err) + } + defer repo.Close() + + ctx := context.Background() + + status, err := gitrepo.Status(ctx, repo) + if err != nil { + t.Fatalf("Status() error = %v", err) + } + if _, ok := status["new.txt"]; ok { + t.Fatalf("Status() reported new.txt before it was created") + } + + testutil.WriteFile(t, dir, "new.txt", "hello") + + status, err = gitrepo.Status(ctx, repo) + if err != nil { + t.Fatalf("Status() after write error = %v", err) + } + if got := status["new.txt"]; got == nil || got.Worktree != git.Untracked { + t.Errorf("Status() without cache did not report new.txt as untracked, got %+v", got) + } +} + +func TestStatus_WithCacheReusesFirstResult(t *testing.T) { + t.Parallel() + + dir := newRepoWithCommit(t) + repo, err := gitrepo.OpenPath(dir) + if err != nil { + t.Fatalf("OpenPath(%q) error = %v", dir, err) + } + defer repo.Close() + + ctx := gitrepo.WithStatusCache(context.Background()) + + if _, err := gitrepo.Status(ctx, repo); err != nil { + t.Fatalf("Status() error = %v", err) + } + + // Changing the worktree after the first read is the observable proof that + // the second call did not recompute: a fresh walk would report new.txt. + testutil.WriteFile(t, dir, "new.txt", "hello") + + status, err := gitrepo.Status(ctx, repo) + if err != nil { + t.Fatalf("Status() second call error = %v", err) + } + if _, ok := status["new.txt"]; ok { + t.Errorf("Status() with cache recomputed instead of reusing the first result") + } +} + +func TestStatus_CacheIsPerWorktree(t *testing.T) { + t.Parallel() + + dirA := newRepoWithCommit(t) + dirB := newRepoWithCommit(t) + testutil.WriteFile(t, dirB, "only-in-b.txt", "hello") + + repoA, err := gitrepo.OpenPath(dirA) + if err != nil { + t.Fatalf("OpenPath(%q) error = %v", dirA, err) + } + defer repoA.Close() + + repoB, err := gitrepo.OpenPath(dirB) + if err != nil { + t.Fatalf("OpenPath(%q) error = %v", dirB, err) + } + defer repoB.Close() + + ctx := gitrepo.WithStatusCache(context.Background()) + + statusA, err := gitrepo.Status(ctx, repoA) + if err != nil { + t.Fatalf("Status(repoA) error = %v", err) + } + if _, ok := statusA["only-in-b.txt"]; ok { + t.Fatalf("Status(repoA) reported a file that only exists in repoB") + } + + statusB, err := gitrepo.Status(ctx, repoB) + if err != nil { + t.Fatalf("Status(repoB) error = %v", err) + } + if got := statusB["only-in-b.txt"]; got == nil || got.Worktree != git.Untracked { + t.Errorf("Status(repoB) reused repoA's entry instead of keying per worktree, got %+v", got) + } +} + +// TestStatus_CachePrunesIgnoredSubtreeRule guards the .gitignore placement that +// keeps go-git from walking e2e/artifacts. go-git only prunes a directory when +// the matching pattern comes from that directory's own parent .gitignore, so a +// nested "outer/ignored/" rule in the root .gitignore does not prune, while +// "ignored/" in outer/.gitignore does. Both must stay ignored either way. +func TestStatus_NestedIgnoreRuleStillIgnores(t *testing.T) { + t.Parallel() + + dir := newRepoWithCommit(t) + testutil.WriteFile(t, dir, "outer/.gitignore", "ignored/\n") + testutil.GitAdd(t, dir, "outer/.gitignore") + testutil.GitCommit(t, dir, "add nested gitignore") + + if err := os.MkdirAll(filepath.Join(dir, "outer", "ignored", "deep"), 0o750); err != nil { + t.Fatalf("MkdirAll() error = %v", err) + } + testutil.WriteFile(t, dir, "outer/ignored/deep/junk.txt", "junk") + + repo, err := gitrepo.OpenPath(dir) + if err != nil { + t.Fatalf("OpenPath(%q) error = %v", dir, err) + } + defer repo.Close() + + status, err := gitrepo.Status(context.Background(), repo) + if err != nil { + t.Fatalf("Status() error = %v", err) + } + + for path := range status { + if filepath.ToSlash(path) == "outer/ignored/deep/junk.txt" { + t.Errorf("Status() reported an ignored file: %s", path) + } + } +} diff --git a/cmd/entire/cli/lifecycle.go b/cmd/entire/cli/lifecycle.go index dfef9d72da..646b225153 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,13 @@ 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 fires + // before the agent runs and only writes under .entire/ and .git/, neither of + // which git reports, so the status cannot change while this hook executes. + // 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/state.go b/cmd/entire/cli/state.go index f345024029..48671a7763 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" @@ -438,12 +439,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/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..7757227f34 --- /dev/null +++ b/e2e/.gitignore @@ -0,0 +1,10 @@ +# 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. +artifacts/ From 45406d520091b1de12ed64003228f87752781e95 Mon Sep 17 00:00:00 2001 From: Stefan Haubold Date: Thu, 6 Aug 2026 14:43:23 +0200 Subject: [PATCH 2/4] refactor: make gitrepo.Status the enforced worktree-status entry point MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cleanup pass over the preceding perf commit. Simplifications: - Cache successes only. The statusResult struct existed solely to memoize errors, which both callers treat as "skip", so nothing consumed them. - Drop the unreachable `cache == nil` half of the type-assertion guard. - Fold the two mirror-image cache tests into one table-driven test and move them into package gitrepo, reusing the existing initRepoWithFile helper from repository_test.go. That removes the external test package and its testutil import-cycle workaround. - Delete a test that claimed to guard the .gitignore placement but only asserted that go-git honors a nested .gitignore. It passed identically with the rule in either location, so it could not fail for the regression it named, and its doc comment referenced a function name that did not exist. Altitude: the new package doc claimed Status was "the single entry point for reading worktree status" while three call sites still called worktree.Status() directly. Rather than weaken the claim, make it true: - Migrate DetectFileChanges (state.go), checkCanRewindWithWarning (strategy/common.go) and checkResetSafety (rewind.go). Behavior is unchanged — without a cache in ctx, gitrepo.Status is exactly worktree.Status(), and none of these paths install one. DetectFileChanges on the post-agent TurnEnd path therefore still sees fresh state. - Add a forbidigo rule for go-git Worktree.Status, matching the existing rules for Reset and Checkout. Verified it fires: reintroducing a bare call is reported at the call site. - Document the convention in CLAUDE.md under Git Operations, including the .gitignore placement rule and the constraint on WithStatusCache, next to the sibling gitrepo and git-CLI entries. 8534 unit + 448 integration tests pass; hook stays at 0.76s. Co-Authored-By: Claude Opus 5 (1M context) Entire-Checkpoint: 01KZBHK54DHE4F5QJ0PGQ0TEJ1 --- .golangci.yaml | 3 + CLAUDE.md | 29 ++++ cmd/entire/cli/gitrepo/status.go | 45 +++-- .../cli/gitrepo/status_external_test.go | 162 ------------------ cmd/entire/cli/gitrepo/status_test.go | 96 +++++++++++ cmd/entire/cli/rewind.go | 7 +- cmd/entire/cli/state.go | 7 +- cmd/entire/cli/strategy/common.go | 7 +- 8 files changed, 153 insertions(+), 203 deletions(-) delete mode 100644 cmd/entire/cli/gitrepo/status_external_test.go create mode 100644 cmd/entire/cli/gitrepo/status_test.go 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..9403a059bf 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -492,6 +492,35 @@ 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`. + +`gitrepo.WithStatusCache(ctx)` memoizes the walk for callers that read status +more than once. Install it **only** across a window where the worktree cannot +change: the TurnStart hook qualifies (it runs before the agent acts and writes +only under `.entire/` and `.git/`), post-agent hooks such as TurnEnd do not — +`DetectFileChanges` there must observe the agent's edits. + #### 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 index 203e60d4ab..a42c29c826 100644 --- a/cmd/entire/cli/gitrepo/status.go +++ b/cmd/entire/cli/gitrepo/status.go @@ -7,26 +7,22 @@ import ( "github.com/go-git/go-git/v6" ) -// go-git's Worktree.Status() is expensive: it walks the whole worktree twice -// (once collecting .gitignore patterns, once diffing), and it does not prune -// ignored subtrees whose pattern was declared by an ancestor .gitignore. A -// single call costs seconds in a repo with a large ignored directory, so hooks -// that need the status more than once must not recompute it. +// Status is the single entry point for reading go-git worktree status; the +// forbidigo rule in .golangci.yaml keeps callers off worktree.Status directly. // -// Status is the single entry point for reading worktree status. When ctx -// carries a cache installed by WithStatusCache, the first call for a worktree -// computes the status and later calls with that ctx reuse it. +// 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 statusResult struct { - status git.Status - err error -} - type statusCache struct { - mu sync.Mutex - results map[string]statusResult + mu sync.Mutex + statuses map[string]git.Status } // WithStatusCache returns a context that memoizes Status results. @@ -37,7 +33,7 @@ type statusCache struct { // agent has edited files is not. func WithStatusCache(ctx context.Context) context.Context { return context.WithValue(ctx, statusCacheKey{}, &statusCache{ - results: make(map[string]statusResult), + statuses: make(map[string]git.Status), }) } @@ -53,8 +49,8 @@ func Status(ctx context.Context, repo *git.Repository) (git.Status, error) { } cache, ok := ctx.Value(statusCacheKey{}).(*statusCache) - if !ok || cache == nil { - return worktree.Status() //nolint:wrapcheck // callers add their own context + if !ok { + return worktree.Status() //nolint:wrapcheck,forbidigo // the sanctioned call site } // Key on the worktree root rather than the repository pointer: callers on @@ -64,12 +60,15 @@ func Status(ctx context.Context, repo *git.Repository) (git.Status, error) { cache.mu.Lock() defer cache.mu.Unlock() - if cached, ok := cache.results[root]; ok { - return cached.status, cached.err + if cached, hit := cache.statuses[root]; hit { + return cached, nil } - status, statusErr := worktree.Status() - cache.results[root] = statusResult{status: status, err: statusErr} + 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, statusErr //nolint:wrapcheck // callers add their own context + return status, nil } diff --git a/cmd/entire/cli/gitrepo/status_external_test.go b/cmd/entire/cli/gitrepo/status_external_test.go deleted file mode 100644 index 567101648c..0000000000 --- a/cmd/entire/cli/gitrepo/status_external_test.go +++ /dev/null @@ -1,162 +0,0 @@ -// Package gitrepo_test holds gitrepo tests that need testutil. testutil imports -// gitrepo, so these cannot live in the gitrepo package itself. -package gitrepo_test - -import ( - "context" - "os" - "path/filepath" - "testing" - - "github.com/entireio/cli/cmd/entire/cli/gitrepo" - "github.com/entireio/cli/cmd/entire/cli/testutil" - - "github.com/go-git/go-git/v6" -) - -// newRepoWithCommit returns a repo directory holding one committed file. -func newRepoWithCommit(t *testing.T) string { - t.Helper() - - dir := t.TempDir() - testutil.InitRepo(t, dir) - testutil.WriteFile(t, dir, "tracked.txt", "initial") - testutil.GitAdd(t, dir, "tracked.txt") - testutil.GitCommit(t, dir, "initial commit") - - return dir -} - -func TestStatus_WithoutCacheSeesWorktreeChanges(t *testing.T) { - t.Parallel() - - dir := newRepoWithCommit(t) - repo, err := gitrepo.OpenPath(dir) - if err != nil { - t.Fatalf("OpenPath(%q) error = %v", dir, err) - } - defer repo.Close() - - ctx := context.Background() - - status, err := gitrepo.Status(ctx, repo) - if err != nil { - t.Fatalf("Status() error = %v", err) - } - if _, ok := status["new.txt"]; ok { - t.Fatalf("Status() reported new.txt before it was created") - } - - testutil.WriteFile(t, dir, "new.txt", "hello") - - status, err = gitrepo.Status(ctx, repo) - if err != nil { - t.Fatalf("Status() after write error = %v", err) - } - if got := status["new.txt"]; got == nil || got.Worktree != git.Untracked { - t.Errorf("Status() without cache did not report new.txt as untracked, got %+v", got) - } -} - -func TestStatus_WithCacheReusesFirstResult(t *testing.T) { - t.Parallel() - - dir := newRepoWithCommit(t) - repo, err := gitrepo.OpenPath(dir) - if err != nil { - t.Fatalf("OpenPath(%q) error = %v", dir, err) - } - defer repo.Close() - - ctx := gitrepo.WithStatusCache(context.Background()) - - if _, err := gitrepo.Status(ctx, repo); err != nil { - t.Fatalf("Status() error = %v", err) - } - - // Changing the worktree after the first read is the observable proof that - // the second call did not recompute: a fresh walk would report new.txt. - testutil.WriteFile(t, dir, "new.txt", "hello") - - status, err := gitrepo.Status(ctx, repo) - if err != nil { - t.Fatalf("Status() second call error = %v", err) - } - if _, ok := status["new.txt"]; ok { - t.Errorf("Status() with cache recomputed instead of reusing the first result") - } -} - -func TestStatus_CacheIsPerWorktree(t *testing.T) { - t.Parallel() - - dirA := newRepoWithCommit(t) - dirB := newRepoWithCommit(t) - testutil.WriteFile(t, dirB, "only-in-b.txt", "hello") - - repoA, err := gitrepo.OpenPath(dirA) - if err != nil { - t.Fatalf("OpenPath(%q) error = %v", dirA, err) - } - defer repoA.Close() - - repoB, err := gitrepo.OpenPath(dirB) - if err != nil { - t.Fatalf("OpenPath(%q) error = %v", dirB, err) - } - defer repoB.Close() - - ctx := gitrepo.WithStatusCache(context.Background()) - - statusA, err := gitrepo.Status(ctx, repoA) - if err != nil { - t.Fatalf("Status(repoA) error = %v", err) - } - if _, ok := statusA["only-in-b.txt"]; ok { - t.Fatalf("Status(repoA) reported a file that only exists in repoB") - } - - statusB, err := gitrepo.Status(ctx, repoB) - if err != nil { - t.Fatalf("Status(repoB) error = %v", err) - } - if got := statusB["only-in-b.txt"]; got == nil || got.Worktree != git.Untracked { - t.Errorf("Status(repoB) reused repoA's entry instead of keying per worktree, got %+v", got) - } -} - -// TestStatus_CachePrunesIgnoredSubtreeRule guards the .gitignore placement that -// keeps go-git from walking e2e/artifacts. go-git only prunes a directory when -// the matching pattern comes from that directory's own parent .gitignore, so a -// nested "outer/ignored/" rule in the root .gitignore does not prune, while -// "ignored/" in outer/.gitignore does. Both must stay ignored either way. -func TestStatus_NestedIgnoreRuleStillIgnores(t *testing.T) { - t.Parallel() - - dir := newRepoWithCommit(t) - testutil.WriteFile(t, dir, "outer/.gitignore", "ignored/\n") - testutil.GitAdd(t, dir, "outer/.gitignore") - testutil.GitCommit(t, dir, "add nested gitignore") - - if err := os.MkdirAll(filepath.Join(dir, "outer", "ignored", "deep"), 0o750); err != nil { - t.Fatalf("MkdirAll() error = %v", err) - } - testutil.WriteFile(t, dir, "outer/ignored/deep/junk.txt", "junk") - - repo, err := gitrepo.OpenPath(dir) - if err != nil { - t.Fatalf("OpenPath(%q) error = %v", dir, err) - } - defer repo.Close() - - status, err := gitrepo.Status(context.Background(), repo) - if err != nil { - t.Fatalf("Status() error = %v", err) - } - - for path := range status { - if filepath.ToSlash(path) == "outer/ignored/deep/junk.txt" { - t.Errorf("Status() reported an ignored file: %s", path) - } - } -} 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/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 48671a7763..a6d26de719 100644 --- a/cmd/entire/cli/state.go +++ b/cmd/entire/cli/state.go @@ -270,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) } 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 } From 4a2486f0fa73b784c190ac853fc5fb5fa2408c36 Mon Sep 17 00:00:00 2001 From: Stefan Haubold Date: Thu, 6 Aug 2026 18:11:04 +0200 Subject: [PATCH 3/4] fix: anchor the relocated e2e artifacts ignore rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Moving `e2e/artifacts/` out of the root .gitignore into e2e/.gitignore as bare `artifacts/` also dropped git's root anchoring: a pattern with no separator matches at any depth, so the rule newly covered any directory named `artifacts` below e2e/ — a future e2e/tests/artifacts/ fixture would have been silently untracked. Anchor it as `/artifacts/` to restore the original scope. Verified with git check-ignore: `/artifacts/` matches e2e/artifacts/ and does not match e2e/tests/artifacts/, while bare `artifacts/` matched both. Anchoring does not affect the go-git pruning this rule exists for — both forms prune a 3000-directory subtree in 0ms, and the hook stays at 0.89s with 15,742 artifact subdirectories present. Co-Authored-By: Claude Opus 5 (1M context) Entire-Checkpoint: 01KZBXFD5VWBW1RZMPGMWYJHVP --- CLAUDE.md | 7 ++++++- e2e/.gitignore | 8 +++++++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 9403a059bf..ae40f5bad4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -510,11 +510,16 @@ 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, +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 where the worktree cannot change: the TurnStart hook qualifies (it runs before the agent acts and writes diff --git a/e2e/.gitignore b/e2e/.gitignore index 7757227f34..f9302a3d71 100644 --- a/e2e/.gitignore +++ b/e2e/.gitignore @@ -7,4 +7,10 @@ # 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. -artifacts/ +# +# 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/ From 6c6f14419ac8112e5921c07b0eeb6d73c206bf10 Mon Sep 17 00:00:00 2001 From: Stefan Haubold Date: Thu, 6 Aug 2026 18:28:43 +0200 Subject: [PATCH 4/4] docs: state the status-cache precondition in terms of index writes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review question in Slack: does Entire do index operations, and if so does the status cache need invalidation? Answer is no, but the justification as written was wrong in a way that would have misled the next reader. It said TurnStart "only writes under .entire/ and .git/, neither of which git reports" — but .git/index is inside .git/, and staging very much changes reported status. Anyone adding an index write to that window would have read the comment as permission. Restate the precondition as "no tracked-file writes and no index writes", and record what makes it hold today: no SetIndex calls anywhere, the single Storer.Index() use (content_overlap.go) is a read, and every git subcommand on the strategy/checkpoint paths is index-read-only (update-ref writes refs, not the index). Checkpoints build trees in-memory via plumbing rather than staging. Verified empirically: across a TurnStart hook run, .git/index is unchanged by both sha256 and mtime — it is not even rewritten. Also note in CLAUDE.md that the cache is context-scoped to one short-lived hook process, so it cannot go stale across turns, and flag that new index-mutating operations must be checked against cache windows. Co-Authored-By: Claude Opus 5 (1M context) Entire-Checkpoint: 01KZBYFRVEV9W89CQ6FA7J91Q0 --- CLAUDE.md | 18 ++++++++++++++---- cmd/entire/cli/gitrepo/status.go | 11 +++++++---- cmd/entire/cli/lifecycle.go | 9 ++++++--- 3 files changed, 27 insertions(+), 11 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index ae40f5bad4..c72f5e41a1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -521,10 +521,20 @@ 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 where the worktree cannot -change: the TurnStart hook qualifies (it runs before the agent acts and writes -only under `.entire/` and `.git/`), post-agent hooks such as TurnEnd do not — -`DetectFileChanges` there must observe the agent's edits. +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 diff --git a/cmd/entire/cli/gitrepo/status.go b/cmd/entire/cli/gitrepo/status.go index a42c29c826..fcab26ef98 100644 --- a/cmd/entire/cli/gitrepo/status.go +++ b/cmd/entire/cli/gitrepo/status.go @@ -27,10 +27,13 @@ type statusCache struct { // WithStatusCache returns a context that memoizes Status results. // -// Install it only across a window in which the worktree cannot change — -// otherwise later callers observe a stale status. A hook that runs before the -// agent acts (such as turn start) is such a window; a hook that runs after the -// agent has edited files is not. +// 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), diff --git a/cmd/entire/cli/lifecycle.go b/cmd/entire/cli/lifecycle.go index 646b225153..2ed336f994 100644 --- a/cmd/entire/cli/lifecycle.go +++ b/cmd/entire/cli/lifecycle.go @@ -550,9 +550,12 @@ 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 fires - // before the agent runs and only writes under .entire/ and .git/, neither of - // which git reports, so the status cannot change while this hook executes. + // 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)