fix(list): Keep merge analysis sandbox-safe

[reneleonhardt]

fix(list): Keep merge analysis sandbox-safe

🧢 Changes

• Run existing object-writing list probes in a scoped temporary Git object directory.
• Preserve conflict and integration facts when the real object database is read-only.
• Keep normal content-addressed and network-result caches warm across invocations.
• If temporary storage is unavailable, skip only merge analysis and explain why.

☕️ Reasoning

wt list is observational, but its existing merge and conflict probes create Git objects. That made otherwise useful inventory incomplete in managed read-only sandboxes and could leave generated analysis artifacts behind.

This removes a real-world agentic coding blocker: existing list analysis remains useful without elevation or persistent object-store changes.

🧪 Reproduction

With the repository object database read-only:

wt list --full --format=json

Before this fix, merge analysis could report:

working-tree-conflicts (error: unable to create temporary file: Operation not permitted)

✅ Verification

• Focused committed and dirty read-only-object-database regressions pass.
• Inherited-object-location, persistent-cache, temporary-storage-failure, and merge-tree-cache regressions pass.

Developed with carefully directed, manually reviewed AI assistance.

Co-Authored-By: GPT-5.6 Sol codex@openai.com

[worktrunk-bot]

Redirecting the object-writing plumbing (write-tree, commit-tree, merge-tree) into a temporary object DB is the right core fix for a read-only .git/objects, and the tests around it are thorough. One design concern before this lands, though — the change also disables wt list's persistent caches in every repo, not just read-only sandboxes.

collect() now wraps the repo in with_temporary_object_directory() unconditionally (src/commands/list/collect/mod.rs), and that call succeeds in any normal repo, so temp-object mode is the default for every wt list and every wt switch picker (both route through collect::collect). In that mode, write_json_for_repo and write_with_lru (src/cache.rs) skip all persistent writes — the disk sha_cache (ahead/behind, merge-base, merge-tree-conflicts, merge-add-probe, diff-stats, is-ancestor, has-added-changes), the CI-status cache, the summary cache, and the MaxPrNumber ratchet.

That contradicts two contracts this PR leaves in place and doesn't update:

• src/git/repository/mod.rs (Caching): "Expensive, worth persisting across invocations (merge-tree, patch-id, diff stats, ahead/behind) → the disk sha_cache; content-addressed by SHA, so never stale."
• The ahead_behind docstring in src/git/repository/diff.rs: "wt list populates this per branch (and in bulk … when most entries are cold); subsequent runs are pure cache reads."

The concrete cost: after this change, back-to-back wt list runs re-fork merge-base/merge-tree per row (the merge_base_by_sha docstring cites 10–25 ms/row on macOS) and re-fetch CI status over the network instead of reading the warm cache the previous run would have written — a regression on exactly the hot/network-sensitive paths CLAUDE.md's network section guards.

Root cause looks like a conflation of two orthogonal things: a read-only object database needs object writes redirected (which you've done), but the disk sha_cache lives under .git/wt/cache — a different location — and is content-addressed by SHA, so its entries are valid regardless of which object store computed them. There's no correctness reason to skip them. And write_json already "[d]egrades silently on any failure" (src/cache.rs), so a genuinely read-only .git/wt/cache never produced an error to begin with — the skip isn't buying sandbox-safety there either.

Suggested direction: keep the temp-object redirection scoped to the object-writing plumbing, but decouple persistent-cache skipping from uses_temporary_object_directory() — either drop the gate and rely on write_json's existing silent degradation, or gate on actual cache-dir writability. If disabling list's cross-run persistence is actually intended, the two docstrings above (and the network-behavior notes in CLAUDE.md) need updating to match — per CLAUDE.md, system docstrings must be kept current.

This is a maintainer design call rather than a clear-cut bug, so flagging for @max-sixty rather than blocking.

Anchors

• Unconditional wrap: let (temporary_object_repo, temporary_object_error) = match repo.with_temporary_object_directory() in collect() (src/commands/list/collect/mod.rs).
• The gate: write_json_for_repo and the early return in write_with_lru (src/cache.rs).
• Callers of collect::collect: src/commands/list/mod.rs (wt list) and src/commands/picker/mod.rs (wt switch picker).
• Statusline is unaffected — it calls list::populate_item(&repo, …) with the plain repo, so its CI-status writes still persist; only the collect() path changed. That asymmetry (statusline caches CI, list no longer does) is itself worth a second look.

[worktrunk-bot]

The rework addresses my earlier caching concern well — object-writing plumbing is now redirected into the temp object DB via the run_object_store_command helpers, the persistent sha_cache under .git/wt/cache is no longer gated off (the new test_list_temporary_object_store_preserves_persistent_cache_writes pins that), and the in-memory merge_tree cache is keyed by object_store_cache_id so temp and persistent stores don't hand each other tree SHAs that only exist in one store. The scoping (only WouldMergeAdd/MergeTreeConflicts/WorkingTreeConflicts skipped when the temp store can't be created, everything read-only still runs) reads correctly. Two CI failures to clear before this can land:

