Skip to content

feat(list): run merge analysis in a read-only object database - #3535

Merged
max-sixty merged 5 commits into
mainfrom
read-only-env
Jul 22, 2026
Merged

feat(list): run merge analysis in a read-only object database#3535
max-sixty merged 5 commits into
mainfrom
read-only-env

Conversation

@max-sixty

@max-sixty max-sixty commented Jul 22, 2026

Copy link
Copy Markdown
Owner

wt list's merge and conflict probes create ephemeral Git objects (write-tree, commit-tree, merge-tree --write-tree), so in a read-only checkout the analysis errored with insufficient permission for adding an object to repository database. This lets wt list and wt list statusline run their full merge analysis in a read-only object database (a managed sandbox) instead.

Approach

When the object database is read-only, Repository::redirect_objects_if_read_only returns a clone whose git plumbing points GIT_OBJECT_DIRECTORY at a temporary store, with the real database as a read-only alternate (GIT_ALTERNATE_OBJECT_DIRECTORIES). New objects land in the throwaway store; existing ones resolve through the alternate. Detection is a one-time writability probe (create a temp file in the object database), so the writable path is byte-for-byte unchanged and never redirects. Performance is worse in a read-only checkout (a fresh store per invocation, no cross-run object reuse), the accepted tradeoff.

The redirect is a per-command resource installed at each of the two observational list entry points: collect() (wt list) and populate_item() (wt list statusline, which renders on every Claude Code prompt inside exactly such a sandbox). It is deliberately not installed on mutating commands. Because a read-only object database also blocks the ref/index/worktree writes those commands need, keeping them on the persistent database makes them fail loudly rather than write a commit into a store that vanishes at process exit.

This is a lighter alternative to #3512: gating on read-only means only one object store is ever live per process, which removes that PR's parallel command API, the merge-tree cache-key change, and the task-skipping filter.

Testing

Unit tests cover the mechanism: a redirected merge tree is computed correctly but stays out of the real database, and a writable repo never redirects. Two integration tests freeze .git/objects read-only and assert the full merge analysis survives with no permission error, one via wt list --full (exercising the merge-tree and temp-index write-tree paths) and one via wt list statusline (the separate populate_item entry point).

This was written by Claude Code on behalf of max

🤖 Generated with Claude Code

max-sixty and others added 3 commits July 21, 2026 19:07
`wt list`'s merge and conflict probes write ephemeral Git objects
(write-tree, commit-tree, merge-tree --write-tree), so in a read-only
checkout the analysis errored with "insufficient permission for adding an
object to repository database". When the object database is read-only,
redirect those writes into a temporary object database with the real one
as a read-only alternate, so existing objects still resolve and the
analysis runs. Performance is worse there (a fresh store per invocation,
no cross-run object reuse), the accepted tradeoff.

The redirect is scoped to the observational list path only. Mutating
commands keep the persistent object database and fail loudly on a
read-only store rather than writing a commit into a throwaway that
vanishes at process exit.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Make redirect_objects_if_read_only / with_temporary_object_directory return
Option<Self> instead of Result: a failed temp-dir setup degrades to the real
database (`.ok()?`) rather than propagating, dropping the never-covered failure
closure in collect(). Drop the inherited GIT_ALTERNATE_OBJECT_DIRECTORIES
handling (the same rare git-alias case already dropped for GIT_OBJECT_DIRECTORY).
Rewrite the ephemerality unit test to read the merge tree via a raw git command,
removing the never-taken Conflict arm of the test helper.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@worktrunk-bot worktrunk-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewing as a draft — flagging anything that looks worth a quick fix. Mark ready for a full review.

The mechanism reads cleanly: probe-then-redirect, real DB as a read-only alternate, Arc<TemporaryObjectDirectory> so the temp store outlives every wt list task clone, and all three object-writing probes (merge-tree --write-tree, commit-tree, temp-index write-tree) route through wrapped runners. The unwrapped git spawns (rev-parse/config --list) are all read-only, so leaving them un-redirected is correct. Scoping mutating commands off the throwaway store is the right call.

