From 8067ee4ec409b3e6f5f69139b594ac4eeb40013c Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Mon, 31 Aug 2026 15:53:01 +0700 Subject: [PATCH 1/2] fleetd #185: opt-in worktreeGroup config for group-shared worktrees Adds worktreeGroup (top-level FleetConfig key), Worktrees.shareWithGroup (GitWorktrees impl: git config core.sharedRepository group + one-time chgrp/chmod g+rwX/setgid fix-up over the worktree, .git/objects, refs, logs, worktrees, and packed-refs when present), and wires SessionManager to call it AFTER overlayParity so overlay files are covered too. Off by default (byte-identical behaviour when unset). Documents the credentials-not-repository caveat in the javadoc and example config. --- fleetd/fleetd.example.yaml | 11 ++ .../src/main/java/dev/ltms/fleet/Fleetd.java | 2 +- .../dev/ltms/fleet/config/FleetConfig.java | 31 ++++- .../dev/ltms/fleet/session/GitWorktrees.java | 121 +++++++++++++++++- .../ltms/fleet/session/SessionManager.java | 4 + .../dev/ltms/fleet/session/Worktrees.java | 17 +++ .../ltms/fleet/config/FleetConfigTest.java | 29 +++++ .../dev/ltms/fleet/session/FakeWorktrees.java | 27 ++++ .../ltms/fleet/session/GitWorktreesTest.java | 99 ++++++++++++++ .../fleet/session/SessionManagerTest.java | 4 + .../session/WorktreeSessionManagerTest.java | 31 +++++ 11 files changed, 368 insertions(+), 8 deletions(-) diff --git a/fleetd/fleetd.example.yaml b/fleetd/fleetd.example.yaml index 21c718e..ad4641a 100644 --- a/fleetd/fleetd.example.yaml +++ b/fleetd/fleetd.example.yaml @@ -634,6 +634,17 @@ guard: # to a sibling directory of the repo root. # worktreeRoot: /Users/me/src/.bridged-worktrees +# Worktree group sharing (fleetd #185 stage 3). OPTIONAL, off by default. Names an OS group +# that a provisioned worktree's repo is made group-writable for (git config +# core.sharedRepository group, plus a one-time chgrp/chmod/setgid fix-up), so a member spawned +# under a DIFFERENT OS user (see memberHerdrSocket) can write its own worktree, its +# per-worktree git metadata, and its own commit objects — without it, every file GitWorktrees +# creates is owned by fleetd's own uid and unwritable by another user. +# CAUTION: this isolates credentials, not the repository — a member in the group can still +# write the operator's git objects and refs in the shared repo. The operator running fleetd +# must already be a member of the named group, or every provisioning spawn fails loudly. +# worktreeGroup: fleet-workers + # Session lifecycle limits (CB-303). All knobs are opt-in; omit or set to null to keep # the feature disabled. By default the daemon never reaps, caps, or drains sessions. # idleTtlSeconds → reap READY/DONE sessions idle longer than this (never BUSY/SPAWNING) diff --git a/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java b/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java index 4988c95..5726a96 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java +++ b/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java @@ -224,7 +224,7 @@ public final class Fleetd { contextCap = cfg.lifecycle().contextCap(); } boolean clearAfterTurn = cfg.lifecycle() != null && cfg.lifecycle().clearAfterTurn(); - SessionManager sessions = new SessionManager(workers, new GitWorktrees(cfg.worktreeRoot()), + SessionManager sessions = new SessionManager(workers, new GitWorktrees(cfg.worktreeRoot(), cfg.worktreeGroup()), System::nanoTime, contextCap, clearAfterTurn); liveCountRef.set(profileName -> (int) sessions.roster().stream() .filter(s -> profileName.equals(s.profile())) diff --git a/fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java b/fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java index 96b3664..8793c44 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java +++ b/fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java @@ -76,6 +76,14 @@ import java.util.Set; * stay on {@code broker}'s vhost). {@code null} → no lead mailbox is opened. * Config parsing + accessors only — nothing here wires it into a live * {@code LeadMailbox}; that is a separate ticket. See {@link Coordinator}. + * @param worktreeGroup optional OS group name (fleetd #185 stage 3) that makes a provisioned + * worktree's repo group-shared, so a member running as a different OS user + * (see {@code memberHerdrSocket}) can write its own worktree, its per-worktree + * git metadata, and its own commit objects. {@code null}/blank/empty ⇒ off, + * today's behaviour unchanged (every file stays owned by fleetd's own uid). + * This isolates credentials, not the repository: a member in + * the group can still write the operator's git objects and refs in the shared + * repo. See {@link dev.ltms.fleet.session.Worktrees#shareWithGroup}. */ @JsonIgnoreProperties(ignoreUnknown = true) public record FleetConfig( @@ -98,7 +106,20 @@ public record FleetConfig( ConfigReload configReload, Integer quarantineCooldownSeconds, MemberCredentials memberCredentials, - Coordinator coordinator) { + Coordinator coordinator, + String worktreeGroup) { + + /** Back-compat form before the {@code worktreeGroup} key was added. */ + public FleetConfig(Bind bind, String herdrSocket, String memberHerdrSocket, Map profiles, + Guard guard, String worktreeRoot, Lifecycle lifecycle, Integer spawnReadyTimeoutMs, + Integer spawnReadyPollMs, Broker broker, Primary primary, Fleet fleet, + LeadHeartbeat leadHeartbeat, Health health, String placement, Auth auth, + ConfigReload configReload, Integer quarantineCooldownSeconds, + MemberCredentials memberCredentials, Coordinator coordinator) { + this(bind, herdrSocket, memberHerdrSocket, profiles, guard, worktreeRoot, lifecycle, spawnReadyTimeoutMs, + spawnReadyPollMs, broker, primary, fleet, leadHeartbeat, health, placement, auth, + configReload, quarantineCooldownSeconds, memberCredentials, coordinator, null); + } /** Back-compat form before the {@code coordinator:} block was added. */ public FleetConfig(Bind bind, String herdrSocket, Map profiles, Guard guard, @@ -109,7 +130,7 @@ public record FleetConfig( MemberCredentials memberCredentials) { this(bind, herdrSocket, null, profiles, guard, worktreeRoot, lifecycle, spawnReadyTimeoutMs, spawnReadyPollMs, broker, primary, fleet, leadHeartbeat, health, placement, auth, - configReload, quarantineCooldownSeconds, memberCredentials, null); + configReload, quarantineCooldownSeconds, memberCredentials, null, null); } /** Back-compat form before the CB-596 {@code memberCredentials:} block was added. */ @@ -1325,7 +1346,7 @@ public record FleetConfig( "bind", "herdrSocket", "memberHerdrSocket", "profiles", "guard", "worktreeRoot", "lifecycle", "spawnReadyTimeoutMs", "spawnReadyPollMs", "broker", "primary", "fleet", "leadHeartbeat", "health", "placement", "auth", "configReload", "quarantineCooldownSeconds", - "memberCredentials", "coordinator"); + "memberCredentials", "coordinator", "worktreeGroup"); /** Load and validate config from {@code path}. */ public static FleetConfig load(Path path) { @@ -1943,9 +1964,11 @@ public record FleetConfig( : new MemberCredentials(null, List.of(), List.of()); // coordinator is left as-is, like broker/primary above: null keeps no LeadMailbox opened, // and this ticket's Coordinator is config-only anyway (nothing yet reads it at startup). + // worktreeGroup is left as-is (fleetd #185 stage 3): null/blank is "off", and there is no + // sane non-null default — an OS group name is operator-specific. return new FleetConfig(b, herdrSocket, memberHerdrSocket, profiles, g, worktreeRoot, l, timeout, pollMs, broker, primary, f, leadHeartbeat, health, placementOrDefault, a, configReload, - quarantineCooldown, mc, coordinator); + quarantineCooldown, mc, coordinator, worktreeGroup); } /** 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 fb0a5fc..23d7740 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java +++ b/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java @@ -23,6 +23,7 @@ import java.util.Set; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicLong; import java.util.function.Consumer; +import java.util.function.Function; import java.util.stream.Collectors; /** @@ -88,24 +89,64 @@ public final class GitWorktrees implements Worktrees { ); private final String configuredRoot; + /** OS group name for {@link #shareWithGroup} (fleetd #185 stage 3); {@code null} ⇒ feature off. */ + private final String group; private final Consumer afterWorktreeAdded; + /** How {@link #shareWithGroup}'s processes (git config / chgrp / chmod / find) actually run. + * Defaults to the real {@link #exec(String...)}. Package-private test seam so a unit test can + * prove "no group configured ⇒ zero processes spawned" and inspect exactly what a configured + * group runs, without a real second OS user or OS group on this host. */ + private final Function shareGroupRunner; private final SecureRandom random = new SecureRandom(); private final AtomicLong seq = new AtomicLong(); /** Default constructor: worktree root is derived per-repo as {@code /../.bridged-worktrees}. */ public GitWorktrees() { - this(null); + this(null, (String) null); } - /** @param configuredRoot nullable absolute or relative path; null/blank derives a sibling of the repo root. */ + /** + * @param configuredRoot nullable absolute or relative path; null/blank derives a sibling of + * the repo root. No {@code worktreeGroup} configured — {@link #shareWithGroup} + * is a no-op. + */ public GitWorktrees(String configuredRoot) { - this(configuredRoot, _ -> {}); + this(configuredRoot, (String) null); + } + + /** + * @param configuredRoot nullable absolute or relative path; null/blank derives a sibling of + * the repo root. + * @param group optional OS group name (fleetd #185 stage 3, {@code worktreeGroup:} in + * config); null/blank ⇒ {@link #shareWithGroup} is a no-op. + */ + public GitWorktrees(String configuredRoot, String group) { + this(configuredRoot, group, _ -> {}); } /** Test seam for changing a real worktree between its creation and its security check. */ GitWorktrees(String configuredRoot, Consumer afterWorktreeAdded) { + this(configuredRoot, null, afterWorktreeAdded); + } + + /** Test seam combining a configurable {@code group} with {@link #afterWorktreeAdded}. */ + GitWorktrees(String configuredRoot, String group, Consumer afterWorktreeAdded) { + this(configuredRoot, group, afterWorktreeAdded, null); + } + + /** + * Full test seam: also overrides how {@link #shareWithGroup}'s processes run (fleetd #185 + * stage 3), so a unit test can prove "no group configured ⇒ no process spawned" and inspect + * exactly what commands a configured group runs, without a real second OS user/group. + * + * @param shareGroupRunner {@code null} ⇒ the real {@link #exec(String...)}. + */ + GitWorktrees(String configuredRoot, String group, Consumer afterWorktreeAdded, + Function shareGroupRunner) { this.configuredRoot = configuredRoot; + this.group = (group == null || group.isBlank()) ? null : group; this.afterWorktreeAdded = afterWorktreeAdded == null ? _ -> {} : afterWorktreeAdded; + this.shareGroupRunner = shareGroupRunner != null ? shareGroupRunner : this::exec; } @Override @@ -607,6 +648,80 @@ public final class GitWorktrees implements Worktrees { return deleted; } + /** + * {@inheritDoc} + * + *

