Skip to content

perf(#2351): batch path-existence checks via Git Trees API - #2360

Merged
ifireball merged 3 commits into
mainfrom
agent/2351-batch-path-presence
Jul 7, 2026
Merged

perf(#2351): batch path-existence checks via Git Trees API#2360
ifireball merged 3 commits into
mainfrom
agent/2351-batch-path-presence

Conversation

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor

Add forge.Client.ListRepositoryFiles to retrieve all file paths in a repository's default branch with a single Git Trees API call (refs → commit → tree?recursive=1). This replaces the O(N) GetFileContent pattern used by ComparePathPresence, reducing 100+ sequential API calls to 3 fixed calls regardless of path count.

Changes:

  • forge.Client: add ListRepositoryFiles(ctx, owner, repo)
  • github.LiveClient: implement using Git Trees API (reuses the
    same refs/commits/trees pattern as CommitFiles)
  • forge.FakeClient: implement using FileContents map keys
  • scaffold.ComparePathPresence: new batch implementation that
    calls ListRepositoryFiles once and checks membership locally
  • Tests: 6 ComparePathPresence tests including a guard that
    GetFileContent is never called; error injection and thread
    safety coverage for the new forge method

PR #1954 introduces a naive ComparePathPresence in vendormanifest.go that loops GetFileContent per path. When that PR merges, its version should be replaced with this batch implementation.


Closes #2351

Post-script verification

  • Branch is not main/master (agent/2351-batch-path-presence)
  • Secret scan passed (gitleaks — 32f73a4f93301493d2c31be3970aa4c51a26acc7..HEAD)
  • Pre-commit hooks passed (authoritative run on runner)
  • Tests ran inside sandbox

Add forge.Client.ListRepositoryFiles to retrieve all file paths
in a repository's default branch with a single Git Trees API
call (refs → commit → tree?recursive=1). This replaces the O(N)
GetFileContent pattern used by ComparePathPresence, reducing
100+ sequential API calls to 3 fixed calls regardless of path
count.

Changes:
- forge.Client: add ListRepositoryFiles(ctx, owner, repo)
- github.LiveClient: implement using Git Trees API (reuses the
  same refs/commits/trees pattern as CommitFiles)
- forge.FakeClient: implement using FileContents map keys
- scaffold.ComparePathPresence: new batch implementation that
  calls ListRepositoryFiles once and checks membership locally
- Tests: 6 ComparePathPresence tests including a guard that
  GetFileContent is never called; error injection and thread
  safety coverage for the new forge method

PR #1954 introduces a naive ComparePathPresence in
vendormanifest.go that loops GetFileContent per path. When that
PR merges, its version should be replaced with this batch
implementation.

Closes #2351
@github-actions

github-actions Bot commented Jun 16, 2026

Copy link
Copy Markdown

E2E tests are running

Authorization passed for this commit. See the E2E Tests workflow for results.

@github-actions

github-actions Bot commented Jun 16, 2026

Copy link
Copy Markdown

Site preview

Preview: https://bab75538-site.fullsend-ai.workers.dev

Commit: ee77525b5f06145397eb9d6cb3d26da48886f652

@fullsend-ai-review

fullsend-ai-review Bot commented Jun 16, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 7:30 PM UTC · Completed 7:41 PM UTC
Commit: dd9fc10 · View workflow run →

@codecov

codecov Bot commented Jun 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.61039% with 8 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/forge/github/github.go 84.00% 4 Missing and 4 partials ⚠️

📢 Thoughts on this report? Let us know!

@fullsend-ai-review

Copy link
Copy Markdown

Review

Findings

Medium

  • [error-handling] internal/forge/github/github.go:1024 — When the Git Trees API response is truncated (repository too large), the function returns a hard error with no sentinel type, making it impossible for callers to detect and fall back to per-path checks. The interface comment in forge.go does not document this failure mode. If any repository legitimately exceeds the GitHub tree size limit (~100k entries or 7MB response), every call to ComparePathPresence will fail permanently with no recourse.
    Remediation: Add a sentinel error (e.g., ErrTreeTruncated) so callers can detect this specific failure and fall back to per-path checks. Document the truncation risk in the ListRepositoryFiles interface comment. See also: [api-documentation-completeness] finding at internal/forge/forge.go:195.

Low

  • [api-documentation-completeness] internal/forge/forge.go:195 — The interface documentation for ListRepositoryFiles states "Returns ErrNotFound if the repository does not exist" but does not document the truncation behavior when the repository tree is too large. See also: [error-handling] finding at internal/forge/github/github.go:1024.
  • [error-wrapping-inconsistency] internal/forge/github/github.go:968 — Error wrapping uses get repo: %w which is slightly abbreviated compared to some other methods that use more descriptive prefixes like get repo for default branch: %w.
  • [error-handling-inconsistency] internal/forge/github/github.go:970 — The retryOnTransient call uses label get branch ref and the inner function also wraps errors with get branch ref:, creating double-wrapping on exhaustion. This matches the existing pattern in commitFilesTo but is still a pre-existing code smell.
  • [non-deterministic-output] internal/forge/fake.go:411FakeClient.ListRepositoryFiles iterates over a map (f.FileContents), producing non-deterministic ordering of the returned paths. Current callers build a map from the result so order does not matter.
  • [retry-consistency] internal/forge/github/github.go:958 — Only step 2 (get branch ref) is wrapped in retryOnTransient. Steps 1, 3, and 4 are not retried. This is consistent with the existing pattern in commitFilesTo.

Info

  • [documentation-style] internal/scaffold/pathpresence.go:10 — The function comment mentions "Git Trees API call" which leaks GitHub-specific implementation details into a forge-agnostic layer.
  • [scope-alignment] PR scope aligns with issue Vendor analyze: batch path existence checks instead of O(N) GetFileContent calls #2351. No scope creep detected.
  • [architectural-coherence] The new forge.Client interface method correctly follows the forge abstraction pattern (ADR-0005). The method name ListRepositoryFiles is appropriately forge-neutral.
  • [internal-interface-evolution] Additive change to internal interface. No cross-repo contract risk.

@ifireball

Copy link
Copy Markdown
Member

/fs-fix resolve merge conflicts, address review findings, and make codecov happy please

@ifireball
ifireball requested a review from a team July 7, 2026 10:43
@fullsend-ai-coder

fullsend-ai-coder Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 10:43 AM UTC · Completed 11:05 AM UTC
Commit: e8381e3 · View workflow run →

- Fix retryOnTransient -> retryOnRepoRace in ListRepositoryFiles
- Add ErrTreeTruncated sentinel error for truncated tree responses
- Update ListRepositoryFiles interface docs with truncation behavior
- Remove GitHub-specific reference from pathpresence.go comment
- Move ComparePathPresence from vendormanifest.go to pathpresence.go
  using batch ListRepositoryFiles instead of per-path GetFileContent
- Add httptest-based tests for ListRepositoryFiles (success, truncated,
  not-found, empty-repo) to improve codecov patch coverage
- Add TestComparePathPresence_TreeTruncated for sentinel error propagation
- Remove stale ComparePathPresence tests from vendormanifest_test.go

Addresses review feedback on #2360
@ifireball

Copy link
Copy Markdown
Member

/fs-fix resolve merge conflicts, and make codecov happy please

@fullsend-ai-coder

fullsend-ai-coder Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 11:07 AM UTC · Completed 11:33 AM UTC
Commit: e8381e3 · View workflow run →

Merge origin/main into branch, resolving conflicts in:
- internal/forge/forge.go: keep ErrTreeTruncated (needed by ListRepositoryFiles)
- internal/forge/fake.go: keep FakeClient.ListRepositoryFiles
- internal/forge/github/github_test.go: keep ListRepositoryFiles tests
- internal/scaffold/vendormanifest.go: discard main's naive ComparePathPresence
  (batch version lives in pathpresence.go)
- internal/scaffold/vendormanifest_test.go: discard main's naive tests
  (batch tests live in pathpresence_test.go)

Add tests to improve codecov patch coverage:
- TestIsTreeTruncated: cover the new error sentinel helper
- TestFakeClient_ListRepositoryFiles: cover FakeClient implementation
- TestFakeClient_ListRepositoryFiles_ConcurrentSafe: verify thread safety
- TestListRepositoryFiles_RefError: cover ref lookup failure path
- TestListRepositoryFiles_CommitError: cover commit fetch failure path
- TestListRepositoryFiles_TreeFetchError: cover tree fetch failure path

Addresses review feedback on #2360
@ifireball ifireball added the ok-to-test Allow e2e CI to run after maintainer review (must be re-applied after each push) label Jul 7, 2026
@ifireball
ifireball added this pull request to the merge queue Jul 7, 2026
Merged via the queue into main with commit fb53c86 Jul 7, 2026
20 of 21 checks passed
@ifireball
ifireball deleted the agent/2351-batch-path-presence branch July 7, 2026 12:26
@fullsend-ai-retro

fullsend-ai-retro Bot commented Jul 7, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 12:28 PM UTC · Completed 12:38 PM UTC
Commit: ee77525 · View workflow run →

@fullsend-ai-retro

Copy link
Copy Markdown

PR #2360 batched path-existence checks via the Git Trees API, replacing O(N) sequential GetFileContent calls with a single ListRepositoryFiles call. The code agent created the initial implementation on June 16, the review agent found valid findings (1 medium, 5 low, 4 info) within 15 minutes, and the PR then sat idle for 21 days until a human engaged on July 7. The human triggered two /fs-fix iterations: the first addressed review findings and added tests but did not resolve merge conflicts; the second properly merged main and added more coverage tests. The human approved and merged the same day with no additional comments beyond the review agent's findings.

Review quality was strong — the medium finding (missing ErrTreeTruncated sentinel for truncated Git Trees API responses) was a real correctness issue that the code agent should have caught by replicating the existing truncation handling already present in the same file's commitFilesTo method. The 21-day idle period adds evidence to #966 (surface agent-approved PRs awaiting human review). The fix agent's two-iteration pattern adds evidence to #2728 (fix agent should verify explicit success criteria). Skipping a dedicated proposal for each since those issues already capture the pattern well.

Proposals filed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Allow e2e CI to run after maintainer review (must be re-applied after each push) requires-manual-review Review requires human judgment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Vendor analyze: batch path existence checks instead of O(N) GetFileContent calls

1 participant