Skip to content

fix(git): keep merge-tree's throwaway trees out of the real object store - #22356

Merged
brennanb2025 merged 27 commits into
mainfrom
brennanb2025/merge-tree-temp-object-dir
Oct 15, 2026
Merged

brennanb2025 merged 27 commits into
mainfrom
brennanb2025/merge-tree-temp-object-dir

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 9 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​951 $\color{#cf222e}{\Huge{\mathbf{−}}}$​5 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​946
Prod 9 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​493 $\color{#cf222e}{\Huge{\mathbf{−}}}$​21 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​472

What this fixes

Orca's background Git checks were permanently growing the user's repository with thousands of junk objects every hour. This PR stops that without changing anything the user sees.

  • Who hits it: anyone with a Git repository in Orca, in two places. (a) The pull request card, and the check before merging a PR, list the files a conflicting PR conflicts on. (b) Before deleting a workspace's branch, Orca checks whether that branch was already squash-merged.
  • Where: both checks run on the machine that runs the repository's Git: this computer, WSL, or the remote Orca server that SSH repositories now run on.
  • Not affected: folder workspaces (no Git), the conflict yes/no from GitHub, every other Git command, and what the pull request card and branch cleanup show.
  • Not a recent regression: the conflict check has written these objects since at least Orca 1.0.73. It became visible on 2026-09-28 while investigating slow workspace creation (fix(worktree): a create does one fetch and one checkout, and background work waits for it #23699).

ELI5

To list a pull request's conflicting files, and to check whether a deleted workspace's branch was already squash-merged, Orca does a "practice merge". Git saves every practice result into the repository's permanent storage, even though Orca throws it away, so the repository keeps growing with junk. Now each practice merge writes into a scratch folder instead, and Orca deletes that folder as soon as Git finishes.

What Changed

The problem. On 2026-09-28 the Orca repository on a maintainer's machine gained 18,048 new loose Git objects in one hour and 184,293 in 24 hours; 84% of the sampled new files contained conflict markers. That repository has automatic garbage collection off (gc.auto=0, the user's choice), so nothing removed them.

Why it happened. Both checks ask Git with git merge-tree --write-tree. Git has no mode that reports the result without also saving the merged files and folder listings into .git/objects. Orca only reads the answer, so every check leaves objects nothing will ever reference. main still does this in src/main/github/conflict-summary.ts and src/shared/git-branch-cleanup.ts; its cache only makes it happen less often.

What is different for the user. Same results as before. The practice merges no longer add to the repository's permanent storage, apart from the first check per machine per Orca session, which still writes its few objects while Orca learns the Git version. Objects written before this change stay until Git's normal cleanup removes them.

Mechanism. src/shared/git-object-quarantine.ts runs one Git command with two standard Git variables:

  • GIT_OBJECT_DIRECTORY points at a fresh scratch folder, objects/tmp_objdir-orca-merge-tree-XXXXXX, so every write goes there.
  • GIT_ALTERNATE_OBJECT_DIRECTORIES points at the real objects folder, so Git still reads every existing object. An inherited value is kept, with the real folder added after it, as Git does for its own scratch folders.
  • Afterwards, in a partial clone (a clone with --filter, where Git downloads file contents on demand), contents downloaded during the check are moved into the real objects/pack, index file last, leaving any file already there. Then the scratch folder is deleted (with the existing Windows retry). A failed move or delete is logged and never changes the check's result.

src/main/git/local-git-object-quarantine.ts finds the objects folder with the existing readRepoCommonDirFromGit, for native Git and WSL Git (it creates the folder through the Windows form of the path and hands WSL Git the Linux form, /mnt/c/... for a Windows drive). Both callers use it: the conflict list (both merge-tree forms) and branch cleanup (worktree-branch-removal.ts, which marks only its merge-tree call discardWrittenObjects).

WSL. WSL Git for these commands starts through the user's login shell. The runner now puts both variables on WSL Git's command line, the same way it already passes Git's output language, and appends the real folder to any alternates the shell profile set. Before this round they travelled through WSLENV, so a profile that set GIT_ALTERNATE_OBJECT_DIRECTORIES replaced Orca's value and the check failed where main succeeds; that failure is gone.

When the scratch folder is not used (the command runs exactly as on main):

  • until an earlier check on the same machine in the same session (including one that found a conflict) showed this Git supports merge-tree --write-tree (Git 2.38+), so Git older than 2.38 never gets a scratch folder, and Git older than 2.35, which cannot delete a leftover one, never sees one;
  • when Orca's environment already sets GIT_OBJECT_DIRECTORY, or the repository borrows objects from another (objects/info/alternates);
  • on the WSL linked-worktree route that falls back to Windows Git;
  • when the objects folder cannot be found or the scratch folder cannot be created (bookkeeping never blocks the check).

Leftover folders. A crash can leave one scratch folder (measured: 0 to 23 objects, under 320 KB), where main leaves that much on every check. git gc deletes it after two weeks, as it does Git's own; in a repository with automatic cleanup turned off it stays until the user runs git gc, like Git's own.

This round: SSH relay part removed. main deleted the SSH relay and its Git handlers (#27215, #27644); SSH repositories now run Orca's main-process code on a remote Orca server, so they get this fix through the same code as local repositories. The relay adapter, its Git-handler plumbing and relay-only tests are gone from this PR; nothing moved elsewhere.

Why

Both variables exist in Git 2.25; the fallback for Git older than 2.38 is unchanged.

Differences from the common pattern

The common pattern is Git's own handling of objects it may throw away: a tmp_objdir-* folder inside objects/, GIT_OBJECT_DIRECTORY pointed at it, the real store appended to GIT_ALTERNATE_OBJECT_DIRECTORIES, the folder deleted after use, and crash leftovers deleted by git gc after two weeks. This PR uses that as is. The conflicting-file list before a merge and the squash-merge proof before deleting a branch are existing Orca features with no counterpart in the common pattern; this PR leaves both unchanged.

  • Intended: no GIT_QUARANTINE_PATH. That variable refuses branch and tag updates while objects sit in a scratch folder; these commands never update either.
  • Intended: the scratch folder is used only once Orca knows this Git supports the command. Git uses its own on every run. Safe because the other runs are exactly main; the cost is the first check per session leaving its few objects.
  • Intended: if the scratch folder cannot be set up, the check runs as before. Git's own practice merge stops with an error instead; Orca never blocks the check on bookkeeping.
  • Intended: repositories that borrow objects (objects/info/alternates) or an inherited GIT_OBJECT_DIRECTORY run as before. Git's chain of borrowed stores stops after five levels, and a scratch folder adds one; running as before keeps main's result.
  • Intended: in a partial clone, downloaded contents are moved into the real store. Git's own download writes there directly; Orca's Git child passes the scratch folder on to its download, so Orca moves it afterwards. Without this, every check would download again and fail offline where main succeeds.
  • Resolved this round: a WSL shell profile that sets GIT_ALTERNATE_OBJECT_DIRECTORIES. Previously labeled Intended (the check failed where main succeeds). Now the variables are set on WSL Git's command line after the profile, appending to its value, so the profile no longer breaks the check.
  • Intended: a WSL shell profile that exports GIT_OBJECT_DIRECTORY is overridden for these two checks. Native Git skips the scratch folder when that variable is inherited; inside WSL the profile runs after Orca decides. Safe in practice because such a profile points every repository at one object store and breaks Git everywhere else.

Not in this PR

  • main's cleanup decisions are kept exactly, including one known case where main deletes a branch it should keep (a bare-repository project whose HEAD points at the branch being cleaned up); fixing that changes behaviour and is tracked separately.

Linked Issue

No tracking issue. Found while investigating slow workspace creation (#23699). Related to #22344 (idle packing of loose objects).

Visual Proof

This round changes runtime behaviour only on the WSL route (how the two variables reach WSL Git), so the earlier macOS live QA on a second Mac stands and the WSL route was re-checked on a real Windows machine (see Testing). The screenshots below are from earlier heads.

No UI change: the app looks identical before and after; the difference is on disk. macOS (earlier round): live QA ran the real app on a second Mac (BEFORE = main 2d358df, AFTER = this PR 610627a, same test repository starting from 0 loose objects), deleting workspaces through the UI:

Build Delete workspace Loose objects in the repo Leftover scratch folders Branch
main feature-extra (unmerged changes) 0 → 2 none kept, with the "Worktree deleted, branch kept" notice
main feature-squashed (squash-merged) 2 → 2 none deleted
this PR feature-extra 0 → 0 none kept, same notice
this PR feature-squashed 0 → 0 none deleted

A watcher sampling .git/objects every 50-75 ms saw one short-lived scratch folder during each delete on this PR's build and none on main.

Not run live: the pull request card's conflict list, because it needs a GitHub login inside the test app, and test apps are not given credentials. It runs the same shared code, and the real-Git tests cover it.

main, deleting feature-extra (repository gains 2 loose objects):

main: worktree deleted, branch kept

this PR, deleting feature-extra (same notice, no new objects):

this PR: worktree deleted, branch kept

main, then this PR, deleting feature-squashed (branch deleted, no notice):

main: squash-merged branch deleted

this PR: squash-merged branch deleted

Windows and WSL (this round, PR head 558f13e vs main 3047353, Windows Git 2.55 and WSL Ubuntu 24.04 Git 2.43, each test repository starting from 0 loose objects):

Scenario main this PR
Windows repo (C:\...), delete feature-extra 0 → 2 loose objects, branch kept 0 → 0, branch kept; the scratch folder lived about 30 ms
Windows repo, delete feature-squashed branch deleted branch deleted, 0 new objects
Repo inside WSL (opened as \\wsl.localhost\Ubuntu-24.04\...), delete feature-extra 0 → 2 loose objects, branch kept 0 → 0, branch kept; the scratch folder appeared inside the distro's objects folder and was removed
Repo inside WSL, delete feature-squashed branch deleted branch deleted, 0 new objects

Both live rounds ran on earlier heads of this PR. The changes since then remove Orca's own leftover-folder cleanup, simplify the partial-clone move (which does nothing in these test repositories because they are not partial clones), and use the scratch folder only once Orca knows the Git supports the command, so the first check per machine per Orca session now writes its few objects as main does and later checks add 0. The comparison harness under Testing measures that on the current head.

Not run live on Windows: a C:\ repo whose project runtime is set to WSL. Deleting any workspace in that setup fails on main too ("Access denied: path resolves outside allowed directories") before Git runs, so it is a separate existing bug. That path, and the Git older than 2.31 case, are covered by unit tests.

Windows repo, main (branch kept, 2 new loose objects):

Windows main: worktree deleted, branch kept

Windows repo, this PR (same notice, no new objects):

Windows this PR: worktree deleted, branch kept

Repo inside WSL, main:

WSL main: worktree deleted, branch kept

Repo inside WSL, this PR:

WSL this PR: worktree deleted, branch kept

Testing

This round (head 240e830e819, after merging main 6ade7a5):

  • Comparison against main, real Git (a harness run outside the repo, not committed): 11 repository shapes (plain; spaces, unicode, :, " and \ in the path; linked worktree; inherited GIT_ALTERNATE_OBJECT_DIRECTORIES; inherited GIT_OBJECT_DIRECTORY; clone --shared; a 6-deep borrowed-object chain; read-only objects folder) plus a partial clone checked online, online again and offline. 104 branch-cleanup cases and 39 conflict-list cases per run, on Git 2.50.1 and Git 2.25.5, each with nothing yet known about the Git version and after one earlier check: 0 differences from main in all four runs. Loose objects added after the first check: main 10-11 per shape, this PR 0 (the three shapes that run as before by design: 10, same as main). No scratch folder left behind. On Git 2.25.5 neither adds objects and no scratch folder is created.

  • Real WSL on Windows (Ubuntu 24.04, Git 2.43, Orca's own Git runner through wsl.exe and the login shell), a repo inside WSL and a C:\ repo run with WSL Git: same conflict file list and the same branch-cleanup results as main; after the first check main added 1 loose object per repo, this PR 0, no scratch folder left.

  • src/main/git/command-runner/wsl-command-resolution-object-store.test.ts (new): runs the WSL command line in a real POSIX shell with and without a profile-set GIT_ALTERNATE_OBJECT_DIRECTORIES; Git sees the real store appended. Fails with the command-line assignments removed.

  • The PR's existing tests (real-Git loose-object counts for both checks from the main and a linked worktree, partial clone online and offline, cold capability cache; scratch-folder unit tests; Git 2.25/2.38/2.49 compatibility contract) pass; the relay suites were removed with the relay.

  • CI: green at 240e830e819 (19 passed, 15 skipped, 0 failed).

  • I manually tested these changes locally

  • Automated tests added/updated, or explained why not below

AI Disclosure

Written with Claude (Claude Code).

Review

Live app QA ran on macOS, Windows and WSL on earlier heads; this round re-checked the changed WSL route with Orca's real Git runner on Windows. Not tested by hand against a remote Orca server for an SSH repository; it runs the same main-process code as a local repository.

Agent skill upstream boundary

  • Not applicable, or this change follows docs/reference/agent-skill-sharing-upstream-boundary.md and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

  • Security: no new inputs. The scratch folder is created with mkdtemp inside the repository's own objects/ folder, and only paths mkdtemp returned are ever deleted.
  • Cross-platform: paths are built with the path rules that match the path's own syntax, because a Windows app drives Linux Git under WSL. The alternates value is a single path; Git splits it on : (; for Git for Windows), so it is quoted when the path contains that separator or a " (and on POSIX also a \). WSL Git gets both variables as assignments on its command line, the same way Orca passes Git's output language. Deletion retries briefly locked files on Windows.
  • SSH: SSH repositories run this code on the remote Orca server, which creates, moves and deletes the scratch folder on that machine. No request or response shape changes, so mixed versions are unaffected.
  • Folder workspaces: unaffected; these checks only run in Git repositories.
  • Mobile: not affected.
  • Compatibility: both variables exist in Git 2.25. Any failure to set up the scratch folder falls back to the old behaviour.
  • Performance: one extra rev-parse --git-common-dir per conflict summary or cleanup check, and a folder create and delete. The conflict summary is cached in memory per pair of head and base commits, so this normally runs again only when one of them moves (or after a failed check, which is retried after a minute).

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

The PR conflict summary and the squash-merged branch cleanup both run
`git merge-tree --write-tree` only to read its output, but every divergent
run left unreachable loose objects behind. Run it with a per-call scratch
object directory (inside objects/, alternates pointing at the real store)
that is deleted afterwards, on native, WSL and SSH relay hosts alike.
@coderabbitai

coderabbitai Bot commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 84c82d44-23e7-4aa3-bc97-1ecd9df2f767

📥 Commits

Reviewing files that changed from the base of the PR and between 0f3ed37 and d3c2df6.

📒 Files selected for processing (5)
  • src/main/git/local-git-object-quarantine.test.ts
  • src/main/git/local-git-object-quarantine.ts
  • src/main/github/conflict-summary.test.ts
  • src/shared/git-object-quarantine.test.ts
  • src/shared/git-object-quarantine.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.

📝 Walkthrough

The change adds a shared Git object quarantine runner and connects it to local and relay merge-tree operations. Commands can write objects to per-run scratch directories while reading existing objects through alternates. Complete fetched packs are retained in the real object store, and scratch directories are removed after execution. The change also adds local path and environment handling, relay per-command environment overrides, and unit and real-Git tests.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to d3c2d

The quarantine change has no established merge-blocking defect. Failed pack retention may require another fetch; the concurrency check did not demonstrate a broader failure.

Security architecture risk: 🔵 Low · up to d3c2d

The change reduces unnecessary repository growth and keeps branch-deletion safeguards intact. The main remaining risk is fetched-pack recovery during overlapping operations or interruption. No privilege escalation or unauthorized data access was established, but repository-path trust and cross-platform failure behavior are not fully verified.

Retained concerns

  • Low · reliability · inferred: Fetched-pack promotion lacks ownership-aware rollback across concurrent finalizers. If one finalizer installs a pack, another adopts it and publishes its index, and the first subsequently encounters an installation failure, its rollback can remove the shared pack. This weakens containment of cleanup failures and can require refetching promised objects. Repository ref corruption and an attacker-induced failure were not demonstrated.
Security review details

Security Blast Radius

  • inferred — The principal failure scope is the resolved repository object store, shared by its worktrees and potentially multiple application processes. Local and WSL operations use the local process's filesystem authority; relay cleanup uses the remote process's authority. Cross-tenant, cloud IAM or secret-authority expansion was not established by the inspected change.

Trust Boundaries and Controls

  • observed — Adapters resolve Git-reported common directories without enforcing containment under the worktree. This accommodates external common directories but makes repository metadata part of the filesystem trust assumption. Existing relay worktree operations already follow the common directory; the inspected evidence does not establish that a remote PR author can redirect it or obtain additional authority through this change.

Resilience and Maintainability Implications

  • observed — Source tests cover complete-pack preservation, sequential rollback after index-installation failure, and preservation of already installed packs. These are meaningful countermeasures, but the inspected tests do not exercise concurrent promotion and rollback ownership.

Hardening Proposals

  • proposed — Coordinate pack promotion across processes or use a no-clobber installation protocol with ownership-aware rollback. Validate recovery with concurrent finalizers, injected installation failures and interruption between pack and index publication.
Pre-merge checks: Passed 3 · Failed 2

Failed Failed checks: 2 warnings

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 30.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 30 functions across 20 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check Warning The description is detailed and covers the required explanation, implementation, rationale, testing, compatibility, security, performance, and checklist sections. However, the Linked Issue section exp… Add an actual linked issue in the required format, such as Fixes #12345. If the author is a maintainer and the repository policy permits omitting the issue, state that the maintainer exception applies.
Passed Passed checks: 3 passed
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Title check Passed The title clearly identifies the primary change: preventing merge-tree throwaway objects from accumulating in the real Git object store.
Full details: Description check

Explanation

The description is detailed and covers the required explanation, implementation, rationale, testing, compatibility, security, performance, and checklist sections. However, the Linked Issue section explicitly states that no tracking issue exists and does not provide the required actual issue link.

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@pullfrog pullfrog Bot 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.

Important

WSL Git on a Windows-drive repo with Git < 2.31 is handed a Windows-spelled GIT_OBJECT_DIRECTORY it cannot resolve, so merge-tree fails instead of falling back. Details inline.

Reviewed changes

  • Core quarantine module — new src/shared/git-object-quarantine.ts runs a Git command with GIT_OBJECT_DIRECTORY = a fresh scratch dir under objects/ and GIT_ALTERNATE_OBJECT_DIRECTORIES = the real object store, deleting the scratch dir in a finally; falls back to unquarantined when the objects dir or scratch dir is unavailable.
  • Local adapter — src/main/git/local-git-object-quarantine.ts resolves the common dir via readRepoCommonDirFromGit, translates Linux spellings to WSL UNC host paths, and adds both vars to WSLENV on Windows.
  • Relay adapter and env plumbing — src/relay/relay-git-object-quarantine.ts plus opts.env merged over buildRelayGitEnv() in git-handler.ts, with the type threaded through git-handler-ops.ts / git-handler-operation-context.ts.
  • Callers — conflict-summary.ts wraps both merge-tree invocations; git-branch-cleanup.ts marks its merge-tree call discardWrittenObjects: true, honored by worktree-branch-removal.ts and git-handler-branch-cleanup.ts.
  • Tests — real-Git fixture and loose-object-count tests (local + relay), plus unit tests for fallback/cleanup/quoting/WSLENV, and a branch-cleanup assertion that only merge-tree asks for the scratch store.

ℹ️ Nitpicks

  • alternateObjectDirectoriesValue leaves a Windows path that itself contains ; (the Windows separator) unquoted, because the shortcut keys off isWindowsAbsolutePathLike and the special-char regex is only /[:"\\]/. A ;-containing directory is legal NTFS and native Windows Git would then split it into two bogus entries. Rare enough that it may not be worth the added branching, but the shortcut assumes a separator it never checks.
  • The alternates-quoting branch is only asserted as the produced string; the real-Git fixtures live under mkdtemp in tmpdir (no :), so that branch is never exercised against the real Git parser. The scheme itself checks out against Git 2.25+, so this is coverage rather than a live defect.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Comment thread src/main/git/local-git-object-quarantine.ts
@greptile-apps

greptile-apps Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[High risk] Adds object store quarantine to Git merge operations.

The PR appears safe to merge, with the previously reported alternates limitation remaining non-blocking.

Findings

  1. P2 Alternates bypass the quarantine ▶

Summary

This PR routes throwaway git merge-tree writes to temporary object directories for local, WSL, and SSH checks while preserving conflict summaries and branch-cleanup decisions.

  • Keeps packs fetched by partial clones and removes scratch directories after checks.
  • Leaves repositories with objects/info/alternates unquarantined; the existing unresolved thread covers that limitation.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart LR
  Check[Conflict or branch-cleanup check] --> Route{Quarantine supported?}
  Route -->|Yes| Scratch[Run merge-tree with scratch object directory]
  Route -->|No| Ordinary[Run merge-tree normally]
  Scratch --> Packs[Keep fetched packs]
  Packs --> Cleanup[Remove scratch directory]
  Cleanup --> Result[Return check result]
  Ordinary --> Result
Loading

Reviews (13) · Last reviewed commit: "Merge remote-tracking branch 'origin/mai..."

Comment thread src/shared/git-object-quarantine.ts Outdated
Comment thread src/shared/git-object-quarantine.ts Outdated
…stores

- A partial clone fetches missing blobs during merge-tree; those packs landed in the
  scratch store and were deleted, so every check re-downloaded them. Move fetched packs
  (index last) into the real store before removing the scratch dir.
- Sweep this app's own scratch dirs older than an hour, once per objects dir per process,
  so a crash or failed delete cannot strand one forever in repos where gc never runs.
- Remove with Windows transient-lock retries; the shared core now owns all filesystem
  work and host adapters only resolve the objects dir.
Comment thread src/shared/git-object-quarantine.ts Outdated
Comment thread src/shared/git-object-quarantine.ts Outdated

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: b406813f-8d2a-46b8-8b84-bf9fb07b35a6

📥 Commits

Reviewing files that changed from the base of the PR and between 643f93f and 500da40.

📒 Files selected for processing (20)
  • src/main/git/local-git-object-quarantine.test.ts
  • src/main/git/local-git-object-quarantine.ts
  • src/main/git/merge-tree-object-quarantine-real-git.test.ts
  • src/main/git/worktree-branch-removal.ts
  • src/main/github/client-merge-queue-auto-merge.test.ts
  • src/main/github/client-pr-conflict-summary.test.ts
  • src/main/github/client-test-mocks.ts
  • src/main/github/conflict-summary.test.ts
  • src/main/github/conflict-summary.ts
  • src/relay/git-handler-branch-cleanup.ts
  • src/relay/git-handler-operation-context.ts
  • src/relay/git-handler-ops.ts
  • src/relay/git-handler.ts
  • src/relay/relay-git-object-quarantine-real-git.test.ts
  • src/relay/relay-git-object-quarantine.ts
  • src/shared/git-branch-cleanup.test.ts
  • src/shared/git-branch-cleanup.ts
  • src/shared/git-object-quarantine-real-git.test-fixture.ts
  • src/shared/git-object-quarantine.test.ts
  • src/shared/git-object-quarantine.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.

Comment thread src/shared/git-object-quarantine.ts

@pullfrog pullfrog Bot 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.

ℹ️ No new issues in this delta.

Reviewed changes

  • Partial-clone downloads preserved — keepFetchedPacks moves a pack Git lazily fetched into the scratch store (plus its .promisor file) into the real objects/pack, renaming .idx last, so the next check does not re-download the same blobs.
  • Stranded scratch stores swept — sweepStaleScratchDirectories deletes this app's own tmp_objdir-orca-merge-tree-* directories older than an hour, once per objects dir per process, leaving recent ones and Git's own tmp_objdir-* alone.
  • Host adapters simplified — the shared core now owns the filesystem work (mkdtemp/removeTree with Windows lock retries); the local and relay adapters only resolve the objects dir.
  • Tests — a real-Git partial-clone test proving the second check does not re-fetch, unit tests for the pack move and the stale sweep, and fixture helpers (createPartialClone, looseObjectCountIn, scratchDirectoriesIn).

Pullfrog  | Fix it ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot 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.

ℹ️ No new issues in this delta.

Reviewed changes

  • Windows alternates quoting — alternateObjectDirectoriesValue now quotes a Windows-spelled objects path containing ; (Git for Windows' separator) or ", so a legal-but-rare NTFS directory name is not split into two bogus alternate entries; POSIX paths keep the existing :/"/\ rule. Verified against Git source: odb.c:parse_alternates splits unquoted entries on PATH_SEP (: on POSIX, ; on Git for Windows) but routes a leading " through unquote_c_style, so a separator inside quotes is never split.
  • Cleanup failures now logged — a failed keepFetchedPacks rename and a failed scratch-dir removeTree now emit a console.warn with the path/pack and error, matching the neighbouring Git modules, instead of swallowing silently while the check result still stands.
  • Test coverage extended — a C:\a;b\.git\objects case pins the new quoted Windows spelling, while the plain C:\repo\... case still asserts an unquoted value; the old || shortcut left every Windows path unquoted, so the only behavior change for Windows paths is this ;/" case.

Pullfrog  | Fix it ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Status: ready for review (not merged)

Problem this fixes: Orca's pull request conflict check and its branch cleanup after deleting a workspace ran git merge-tree --write-tree, which saved every practice merge into the user's real .git/objects. On a maintainer's machine that was 18,048 junk objects in one hour and 184,293 in a day, never removed because that repository has garbage collection off.

What changes for the user: nothing visible. The conflict list, the merge check and branch cleanup give the same results; the repository simply stops growing from these checks.

What changed in this round (head 0f3ed37e90f, up to date with main):

  • Partial clones: file contents Git downloads during a check are now kept (moved into the real objects/pack, index last) instead of being thrown away and re-downloaded on every check.
  • A scratch folder stranded by a crash or failed delete is removed on the next check (Orca's own folders older than an hour, once per repository per Orca process), so it cannot sit forever in repositories where garbage collection never runs.
  • Deletion uses the existing Windows retry for briefly locked files; failed cleanup is logged and never changes a check's result.
  • Windows repository paths containing ; are quoted so Git does not split them.

Review:

  • An independent architecture review confirmed the approach: Git has no mode that lists conflicting files without writing objects, and pruning afterwards would cost more than the merge and race with the user's own Git work. It found the partial-clone and stranded-folder gaps fixed above.
  • Two review loops, both clean. Readiness checks at the start and end passed with no blockers.
  • The partial-clone test and the loose-object tests fail when their fix is removed.

Live QA on a second Mac, real app, main versus this PR: deleting a workspace with unmerged changes took the test repository from 0 to 2 loose objects on main and 0 to 0 with this PR. A planted 3-hour-old scratch folder was swept, a fresh one was left alone, and the notices and branch outcomes were identical. Screenshots are in the PR description.

Not verified:

  • The pull request card's conflict list was not exercised live, because it needs a GitHub login inside the test app and test apps are not given credentials. Real-Git tests cover it.
  • Nothing was tested by hand on Windows, WSL or a real SSH host; unit tests and the relay's real-Git test cover those paths.

Checks: CI green on 0f3ed37e90f (37 passed, 12 skipped). Node typecheck, lint and the changed-code quality gate pass locally.

…bly gone

Another Orca process on the same repo (a second instance, dev and release
builds) could delete a scratch object dir still in use once its mtime passed
an hour. Each scratch dir now records its owner (pid, hostname, platform,
Linux pid namespace); the sweep removes it only when that owner shares this
pid space and has exited, or, with no checkable owner, at Git's own two-week
tmp_ expiry.

Also:
- A fetched pack whose later rename fails is rolled back, so the real store
  never keeps an index-less pack; a pack already installed is left alone.
- WSL Git gets the /mnt/<drive> spelling of a drive-path common dir (Git
  < 2.31 prints it relative, resolved against the C:\ repo path).
- conflict-summary tests no longer create scratch dirs in the cwd.
Comment thread src/shared/git-object-quarantine.ts Outdated
… owner included

A reused pid held by a long-lived process could keep a leftover scratch dir
forever in a repo where git gc never runs. The two-week age (git gc's own
tmp_ expiry) now removes any scratch dir; a younger one is still swept only
when its owner shares this pid space and has exited.

@pullfrog pullfrog Bot 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.

✅ No new issues found.

Reviewed changes

  • WSL drive-path translation — localGitObjectsDirectory now hands WSL Git toLinuxPath(commonDir), so a Windows-drive repo whose common dir arrives Windows-spelled (Git < 2.31's relative .git) gets /mnt/c/... instead of an unresolvable C:\...; the host spelling stays Windows. This resolves the prior blocking review thread.
  • Owner-recorded scratch sweep with a two-week cap — each scratch dir carries orca-owner.json (pid, hostname, platform, pidNamespace?). A dir under the one-hour floor is kept; one at or past MAX_GIT_OBJECT_QUARANTINE_AGE_MS (14 days, Git's own tmp_* expiry) is removed regardless of owner; otherwise it is removed only when the owner shares this pid space and its pid is dead.
  • Pack import rollback and skip — keepFetchedPacks skips destination files that already exist and rolls back the files it installed if the .idx move fails.
  • Tests — an owner-liveness matrix (including the 15-day live-owner cap), sweep-once coverage, pack rollback/existing-pack cases, the WSL drive-spelling case, and a conflict-summary mock that prevents a real scratch dir being created.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot 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.

✅ No new issues found.

Reviewed changes

  • Two-week hard cap on every scratch dir — isStaleScratchDirectory now short-circuits to stale once a dir reaches MAX_GIT_OBJECT_QUARANTINE_AGE_MS (14 days, matching Git's own objects/tmp_* expiry), before the owner-liveness branch. This closes the one open edge where a dead owner's pid reused by a long-lived process would otherwise keep a leftover directory in a gc.auto=0 repository indefinitely. A test pins the live-owner-but-15-days-old case.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

… path

- The stale sweep now removes any scratch dir at least two weeks old, Git's
  own expiry for its objects/tmp_* dirs; no merge-tree runs that long. The
  owner file and pid-space probe are gone, and with them the cases where a
  sweeper misjudged another process's pid.
- The sweep no longer delays the user's check: it starts without being
  awaited and can only touch dirs far older than the current run's.
- A fetched pack's .keep never moves into the real store, so a transient one
  from an aborted lazy fetch cannot stop gc from repacking that pack.
- Tests: the legacy merge-tree fallback gets the scratch env; the local
  quarantine's WSL decisions (distro from a UNC path, drvfs spelling,
  host-Git linked-worktree skip, WSLENV hand-off).
Comment thread src/shared/git-object-quarantine.ts Outdated

@pullfrog pullfrog Bot 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.

✅ No new issues found.

Reviewed changes

Reviewed the delta since the prior pullfrog review (d3c2df63ff → 558f13e0cd): a single commit that simplifies the scratch object-store lifecycle.

  • Stale sweep reduced to age alone — the owner record (orca-owner.json, pid/hostname/pidNamespace) and its liveness probe were removed. A tmp_objdir-orca-merge-tree-* dir is now removed solely when its mtime is older than STALE_GIT_OBJECT_QUARANTINE_AGE_MS (14 days, matching Git's own objects/tmp_* expiry). An in-flight run's dir is always fresh, so the sweep can no longer delete a live one.
  • Sweep moved off the check's path — startSweepOnce fires the sweep without awaiting, tracked per objects dir in a Map<string, Promise>; a new test gates the objects/ listing to prove the Git command does not wait for it.
  • Fetched-pack import drops .keep — keepFetchedPacks excludes a transient .keep (an aborted lazy fetch can strand one, and gc never repacks a kept pack) while still moving .pack/.idx/.promisor with .idx last, skip-if-exists and rollback intact.
  • Test rework — the owner-liveness matrix became age-threshold/sweep-once cases; local-git-object-quarantine.test.ts gained Windows-host routing tests (WSLENV forwarding, drive-path /mnt/c spelling, linked-worktree fallback) through a mocked shared module; conflict-summary.test.ts accounts for the extra rev-parse --git-common-dir read and covers the legacy merge-tree fallback against a real scratch store.

Verified locally: the three changed suites pass (43 tests) and oxlint is clean.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@brennanb2025

brennanb2025 commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor Author

Status: ready for review (head f7d0cc0)

The problem. Orca's pull request conflict check and its squash-merged-branch cleanup both run git merge-tree --write-tree. Git saves every one of those practice merges into the repository's permanent .git/objects, even though Orca only reads the output. On a maintainer's repository with automatic garbage collection off, that added 184,293 junk objects in one day.

What changes for the user. Nothing visible: the pull request card and branch cleanup give the same results as main. The repository simply stops growing from Orca's checks, because each check writes into a scratch folder inside objects/ that is deleted as soon as Git finishes.

Changed in this round: simpler, closer to Git's own handling. The description listed ten differences from the common pattern; three extra mechanisms were removed or cut down:

  • Orca's own 14-day cleanup of leftover scratch folders is gone. A folder left by a crash is now removed by git gc, exactly as Git removes its own leftover temporary folders.
  • In a partial clone, contents Git downloads during a check are still moved into the real store (otherwise every check would download them again, and fail offline where main succeeds). The move is now the plain form Git uses: index file last, any file already present left alone. The undo of a half-finished move and the special handling of .keep files are gone.
  • The note about a cancelled check is gone: nothing in Orca cancels these commands.
  • Found while reviewing the trim and fixed: Git older than 2.35 cannot delete a leftover scratch folder, and its git gc would then warn on every run and stop cleaning up automatically. Orca now uses the scratch folder only once it knows, from an earlier check on the same machine in the same session, that this Git supports merge-tree --write-tree (2.38 and newer). On older Git no scratch folder is ever created, exactly as on main. A check that finds a conflict counts as that proof, so machines reached over SSH, where branch cleanup is the only check, also get the scratch folder after their first check.
  • "Differences from the common pattern" in the description now separates the two existing Orca features this PR leaves unchanged (the conflicting-file list and the squash-merge proof) from the six intended differences it introduces, each with why it is safe.

Verified

  • A comparison harness runs the real main code and this PR's code against real Git 2.50.1 and 2.25.5: 45 branch-cleanup scenarios, 16 unusual repository shapes, and a partial clone checked online, online again, and offline, for both the conflict list and branch cleanup. Each runs once with nothing known about the Git version and once after an earlier check. Result: 0 differences from main. On Git 2.25.5 no scratch folder was created anywhere. On Git 2.50.1, once the version is known, this PR adds 0 loose objects where main adds them, downloads the same bytes in the partial clone, and lists the same files offline.
  • Tests: the PR's test files plus every relay git-handler suite, 44 files, 399 passed (one relay timing test, unchanged from main, timed out once under load and passed 3 of 3 runs on its own); git-binary-compatibility.test.ts 29/29 against real Git 2.25.5 and 2.50.1 (CI also runs it against 2.38.1 and 2.49.1). Typecheck (node and web), oxlint and the changed-code quality gate pass. CI: 46 passed, 14 skipped, 0 failed; the PR is mergeable with no conflicts.
  • Three fresh narrow reviews, one per change in this round. The first flagged the old-Git leftover above, now fixed. The second and third approved, with only minor notes left.
  • Earlier rounds: live QA on macOS and Windows (native and WSL) showed main adding 2 loose objects per unmerged-branch check and this PR adding 0, with identical branch outcomes. Those rounds ran before this round's changes. With them, the first check per machine per Orca session now writes its few objects as main does, and later checks add 0; the harness above measures exactly that.

Not verified

  • No new live app QA this round: the conflict list and branch-cleanup results are unchanged, and the harness compares them directly against main.
  • The pull request card's conflict list live, because it needs a GitHub login inside the test app, which test apps are not given. Real-Git tests and the harness cover it.
  • Windows and WSL were not re-run for this round's changes. The path handling there is unchanged.

…t sees without it

The scratch object store must not change what merge-tree can read, or the
conflict list and the squash-merged branch cleanup differ from main:

- An inherited GIT_ALTERNATE_OBJECT_DIRECTORIES (from Orca's or the SSH
  relay's environment) was replaced; it is now kept and the real store
  appended after it, as Git's own temporary object stores do.
- An inherited GIT_OBJECT_DIRECTORY is the store Git reads and writes, so the
  command now runs unquarantined. On the WSL route a Windows value only
  counts if WSLENV forwards it, and then the quarantine is skipped too.
- The real store's own info/alternates entries are listed before it, so an
  alternates chain is linked at the same depth as without the quarantine
  (a 6-deep chain no longer hits Git's nesting limit). An alternates file
  that cannot be read or holds a quoted entry runs unquarantined.

@pullfrog pullfrog Bot 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.

✅ No new issues found.

Reviewed changes

Reviewed the delta since the prior pullfrog review (558f13e0cd → 710b17dc): one substantive commit plus two origin/main syncs.

  • Quarantine now mirrors inherited object stores — a new inheritedObjectStore (shared) and inheritedLocalObjectStore (local/WSL) decide whether the scratch store can see exactly what Git would. An inherited GIT_OBJECT_DIRECTORY disables quarantine (it is the store Git both reads and writes); an inherited GIT_ALTERNATE_OBJECT_DIRECTORIES is passed through, and on Windows it is dropped when WSLENV would not forward it to the distro's Git.
  • The real store's own info/alternates are promoted into the scratch env — readRealStoreAlternates resolves each entry as Git does (absolute kept, relative joined to the objects path, C-quoted or unreadable files fall back to unquarantined), and quarantineAlternates lists them ahead of the real store so an alternates chain keeps the same nesting depth Git allows without the quarantine. The relay adapter feeds the same inherited state from the relay process env.
  • Tests — three real-Git cases pin reading objects reachable only through inherited alternates, an inherited GIT_OBJECT_DIRECTORY writing where Git would, and a six-deep info/alternates chain; unit tests cover the two wrappers, the WSLENV-forwarding decision, and the new quoting/ordering. All five quarantine suites pass locally (52 tests).

The default (no inherited variables, no info/alternates) path is unchanged: quarantineAlternates reduces to the previous single quoted real-store entry.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

…own alternates

Mirroring Git's alternates resolution could not match it in every shape: a
path back to the real store (mutual alternates, or an inherited alternates
value naming the repo itself) still linked its chain one level deeper, and a
NUL byte in info/alternates broke the spawn. When objects/info/alternates
exists, or cannot be checked, the command now runs exactly as before. Repos
cloned with --reference or --shared keep writing throwaway objects, as they
always have. Inherited alternates are still appended to, and an inherited
GIT_OBJECT_DIRECTORY still skips the quarantine.
Comment thread src/shared/git-object-quarantine.ts

@pullfrog pullfrog Bot 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.

✅ No new issues found.

Reviewed changes

Reviewed the delta since the prior pullfrog review (710b17dc → 3a96c33b): one commit that simplifies how the quarantine handles a real store with its own alternates.

  • Quarantine is skipped for a store with info/alternates — quarantineAlternates no longer reads and flattens the real store's info/alternates into the scratch env. It now only stats the file: if it exists, or stat fails for any reason other than ENOENT/ENOTDIR, the command runs unquarantined (the exact pre-PR command). The removed mirroring could still link the chain one level deeper (mutual alternates, or an inherited alternates value naming the repo itself), and a NUL byte in the file broke the spawn.
  • Tradeoff is bounded and documented — repos cloned with --reference/--shared (or any store carrying info/alternates) revert to leaving throwaway objects, as they did before this PR. The commit message states this. Inherited alternates are still appended to, and an inherited GIT_OBJECT_DIRECTORY still skips the quarantine.
  • Tests reworked around the new predicate — unit cases cover an existing file and an unreadable info/ (both unquarantined) plus a non-directory info (ENOTDIR → quarantined), alongside a UNC append case and a WSLENV-names-keys-without-values case; the real-Git six-deep chain test now asserts unquarantined writes instead of zero loose objects.

Verified locally: the four changed suites pass (65 tests) and oxlint is clean.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Status update: same results as main, proven (head 3a96c33)

What changed since the last update. We checked whether this PR could change any conflict or branch-cleanup result, using 45 real-Git scenarios compared directly against main on Git 2.50.1 and 2.25.5. It matched in every case except two, and both are now fixed:

  • If Orca's own environment already set GIT_ALTERNATE_OBJECT_DIRECTORIES, this PR used to replace it. It now adds the real object folder after the existing value, as Git does for its own temporary object folders. If the environment sets GIT_OBJECT_DIRECTORY, the command now runs exactly as before.
  • In a repository that borrows objects from another repository (objects/info/alternates, as after git clone --reference or --shared), a deep chain of borrowed folders could exceed Git's limit under the scratch folder. Those repositories now run exactly as before: they keep collecting throwaway objects, as they do today.
    After both fixes: 0 differences from main in all 45 scenarios and 16 extra edge cases, on both Git versions, with no junk objects or leftover folders.

What the user sees. Nothing different: the pull request card's conflicting-file list and branch cleanup after deleting a workspace behave exactly as on main. The repository just stops filling up with throwaway objects from those checks.

Plan. Removing these checks instead (#24266, #24269) was explored and set aside, because users rely on both features. This PR is now the whole fix for both places Orca runs git merge-tree.

Verified: a fresh independent review of the fixes, with the comparison harness re-run; 117 tests; node and web typecheck; lint. Earlier rounds verified Windows and WSL live.

Not in this PR: a known main bug in branch cleanup for a bare-repository project whose HEAD points at the branch being cleaned up (main deletes it even with unpushed commits). It is tracked separately, because fixing it changes behaviour.

Known limitation: on WSL, a shell profile that sets (rather than adds to) GIT_ALTERNATE_OBJECT_DIRECTORIES replaces Orca's value, so the check fails where main's would succeed. Explained in the description.

CI: red only on the static-analysis lint for src/main/native-chat/agent-session-wire/structured-agent-session-adapter.ts (too many lines). That file is identical on main and is fixed by #24323; I'll merge main once it lands.

…m merge-tree's scratch store

A scratch folder left by a crash is a tmp_* dir inside objects/, which
Git's own gc prunes after gc.pruneExpire, as it does Git's own leftovers,
so Orca's 14-day sweep and its test hooks go.

Packs a partial clone fetched into the scratch store are moved in Git's
migration order: pack- files only, index last, existing names left in
place, stopping at the first failed rename with no rollback. A failed
move is logged and never stops the scratch dir's removal or the result.

Adds a real-Git test that a partial clone lists its conflicts offline
once an earlier check downloaded the contents.

Co-Authored-By: Claude <noreply@anthropic.com>

@pullfrog pullfrog Bot 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.

Important

The pack-import rewrite drops two safeguards the PR description still guarantees: the per-pack rollback and the .keep exclusion. keepFetchedPacks still runs in finally (including after a failed or cancelled command) and no longer requires an .idx, so a failed .idx rename can leave an orphan pack pinned by its .keep in the real store. Details inline.

Reviewed changes

  • Stale scratch sweep removed — sweepStaleScratchDirectories / startSweepOnce / STALE_GIT_OBJECT_QUARANTINE_AGE_MS and the test hooks are gone; leftover tmp_objdir-orca-merge-tree-* dirs are now left to git gc.
  • Pack import simplified — keepFetchedPacks moves any pack-* file (.idx last), skipping existing targets and stopping at the first failed rename with no rollback; the failure is logged and the scratch dir is still removed.
  • New real-Git coverage — a partial clone whose contents an earlier check downloaded now lists its conflicts offline after the remote URL is made unreachable.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

Comment thread src/shared/git-object-quarantine.ts
Comment thread src/shared/git-object-quarantine.ts Outdated
…t --write-tree

Git before 2.35 cannot prune a leftover scratch folder under objects/tmp_*, so
a crash in the moment one exists makes every later `git gc` warn and stops
automatic cleanup. On Git before 2.38 merge-tree --write-tree is rejected, yet
the first check per host per session still made and removed a folder around it.

Branch cleanup and the PR conflict list now use the scratch object store only
when that host's capability cache already knows merge-tree --write-tree works.
Until then the command runs exactly as it does on main: on a Git that
rejects it no folder is ever created, and every Git that does get one is 2.38+,
whose gc prunes it. Also reword two comments in the quarantine module.

Co-Authored-By: Claude <noreply@anthropic.com>

@pullfrog pullfrog Bot 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.

ℹ️ No critical issues — minor suggestions inline.

Reviewed changes

Reviewed the delta since the prior pullfrog review (14c3c5039 → 2fae54e5d): one commit that gates the scratch object store on the host's capability cache.

  • Quarantine gated on known support — both callers now run merge-tree --write-tree unquarantined until isKnownSupported('merge-tree-write-tree') is true, so a Git that rejects --write-tree (Git < 2.38) or predates git gc's tmp_* pruning (Git < 2.35) never gets a scratch folder.
  • conflict-summary.ts reworked — a per-command runMergeTree closure consults the capability cache and is threaded through the legacy fallback, replacing the previous LocalGitObjectQuarantine parameter.
  • git-branch-cleanup.ts — branchMergesWithoutTreeChanges passes discardWrittenObjects from the same capability check instead of always true.
  • Tests — capability priming in the real-Git fixtures, new first-run-unquarantined/later-runs-quarantined cases for conflict-summary and branch cleanup, a binary-compatibility case pinning one scratch folder across two checks, and comment rewording in the quarantine module.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

Comment thread src/shared/git-branch-cleanup.ts
brennanb2025 and others added 3 commits October 2, 2026 16:46
…check

Branch cleanup quarantines merge-tree only once the host's capability cache
knows --write-tree works. A conflicting merge-tree exits 1, so the check threw
and support was never recorded; on a host whose only checks conflict (likely
over SSH, where the relay has no conflict summary to teach its cache) every
check kept writing loose objects into the real store.

A failure whose stdout starts with a tree ID proves --write-tree ran (old Git
that rejects the option prints nothing there), so it now resolves to the same
"not merged" result and lets the cache record support. Every other failure is
rethrown unchanged. Also shorten two comments and soften a third.

Co-Authored-By: Claude <noreply@anthropic.com>
…ee-objects-ready

# Conflicts:
#	src/shared/git-binary-compatibility.test.ts

@pullfrog pullfrog Bot 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.

ℹ️ No new issues in this delta — the new conflict-learning path is correct.

Reviewed changes

Reviewed the delta since the prior pullfrog review (2fae54e5d8 → b3ae921d94): one behavioral commit plus two origin/main syncs.

  • A conflicting check now teaches the capability cache — git-branch-cleanup.ts catches merge-tree --write-tree's conflict exit (exit 1) when its first stdout line is a 40/64-hex tree ID (printedMergedTreeId) and returns null instead of rethrowing, so runWithFallback records merge-tree-write-tree as supported and the next branch-cleanup check runs under the scratch object store. This closes the prior minor finding where a session whose only merge-tree calls conflicted stayed permanently unquarantined.
  • Tests — unit cases pin that a conflict carrying a SHA-1/SHA-256 tree ID flips the second check to discardWrittenObjects: true, while empty or non-tree stdout does not; a relay real-Git test removes a conflicting branch from a linked worktree twice and asserts the first check writes loose objects unquarantined, then the second makes exactly one scratch folder and writes none.
  • Comment rewording only — the capability-gating rationale in conflict-summary.ts and git-object-quarantine.ts tightened; no behavior change.

Verified locally: git-branch-cleanup.test.ts (16 tests) and relay-git-object-quarantine-real-git.test.ts (3 tests) pass on this head.

Pullfrog  | Fix it ➔ | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

…ee-objects-ready

# Conflicts:
#	src/main/git/worktree-branch-removal.ts
#	src/relay/git-handler.ts
#	src/shared/git-binary-compatibility.test.ts
…ee-objects-ready

# Conflicts:
#	src/main/git/worktree-branch-removal.ts
#	src/main/github/client-merge-queue-auto-merge.test.ts
#	src/main/github/client-pr-conflict-summary.test.ts
#	src/main/github/client-test-mocks.ts
#	src/relay/git-handler-branch-cleanup.ts
#	src/relay/git-handler-operation-context.ts
#	src/relay/git-handler-ops.ts
#	src/relay/git-handler.ts
#	src/shared/git-binary-compatibility.test.ts
#	src/wsl-guest/relay-git-object-quarantine-real-git.test.ts
#	src/wsl-guest/relay-git-object-quarantine.test.ts
#	src/wsl-guest/relay-git-object-quarantine.ts
A WSL login profile that set GIT_ALTERNATE_OBJECT_DIRECTORIES replaced the
WSLENV-forwarded value, so a quarantined merge-tree lost the real store and
failed where main succeeds. The WSL runner now passes both variables as
command-line assignments after the profile, appending to its alternates.
… already pin

The cold-capability gating, scratch-folder lifecycle and WSL path spelling
are covered end to end by merge-tree-object-quarantine-real-git.test.ts, the
real-binary compatibility contract (Git 2.25/2.38/2.49) and the
localGitObjectsDirectory tests, so the unit copies only mirrored them.
@brennanb2025
brennanb2025 merged commit 0fea878 into main Oct 15, 2026
34 checks passed
@brennanb2025
brennanb2025 deleted the brennanb2025/merge-tree-temp-object-dir branch October 15, 2026 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant