Repository navigation
Fix Git repository search root traversal - #4557
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR refactors Git repository search termination logic in ChangesGit Repository Search Termination Centralization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 16 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (16 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR fixes an infinite-loop memory leak in the Git repository search upward traversal (
Confidence Score: 4/5Safe to merge; the fix is minimal, empirically verified on the repro machine, and backed by a targeted regression test. The traversal termination logic is correct for all tested root-escape variants. The only open item is a missing comment explaining why Sources/TabManager.swift — the new helper's access level should be annotated to explain testability intent. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[startURL] --> B{Is directory?}
B -- No --> C[deleteLastPathComponent]
B -- Yes --> D
C --> D[Loop: check for .git]
D --> E{.git exists?}
E -- Yes --> F[Return ResolvedGitRepository]
E -- No --> G[parentURL = deletingLastPathComponent]
G --> H{shouldStopGitRepositorySearch?}
H --> I{rawParent == rawCurrent?}
I -- Yes --> J[return nil — stop]
I -- No --> K{standardize current == slash?}
K -- Yes --> J
K -- No --> L{standardize parent == standardize current?}
L -- Yes --> J
L -- No --> M[currentURL = parentURL]
M --> D
Reviews (1): Last reviewed commit: "Stop Git search at root-equivalent paths" | Re-trigger Greptile |
Summary
/can produce/..and continue upwardVerification
cmux-unitx86_64:GitRepositorySearchRootStopTests.testRootParentVariantsStopRepositorySearchfailed for/ -> /..and/.. -> /../...xcodebuild test -project cmux.xcodeproj -scheme cmux-unit -destination platform=macOS,arch=x86_64 -derivedDataPath /Users/austinwang/Library/Developer/Xcode/DerivedData/cmux-issue-4520-repro-memory-leak-unit-x86_64 -only-testing:cmuxTests/GitRepositorySearchRootStopTests.Repro evidence
Issue repro details and vmmap/sample evidence were posted at #4520 (comment).
Remote dev build
A fixed x86_64 dev app was copied to the macOS 15.7.5 repro host at
/Users/austinywang/Applications/cmux-dev-issue-4520-repro-memory-leak/cmux DEV issue-4520-repro-memory-leak.app.Zip SHA256:
3752598358b058553bbf339809441ebc218ea94c9cd251cd560382eb6de96933.Fixes #4520
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Adjusts filesystem traversal termination for Git repository detection, which could affect when repos are discovered across different Foundation URL behaviors. Change is small and covered by new unit tests for root/
/..edge cases.Overview
Fixes Git repository discovery to stop traversing when reaching root-equivalent paths (including older Foundation cases where
deletingLastPathComponent()can yield/..and continue upward), preventing unbounded parent-path growth.Refactors the stop condition into
TabManager.shouldStopGitRepositorySearch(...)and addsGitRepositorySearchRootStopTeststo lock in the expected behavior at/,/.., and a normal non-root directory.Reviewed by Cursor Bugbot for commit 3640031. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Stop Git repository search at root-equivalent paths to prevent walking to
/..and beyond. This fixes the memory growth loop in non‑Git workspaces (fixes #4520).TabManager.shouldStopGitRepositorySearch(...)to stop at/, when the standardized parent equals the current path, or when Foundation returns/..-style parents./ -> /.. -> /../..traversal and to ensure normal parent traversal still works.Written for commit 3640031. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
Bug Fixes
Tests