[do not review] Jt/lifetime

[jonathantanmy2]

See what CI says.

[Copilot]

🟡 Changes recommended

Critical and moderate findings remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This pull request refactors workspace access to use owned projections and updates related API, CLI, and TUI callers.

Changes:

• Refactors workspace caching and invalidation.
• Updates branch, stack, virtual-branch, and uncommit operations.
• Adjusts workspace recreation and TUI reload handling.

File summaries

File	Summary
crates/but/src/command/legacy/status/tui/operations.rs	Invalidates workspace state before TUI reloads.
crates/but/src/command/legacy/branch/new.rs	Adapts branch application to owned workspaces.
crates/but-ctx/src/lib.rs	Critical (1 vote): owned return breaks existing debug callers. Moderate (1 vote each): accessors can panic on active borrows, and cache removal conflicts with the documented contract.
crates/but-api/src/workspace.rs	Critical (3 votes): the reapply loop can use stale workspace projections.
crates/but-api/src/legacy/virtual_branches.rs	Adapts legacy virtual-branch operations.
crates/but-api/src/legacy/stack.rs	Adapts legacy stack operations.
crates/but-api/src/commit/uncommit.rs	Adjusts workspace borrowing for owned values.
crates/but-api/src/branch.rs	Adapts branch apply, create, and remove operations.

Review details

Suppressed comments (3)

crates/but-ctx/src/lib.rs:599

• RefCell::take() panics when workspace is already borrowed. Before this change, try_borrow_mut()? converted an outstanding workspace borrow into the method's anyhow::Result; a caller that holds a read projection and requests this writable accessor now gets an unrecoverable panic. Preserve the fallible borrow before taking the cached value.

        if let Some(cached) = self.workspace.take() {
            return Ok((self.repo.get_mut()?, cached, self.db.get_cache_mut()?));
        }

crates/but-ctx/src/lib.rs:700

• This sibling writable accessor has the same regression: RefCell::take() panics on an outstanding workspace borrow, whereas the previous try_borrow_mut()? implementation returned the borrow error. Use a fallible borrow before taking the cached value so callers do not crash when a read projection is still alive.

        if let Some(cached) = self.workspace.take() {
            return Ok((self.repo.get_mut()?, cached, self.db.get_cache()?));
        }

crates/but-ctx/src/lib.rs:597

• self.workspace.take() means these mutable accessors now remove the cached workspace and never put it back, but the surrounding documentation still promises a cached workspace that callers update in place. This also forces the next read accessor to rebuild the projection from HEAD after every mutable access. Either provide an ownership guard that restores the cache, or update the API contract and callers to make the uncached behavior explicit.

        if let Some(cached) = self.workspace.take() {

• Files reviewed: 8/8 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Unresolved workspace cache and stale-projection issues can break successive or unstacked branch operations.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

crates/but-api/src/branch.rs:860

• apply_only_with_perm still documents that it updates the in-memory workspace in ctx, but this hunk removes the only assignment of out.workspace back to that cache. After this call, the next read must re-project instead of observing the documented cached result; either preserve the cache update or revise the public contract and its test/documentation.

    let (repo, ws, _db) = ctx.workspace_mut_and_db_with_perm(perm)?;

crates/but-api/src/workspace.rs:125

• The first apply can change the workspace from ad-hoc to managed, and the saved-stack loop below applies each subsequent branch against ws.clone(). Since outcome.workspace is no longer assigned here (nor apply_outcome.workspace in the loop), ws remains the pre-apply projection; with multiple saved stacks, later applies can rebuild/merge from stale graph state and drop or misclassify earlier stacks. Keep ws mutable and replace it with each persisted outcome before the next apply.

            if !outcome.status.persisted_mutation() {
                anyhow::bail!(
                    "BUG: failed to apply head ref ({head_name}). Failed with {:?}",
                    outcome.status
                )

crates/but/src/command/legacy/branch/new.rs:367

• After applying head_name, this path immediately starts with_transaction_with_perm_only while meta is still alive. set_workspace() only marks the legacy metadata for write-on-drop, and the owned ws is no longer put into Context, so the transaction reprojects from the on-disk metadata and can miss the just-applied head branch. This breaks the unstacked-branch flow when the current branch has commits (for example, creating a second switched branch in single-branch mode). Preserve outcome.workspace for the transaction or otherwise flush/reuse the updated projection before starting it.

                if !outcome.status.persisted_mutation() {

• Files reviewed: 10/10 changed files
• Comments generated: 1
• Review effort level: Lite