fleetd #185 stage 3. No-op — no process spawned, nothing logged — when {@link #group} is + * null/blank. Otherwise: + *

    + *
  1. {@code git -C repoRoot config core.sharedRepository group} so every future write by + * either uid stays group-writable;
    2. + *
  2. a one-time {@code chgrp}/{@code chmod g+rwX} fix-up over the worktree directory, the + * repo's {@code .git/objects}, {@code refs}, {@code logs}, {@code worktrees}, and (when + * present) {@code packed-refs}, with setgid ({@code chmod g+s}) applied only to the + * directories among them so files created later inherit the group;
    3. + *
  3. one INFO line naming the group and the paths touched.
    4. + *
+ * + *

This only fixes up file ownership/permissions on the operator's shared repo so a + * different-uid member can write to it — it isolates credentials, not the repository. A member + * in the group can still write the operator's git objects and refs. + * + *

Fails loudly: a missing group, or a {@code chgrp}/{@code chmod} refused because the + * operator is not a member of it, becomes a {@link WorktreeException} naming the group — never + * a silent skip that leaves a member unable to work with nothing in the log to explain why. + */ + @Override + public void shareWithGroup(String repoRoot, String worktreePath) { + if (group == null) { + return; + } + List touched = new ArrayList<>(); + try { + shareGroupRunner.apply(new String[]{"git", "-C", repoRoot, "config", "core.sharedRepository", "group"}); + for (String dir : List.of(worktreePath, repoRoot + "/.git/objects", repoRoot + "/.git/refs", + repoRoot + "/.git/logs", repoRoot + "/.git/worktrees")) { + shareGroupPath(dir, true); + touched.add(dir); + } + String packedRefs = repoRoot + "/.git/packed-refs"; + if (Files.exists(Path.of(packedRefs))) { + shareGroupPath(packedRefs, false); + touched.add(packedRefs); + } + } catch (WorktreeException e) { + throw new WorktreeException("cannot share worktree with group '" + group + "': " + + e.getMessage() + " — the group must exist, and the fleetd operator (" + + System.getProperty("user.name") + ") must be a member of it", e); + } + log.info("worktreeGroup={} shared repoRoot={} worktreePath={} paths={}", + group, repoRoot, worktreePath, touched); + } + + /** + * {@code chgrp}/{@code chmod g+rwX} {@code path} to {@link #group}. When {@code recursive}, + * also walks the directories under {@code path} (including {@code path} itself, when it is a + * directory) and sets setgid on each — directories only, per the javadoc on + * {@link #shareWithGroup}. + */ + private void shareGroupPath(String path, boolean recursive) { + List chgrp = new ArrayList<>(List.of("chgrp")); + if (recursive) chgrp.add("-R"); + chgrp.add(group); + chgrp.add(path); + shareGroupRunner.apply(chgrp.toArray(new String[0])); + + List chmod = new ArrayList<>(List.of("chmod")); + if (recursive) chmod.add("-R"); + chmod.add("g+rwX"); + chmod.add(path); + shareGroupRunner.apply(chmod.toArray(new String[0])); + + if (recursive) { + shareGroupRunner.apply(new String[]{"find", path, "-type", "d", "-exec", "chmod", "g+s", "{}", "+"}); + } + } + /** * Every {@code refs/wip/*} ref (see {@link WipRef}). The committer date is read as a unix * count of seconds and converted to millis. {@code %00} (NUL) separates the fields because a diff --git a/fleetd/src/main/java/dev/ltms/fleet/session/SessionManager.java b/fleetd/src/main/java/dev/ltms/fleet/session/SessionManager.java index c502dbe..e135bb7 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/session/SessionManager.java +++ b/fleetd/src/main/java/dev/ltms/fleet/session/SessionManager.java @@ -472,6 +472,10 @@ public final class SessionManager implements TurnListener { try { path = worktrees.add(repoRoot, branch, wt.baseRef()); worktrees.overlayParity(repoRoot, path, launcher.parityOverlay(preResolvedProfile)); + // fleetd #185 stage 3: MUST run after overlayParity, not folded into add() — overlayParity + // copies more files into the worktree after add() returns, so sharing the group any earlier + // leaves those overlay files operator-owned and read-only for a different-uid member. + worktrees.shareWithGroup(repoRoot, path); handle = launcher.spawn(new SpawnRequest(profile, path, callerCwd, sessionName, resumeSessionId, memberRole)); } catch (RuntimeException e) { log.warn("spawn failed for profile={} role={} branch={} path={}: {}", diff --git a/fleetd/src/main/java/dev/ltms/fleet/session/Worktrees.java b/fleetd/src/main/java/dev/ltms/fleet/session/Worktrees.java index e49e928..2005f3c 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/session/Worktrees.java +++ b/fleetd/src/main/java/dev/ltms/fleet/session/Worktrees.java @@ -98,4 +98,21 @@ public interface Worktrees { /** CB-586: the operator-visible census of {@code refs/wip/*} in one repository. */ record WipRefStats(int count, long costBytes) { } + + /** + * Make {@code repoRoot}'s git store and {@code worktreePath} writable by the configured group + * (fleetd #185 stage 3), so a member spawned as a different OS user (see + * {@code memberHerdrSocket}) can write its own worktree, its per-worktree git metadata, and + * its own commit objects. No-op when no group is configured. + * + *

This isolates credentials, not the repository. A member in the group can + * still write the operator's git objects and refs in the shared repo — this only fixes file + * ownership/permissions so a different-uid member can work at all, it grants no narrower access + * than that. + * + * @param repoRoot the repository whose git store ({@code .git/objects}, {@code refs}, + * {@code logs}, {@code worktrees}, {@code packed-refs}) needs sharing + * @param worktreePath the linked worktree's own directory + */ + void shareWithGroup(String repoRoot, String worktreePath); } diff --git a/fleetd/src/test/java/dev/ltms/fleet/config/FleetConfigTest.java b/fleetd/src/test/java/dev/ltms/fleet/config/FleetConfigTest.java index 7b90c71..7a4f09f 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/config/FleetConfigTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/config/FleetConfigTest.java @@ -1036,6 +1036,35 @@ class FleetConfigTest { assertEquals(LeadMailbox.DEFAULT_PREFETCH, noEnv.prefetchOrDefault()); } + @Test + void absentWorktreeGroupLeavesItNull(@TempDir Path dir) throws Exception { + Path f = dir.resolve("no-worktree-group.yaml"); + Files.writeString(f, "bind:\n port: 8080\n"); + + FleetConfig cfg = FleetConfig.load(f); + assertNull(cfg.worktreeGroup(), "no worktreeGroup: key → null → GitWorktrees.shareWithGroup is a no-op"); + } + + @Test + void worktreeGroupKeyParses(@TempDir Path dir) throws Exception { + Path f = dir.resolve("worktree-group.yaml"); + Files.writeString(f, "bind:\n port: 8080\nworktreeGroup: fleet-workers\n"); + + FleetConfig cfg = FleetConfig.load(f); + assertEquals("fleet-workers", cfg.worktreeGroup()); + } + + @Test + void absentWorktreeGroupSurvivesTheBackCompatConstructorChain() { + // fleetd #185 stage 3: withDefaults() (and every pre-existing call site) must not silently + // drop a live worktreeGroup by routing through a back-compat constructor that defaults it + // to null. + FleetConfig cfg = new FleetConfig(null, null, null, Map.of(), null, null, null, null, null, + null, null, null, null, null, null, null, null, null, null, null, "fleet-workers"); + assertEquals("fleet-workers", cfg.withDefaults().worktreeGroup(), + "withDefaults() must carry a configured worktreeGroup through unchanged"); + } + @Test void absentPrimaryBlockLeavesPrimaryNull(@TempDir Path dir) throws Exception { Path f = dir.resolve("no-primary.yaml"); diff --git a/fleetd/src/test/java/dev/ltms/fleet/session/FakeWorktrees.java b/fleetd/src/test/java/dev/ltms/fleet/session/FakeWorktrees.java index 3f1ac5a..847514f 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/session/FakeWorktrees.java +++ b/fleetd/src/test/java/dev/ltms/fleet/session/FakeWorktrees.java @@ -30,12 +30,19 @@ public final class FakeWorktrees implements Worktrees { public record PruneCall(String repoRoot, long minAgeMillis) { } + public record ShareCall(String repoRoot, String worktreePath) { + } + private final List addCalls = new CopyOnWriteArrayList<>(); private final List removeCalls = new CopyOnWriteArrayList<>(); private final List overlayCalls = new CopyOnWriteArrayList<>(); private final List repoRootCalls = new CopyOnWriteArrayList<>(); private final List snapshotCalls = new CopyOnWriteArrayList<>(); private final List pruneCalls = new CopyOnWriteArrayList<>(); + private final List shareCalls = new CopyOnWriteArrayList<>(); + /** Tags every {@code overlayParity}/{@code shareWithGroup} call in call order, so a test can + * pin that sharing runs after the overlay copy (fleetd #185 stage 3). */ + private final List overlayShareOrder = new CopyOnWriteArrayList<>(); private final Set existingPaths = ConcurrentHashMap.newKeySet(); private final Set trackedPaths = ConcurrentHashMap.newKeySet(); private final AtomicLong snapshotSeq = new AtomicLong(); @@ -136,6 +143,13 @@ public final class FakeWorktrees implements Worktrees { } overlayCalls.add(new OverlayCall(repoRoot, worktreePath, List.copyOf(overlay), List.copyOf(copied), List.copyOf(skipped))); + overlayShareOrder.add("overlay:" + worktreePath); + } + + @Override + public void shareWithGroup(String repoRoot, String worktreePath) { + shareCalls.add(new ShareCall(repoRoot, worktreePath)); + overlayShareOrder.add("share:" + worktreePath); } @Override @@ -203,4 +217,17 @@ public final class FakeWorktrees implements Worktrees { public SnapshotCall lastSnapshot() { return snapshotCalls.isEmpty() ? null : snapshotCalls.getLast(); } + + public List shareCalls() { + return List.copyOf(shareCalls); + } + + public ShareCall lastShare() { + return shareCalls.isEmpty() ? null : shareCalls.getLast(); + } + + /** Call-order tags ({@code "overlay:"}/{@code "share:"}) — see field javadoc. */ + public List overlayShareOrder() { + return List.copyOf(overlayShareOrder); + } } 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 74e99eb..c104a46 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/session/GitWorktreesTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/session/GitWorktreesTest.java @@ -924,4 +924,103 @@ class GitWorktreesTest { assertEquals(2, stats.count(), "two snapshot refs are reported"); assertTrue(stats.costBytes() > 0, "the cost of the snapshots is a positive byte count"); } + + /** + * fleetd #185 stage 3: a recording {@link java.util.function.Function} test seam stands in for + * every process {@link GitWorktrees#shareWithGroup} would run — no real second OS user/group + * exists on this host, so these are unit tests against that seam, not a live-group integration + * test (out of scope per the ticket). + */ + private static List joined(String[] command) { + return List.of(command); + } + + /** {@code worktreeGroup} absent ⇒ zero processes spawned and no git config written. */ + @Test + void shareWithGroupIsNoopWhenNoGroupConfigured(@TempDir Path tmp) throws Exception { + Path repo = initRepo(tmp.resolve("repo")); + List> recorded = new java.util.ArrayList<>(); + java.util.function.Function recordingRunner = cmd -> { + recorded.add(joined(cmd)); + return ""; + }; + GitWorktrees gitWorktrees = new GitWorktrees(tmp.resolve("wts").toString(), null, _ -> {}, recordingRunner); + + gitWorktrees.shareWithGroup(repo.toString(), repo.resolve("some-worktree").toString()); + + assertTrue(recorded.isEmpty(), "no group configured must spawn no process at all: " + recorded); + } + + /** A configured group runs {@code git config core.sharedRepository group} first, then + * chgrp/chmod/setgid over every path {@link GitWorktrees#shareWithGroup} documents. */ + @Test + void shareWithGroupRunsConfigThenChgrpChmodSetgidPerPath(@TempDir Path tmp) throws Exception { + Path repo = initRepo(tmp.resolve("repo")); + String repoRoot = repo.toString(); + String worktreePath = repo.resolve("some-worktree").toString(); + List> recorded = new java.util.ArrayList<>(); + java.util.function.Function recordingRunner = cmd -> { + recorded.add(joined(cmd)); + return ""; + }; + GitWorktrees gitWorktrees = + new GitWorktrees(tmp.resolve("wts").toString(), "devteam", _ -> {}, recordingRunner); + + gitWorktrees.shareWithGroup(repoRoot, worktreePath); + + assertEquals(List.of("git", "-C", repoRoot, "config", "core.sharedRepository", "group"), recorded.get(0), + "core.sharedRepository must be set first, so it keeps working after the one-time fix-up"); + + for (String dir : List.of(worktreePath, repoRoot + "/.git/objects", repoRoot + "/.git/refs", + repoRoot + "/.git/logs", repoRoot + "/.git/worktrees")) { + assertTrue(recorded.contains(List.of("chgrp", "-R", "devteam", dir)), "missing chgrp -R for " + dir); + assertTrue(recorded.contains(List.of("chmod", "-R", "g+rwX", dir)), "missing chmod -R for " + dir); + assertTrue(recorded.contains(List.of("find", dir, "-type", "d", "-exec", "chmod", "g+s", "{}", "+")), + "missing setgid find pass for " + dir); + } + // packed-refs does not exist in a freshly-init'd repo (only git gc / pack-refs creates it) — + // tolerated absence, so it must not appear at all: no recursive/-R treatment for a plain file. + String packedRefs = repoRoot + "/.git/packed-refs"; + assertTrue(recorded.stream().noneMatch(c -> c.contains(packedRefs)), + "packed-refs is absent here and must be skipped, not chgrp'd: " + recorded); + } + + /** {@code packed-refs}, when present, is chgrp/chmod'd but never setgid'd (it is a file, not a dir). */ + @Test + void shareWithGroupIncludesPackedRefsWhenPresent(@TempDir Path tmp) throws Exception { + Path repo = initRepo(tmp.resolve("repo")); + String repoRoot = repo.toString(); + Path packedRefsPath = repo.resolve(".git/packed-refs"); + Files.writeString(packedRefsPath, ""); + List> recorded = new java.util.ArrayList<>(); + java.util.function.Function recordingRunner = cmd -> { + recorded.add(joined(cmd)); + return ""; + }; + GitWorktrees gitWorktrees = + new GitWorktrees(tmp.resolve("wts").toString(), "devteam", _ -> {}, recordingRunner); + + gitWorktrees.shareWithGroup(repoRoot, repo.resolve("some-worktree").toString()); + + String packedRefs = packedRefsPath.toString(); + assertTrue(recorded.contains(List.of("chgrp", "devteam", packedRefs)), + "packed-refs must be chgrp'd non-recursively when present: " + recorded); + assertTrue(recorded.contains(List.of("chmod", "g+rwX", packedRefs)), + "packed-refs must be chmod'd non-recursively when present: " + recorded); + assertTrue(recorded.stream().noneMatch(c -> c.contains("find") && c.contains(packedRefs)), + "packed-refs (a file) must never get the recursive setgid pass: " + recorded); + } + + /** A group that does not exist (or that the operator is not a member of) fails loudly, naming it. */ + @Test + void shareWithGroupThrowsNamingTheGroupWhenChgrpFails(@TempDir Path tmp) throws Exception { + Path repo = initRepo(tmp.resolve("repo")); + GitWorktrees gitWorktrees = new GitWorktrees(tmp.resolve("wts").toString(), "cb185-nonexistent-group-zz"); + String wt = new GitWorktrees(tmp.resolve("wts").toString()).add(repo.toString(), "cb-185-share", "HEAD"); + + WorktreeException e = assertThrows(WorktreeException.class, + () -> gitWorktrees.shareWithGroup(repo.toString(), wt)); + assertTrue(e.getMessage().contains("cb185-nonexistent-group-zz"), + "exception must name the missing/refused group: " + e.getMessage()); + } } diff --git a/fleetd/src/test/java/dev/ltms/fleet/session/SessionManagerTest.java b/fleetd/src/test/java/dev/ltms/fleet/session/SessionManagerTest.java index 89bfc87..08f44b6 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/session/SessionManagerTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/session/SessionManagerTest.java @@ -148,6 +148,10 @@ class SessionManagerTest { return 0; } + @Override + public void shareWithGroup(String repoRoot, String worktreePath) { + } + List removeCalls() { return List.copyOf(removeCalls); } diff --git a/fleetd/src/test/java/dev/ltms/fleet/session/WorktreeSessionManagerTest.java b/fleetd/src/test/java/dev/ltms/fleet/session/WorktreeSessionManagerTest.java index 5e1b0eb..b6656e2 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/session/WorktreeSessionManagerTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/session/WorktreeSessionManagerTest.java @@ -163,6 +163,37 @@ class WorktreeSessionManagerTest { "tracked copied paths are --skip-worktree'd"); } + /** + * fleetd #185 stage 3, THE TRAP: {@code overlayParity} copies more files into the worktree + * AFTER {@code add} returns, so {@code shareWithGroup} must run after it, not folded into + * {@code add()} — otherwise every overlay file lands operator-owned and unwritable for a + * different-uid member, with a green test suite hiding it. + */ + @Test + void shareWithGroupRunsAfterOverlayParityNotBeforeIt() { + FakeHerdr herdr = new FakeHerdr(); + FakeWorktrees worktrees = new FakeWorktrees().withRepoRoot("/repo").withPrefix("/wt") + .track(".envrc"); + SessionManager sessions = new SessionManager(workerService(herdr), worktrees); + + MemberSession s = sessions.acquire("ltms-local", null, "/caller/proj", null, + new WorktreeRequest("cb-185", null)); + + assertEquals(1, worktrees.overlayCalls().size(), "overlayParity ran exactly once"); + assertEquals(1, worktrees.shareCalls().size(), "shareWithGroup ran exactly once"); + FakeWorktrees.OverlayCall overlay = worktrees.lastOverlay(); + FakeWorktrees.ShareCall share = worktrees.lastShare(); + assertEquals(s.worktree(), overlay.worktreePath()); + assertEquals(s.worktree(), share.worktreePath()); + + List order = worktrees.overlayShareOrder(); + int overlayIndex = order.indexOf("overlay:" + s.worktree()); + int shareIndex = order.indexOf("share:" + s.worktree()); + assertTrue(overlayIndex >= 0 && shareIndex >= 0, "both calls must be recorded: " + order); + assertTrue(overlayIndex < shareIndex, + "shareWithGroup MUST run after overlayParity, not before/inside add(): " + order); + } + @Test void releaseRemovesWorktreeButDoesNotDeleteBranch() { FakeHerdr herdr = new FakeHerdr(); From 6d82ca95a45b97f32a643b2e5c26e4c32c799087 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Mon, 31 Aug 2026 21:34:49 +0700 Subject: [PATCH 2/2] #185: skip absent git paths, and resolve the git dir instead of assuming .git Two defects in the stage-3 share pass, both of which would have failed EVERY provisioning spawn once worktreeGroup was set, not only the two-user case. .git/logs was handed to chgrp unguarded while packed-refs was guarded. It does not exist with core.logAllRefUpdates=false, or before the first ref update, and chgrp on a missing path exits non-zero -- surfacing as a WorktreeException that blames a group which is in fact fine. Every path is now skipped when absent. repoRoot + "/.git" was hardcoded. That is a FILE, not a directory, when the checkout is itself a linked worktree -- the very thing this class creates for every member. It now asks git: rev-parse --git-common-dir, resolved against repoRoot because git answers relatively for an ordinary checkout. Both new tests were watched failing with the fix removed before being kept. --- .../dev/ltms/fleet/session/GitWorktrees.java | 72 ++++++++++++++--- .../ltms/fleet/session/GitWorktreesTest.java | 80 ++++++++++++++++++- 2 files changed, 137 insertions(+), 15 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 23d7740..2314d5a 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java +++ b/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java @@ -656,13 +656,31 @@ public final class GitWorktrees implements Worktrees { *

    *
  1. {@code git -C repoRoot config core.sharedRepository group} so every future write by * either uid stays group-writable;
    2. - *
  2. a one-time {@code chgrp}/{@code chmod g+rwX} fix-up over the worktree directory, the - * repo's {@code .git/objects}, {@code refs}, {@code logs}, {@code worktrees}, and (when - * present) {@code packed-refs}, with setgid ({@code chmod g+s}) applied only to the - * directories among them so files created later inherit the group;
    3. + *
  3. a one-time {@code chgrp}/{@code chmod g+rwX} fix-up over the worktree directory and, + * under the repo's common git directory, {@code objects}, {@code refs}, + * {@code logs}, {@code worktrees} and {@code packed-refs} — with setgid + * ({@code chmod g+s}) applied only to the directories among them, so files created later + * inherit the group;
    4. *
  4. one INFO line naming the group and the paths touched.
    5. *
* + *

Every path is skipped when it does not exist. {@code .git/logs} is absent in a repo + * with {@code core.logAllRefUpdates=false} or one that has had no ref update yet, and + * {@code packed-refs} is absent until refs are packed. Passing a missing path to {@code chgrp} + * exits non-zero, which would fail every provisioning spawn with a message blaming a + * group that is in fact fine. + * + *

The git directory is resolved, not assumed. {@code /.git} is a + * file, not a directory, when the checkout is itself a linked worktree — the very + * thing this class creates for every member. {@code git rev-parse --git-common-dir} gives the + * real shared store, and it may answer relatively, so it is resolved against {@code repoRoot}. + * + *

The fix-up re-runs on every spawn, by design. {@code core.sharedRepository=group} + * governs only what git writes after it is set; the walk is what covers everything + * already on disk. It is not redundant work to optimise away — dropping it silently leaves + * pre-existing objects unreadable to the member. It costs three walks of the object store per + * spawn (about 3000 files in this repo, well under a second, but it grows with the repo). + * *

This only fixes up file ownership/permissions on the operator's shared repo so a * different-uid member can write to it — it isolates credentials, not the repository. A member * in the group can still write the operator's git objects and refs. @@ -679,16 +697,12 @@ public final class GitWorktrees implements Worktrees { List touched = new ArrayList<>(); try { shareGroupRunner.apply(new String[]{"git", "-C", repoRoot, "config", "core.sharedRepository", "group"}); - for (String dir : List.of(worktreePath, repoRoot + "/.git/objects", repoRoot + "/.git/refs", - repoRoot + "/.git/logs", repoRoot + "/.git/worktrees")) { - shareGroupPath(dir, true); - touched.add(dir); - } - String packedRefs = repoRoot + "/.git/packed-refs"; - if (Files.exists(Path.of(packedRefs))) { - shareGroupPath(packedRefs, false); - touched.add(packedRefs); + String commonDir = gitCommonDir(repoRoot); + shareGroupPathIfPresent(worktreePath, true, touched); + for (String name : List.of("objects", "refs", "logs", "worktrees")) { + shareGroupPathIfPresent(commonDir + "/" + name, true, touched); } + shareGroupPathIfPresent(commonDir + "/packed-refs", false, touched); } catch (WorktreeException e) { throw new WorktreeException("cannot share worktree with group '" + group + "': " + e.getMessage() + " — the group must exist, and the fleetd operator (" @@ -698,6 +712,38 @@ public final class GitWorktrees implements Worktrees { group, repoRoot, worktreePath, touched); } + /** + * The repo's common git directory as an absolute path — where {@code objects}, + * {@code refs} and {@code worktrees} actually live. {@code git rev-parse --git-common-dir} + * answers relative to {@code repoRoot} in the ordinary case ({@code .git}) and absolutely for a + * linked worktree, so the answer is resolved against {@code repoRoot} either way. Never + * hardcode {@code repoRoot + "/.git"}: that is a FILE when the checkout is itself a linked + * worktree. + */ + private String gitCommonDir(String repoRoot) { + String answer = shareGroupRunner.apply( + new String[]{"git", "-C", repoRoot, "rev-parse", "--git-common-dir"}); + String trimmed = answer == null ? "" : answer.trim(); + if (trimmed.isEmpty()) { + trimmed = ".git"; + } + return Path.of(repoRoot).resolve(trimmed).normalize().toString(); + } + + /** + * {@link #shareGroupPath} when {@code path} exists, recording it in {@code touched}; otherwise + * nothing at all. A missing path is normal, not an error — see {@link #shareWithGroup}'s + * javadoc for which ones are routinely absent and why passing them to {@code chgrp} would fail + * every spawn. + */ + private void shareGroupPathIfPresent(String path, boolean recursive, List touched) { + if (!Files.exists(Path.of(path))) { + return; + } + shareGroupPath(path, recursive); + touched.add(path); + } + /** * {@code chgrp}/{@code chmod g+rwX} {@code path} to {@link #group}. When {@code recursive}, * also walks the directories under {@code path} (including {@code path} itself, when it is a 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 c104a46..21f5402 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/session/GitWorktreesTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/session/GitWorktreesTest.java @@ -935,6 +935,21 @@ class GitWorktreesTest { return List.of(command); } + /** Remove {@code path} and anything under it. Tolerates an already-absent path. */ + private static void deleteRecursively(Path path) throws Exception { + if (!Files.exists(path)) { + return; + } + if (Files.isDirectory(path)) { + try (java.util.stream.Stream children = Files.list(path)) { + for (Path child : children.toList()) { + deleteRecursively(child); + } + } + } + Files.delete(path); + } + /** {@code worktreeGroup} absent ⇒ zero processes spawned and no git config written. */ @Test void shareWithGroupIsNoopWhenNoGroupConfigured(@TempDir Path tmp) throws Exception { @@ -957,11 +972,14 @@ class GitWorktreesTest { void shareWithGroupRunsConfigThenChgrpChmodSetgidPerPath(@TempDir Path tmp) throws Exception { Path repo = initRepo(tmp.resolve("repo")); String repoRoot = repo.toString(); - String worktreePath = repo.resolve("some-worktree").toString(); + Path worktree = Files.createDirectories(repo.resolve("some-worktree")); + String worktreePath = worktree.toString(); + Files.createDirectories(repo.resolve(".git/worktrees")); List> recorded = new java.util.ArrayList<>(); java.util.function.Function recordingRunner = cmd -> { recorded.add(joined(cmd)); - return ""; + // What real git answers for an ordinary (non-linked) checkout: relative to repoRoot. + return List.of(cmd).contains("--git-common-dir") ? ".git\n" : ""; }; GitWorktrees gitWorktrees = new GitWorktrees(tmp.resolve("wts").toString(), "devteam", _ -> {}, recordingRunner); @@ -970,6 +988,9 @@ class GitWorktreesTest { assertEquals(List.of("git", "-C", repoRoot, "config", "core.sharedRepository", "group"), recorded.get(0), "core.sharedRepository must be set first, so it keeps working after the one-time fix-up"); + assertTrue(recorded.contains(List.of("git", "-C", repoRoot, "rev-parse", "--git-common-dir")), + "the git dir must be asked for, never hardcoded as /.git — that is a FILE " + + "when the checkout is itself a linked worktree: " + recorded); for (String dir : List.of(worktreePath, repoRoot + "/.git/objects", repoRoot + "/.git/refs", repoRoot + "/.git/logs", repoRoot + "/.git/worktrees")) { @@ -985,6 +1006,61 @@ class GitWorktreesTest { "packed-refs is absent here and must be skipped, not chgrp'd: " + recorded); } + /** + * A path that does not exist is skipped, never handed to {@code chgrp}. {@code .git/logs} is + * absent whenever {@code core.logAllRefUpdates} is false or no ref has been updated yet, and + * {@code chgrp} on a missing path exits non-zero — which would fail EVERY provisioning spawn + * with a message blaming a group that is in fact fine. + */ + @Test + void shareWithGroupSkipsPathsThatDoNotExist(@TempDir Path tmp) throws Exception { + Path repo = initRepo(tmp.resolve("repo")); + String repoRoot = repo.toString(); + deleteRecursively(repo.resolve(".git/logs")); + assertFalse(Files.exists(repo.resolve(".git/logs")), "fixture: .git/logs must be gone"); + List> recorded = new java.util.ArrayList<>(); + java.util.function.Function recordingRunner = cmd -> { + recorded.add(joined(cmd)); + return List.of(cmd).contains("--git-common-dir") ? ".git\n" : ""; + }; + GitWorktrees gitWorktrees = + new GitWorktrees(tmp.resolve("wts").toString(), "devteam", _ -> {}, recordingRunner); + + gitWorktrees.shareWithGroup(repoRoot, repo.resolve("no-such-worktree").toString()); + + String logs = repoRoot + "/.git/logs"; + assertTrue(recorded.stream().noneMatch(c -> c.contains(logs)), + "a missing .git/logs must be skipped, not chgrp'd: " + recorded); + assertTrue(recorded.stream().noneMatch(c -> c.contains(repo.resolve("no-such-worktree").toString())), + "a missing worktree path must be skipped too: " + recorded); + assertTrue(recorded.contains(List.of("chgrp", "-R", "devteam", repoRoot + "/.git/objects")), + "paths that DO exist are still shared: " + recorded); + } + + /** + * The git store is located by {@code rev-parse --git-common-dir}, not by appending + * {@code /.git}. When git answers with an absolute path — what it does for a linked worktree, + * where {@code /.git} is a file — every shared path must follow that answer. + */ + @Test + void shareWithGroupFollowsAnAbsoluteGitCommonDir(@TempDir Path tmp) throws Exception { + Path repo = initRepo(tmp.resolve("repo")); + Path realGitDir = repo.resolve(".git"); + List> recorded = new java.util.ArrayList<>(); + java.util.function.Function recordingRunner = cmd -> { + recorded.add(joined(cmd)); + return List.of(cmd).contains("--git-common-dir") ? realGitDir + "\n" : ""; + }; + GitWorktrees gitWorktrees = + new GitWorktrees(tmp.resolve("wts").toString(), "devteam", _ -> {}, recordingRunner); + + gitWorktrees.shareWithGroup(tmp.resolve("some/linked/worktree").toString(), + repo.resolve("wt").toString()); + + assertTrue(recorded.contains(List.of("chgrp", "-R", "devteam", realGitDir + "/objects")), + "objects must be taken from the reported common dir, not /.git: " + recorded); + } + /** {@code packed-refs}, when present, is chgrp/chmod'd but never setgid'd (it is a file, not a dir). */ @Test void shareWithGroupIncludesPackedRefsWhenPresent(@TempDir Path tmp) throws Exception {