From ef8c97871e0e4f724861206f70c22cd42a87ebaa Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Thu, 3 Sep 2026 11:09:50 +0700 Subject: [PATCH] fleetd #134/#148 point 3: make overlayParity's copy and skip-worktree visible MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit overlayParity logged everything at debug, so at the default level nobody could tell which overlay files a spawn actually received (#148 pt 3), and a tracked file marked --skip-worktree gave no warning that it can no longer be edited from that worktree (#134). Report the outcome at info: a per-spawn summary naming the denominator (every configured candidate), what was copied, and why anything was not — plus a separate line naming every file marked --skip-worktree, stating plainly that it cannot be committed from this worktree. No worktree-local marker file: the worktree must hold exactly the configured overlay set and nothing else, so an extra file would violate that invariant. --- .../dev/ltms/fleet/session/GitWorktrees.java | 27 ++++- .../ltms/fleet/session/GitWorktreesTest.java | 108 ++++++++++++++++++ 2 files changed, 132 insertions(+), 3 deletions(-) diff --git a/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java b/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java index d598889..ca7fd26 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java +++ b/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java @@ -486,25 +486,46 @@ public final class GitWorktrees implements Worktrees { } Path srcRoot = Path.of(repoRoot).toAbsolutePath().normalize(); Path dstRoot = Path.of(worktreePath).toAbsolutePath().normalize(); + List copied = new ArrayList<>(); + List skipped = new ArrayList<>(); + List neutralized = new ArrayList<>(); for (String rel : overlay) { Path src = srcRoot.resolve(rel).normalize(); if (!Files.exists(src)) { - log.debug("parity overlay source missing — skipping {}", rel); + skipped.add(rel + " absent"); continue; } Path dst = dstRoot.resolve(rel).normalize(); try { Files.createDirectories(dst.getParent()); Files.copy(src, dst, StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.COPY_ATTRIBUTES); - log.debug("copied parity overlay {}", rel); + copied.add(rel); } catch (IOException e) { throw new WorktreeException("cannot copy overlay " + rel + ": " + e.getMessage(), e); } if (isTracked(dstRoot, rel)) { exec("git", "-C", worktreePath, "update-index", "--skip-worktree", rel); - log.debug("marked overlay --skip-worktree {}", rel); + neutralized.add(rel); } } + // CB-148 point 3: a bare count ("copied 1") hides which candidates were even considered — the + // same shape of under-reporting this repo has been bitten by before. Name the denominator + // (every configured candidate), what was actually copied, and — for anything not copied — + // why, so a spawn's overlay outcome is legible from the log alone, no filesystem dig required. + String detail = copied.isEmpty() ? String.join(", ", skipped) + : skipped.isEmpty() ? String.join(", ", copied) + : String.join(", ", copied) + " (" + String.join(", ", skipped) + ")"; + log.info("parity overlay: copied {} of {} candidates: {}", copied.size(), overlay.size(), detail); + // CB-134: a neutralized tracked file is otherwise a silent trap — a worker edits it, git + // ignores the change with no error, and nothing anywhere said the file could not be + // committed from this worktree. Name every file marked --skip-worktree here, with the + // consequence stated in the message itself, rather than adding a worktree-local marker + // file: acceptance criterion 1 requires the worktree hold exactly the configured overlay + // set and nothing else, so an extra marker would itself violate the fix. + if (!neutralized.isEmpty()) { + log.info("parity overlay marked --skip-worktree (cannot be committed from this worktree): {}", + String.join(", ", neutralized)); + } } @Override diff --git a/fleetd/src/test/java/dev/ltms/fleet/session/GitWorktreesTest.java b/fleetd/src/test/java/dev/ltms/fleet/session/GitWorktreesTest.java index 75396fd..0dc8a8c 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/session/GitWorktreesTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/session/GitWorktreesTest.java @@ -1177,4 +1177,112 @@ class GitWorktreesTest { + "the refusal must happen before `git worktree add` ever runs"); } } + + // ---- fleetd #134 / #148 point 3: overlayParity must report what it did, and marking a tracked + // file --skip-worktree must say the file can no longer be committed from this worktree. Drives + // overlayParity directly against a real worktree (git worktree add, no GitWorktrees#add) so these + // tests are independent of origin/credential-helper provisioning, which is not under test here. ---- + + /** A bare worktree, sibling to {@code repo}, created with plain git — the target overlayParity + * copies into. Deliberately not {@link GitWorktrees#add}: that method does unrelated + * provisioning (origin rewrite, .mcp.json neutralization, credential helper) that would only + * add noise to the log assertions below. */ + private static Path bareWorktree(Path repo, Path wtDir, String branch) throws Exception { + git(repo, "worktree", "add", "-q", wtDir.toString(), "-b", branch, "HEAD"); + return wtDir; + } + + /** Acceptance criterion 1: only the configured candidates land in the worktree — nothing else + * from the source tree leaks in alongside them. */ + @Test + void overlayParityCopiesExactlyTheConfiguredFilesAndNothingElse(@TempDir Path tmp) throws Exception { + Path repo = initRepo(tmp.resolve("repo")); + Files.writeString(repo.resolve(".env"), "A=1\n"); + Files.writeString(repo.resolve(".envrc"), "export A=1\n"); + Files.writeString(repo.resolve("not-overlaid.txt"), "must not be copied\n"); + Path wt = bareWorktree(repo, tmp.resolve("wt"), "cb134-exact"); + + new GitWorktrees(tmp.resolve("wts").toString()) + .overlayParity(repo.toString(), wt.toString(), List.of(".env", ".envrc")); + + assertEquals("A=1\n", Files.readString(wt.resolve(".env"))); + assertEquals("export A=1\n", Files.readString(wt.resolve(".envrc"))); + assertFalse(Files.exists(wt.resolve("not-overlaid.txt")), + "overlayParity must copy only the configured candidates, not the whole source tree"); + } + + /** Criterion 2: both candidates present — the summary line names both and the denominator. */ + @Test + void overlayParityLogsBothCopiedWhenBothCandidatesArePresent(@TempDir Path tmp) throws Exception { + reportingLogger.setLevel(Level.INFO); + Path repo = initRepo(tmp.resolve("repo")); + Files.writeString(repo.resolve(".env"), "A=1\n"); + Files.writeString(repo.resolve(".envrc"), "export A=1\n"); + Path wt = bareWorktree(repo, tmp.resolve("wt"), "cb134-both"); + + new GitWorktrees(tmp.resolve("wts").toString()) + .overlayParity(repo.toString(), wt.toString(), List.of(".env", ".envrc")); + + assertTrue(capturedMessages().contains("parity overlay: copied 2 of 2 candidates: .env, .envrc"), + "expected the both-copied summary line, got:\n" + capturedMessages()); + } + + /** Criterion 2: one candidate present, one absent — the summary must name the copied file, the + * denominator, and why the other candidate was not copied. */ + @Test + void overlayParityLogsOneCopiedOneAbsent(@TempDir Path tmp) throws Exception { + reportingLogger.setLevel(Level.INFO); + Path repo = initRepo(tmp.resolve("repo")); + Files.writeString(repo.resolve(".env"), "A=1\n"); + // .envrc deliberately not created — the absent candidate. + Path wt = bareWorktree(repo, tmp.resolve("wt"), "cb134-partial"); + + new GitWorktrees(tmp.resolve("wts").toString()) + .overlayParity(repo.toString(), wt.toString(), List.of(".env", ".envrc")); + + assertTrue(Files.exists(wt.resolve(".env"))); + assertFalse(Files.exists(wt.resolve(".envrc"))); + assertTrue(capturedMessages().contains("parity overlay: copied 1 of 2 candidates: .env (.envrc absent)"), + "expected the copied/absent summary line, got:\n" + capturedMessages()); + } + + /** Criterion 3: a tracked candidate is marked --skip-worktree, and that must be named in the log + * with the consequence spelled out — a worker editing it afterward finds git ignoring the + * change, silently, unless this line told it so beforehand. */ + @Test + void overlayParityLogsSkipWorktreeConsequenceForATrackedFile(@TempDir Path tmp) throws Exception { + reportingLogger.setLevel(Level.INFO); + Path repo = initRepo(tmp.resolve("repo")); + Files.writeString(repo.resolve(".env"), "A=1\n"); + git(repo, "add", ".env"); + git(repo, "commit", "-q", "-m", "track env"); + Path wt = bareWorktree(repo, tmp.resolve("wt"), "cb134-tracked"); + // Change the source after the worktree checkout, so the overlay copy actually overwrites it. + Files.writeString(repo.resolve(".env"), "A=2\n"); + + new GitWorktrees(tmp.resolve("wts").toString()) + .overlayParity(repo.toString(), wt.toString(), List.of(".env")); + + assertEquals("A=2\n", Files.readString(wt.resolve(".env"))); + assertEquals("", status(wt, ".env"), + "the skip-worktree'd file must not show as modified even though its content changed"); + assertTrue(capturedMessages().contains( + "parity overlay marked --skip-worktree (cannot be committed from this worktree): .env"), + "expected the skip-worktree consequence line, got:\n" + capturedMessages()); + } + + /** Criterion 5: null and empty overlay lists return quietly — no exception, no log noise. */ + @Test + void overlayParityWithNoCandidatesLogsNothing(@TempDir Path tmp) throws Exception { + reportingLogger.setLevel(Level.INFO); + Path repo = initRepo(tmp.resolve("repo")); + Path wt = bareWorktree(repo, tmp.resolve("wt"), "cb134-empty"); + GitWorktrees worktrees = new GitWorktrees(tmp.resolve("wts").toString()); + + worktrees.overlayParity(repo.toString(), wt.toString(), null); + worktrees.overlayParity(repo.toString(), wt.toString(), List.of()); + + assertTrue(reportingAppender.list.isEmpty(), + "a null/empty overlay must log nothing, got:\n" + capturedMessages()); + } }