One gap worth deciding on before this lands:

wt list statusline runs the same probes but bypasses the redirect. The redirect is installed in collect(), but the statusline path doesn't go through collect()git_status_segments (src/commands/statusline.rs) builds CollectOptions with show_full: true over all_columns() ("Same plan as the other statusline path: the full column set condensed") and calls list::populate_item(repo, ...) directly on a non-redirected repo. populate_item plans WorkingTreeConflictsTask (a write-tree against a temp index) and the merge-tree integration probe — the same object writes wt list --full makes, which the PR's own integration test exercises as the failing case.

So in the read-only object database this PR targets, wt list statusline --claude-code would still hit insufficient permission for adding an object — and that's the surface that renders on every Claude Code prompt redraw inside exactly such a managed sandbox, arguably the hottest read-only path of all. Worth either hoisting the redirect so both callers share it (e.g. into populate_item, or applying it in git_status_segments the way collect() does) or explicitly scoping statusline out with a note on why.

Minor: TempIndex::git (src/git/repository/working_tree.rs) re-implements the GIT_OBJECT_DIRECTORY / GIT_ALTERNATE_OBJECT_DIRECTORIES env application inline rather than sharing with_object_store_env. TempIndex can't hold a &Repository, so some copy is unavoidable, but a small free helper taking (cmd, &(PathBuf, OsString)) would keep the two sites from drifting.

@max-sixty
max-sixty marked this pull request as ready for review July 22, 2026 09:18

@worktrunk-bot worktrunk-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Full review now that this is out of draft. The core mechanism holds up well: probe-then-redirect, the real DB as a read-only alternate, Arc<TemporaryObjectDirectory> so the temp store outlives every task clone, and every object-writing spawn in the collect() path routes through a wrapped runner — merge-tree --write-tree and commit-tree via Repository::run_command_output, the temp-index write-tree via TempIndex::git and WorkingTree::run_command. I traced each object writer reachable from collect(); they're all covered, and the writable path is byte-for-byte unchanged. The integration test exercises the exact diverged+dirty failure case. Good.

One gap still stands from the draft note, and now that it's landing it's worth a decision rather than leaving it silent:

wt list statusline still bypasses the redirect. The redirect is installed in collect(), but neither statusline entry goes through collect()run_json and git_status_segments (src/commands/statusline.rs) both build CollectOptions with show_full: true over all_columns() and call list::populate_item(repo, …) directly on a non-redirected repo. populate_item plans the same object-writing tasks — the merge-tree integration probe and the temp-index write-tree conflict check — so in the read-only object database this PR targets, wt list statusline --claude-code would still hit insufficient permission for adding an object. That's the surface that renders on every Claude Code prompt redraw inside exactly such a managed sandbox, so it's arguably the hottest read-only path this change is meant to cover, yet it's the one caller left out (and the integration test only exercises --full).

The clean fix is to hoist the redirect into populate_item itself: it's reached only by those two statusline callers (collect() doesn't call it), so installing redirect_objects_if_read_only() there mirrors what collect() does without any risk of a double-redirect on the list path. If statusline is instead being deliberately deferred, a one-line note scoping it out (and why) would keep the next reader from assuming it's covered.

Happy to push that populate_item hoist as a commit if you'd like it in this PR — it's mechanical and mirrors the collect() change exactly. Otherwise leaving the verdict as a comment for you to decide scope.

wt list statusline is a separate entry point from collect() — its callers
(run_json, git_status_segments) call populate_item directly, which ran the
object-writing merge/conflict probes on a non-redirected repo. In a read-only
object database the probes failed and the statusline silently dropped
main_state and the integration symbols. Since statusline renders on every
Claude Code prompt inside exactly the managed read-only sandbox this targets,
that was the most important surface left out.

Install the redirect at the top of populate_item, mirroring collect(). Add a
regression test asserting statusline reports the diverged classification under
a read-only object database.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@max-sixty
max-sixty merged commit f14fa69 into main Jul 22, 2026
39 checks passed
@max-sixty
max-sixty deleted the read-only-env branch July 22, 2026 18:14
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.

2 participants