Skip to content

perf: stop go-git walking ignored e2e/artifacts on every agent hook - #1911

Merged
gtrrz-victor merged 4 commits into
mainfrom
soph/gitignore-status-perf
Aug 7, 2026
Merged

perf: stop go-git walking ignored e2e/artifacts on every agent hook#1911
gtrrz-victor merged 4 commits into
mainfrom
soph/gitignore-status-perf

Conversation

@Soph

@Soph Soph commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

https://entire.io/gh/entireio/cli/trails/984

Problem

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 — e.g. right after 773db0d bumped Go to 1.26.5 and invalidated it.

Both seconds-scale costs were go-git's Worktree.Status(), measured here at 5.25s against 0.013s for git status --porcelain.

Root cause

go-git's 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 declaring it. Our root-level e2e/artifacts/ rule was one level too deep, so every Status() descended all ~15k artifact directories.

Verified against go-git main with the same 3,000-directory tree:

root .gitignore rule result
big/ 0ms — pruned
e2e/artifacts/ 121ms — full descent

Reference git prunes both identically. A CPU profile of Status() shows 100% of samples inside collectIgnorePatternsReadPatterns; the diff walk contributes nothing, because it receives the full union of patterns and prunes correctly.

Changes

1. Move the ignore rule to e2e/.gitignore as artifacts/ so it is a direct child of its declaring .gitignore. Semantically identical to git, which reports the directory as ignored either way.

2. Add gitrepo.Status(ctx, repo) as the single entry point for reading worktree status, with gitrepo.WithStatusCache(ctx) memoizing the walk. handleLifecycleTurnStart installs a cache: it runs before the agent acts and writes only under .entire/ and .git/, so the status cannot change mid-hook. Previously CapturePrePromptState and the strategy's prompt attribution each paid for a full walk.

The cache is deliberately not installed on post-agent paths — DetectFileChanges on TurnEnd must observe the agent's edits. That constraint is documented on WithStatusCache.

3. Make the entry point real rather than aspirational: migrated the three remaining direct callers (DetectFileChanges, checkCanRewindWithWarning, checkResetSafety) and added a forbidigo rule alongside the existing Reset/Checkout ones. Behavior-preserving — without a cache in ctx, gitrepo.Status is exactly worktree.Status().

4. Documented both conventions in CLAUDE.md under Git Operations, including the .gitignore placement rule.

Results

before after
hook wall time 11.36s 0.91s
Status() 5.25s 0.045s
second Status() call 5.18s 0.015s (cache hit)

Artifacts were left in place and grew to 15,742 subdirectories during the test run with no regression.

Upstream

go-git#2284 (merged, unreleased — we are on v6.0.0-alpha.5, still the newest tag) 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. Its benchmark uses root-level ignore files, the placement that already prunes, so it cannot catch this.

The .gitignore placement in this PR is therefore a workaround pending a proper upstream fix to ReadPatterns. Once that lands and ships in a tag, the placement rule and possibly WithStatusCache become removable; gitrepo.Status and the forbidigo rule are worth keeping regardless.

Test plan

  • mise run fmt && mise run lint clean
  • mise run test — 8,534 tests pass
  • mise run test:integration — 448 tests pass
  • mise run test:e2e:canary — 59 + 4 tests pass
  • forbidigo rule verified to fire: reintroducing a bare worktree.Status() is reported at the call site
  • Hook re-timed at 0.76s after the cleanup commit

🤖 Generated with Claude Code


Note

Low Risk
Performance and routing changes with behavior-preserving status semantics when no cache is set; stale-cache misuse is documented but limited to the TurnStart window where the worktree is treated as stable.

Overview
Fixes TurnStart / UserPromptSubmit hooks spending ~5s+ per go-git Worktree.Status() by addressing how ignored e2e/artifacts is declared and by deduplicating status reads on the turn-start path.

The root .gitignore no longer lists e2e/artifacts/; e2e/.gitignore now ignores artifacts/ so go-git's ReadPatterns can prune that subtree (root-level nested patterns did not). gitrepo.Status(ctx, repo) is the only supported status API, with gitrepo.WithStatusCache(ctx) memoizing results per worktree root; handleLifecycleTurnStart installs the cache so pre-prompt capture and prompt attribution share one walk. Call sites in DetectFileChanges, rewind safety checks, and strategy rewind warnings now use gitrepo.Status. A forbidigo rule blocks new direct Worktree.Status() usage; CLAUDE.md documents the .gitignore placement rule and when caching is safe (TurnStart yes, TurnEnd no).

Reviewed by Cursor Bugbot for commit 45406d5. Configure here.

Soph and others added 2 commits August 6, 2026 13:47
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) <noreply@anthropic.com>
Entire-Checkpoint: 01KZBEDNJDD7WST4YWWQ3A1HQB
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) <noreply@anthropic.com>
Entire-Checkpoint: 01KZBHK54DHE4F5QJ0PGQ0TEJ1
Copilot AI lite review requested due to automatic review settings August 6, 2026 13:29
@Soph
Soph requested a review from a team as a code owner August 6, 2026 13:29

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses a major performance bottleneck in agent lifecycle hooks caused by go-git’s Worktree.Status() traversing a large ignored subtree (e2e/artifacts) and by repeated status reads in the same hook. It does so by fixing .gitignore placement to enable go-git pruning and by introducing a single status entry point with an optional per-hook cache to deduplicate the expensive walk.

Changes:

  • Moved the e2e/artifacts ignore rule into e2e/.gitignore as artifacts/ to allow go-git’s ignore walker to prune the subtree.
  • Added gitrepo.Status(ctx, repo) as the canonical worktree-status API plus gitrepo.WithStatusCache(ctx) to memoize status reads when safe (TurnStart).
  • Migrated remaining direct callers to gitrepo.Status and added a forbidigo rule to prevent reintroducing Worktree.Status() usage.

Reviewed changes

Copilot reviewed 10 out of 11 changed files in this pull request and generated no comments.

Show a summary per file
File Description
e2e/.gitignore Introduces artifacts/ ignore rule colocated with its parent dir for go-git pruning.
cmd/entire/cli/lifecycle.go Installs a status cache on TurnStart to ensure status is walked at most once per hook.
cmd/entire/cli/gitrepo/status.go Adds the canonical Status API and context-based memoization.
cmd/entire/cli/gitrepo/status_test.go Adds tests validating cache reuse behavior and per-worktree cache keying.
cmd/entire/cli/strategy/manual_commit_hooks.go Switches prompt-attribution status reads to gitrepo.Status to share the cached walk.
cmd/entire/cli/strategy/common.go Migrates rewind warning status read to gitrepo.Status.
cmd/entire/cli/state.go Migrates DetectFileChanges and untracked-file enumeration to gitrepo.Status.
cmd/entire/cli/rewind.go Migrates reset-safety status read to gitrepo.Status.
CLAUDE.md Documents the new “always use gitrepo.Status” convention and .gitignore placement rule.
.golangci.yaml Adds a forbidigo rule preventing new Worktree.Status() call sites.
.gitignore Removes nested e2e/artifacts/ ignore and documents why the rule lives under e2e/.gitignore.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 45406d5. Configure here.

Comment thread e2e/.gitignore Outdated
dipree
dipree previously approved these changes Aug 6, 2026
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) <noreply@anthropic.com>
Entire-Checkpoint: 01KZBXFD5VWBW1RZMPGMWYJHVP
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) <noreply@anthropic.com>
Entire-Checkpoint: 01KZBYFRVEV9W89CQ6FA7J91Q0
@gtrrz-victor
gtrrz-victor merged commit 836a098 into main Aug 7, 2026
12 of 23 checks passed
@gtrrz-victor
gtrrz-victor deleted the soph/gitignore-status-perf branch August 7, 2026 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

5 participants