lint (cargo fmt) — one formatting diff in the new test, suggestion inline.

codecov/patch is red on the new plumbing. Uncovered added lines:

• src/git/repository/mod.rs — the GIT_OBJECT_DIRECTORY-already-set branch (rev-parse --git-path objects) and the GIT_ALTERNATE_OBJECT_DIRECTORIES inherit-and-append branch in with_temporary_object_directory, plus the command-failure Err arm in run_object_store_command.
• src/git/repository/working_tree.rs — the non-temp (None) arm and the command-failure Err arm of WorkingTree::run_object_store_command.

The env-var branches are the interesting gap: they're the sandbox-inheritance case this PR is meant to handle, and they're straightforward to exercise by setting GIT_OBJECT_DIRECTORY / GIT_ALTERNATE_OBJECT_DIRECTORIES in a test before calling with_temporary_object_directory() and asserting the temp store still resolves objects. The error arms and the None arm are defensible as low-value. (The panic! arms flagged in integration.rs are test-internal unreachable branches — ignore those.) codecov/patch gates merge here per CLAUDE.md even though GitHub marks it non-required.

Happy to push a commit with the fmt fix and a test covering the env-var branches if you'd like.

[reneleonhardt]

Thanks—the temporary object database and persistent caches are independent concerns. I removed the cache suppression, added a regression proving merge-conflict cache persistence, rebased onto current main, and reduced the PR from 13 files to 9. The rewritten branch is pushed with focused tests and warnings-denied Clippy passing.

[reneleonhardt]

Fixed: cargo fmt --check is clean, and a new subprocess regression sets both GIT_OBJECT_DIRECTORY and GIT_ALTERNATE_OBJECT_DIRECTORIES to prove temporary analysis retains inherited object locations. The test reports merge_conflicts: null when inherited alternates are removed and passes with the production path restored. The focused object-store tests and warnings-denied Clippy pass; CI is rerunning on the amended signed commit.

[worktrunk-bot]

The rework looks good and the earlier design/caching concern is fully addressed — thanks for the thorough tests. One thing before this can land: codecov/patch is still red on this commit (it completed at 12:37Z, just after your last comment, so you may not have seen it yet). codecov/patch gates merge here per CLAUDE.md even though GitHub marks it non-required.

The env-var regression you added exercises the absolute GIT_OBJECT_DIRECTORY path, so path.is_absolute() is true and the else { self.discovery_path.join(path) } branch in with_temporary_object_directory — the relative-path case — is never hit. That plus a couple of defensive arms are the remaining uncovered lines.

Still-uncovered added lines (from the Codecov compare API)

Worth covering:

• with_temporary_object_directory (src/git/repository/mod.rs) — the else { self.discovery_path.join(path) } arm. A second variant of your inheritance test that points GIT_OBJECT_DIRECTORY at a relative path (git's rev-parse --git-path objects echoes it verbatim) would exercise it.

Defensible-but-still-counted (CLAUDE.md: "remove unused code … where falling through to a general handler suffices" is a valid resolution too):

• The command-failure Err arm of Repository::run_object_store_command and of WorkingTree::run_object_store_command.
• The non-temp (else { command }) arm of WorkingTree::run_object_store_command — in the wt list path this helper only runs on a repo that already holds a temp object dir (the WorkingTreeConflicts task is dropped via requires_object_store() when the temp store can't be created), so that arm is effectively unreachable in production. Either drop it (let the caller assume the temp env) or add a test that calls it on a plain repo.

Ignore: the three MergeTreeOutcome::Conflict => panic!(...) arms in the #[cfg(test)] mod merge_tree_cache_tests test are unreachable-by-design (the topology merges cleanly), and the lone brace flagged in collect/mod.rs is a region artifact inside the warning block your test_list_skips_object_probes_when_temporary_store_is_unavailable already covers.

Happy to push a commit adding the relative-path test case and either covering or trimming the two run_object_store_command defensive arms if you'd like — otherwise flagging for @max-sixty on the coverage-gate call.

[max-sixty]

@worktrunk-bot please can you explain this PR and its tradeoffs?

[worktrunk-bot]

What it solves

wt list is observational, but three of its probes shell out to Git plumbing that writes objects — git write-tree (WorkingTreeConflicts), git commit-tree (the ephemeral merge-base commit), and git merge-tree --write-tree (MergeTreeConflicts / WouldMergeAdd). When .git/objects is read-only — the case in managed agent sandboxes — those writes fail, and the merge/conflict/integration columns surface as error: unable to create temporary file: Operation not permitted instead of a real answer. So the most useful part of wt list --full degrades exactly where an agent most wants it.

How it works

Repository::with_temporary_object_directory() returns a clone of the Repository whose object-writing plumbing is redirected: it sets GIT_OBJECT_DIRECTORY to a fresh tempfile::TempDir and lists the real object store (plus any inherited alternates) in GIT_ALTERNATE_OBJECT_DIRECTORIES. Writes land in the throwaway temp dir; reads still resolve every real object as an alternate. collect() wraps the repo in this mode, so wt list and the wt switch picker both get it. The three writing probes route through new run_object_store_command* helpers; every read-only probe is untouched.

Two supporting pieces make it correct:

• The in-memory merge_tree cache key gains an object_store_cache_id dimension, so a tree SHA computed in a temp store (which only exists there) can't be handed back for the persistent store, which must materialize its own.
• If the temp dir itself can't be created, only the three requires_object_store() tasks are dropped (with a one-time warning) — the rest of wt list still renders.

Tradeoffs

• Temp-object mode is now the default for every wt list / wt switch, not just sandboxes. The upside is one code path with no need to cheaply pre-detect a read-only object DB; the cost is a per-invocation tempdir + two extra env vars on each writing probe. Modest, but non-zero on a hot path.
• Observational merge trees are no longer materialized in the real object store. Previously merge-tree --write-tree left its (unreferenced, later-GC'd) tree in .git/objects; now it's discarded with the temp dir. The persistent sha_cache under .git/wt/cache still records the outcome (conflict flag / clean-tree SHA) across runs — that was the caching regression in my first review, and it's fixed and pinned by test_list_temporary_object_store_preserves_persistent_cache_writes, so cross-run warmth is preserved. What's not reused across runs is the raw tree object.
• Added surface in the git layer. A parallel run_object_store_command* family, a TemporaryObjectDirectory, and a widened cache key — complexity carried by all users to serve a sandbox-specific constraint. It's well-scoped and well-tested, but it is more moving parts in the plumbing.
• Net cleanliness win: an observational command no longer mutates the real object DB at all, even transiently, which is arguably the more correct behavior regardless of sandboxes.

The design concern I raised first time — that the rework also disabled the persistent caches in every repo — was resolved in the rebased branch, and the coverage gap on the env-var inheritance branches is now closed. All required checks plus codecov/patch are green on ad76cd0, so from a review standpoint it's landable; the remaining call is the design one above — whether making temp-object mode unconditional is the tradeoff you want, versus gating it on a detected read-only object DB.

[max-sixty]

ok, let's close this; it's quite bad that everything is no longer cached across runs even when it is writable — is the case with this PR?

[worktrunk-bot]

Good question to check before closing — the answer is no: this PR does not disable cross-run caching in the writable case. That was the regression in my first review (the original branch suppressed the persistent caches everywhere), but it was removed in the rebase, so the current state is:

• Persistent cross-run cache is untouched. The sha_cache writes to .git/wt/cache/{kind}/{key}.json — a different location from .git/objects. Temp-object mode only redirects GIT_OBJECT_DIRECTORY/GIT_ALTERNATE_OBJECT_DIRECTORIES, so it can't gate those writes. The merge-conflict / ancestor / added-changes / merge-add outcomes are cached and reused across runs exactly as on main. Pinned by test_list_temporary_object_store_preserves_persistent_cache_writes, which deletes .git/wt/cache, runs wt list, and asserts the merge-tree-conflicts/*.json entry reappears.
• In-memory cache still dedups within a run. The new object_store_cache_id dimension on the merge_tree key just prevents a temp-store tree SHA (which exists only in that temp dir) from being handed to the persistent store; within one wt list every probe shares the same temp store, so the "one subprocess per row" collapse is unchanged.
• What actually changed: the raw merge-tree --write-tree tree object now lands in the throwaway temp dir instead of .git/objects. But that object was never a cross-run cache — it was unreferenced and GC'd anyway; nothing reads it back on a later run.

So the caching objection doesn't apply here. The real remaining tradeoff is the design one from my earlier comment: temp-object mode is unconditional for every wt list / wt switch, not just sandboxes — a per-invocation tempdir plus two extra env vars on each writing probe, carried by all users to serve a sandbox-specific constraint. That's the call worth making: keep it unconditional for the single-code-path simplicity, or gate it on a detected read-only object DB.

Want me to still close it, or leave it open pending that decision? Happy to do either.