Repository navigation
Never take optional git locks when observing repos - #7179
lawrencecchen wants to merge 4 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a standalone git status provider, centralizes CLI git subprocess helpers, registers the new Swift files in the Xcode project, and adds a regression test that checks status polling does not rewrite the git index. ChangesGit status polling and lock handling
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (5 errors)
✅ Passed checks (20 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
01eef9f to
84aff72
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01eef9febd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let result = CLIProcessRunner.runProcess( | ||
| executablePath: "/usr/bin/env", | ||
| arguments: ["git", "-C", directory] + arguments, | ||
| arguments: ["GIT_OPTIONAL_LOCKS=0", "git", "-C", directory] + arguments, |
There was a problem hiding this comment.
For the cmux diff callers that reach this helper, this env var does not actually make the git diff invocation lock-free when the index stat cache is stale. I checked the same shape as gitDiffPatchArguments (GIT_OPTIONAL_LOCKS=0 git diff --no-ext-diff --no-color --binary --) with Git 2.43 after touching a tracked file, and .git/index was still rewritten, which means a transient index.lock is still taken during cmux diff --unstaged / branch / last-turn reads. The file-explorer status path is fixed, but the CLI diff path advertised in this change can still race user git operations during rebases.
Useful? React with 👍 / 👎.
Greptile SummaryFixes the
Confidence Score: 5/5Safe to merge — the change is narrowly scoped to observing commands, GIT_OPTIONAL_LOCKS=0 leaves required-lock operations (update-ref, stash create) entirely unaffected, and status/diff output is identical. Every observing git spawn in the app and CLI now consistently carries GIT_OPTIONAL_LOCKS=0. The approach is well-documented git behavior, the regression test directly proves the index is not rewritten, and the path-boundary fix is a genuine correctness improvement over the old hasPrefix check. The SSH command in Sources/GitStatusProvider.swift applies GIT_OPTIONAL_LOCKS=0 only to git status --porcelain, not to the preceding git rev-parse --show-toplevel — inconsistent with the local path, though rev-parse --show-toplevel does not take an optional index lock in practice. Important Files Changed
Reviews (6): Last reviewed commit: "Address review: dedup gitStdout overload..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let cleanStatus = GitStatusProvider.fetchStatus(directory: resolvedRoot) | ||
| #expect(cleanStatus.isEmpty) | ||
|
|
||
| // Status output must still be correct with the lock-free invocation. | ||
| try Data("changed\n".utf8).write(to: tracked) | ||
| let untracked = root.appendingPathComponent("untracked.txt") | ||
| try Data("new\n".utf8).write(to: untracked) | ||
| let dirtyStatus = GitStatusProvider.fetchStatus(directory: resolvedRoot) | ||
| #expect(dirtyStatus["\(resolvedRoot)/tracked.txt"] == .modified) | ||
| #expect(dirtyStatus["\(resolvedRoot)/untracked.txt"] == .untracked) | ||
|
|
||
| let indexAfter = try Data(contentsOf: indexURL) | ||
| #expect( | ||
| indexAfter == indexBefore, | ||
| "observing git status must not rewrite .git/index (it takes index.lock and races user git commands)" | ||
| ) | ||
| #expect(!fileManager.fileExists(atPath: root.appendingPathComponent(".git/index.lock").path)) |
There was a problem hiding this comment.
The PR description explicitly states this is a "two-commit structure: the first commit adds only the regression test… which fails on main." Only the test commit has been included; the second commit (setting GIT_OPTIONAL_LOCKS=0 in GitStatusProvider.runGit and --no-optional-locks in fetchStatusSSH) is absent. runGit in FileExplorerStore.swift still spawns /usr/bin/git status --porcelain with no GIT_OPTIONAL_LOCKS=0, so when the stat cache is made stale on line 38-41, git will rewrite .git/index, indexAfter != indexBefore, and this assertion fails on every CI run.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CLI/cmux_open.swift (1)
2578-2600: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFix is correct; consider centralizing the
GIT_OPTIONAL_LOCKS=0prefix to prevent future regressions.The
GIT_OPTIONAL_LOCKS=0env-var prefix is verified via git docs to only suppress optional locks (e.g. status/diff index refresh) — required locks likeupdate-ref's ref lock and stash's object creation are unaffected, so this change is functionally sound. All git read spawns in this file route throughgitStdout/gitStdoutDataor one of the 4 directCLIProcessRunner.runProcesscalls (cat-file ×2, stash create, update-ref -d ×2), and every one of them was updated — coverage looks complete.However, the literal
"GIT_OPTIONAL_LOCKS=0"is now duplicated 8 times across this file. A future git spawn added directly viaCLIProcessRunner.runProcess(bypassinggitStdout) could easily forget this prefix and silently reintroduce#4779. Consider extracting a small helper, e.g.:private func gitEnvArguments(_ arguments: [String]) -> [String] { ["GIT_OPTIONAL_LOCKS=0", "git"] + arguments }and using it at all 8 call sites (including the
-C <dir>andcat-file/stash/update-refones) so the safety prefix can't be omitted by accident.Also applies to: 2606-2625, 2627-2646, 2764-2779, 2840-2855, 3272-3290, 3387-3426
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CLI/cmux_open.swift` around lines 2578 - 2600, The `GIT_OPTIONAL_LOCKS=0` prefix is correct, but it is duplicated across `gitStdout`, `gitStdoutData`, and the direct `CLIProcessRunner.runProcess` git call sites, which makes future regressions likely. Extract a small helper in `cmux_open.swift` (for example around `gitStdout`/`gitStdoutData`) that builds the env-prefixed git arguments, and route every git spawn through it so the prefix is applied consistently for all read and direct git commands.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@CLI/cmux_open.swift`:
- Around line 2578-2600: The `GIT_OPTIONAL_LOCKS=0` prefix is correct, but it is
duplicated across `gitStdout`, `gitStdoutData`, and the direct
`CLIProcessRunner.runProcess` git call sites, which makes future regressions
likely. Extract a small helper in `cmux_open.swift` (for example around
`gitStdout`/`gitStdoutData`) that builds the env-prefixed git arguments, and
route every git spawn through it so the prefix is applied consistently for all
read and direct git commands.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 45744692-0bf5-42c9-b824-c68392f5db0c
📒 Files selected for processing (4)
CLI/cmux_open.swiftSources/FileExplorerStore.swiftcmux.xcodeproj/project.pbxprojcmuxTests/GitStatusProviderOptionalLocksTests.swift
84aff72 to
a8df526
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/GitStatusProviderOptionalLocksTests.swift`:
- Around line 90-94: SwiftLint is flagging the stdout decoding in the test
helper because `String(decoding:as:)` is used where the failable
`String(bytes:encoding:)` initializer is preferred. Update the return path in
the helper that reads from `pipe.fileHandleForReading` to decode with the UTF-8
failable initializer instead, keeping the same trimming behavior and leaving the
surrounding `process.waitUntilExit()` and `#expect` logic unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 48449780-6924-4d56-8af4-193fc686ced9
📒 Files selected for processing (2)
cmux.xcodeproj/project.pbxprojcmuxTests/GitStatusProviderOptionalLocksTests.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f14679442e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) -> [String: GitFileStatus] { | ||
| let escapedDir = directory.replacingOccurrences(of: "'", with: "'\\''") | ||
| let cmd = "cd '\(escapedDir)' 2>/dev/null && git rev-parse --show-toplevel 2>/dev/null && echo '---GIT_STATUS---' && git status --porcelain 2>/dev/null" | ||
| let cmd = "cd '\(escapedDir)' 2>/dev/null && git rev-parse --show-toplevel 2>/dev/null && echo '---GIT_STATUS---' && git --no-optional-locks status --porcelain 2>/dev/null" |
There was a problem hiding this comment.
On SSH hosts with Git older than the --no-optional-locks global option, this chained command exits non-zero after printing the delimiter, so runSSH returns nil and all remote file-explorer git badges disappear. The current git docs describe --no-optional-locks as equivalent to GIT_OPTIONAL_LOCKS=0, while the 2.14.6 synopsis omits that option, so prefer GIT_OPTIONAL_LOCKS=0 git status ... (old Git will just ignore the env var) or gate the flag by remote Git version.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/cmux_open.swift`:
- Around line 2578-2589: The `GIT_OPTIONAL_LOCKS=0` prefix is duplicated across
`gitStdout`, `gitStdoutData`, and the raw `CLIProcessRunner.runProcess` git call
sites, which makes it easy for a future git spawn to miss the lock-safety
setting. Centralize the construction of the git command arguments in a shared
helper (or wrap `runProcess`) and update the existing call sites to use it so
`GIT_OPTIONAL_LOCKS=0` stays a single source of truth.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 946ecbe0-81cb-4d2d-a7a0-098ebd78d192
📒 Files selected for processing (2)
CLI/cmux_open.swiftSources/FileExplorerStore.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…ndex The file explorer polls git status on FSEvents bursts. A bare git status opportunistically refreshes the stat cache and rewrites .git/index under .git/index.lock, so a concurrent user rebase/commit fails with 'Unable to create index.lock: File exists'. #4779 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fixes #4779 The file explorer refreshes git status on every FSEvents burst (0.3s throttle). A bare `git status` opportunistically rewrites .git/index under .git/index.lock, so during a rebase or commit the user's next git mutation fails with "Unable to create index.lock: File exists". Run every observing git spawn with GIT_OPTIONAL_LOCKS=0: - GitStatusProvider.runGit (file explorer local path) sets the env var - fetchStatusSSH inlines --no-optional-locks (env does not cross ssh) - cmux diff CLI git spawns get the env assignment via /usr/bin/env GIT_OPTIONAL_LOCKS=0 only skips opportunistic index writes; required locks (update-ref, stash create) still work. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
f146794 to
25b2aa2
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 25b2aa2. Configure here.
FileExplorerStore.swift and cmux_open.swift were both at their Swift file-length budget ceiling. Move the Git Status block (GitFileStatus + GitStatusProvider) to Sources/GitStatusProvider.swift and the diff CLI git subprocess helpers to CLI/CMUXCLI+GitProcess.swift. Mechanical moves, no behavior change; the moved CLI helpers drop 'private' since they are now called across files within the same target. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/CMUXCLI`+GitProcess.swift:
- Around line 27-70: The two CMUXCLI+GitProcess.gitStdout overloads duplicate
the same process-running logic, differing only in exit-status validation. Make
the simpler gitStdout(_:,in:,timeout:) delegate to the allowedExitStatuses-based
gitStdout(_:,in:,timeout:,allowedExitStatuses:) with the standard success status
set, so the command execution and timeout handling stay in one place and can’t
drift.
In `@Sources/GitStatusProvider.swift`:
- Around line 96-145: The issue is that GitStatusProvider’s subprocess helpers
can block forever because runGit and runSSH call Process.run() and
waitUntilExit() without any timeout. Update these helpers to use a bounded
process runner or equivalent timeout logic, ideally reusing the same approach as
CLIProcessRunner from the CLI git process code. Make the timeout handling apply
to both git and ssh paths, and ensure the methods still return nil on timeout or
process failure.
- Around line 59-60: The path filter in GitStatusProvider is using a raw
hasPrefix(explorerRoot) match, which can incorrectly include sibling paths that
only share the same string prefix. Update the checks in the repo-root path
handling and in markParentDirectories so they only accept paths that are exactly
explorerRoot or have a path separator boundary after it. Keep the same
normalization logic in both places that build absolutePath/current and compare
against explorerRoot.
- Around line 9-146: GitStatusProvider is currently only a static namespace, so
convert the type into an instance-based service that can be injected instead of
referenced globally. Update the GitStatusProvider API by making fetchStatus,
fetchStatusSSH, parseGitStatus, parseStatusChars, markParentDirectories,
gitRepoRoot, runGit, and runSSH instance methods as needed, then adjust the call
sites in FileExplorerStore and GitStatusProviderOptionalLocksTests to create/use
an injected GitStatusProvider instance rather than calling static methods.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f00865f5-ed67-4ea8-a0bb-b63261249898
📒 Files selected for processing (6)
CLI/CMUXCLI+GitProcess.swiftCLI/cmux_open.swiftSources/FileExplorerStore.swiftSources/GitStatusProvider.swiftcmux.xcodeproj/project.pbxprojcmuxTests/GitStatusProviderOptionalLocksTests.swift
💤 Files with no reviewable changes (1)
- Sources/FileExplorerStore.swift
| /// Runs `git status --porcelain` and parses results into a path-to-status map. | ||
| enum GitStatusProvider { | ||
|
|
||
| static func fetchStatus(directory: String) -> [String: GitFileStatus] { | ||
| guard let repoRoot = gitRepoRoot(for: directory) else { return [:] } | ||
| return parseGitStatus( | ||
| output: runGit(in: repoRoot, arguments: ["status", "--porcelain"]), | ||
| repoRoot: repoRoot, | ||
| explorerRoot: directory | ||
| ) | ||
| } | ||
|
|
||
| static func fetchStatusSSH( | ||
| directory: String, destination: String, port: Int?, | ||
| identityFile: String?, sshOptions: [String] | ||
| ) -> [String: GitFileStatus] { | ||
| let escapedDir = directory.replacingOccurrences(of: "'", with: "'\\''") | ||
| // GIT_OPTIONAL_LOCKS=0 rather than --no-optional-locks: remote hosts | ||
| // with git < 2.15 reject the unknown flag but ignore the env var. | ||
| let cmd = "cd '\(escapedDir)' 2>/dev/null && git rev-parse --show-toplevel 2>/dev/null && echo '---GIT_STATUS---' && GIT_OPTIONAL_LOCKS=0 git status --porcelain 2>/dev/null" | ||
| guard let output = runSSH( | ||
| command: cmd, destination: destination, | ||
| port: port, identityFile: identityFile, sshOptions: sshOptions | ||
| ) else { return [:] } | ||
|
|
||
| let parts = output.components(separatedBy: "---GIT_STATUS---\n") | ||
| guard parts.count == 2 else { return [:] } | ||
| let repoRoot = parts[0].trimmingCharacters(in: .whitespacesAndNewlines) | ||
| return parseGitStatus(output: parts[1], repoRoot: repoRoot, explorerRoot: directory) | ||
| } | ||
|
|
||
| private static func parseGitStatus( | ||
| output: String?, repoRoot: String, explorerRoot: String | ||
| ) -> [String: GitFileStatus] { | ||
| guard let output, !output.isEmpty else { return [:] } | ||
| var statusMap: [String: GitFileStatus] = [:] | ||
|
|
||
| for line in output.components(separatedBy: "\n") where line.count >= 4 { | ||
| let indexStatus = line[line.startIndex] | ||
| let workTreeStatus = line[line.index(after: line.startIndex)] | ||
| var path = String(line.dropFirst(3)) | ||
| .trimmingCharacters(in: .whitespaces) | ||
| .replacingOccurrences(of: "\"", with: "") | ||
|
|
||
| if path.contains(" -> ") { | ||
| path = String(path.split(separator: " -> ").last ?? Substring(path)) | ||
| } | ||
|
|
||
| guard let status = parseStatusChars(index: indexStatus, workTree: workTreeStatus) else { continue } | ||
|
|
||
| let absolutePath = repoRoot.hasSuffix("/") ? repoRoot + path : repoRoot + "/" + path | ||
| guard absolutePath.hasPrefix(explorerRoot) else { continue } | ||
|
|
||
| statusMap[absolutePath] = status | ||
| markParentDirectories(absolutePath: absolutePath, explorerRoot: explorerRoot, status: status, in: &statusMap) | ||
| } | ||
| return statusMap | ||
| } | ||
|
|
||
| private static func parseStatusChars(index: Character, workTree: Character) -> GitFileStatus? { | ||
| if index == "?" && workTree == "?" { return .untracked } | ||
| if index == "A" || workTree == "A" { return .added } | ||
| if index == "D" || workTree == "D" { return .deleted } | ||
| if index == "R" || workTree == "R" { return .renamed } | ||
| if index == "M" || workTree == "M" { return .modified } | ||
| return nil | ||
| } | ||
|
|
||
| private static func markParentDirectories( | ||
| absolutePath: String, explorerRoot: String, | ||
| status: GitFileStatus, in map: inout [String: GitFileStatus] | ||
| ) { | ||
| let dirStatus: GitFileStatus = (status == .untracked) ? .untracked : .modified | ||
| var current = (absolutePath as NSString).deletingLastPathComponent | ||
| while current.hasPrefix(explorerRoot) && current != explorerRoot { | ||
| if map[current] == nil { | ||
| map[current] = dirStatus | ||
| } | ||
| current = (current as NSString).deletingLastPathComponent | ||
| } | ||
| } | ||
|
|
||
| private static func gitRepoRoot(for directory: String) -> String? { | ||
| runGit(in: directory, arguments: ["rev-parse", "--show-toplevel"])? | ||
| .trimmingCharacters(in: .whitespacesAndNewlines) | ||
| } | ||
|
|
||
| private static func runGit(in directory: String, arguments: [String]) -> String? { | ||
| let process = Process() | ||
| process.executableURL = URL(fileURLWithPath: "/usr/bin/git") | ||
| process.arguments = arguments | ||
| // Observing a repo must never take .git/index.lock: a bare `git status` | ||
| // opportunistically rewrites the index under that lock and makes the | ||
| // user's own rebase/commit fail with "index.lock: File exists" (#4779). | ||
| process.environment = ProcessInfo.processInfo.environment | ||
| .merging(["GIT_OPTIONAL_LOCKS": "0"]) { _, new in new } | ||
| process.currentDirectoryURL = URL(fileURLWithPath: directory) | ||
| let pipe = Pipe() | ||
| process.standardOutput = pipe | ||
| process.standardError = FileHandle.nullDevice | ||
| do { | ||
| try process.run() | ||
| let data = pipe.fileHandleForReading.readDataToEndOfFileOrEmpty() | ||
| process.waitUntilExit() | ||
| guard process.terminationStatus == 0 else { return nil } | ||
| return String(data: data, encoding: .utf8) | ||
| } catch { | ||
| return nil | ||
| } | ||
| } | ||
|
|
||
| private static func runSSH( | ||
| command: String, destination: String, | ||
| port: Int?, identityFile: String?, sshOptions: [String] | ||
| ) -> String? { | ||
| let process = Process() | ||
| process.executableURL = URL(fileURLWithPath: "/usr/bin/ssh") | ||
| var args: [String] = [] | ||
| if let port { args += ["-p", String(port)] } | ||
| if let identityFile { args += ["-i", identityFile] } | ||
| for option in sshOptions { args += ["-o", option] } | ||
| args += ["-o", "BatchMode=yes", "-o", "ConnectTimeout=5", "-T"] | ||
| args += [destination, command] | ||
| process.arguments = args | ||
| let pipe = Pipe() | ||
| process.standardOutput = pipe | ||
| process.standardError = FileHandle.nullDevice | ||
| do { | ||
| try process.run() | ||
| let data = pipe.fileHandleForReading.readDataToEndOfFileOrEmpty() | ||
| process.waitUntilExit() | ||
| guard process.terminationStatus == 0 else { return nil } | ||
| return String(data: data, encoding: .utf8) | ||
| } catch { | ||
| return nil | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether CmuxGit package already contains status/porcelain parsing logic
# that GitStatusProvider.swift duplicates.
fd . Packages/macOS/CmuxGit --type f -e swift | xargs -I{} sh -c 'echo "== {} =="; cat {}' 2>/dev/null | head -400Repository: manaflow-ai/cmux
Length of output: 16497
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## GitStatusProvider definition and usages"
rg -n "GitStatusProvider|fetchStatusSSH|fetchStatus\\(" Sources Packages cmuxTests cmuxUITests --glob '!**/DerivedData/**' || true
echo
echo "## File location and size"
git ls-files Sources/GitStatusProvider.swift
wc -l Sources/GitStatusProvider.swift
echo
echo "## GitStatusProvider.swift around the full file"
cat -n Sources/GitStatusProvider.swift | sed -n '1,220p'
echo
echo "## Package boundary candidates in CmuxGit"
rg -n "GitStatus|status/porcelain|porcelain|GitFileStatusReading|GitFileStatusReader|GitMetadataService|GitTrackedChangesSnapshot" Packages/macOS/CmuxGit/Sources/CmuxGit -g '*.swift' || trueRepository: manaflow-ai/cmux
Length of output: 15953
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Context around call sites"
rg -n "fetchStatusSSH|fetchStatus\\(|GitStatusProvider" Sources cmuxTests cmuxUITests --glob '*.swift' || true
echo
echo "## Search for a protocol / injectable shape already used nearby"
rg -n "protocol .*Providing|protocol .*Reading|struct .*Service|class .*Service|actor .*Cache" Sources Packages/macOS/CmuxGit/Sources/CmuxGit --glob '*.swift' | sed -n '1,220p'Repository: manaflow-ai/cmux
Length of output: 2383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## GitStatusProvider usages"
rg -n "GitStatusProvider" Sources Packages cmuxTests cmuxUITests --glob '*.swift' || true
echo
echo "## Nearby injectable/service patterns in Sources"
rg -n "protocol .*Providing|protocol .*Reading|struct .*Service|class .*Service|actor .*Cache|init\\(" Sources --glob '*.swift' | sed -n '1,220p'
echo
echo "## GitStatusProvider.swift"
cat -n Sources/GitStatusProvider.swift | sed -n '1,220p'Repository: manaflow-ai/cmux
Length of output: 27163
Make GitStatusProvider a concrete service type. enum GitStatusProvider is just a caseless static namespace; switch it to an instance-based injectable type and update the small set of call sites in Sources/FileExplorerStore.swift and cmuxTests/GitStatusProviderOptionalLocksTests.swift.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/GitStatusProvider.swift` around lines 9 - 146, GitStatusProvider is
currently only a static namespace, so convert the type into an instance-based
service that can be injected instead of referenced globally. Update the
GitStatusProvider API by making fetchStatus, fetchStatusSSH, parseGitStatus,
parseStatusChars, markParentDirectories, gitRepoRoot, runGit, and runSSH
instance methods as needed, then adjust the call sites in FileExplorerStore and
GitStatusProviderOptionalLocksTests to create/use an injected GitStatusProvider
instance rather than calling static methods.
Source: Path instructions
| private static func runGit(in directory: String, arguments: [String]) -> String? { | ||
| let process = Process() | ||
| process.executableURL = URL(fileURLWithPath: "/usr/bin/git") | ||
| process.arguments = arguments | ||
| // Observing a repo must never take .git/index.lock: a bare `git status` | ||
| // opportunistically rewrites the index under that lock and makes the | ||
| // user's own rebase/commit fail with "index.lock: File exists" (#4779). | ||
| process.environment = ProcessInfo.processInfo.environment | ||
| .merging(["GIT_OPTIONAL_LOCKS": "0"]) { _, new in new } | ||
| process.currentDirectoryURL = URL(fileURLWithPath: directory) | ||
| let pipe = Pipe() | ||
| process.standardOutput = pipe | ||
| process.standardError = FileHandle.nullDevice | ||
| do { | ||
| try process.run() | ||
| let data = pipe.fileHandleForReading.readDataToEndOfFileOrEmpty() | ||
| process.waitUntilExit() | ||
| guard process.terminationStatus == 0 else { return nil } | ||
| return String(data: data, encoding: .utf8) | ||
| } catch { | ||
| return nil | ||
| } | ||
| } | ||
|
|
||
| private static func runSSH( | ||
| command: String, destination: String, | ||
| port: Int?, identityFile: String?, sshOptions: [String] | ||
| ) -> String? { | ||
| let process = Process() | ||
| process.executableURL = URL(fileURLWithPath: "/usr/bin/ssh") | ||
| var args: [String] = [] | ||
| if let port { args += ["-p", String(port)] } | ||
| if let identityFile { args += ["-i", identityFile] } | ||
| for option in sshOptions { args += ["-o", option] } | ||
| args += ["-o", "BatchMode=yes", "-o", "ConnectTimeout=5", "-T"] | ||
| args += [destination, command] | ||
| process.arguments = args | ||
| let pipe = Pipe() | ||
| process.standardOutput = pipe | ||
| process.standardError = FileHandle.nullDevice | ||
| do { | ||
| try process.run() | ||
| let data = pipe.fileHandleForReading.readDataToEndOfFileOrEmpty() | ||
| process.waitUntilExit() | ||
| guard process.terminationStatus == 0 else { return nil } | ||
| return String(data: data, encoding: .utf8) | ||
| } catch { | ||
| return nil | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
No timeout on the git/ssh subprocess calls — a hung git status blocks polling indefinitely.
runGit and runSSH call process.run() + waitUntilExit() with no deadline. If git status/git rev-parse (or the remote git over SSH) ever hangs — e.g., stuck behind another process's lock, a slow/hung filesystem, or a wedged SSH session — this call blocks forever with no recovery path, unlike the CLI's gitStdout in CMUXCLI+GitProcess.swift, which enforces a 60s timeout via CLIProcessRunner. This is exactly the "blocking calls without timeouts" hazard class.
#!/bin/bash
# Locate CLIProcessRunner to see if its timeout implementation is reusable
# from the main app target (Sources/) for GitStatusProvider.
rg -n --type=swift -C3 'struct CLIProcessRunner|class CLIProcessRunner|func runProcess' 🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/GitStatusProvider.swift` around lines 96 - 145, The issue is that
GitStatusProvider’s subprocess helpers can block forever because runGit and
runSSH call Process.run() and waitUntilExit() without any timeout. Update these
helpers to use a bounded process runner or equivalent timeout logic, ideally
reusing the same approach as CLIProcessRunner from the CLI git process code.
Make the timeout handling apply to both git and ssh paths, and ensure the
methods still return nil on timeout or process failure.
There was a problem hiding this comment.
5 issues found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="CLI/CMUXCLI+GitProcess.swift">
<violation number="1" location="CLI/CMUXCLI+GitProcess.swift:18">
P3: These new git failure messages can surface directly from the CLI without localization. The repository docs require CLI-visible strings to use `String(localized:defaultValue:)` with catalog entries, so these errors should be localized with the other supported locales.</violation>
</file>
<file name="Sources/GitStatusProvider.swift">
<violation number="1" location="Sources/GitStatusProvider.swift:10">
P2: This adds a caseless static namespace in production source, which the repository’s pre-merge Swift rules forbid under “No ambient global state.” Modeling this as an injectable owning type, or moving truly private helpers to file scope, would align with the project boundary rule.</violation>
<violation number="2" location="Sources/GitStatusProvider.swift:112">
P2: `runGit` and `runSSH` call `process.waitUntilExit()` with no deadline. If a git subprocess hangs (stuck behind a lock, slow filesystem, or wedged SSH session), polling blocks indefinitely with no recovery path. The CLI counterpart in `CMUXCLI+GitProcess.swift` enforces a 60s timeout via `CLIProcessRunner` for the same operations. Consider adding a timeout mechanism here as well to avoid permanently stalled file-explorer status updates.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| } | ||
|
|
||
| /// Runs `git status --porcelain` and parses results into a path-to-status map. | ||
| enum GitStatusProvider { |
There was a problem hiding this comment.
P2: This adds a caseless static namespace in production source, which the repository’s pre-merge Swift rules forbid under “No ambient global state.” Modeling this as an injectable owning type, or moving truly private helpers to file scope, would align with the project boundary rule.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/GitStatusProvider.swift, line 10:
<comment>This adds a caseless static namespace in production source, which the repository’s pre-merge Swift rules forbid under “No ambient global state.” Modeling this as an injectable owning type, or moving truly private helpers to file scope, would align with the project boundary rule.</comment>
<file context>
@@ -0,0 +1,146 @@
+}
+
+/// Runs `git status --porcelain` and parses results into a path-to-status map.
+enum GitStatusProvider {
+
+ static func fetchStatus(directory: String) -> [String: GitFileStatus] {
</file context>
| do { | ||
| try process.run() | ||
| let data = pipe.fileHandleForReading.readDataToEndOfFileOrEmpty() | ||
| process.waitUntilExit() |
There was a problem hiding this comment.
P2: runGit and runSSH call process.waitUntilExit() with no deadline. If a git subprocess hangs (stuck behind a lock, slow filesystem, or wedged SSH session), polling blocks indefinitely with no recovery path. The CLI counterpart in CMUXCLI+GitProcess.swift enforces a 60s timeout via CLIProcessRunner for the same operations. Consider adding a timeout mechanism here as well to avoid permanently stalled file-explorer status updates.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/GitStatusProvider.swift, line 112:
<comment>`runGit` and `runSSH` call `process.waitUntilExit()` with no deadline. If a git subprocess hangs (stuck behind a lock, slow filesystem, or wedged SSH session), polling blocks indefinitely with no recovery path. The CLI counterpart in `CMUXCLI+GitProcess.swift` enforces a 60s timeout via `CLIProcessRunner` for the same operations. Consider adding a timeout mechanism here as well to avoid permanently stalled file-explorer status updates.</comment>
<file context>
@@ -0,0 +1,146 @@
+ do {
+ try process.run()
+ let data = pipe.fileHandleForReading.readDataToEndOfFileOrEmpty()
+ process.waitUntilExit()
+ guard process.terminationStatus == 0 else { return nil }
+ return String(data: data, encoding: .utf8)
</file context>
| .map(String.init)? | ||
| .trimmingCharacters(in: .whitespacesAndNewlines), | ||
| !line.isEmpty else { | ||
| throw CLIError(message: "git returned empty output for \(arguments.joined(separator: " "))") |
There was a problem hiding this comment.
P3: These new git failure messages can surface directly from the CLI without localization. The repository docs require CLI-visible strings to use String(localized:defaultValue:) with catalog entries, so these errors should be localized with the other supported locales.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At CLI/CMUXCLI+GitProcess.swift, line 18:
<comment>These new git failure messages can surface directly from the CLI without localization. The repository docs require CLI-visible strings to use `String(localized:defaultValue:)` with catalog entries, so these errors should be localized with the other supported locales.</comment>
<file context>
@@ -0,0 +1,97 @@
+ .map(String.init)?
+ .trimmingCharacters(in: .whitespacesAndNewlines),
+ !line.isEmpty else {
+ throw CLIError(message: "git returned empty output for \(arguments.joined(separator: " "))")
+ }
+ return line
</file context>
The no-allowedExitStatuses gitStdout overload now delegates to the full one instead of duplicating its body. parseGitStatus and markParentDirectories match the explorer root at a path-component boundary so an explorer root of /repo/src no longer claims statuses under a sibling like /repo/srcOLD. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/GitStatusProvider.swift (2)
12-18: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winNormalize
explorerRootto Git’s reported root spelling before filtering.
parseGitStatusbuilds keys fromrepoRootreturned by Git, but filters them against the rawdirectory. The regression test already documents the/varvs/private/varmismatch; in productionFileExplorerStorepassesrootPathdirectly, so a symlink-spelled explorer root can make every status failisPath(...). Derive the explorer root fromrepoRoot + git rev-parse --show-prefixfor both local and SSH paths before callingparseGitStatus.Also applies to: 36-37
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/GitStatusProvider.swift` around lines 12 - 18, `fetchStatus(directory:)` is passing the raw explorer root into `parseGitStatus`, which can mismatch Git’s canonical root spelling and break `isPath` filtering. Normalize the explorer root to the same path spelling Git uses by deriving it from `repoRoot` plus `git rev-parse --show-prefix` for both local and SSH cases before calling `parseGitStatus`, so `GitStatusProvider` and `FileExplorerStore` compare consistent paths.
53-55: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winReplace
split(separator:)withcomponents(separatedBy:).split(separator:)only accepts a singleCharacter, so" -> "won’t type-check here;path.components(separatedBy: " -> ").last ?? pathis the direct fix.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/GitStatusProvider.swift` around lines 53 - 55, The path parsing in GitStatusProvider’s handling of renames uses split(separator:) with a multi-character string, which won’t type-check. Update the logic in the path normalization block to use components(separatedBy:) on the " -> " delimiter and take the last component with a fallback to the original path, keeping the existing path variable flow intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Sources/GitStatusProvider.swift`:
- Around line 12-18: `fetchStatus(directory:)` is passing the raw explorer root
into `parseGitStatus`, which can mismatch Git’s canonical root spelling and
break `isPath` filtering. Normalize the explorer root to the same path spelling
Git uses by deriving it from `repoRoot` plus `git rev-parse --show-prefix` for
both local and SSH cases before calling `parseGitStatus`, so `GitStatusProvider`
and `FileExplorerStore` compare consistent paths.
- Around line 53-55: The path parsing in GitStatusProvider’s handling of renames
uses split(separator:) with a multi-character string, which won’t type-check.
Update the logic in the path normalization block to use components(separatedBy:)
on the " -> " delimiter and take the last component with a fallback to the
original path, keeping the existing path variable flow intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e6960553-7557-4f05-b8b7-9df7d3c7f2bc
📒 Files selected for processing (2)
CLI/CMUXCLI+GitProcess.swiftSources/GitStatusProvider.swift
|
Review responses. Applied: Declined as pre-existing traits of moved code, out of scope for this fix: converting Verification at the final SHAs on the AWS M4 Pro runner: test-only commit |
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Fixes #4779
The file explorer refreshes git status on every FSEvents burst (0.3s throttle in
FileExplorerStore.updateDirectoryWatcher). A baregit statusopportunistically refreshes the stat cache and rewrites.git/indexunder.git/index.lock. During a rebase the worktree churns, each change fires the watcher, and the user's next git mutation fails with "Unable to create '.git/index.lock': File exists" — exactly the repro in the issue. The sidebar metadata watcher was already fixed to parse.git/indexin-process (#2797), but the file explorer path and the diff CLI still spawn lock-taking git.Fix: every git spawn cmux uses to observe a repo runs with
GIT_OPTIONAL_LOCKS=0.GitStatusProvider.runGitsets it in the child environment (coversstatus --porcelainandrev-parse).fetchStatusSSHprefixes the remote command withGIT_OPTIONAL_LOCKS=0(env var rather than--no-optional-locksso remote hosts with git < 2.15 keep working — the flag is rejected by old git, the env var is ignored).cmux diffCLI git spawns (git diffrefreshes the index the same way) go through onegitEnvArgumentsbuilder so a future call site can't omit the variable.GIT_OPTIONAL_LOCKS=0only skips opportunistic index writes; commands that require locks (update-ref,stash create) behave unchanged, and status/diff output is identical.Two-commit structure (test first, fix second). The
testsjob currently swallows app-host Swift Testing failures in both directions (evidence posted on #5641), so red/green was proven on the AWS M4 Pro runner at the final SHAs:0cda429887:** TEST FAILED **with exactly one issue,indexAfter == indexBefore— baregit statusrewrote the index.25b2aa25db:** TEST SUCCEEDED **.Live verification on the tagged build (
gitlk): Files sidebar open on a scratch repo, 15 FSEvents churn cycles touching tracked files —.git/indexmd5 unchanged throughout andindex.locknever appeared.Overlaps with #4805 (SpencerJung) but also covers the SSH path and the diff CLI, and adds the regression test.
🤖 Generated with Claude Code
Note
Medium Risk
Touches hot paths (FSEvents-driven status, diff CLI) and changes which paths get git decorations via stricter explorer-root matching; behavior for required-lock git ops is intended unchanged.
Overview
Fixes #4779 by ensuring cmux never takes optional
.git/index.lockwhen it only observes a repo (file explorer status polls, diff CLI, remote SSH status).GitStatusProvidermoves out ofFileExplorerStoreinto its own file. Localgit status/rev-parsenow mergeGIT_OPTIONAL_LOCKS=0into the child environment; SSH uses the same env prefix instead of--no-optional-locksfor older remote git. Explorer path filtering switches from naivehasPrefixto component-boundary matching so sibling paths likesrcOLDare not attributed tosrc.Diff CLI git helpers move into
CMUXCLI+GitProcess, withgitEnvArgumentsprependingGIT_OPTIONAL_LOCKS=0via/usr/bin/envfor all stdout/diff paths; remaining ad-hoccat-file/stash create/update-refspawns incmux_open.swiftuse the same builder.Adds
GitStatusProviderOptionalLocksTeststo assert observing status does not rewrite.git/indexor leaveindex.lock.Reviewed by Cursor Bugbot for commit d0ba047. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor