diff --git a/bridged/src/main/java/dev/ltms/bridged/session/GitWorktrees.java b/bridged/src/main/java/dev/ltms/bridged/session/GitWorktrees.java index c574c5b..9dbcbd1 100644 --- a/bridged/src/main/java/dev/ltms/bridged/session/GitWorktrees.java +++ b/bridged/src/main/java/dev/ltms/bridged/session/GitWorktrees.java @@ -167,6 +167,14 @@ public final class GitWorktrees implements Worktrees { exec("git", "-C", repoRoot, "worktree", "remove", "--force", worktreePath); } + @Override + public boolean hasUncommitted(String worktreePath) { + // No --untracked-files=no: the exact shape of the work lost in CB-576 was a new file + // that was never added, so an untracked-only worktree is still dirty. + String out = exec("git", "-C", worktreePath, "status", "--porcelain"); + return !out.isBlank(); + } + @Override public void overlayParity(String repoRoot, String worktreePath, List overlay) { if (overlay == null || overlay.isEmpty()) { diff --git a/bridged/src/main/java/dev/ltms/bridged/session/SessionManager.java b/bridged/src/main/java/dev/ltms/bridged/session/SessionManager.java index 9ebc167..4f5bc72 100644 --- a/bridged/src/main/java/dev/ltms/bridged/session/SessionManager.java +++ b/bridged/src/main/java/dev/ltms/bridged/session/SessionManager.java @@ -201,6 +201,16 @@ public final class SessionManager implements TurnListener { removed.paneId(), removed.terminalId(), removed.state(), cause); if (preserveWorktree && removed.worktree() != null) { logPreservedForShutdown(removed); + } else if (removed.worktree() != null && worktrees.hasUncommitted(removed.worktree())) { + // CB-576: a release that would otherwise remove the worktree finds it holding + // uncommitted work the bridge cannot see. A worker that ends a turn without + // committing (normally because it stopped to ask a question or refused the turn) + // has its only copy of that work in the worktree. Remove would --force-delete it, + // so preserve the directory and tell an operator where to find it. + preserveWorktree = true; + log.warn("release {} preserves dirty worktree {} for pane={} terminal={}: " + + "the worktree holds uncommitted changes that --force remove would destroy", + cause, removed.worktree(), removed.paneId(), removed.terminalId()); } // CB-516: a send still waiting on this worker can never be answered now. Tell the // listener BEFORE the pane is torn down, so a blocked caller fails fast with a real diff --git a/bridged/src/main/java/dev/ltms/bridged/session/Worktrees.java b/bridged/src/main/java/dev/ltms/bridged/session/Worktrees.java index eaef94e..f858bf0 100644 --- a/bridged/src/main/java/dev/ltms/bridged/session/Worktrees.java +++ b/bridged/src/main/java/dev/ltms/bridged/session/Worktrees.java @@ -10,6 +10,14 @@ public interface Worktrees { /** git -C worktree remove --force . Idempotent (already-gone tolerated). */ void remove(String repoRoot, String worktreePath); + /** + * True when the worktree holds uncommitted changes the bridge cannot see: tracked + * modifications, staged files, or untracked files. {@code git status --porcelain} is the + * test; an empty result means clean. Callers use this to decide whether removing the + * worktree would silently destroy a worker's only copy of its work. + */ + boolean hasUncommitted(String worktreePath); + /** Copy each existing overlay path repoRoot→worktree; mark tracked ones --skip-worktree. */ void overlayParity(String repoRoot, String worktreePath, List overlay); diff --git a/bridged/src/test/java/dev/ltms/bridged/session/FakeWorktrees.java b/bridged/src/test/java/dev/ltms/bridged/session/FakeWorktrees.java index 4aa1dd3..c9b8d9d 100644 --- a/bridged/src/test/java/dev/ltms/bridged/session/FakeWorktrees.java +++ b/bridged/src/test/java/dev/ltms/bridged/session/FakeWorktrees.java @@ -29,6 +29,7 @@ public final class FakeWorktrees implements Worktrees { private final Set existingPaths = ConcurrentHashMap.newKeySet(); private final Set trackedPaths = ConcurrentHashMap.newKeySet(); private volatile RuntimeException addFailure; + private volatile boolean dirty = false; private volatile String repoRoot = "/repo"; private volatile String prefix = "/worktrees"; @@ -61,6 +62,12 @@ public final class FakeWorktrees implements Worktrees { return this; } + /** Mark the worktree dirty so {@link #hasUncommitted} reports true (simulates uncommitted work). */ + public FakeWorktrees withDirty(boolean dirty) { + this.dirty = dirty; + return this; + } + @Override public String add(String repoRoot, String branch, String baseRef) { addCalls.add(new AddCall(repoRoot, branch, baseRef)); @@ -77,6 +84,11 @@ public final class FakeWorktrees implements Worktrees { removeCalls.add(new RemoveCall(repoRoot, worktreePath)); } + @Override + public boolean hasUncommitted(String worktreePath) { + return dirty; + } + @Override public void overlayParity(String repoRoot, String worktreePath, List overlay) { List copied = new java.util.ArrayList<>(); diff --git a/bridged/src/test/java/dev/ltms/bridged/session/GitWorktreesTest.java b/bridged/src/test/java/dev/ltms/bridged/session/GitWorktreesTest.java index 3ebd8c2..b5c6a83 100644 --- a/bridged/src/test/java/dev/ltms/bridged/session/GitWorktreesTest.java +++ b/bridged/src/test/java/dev/ltms/bridged/session/GitWorktreesTest.java @@ -175,6 +175,31 @@ class GitWorktreesTest { assertTrue(Files.exists(Path.of(wt).resolve(".mcp.json")), ".mcp.json stub was dropped"); } + /** + * CB-576. {@code hasUncommitted} must treat a freshly-provisioned worktree as clean, but a + * worktree holding a brand-new, never-added file as dirty. The untracked-file-only shape is + * exactly the work lost in the incident — a worker's draft that compiled but was never + * committed because it stopped to ask its lead a question. + */ + @Test + void anUntrackedOnlyWorktreeCountsAsDirty(@TempDir Path tmp) throws Exception { + Path repo = initRepo(tmp.resolve("repo")); + GitWorktrees gitWorktrees = new GitWorktrees(tmp.resolve("wts").toString()); + String wt = gitWorktrees.add(repo.toString(), "cb-576-u", "HEAD"); + + assertFalse(gitWorktrees.hasUncommitted(wt), + "a freshly provisioned worktree must read as clean"); + + Files.writeString(Path.of(wt).resolve("brand-new.txt"), "draft that was never added\n"); + + assertTrue(gitWorktrees.hasUncommitted(wt), + "an untracked-only file must count as dirty"); + + Files.writeString(Path.of(wt).resolve("README.md"), "edited tracked file\n"); + assertTrue(gitWorktrees.hasUncommitted(wt), + "a tracked modification must also count as dirty"); + } + /** All three protected configs are covered: each one present in a worktree is neutralized and hidden. */ @Test void allThreeConfigsAreNeutralizedWhenPresent(@TempDir Path tmp) throws Exception { diff --git a/bridged/src/test/java/dev/ltms/bridged/session/WorktreeSessionManagerTest.java b/bridged/src/test/java/dev/ltms/bridged/session/WorktreeSessionManagerTest.java index e0b07e7..5fa720b 100644 --- a/bridged/src/test/java/dev/ltms/bridged/session/WorktreeSessionManagerTest.java +++ b/bridged/src/test/java/dev/ltms/bridged/session/WorktreeSessionManagerTest.java @@ -6,10 +6,15 @@ import dev.ltms.bridged.guard.SubscriptionGuard; import dev.ltms.bridged.herdr.AgentControl; import dev.ltms.bridged.herdr.FakeHerdr; import dev.ltms.bridged.herdr.WorkspaceControl; +import ch.qos.logback.classic.Level; +import ch.qos.logback.classic.LoggerContext; +import ch.qos.logback.classic.spi.ILoggingEvent; +import ch.qos.logback.core.read.ListAppender; import dev.ltms.bridged.member.ClaudeCodeLauncher; import dev.ltms.bridged.msg.TestTurnTokens; import dev.ltms.bridged.peer.MemberRole; import org.junit.jupiter.api.Test; +import org.slf4j.LoggerFactory; import java.util.List; import java.util.Map; @@ -179,6 +184,49 @@ class WorktreeSessionManagerTest { assertTrue(sessions.get(paneId).isEmpty(), "released session is no longer retrievable"); } + /** + * CB-576. A normal {@code COMPLETED} release whose worktree holds uncommitted work must NOT + * remove it — {@code --force} would destroy the worker's only copy. The bridge cannot see + * uncommitted files, so the worktree is preserved and the release logged at WARN naming the + * path, the session, and the cause an operator needs to find the work. + */ + @Test + void releasePreservesDirtyWorktreeAndLogsWarn() { + FakeHerdr herdr = new FakeHerdr(); + FakeWorktrees worktrees = new FakeWorktrees().withRepoRoot("/repo").withPrefix("/wt") + .withDirty(true); + SessionManager sessions = new SessionManager(workerService(herdr), worktrees); + MemberSession s = sessions.acquire("ltms-local", null, "/caller/proj", null, + new WorktreeRequest("cb-576", null)); + + LoggerContext ctx = (LoggerContext) LoggerFactory.getILoggerFactory(); + ch.qos.logback.classic.Logger sessionLog = + (ch.qos.logback.classic.Logger) LoggerFactory.getLogger(SessionManager.class); + ListAppender appender = new ListAppender<>(); + appender.setContext(ctx); + appender.start(); + sessionLog.addAppender(appender); + sessionLog.setLevel(Level.WARN); + try { + sessions.release(s.paneId()); + + assertTrue(herdr.called("pane.close"), "release still tears the worker pane down"); + assertTrue(worktrees.removeCalls().isEmpty(), + "a dirty worktree is never removed — it holds the only copy of the work"); + String warn = appender.list.stream() + .filter(e -> e.getLevel().equals(Level.WARN)) + .map(ILoggingEvent::getFormattedMessage) + .filter(m -> m.contains("dirty worktree")) + .findFirst() + .orElse("no dirty-release WARN logged"); + assertTrue(warn.contains(s.worktree()), "the WARN names the worktree path: " + warn); + assertTrue(warn.contains(s.terminalId()), "the WARN names the session: " + warn); + assertTrue(warn.contains("COMPLETED"), "the WARN names the release cause: " + warn); + } finally { + sessionLog.detachAppender(appender); + } + } + @Test void drainAllPreservesWorktreeOfIdleSession() { FakeHerdr herdr = new FakeHerdr(); diff --git a/docs/M4-Fleet-Health.md b/docs/M4-Fleet-Health.md index f954a99..c605172 100644 --- a/docs/M4-Fleet-Health.md +++ b/docs/M4-Fleet-Health.md @@ -237,7 +237,7 @@ state never presents stop as the only action. | Release cause | Process action | Provisioned worktree | |---|---|---| | `SPAWN_ROLLBACK` before registration or delivery | Stop and clean up | Remove | -| `COMPLETED` for `READY` or `DONE` without pending work, idle TTL, or successful context-cap completion | Stop | Remove under completed policy | +| `COMPLETED` for `READY` or `DONE` without pending work, idle TTL, or successful context-cap completion | Stop | Remove only if clean; preserve a dirty worktree (CB-576) | | `NEVER_READY` | Stop | Preserve | | `GONE` | Best-effort stop | Preserve | | `TURN_FAILED` or lead abort while `BUSY` or `FAILED` | Stop | Preserve |