test(desktop): prove the .git directory marker race fails closed - #433
Merged
Conversation
An independent code review of git_dir_for_worktree's Directory branch flagged that it classifies the .git marker with a no-follow stat and returns a path, without itself opening or pinning that directory. The concern: if the marker is swapped from a real directory to a symlink between that classification and later reads, the returned path could redirect a snapshot's reads. The only production caller (GitInspectionRepository::snapshot) already closes this window: it opens the resolved Git directory with a no-follow (O_NOFOLLOW) handle as the very first step after resolving the path, before deriving or reading anything else from inside it. A marker swapped for a symlink in the interim causes that open to fail safely rather than being followed. Add an end-to-end regression test exercising this through GitInspectionRepository::snapshot (not just git_dir_for_worktree in isolation) so the guarantee is proven at the same layer external callers use, and document the reasoning inline at the Directory branch so a future change can't silently reopen the gap. Addresses a High-severity finding from an independent review agent following #426/#431.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new test’s explanatory comment claims to prove a snapshot-layer pin/open rejection that the current control flow doesn’t actually exercise (it fails earlier in .git marker classification), so the documentation should be corrected to match what is truly being tested.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR strengthens regression coverage and in-code reasoning around Git worktree .git marker handling in the desktop Rust Git inspection layer, aiming to ensure symlink-based marker scenarios fail closed at the GitInspectionRepository::snapshot entrypoint.
Changes:
- Adds an end-to-end-ish regression test that calls
GitInspectionRepository::snapshotand asserts a symlinked.gitdirectory marker is rejected. - Adds an inline comment in
git_dir_for_worktreedocumenting why returning a plain.gitdirectory path is safe given downstream pinning behavior.
File summaries
| File | Description |
|---|---|
| native/desktop/psyche-build-tauri/src-tauri/src/git_control.rs | Adds explanatory comment in .git directory-marker handling and a new regression test invoking GitInspectionRepository::snapshot for symlinked .git marker rejection. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Address automated review feedback: the regression test proves snapshot() rejects a .git directory marker that is already a symlink when routed through the full entrypoint (a fact the prior comment understated by overclaiming it specifically exercised a mid-flight race), while the narrower marker-swapped-mid-flight window is closed structurally by O_NOFOLLOW in open_directory_no_follow, independent of timing. Correct the inline comments to state this precisely instead of overclaiming what the test itself exercises.
BunsDev
added a commit
that referenced
this pull request
Sep 11, 2026
Addresses #426 (remaining scope after #431/#433) Follow the independent review by snapshotting the live index into the isolated inspection repository, writing alternates immediately before each Git subprocess handoff from a pinned objects-directory handle, pinning shallow/info snapshot reads to the opened common-directory identity, and preserving Unix directory enumeration errors during recursive ref traversal. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Outcome
An independent code-review agent flagged a High-severity TOCTOU concern in
git_dir_for_worktree'sDirectorymarker branch: it classifies.gitwith a no-followsymlink_metadata()stat and returns a plain path, without opening/pinning that directory itself. The theoretical risk: swapping.gitfor a symlink between that classification and later reads could redirect a snapshot.Analysis
The narrower race window (marker swapped strictly between classification and later reads) is closed structurally, independent of timing:
open_directory_no_followusesO_NOFOLLOW, soGitInspectionRepository::snapshot's pinnedgit_dir_handleopen (the first thing it does with the resolved path) fails closed on a symlink no matter when the swap happens. That guarantee doesn't depend on winning a race against an attacker; it's true by construction.Change
git_inspection_rejects_a_worktree_whose_git_directory_marker_is_a_symlink: an end-to-end regression test proving that a symlinked.gitdirectory marker is rejected when routed through the fullGitInspectionRepository::snapshotentrypoint (previously onlygit_dir_for_worktreein isolation had this coverage).O_NOFOLLOW's structural fail-closed behavior, not from this test's timing).(Revised after automated review feedback correctly pointed out the first draft's comment overclaimed what the test itself exercised versus what the code structurally guarantees.)
Risk class
Validation and evidence