From 08ce9aef11ca6d23cc300399a4682db9aaef1b02 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Mon, 31 Aug 2026 14:15:39 +0700 Subject: [PATCH] #161: resolve a pane by process ancestry, closing a worker->primary escalation PaneLocator's javadoc always claimed it found 'the agent pane whose process tree contains' a pid. It did not: paneOwnsPid matched only the pane's shell_pid and its foreground_processes. A process a member spawned -- python3, curl, any helper opening its own connection to 127.0.0.1:8765 -- matched no pane, so CallerResolver fell through to loopback-trust and resolved it as the PRIMARY. A member escalated to lead by shelling out. terminalForPid now builds the caller's ancestor set once (bounded at 32 generations, with a cycle guard) and matches any ancestor against a pane's pids. The set is reused across both herdr clients on the CB-185 two-daemon path. This only ever ADDS matches, which is the safe direction: the failure mode of the fix is a member correctly restricted, while the failure mode of the bug is a member acting as the lead. The no-match case still returns null, so the lead -- which maps to a pane named by leaders: -- still resolves as primary. Ancestry is walked through a new ParentResolver seam so the tests drive it from a fake pid->parent map rather than spawning real processes. Co-authored-by: fleetd worker --- .../dev/ltms/fleet/herdr/PaneLocator.java | 77 +++++++++-- .../dev/ltms/fleet/herdr/ParentResolver.java | 21 +++ .../ltms/fleet/herdr/FakeParentResolver.java | 27 ++++ .../dev/ltms/fleet/herdr/PaneLocatorTest.java | 123 ++++++++++++++++++ 4 files changed, 239 insertions(+), 9 deletions(-) create mode 100644 fleetd/src/main/java/dev/ltms/fleet/herdr/ParentResolver.java create mode 100644 fleetd/src/test/java/dev/ltms/fleet/herdr/FakeParentResolver.java diff --git a/fleetd/src/main/java/dev/ltms/fleet/herdr/PaneLocator.java b/fleetd/src/main/java/dev/ltms/fleet/herdr/PaneLocator.java index f2a0af4..7c5c89f 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/herdr/PaneLocator.java +++ b/fleetd/src/main/java/dev/ltms/fleet/herdr/PaneLocator.java @@ -2,8 +2,11 @@ package dev.ltms.fleet.herdr; import com.fasterxml.jackson.databind.JsonNode; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; +import java.util.OptionalLong; +import java.util.Set; /** * Resolves which herdr pane a process belongs to — the herdr half of connection-based MCP @@ -12,23 +15,46 @@ import java.util.Map; * is calling without the worker sending anything spoofable. * *

herdr owns the PID→pane truth: {@code pane.process_info} reports each pane's {@code shell_pid} - * and foreground process PIDs. This scans agent panes; a spawn-time {@code pid→terminal} cache is - * the obvious optimization once wired into {@code ClaudeCodeLauncher}. + * and foreground process PIDs. A pid that is neither of those directly — e.g. a grandchild a + * worker spawned, such as a {@code python3} or {@code curl} helper that opens its own MCP + * connection — is resolved by walking its ancestry (via {@link ParentResolver}) up to the root and + * matching any ancestor against a pane's {@code shell_pid} or foreground pids (CB-161). Without + * this walk such a pid matches no pane, and the caller falls through to loopback-trust and is + * resolved as the primary — a worker→primary privilege escalation. + * + *

This scans agent panes; a spawn-time {@code pid→terminal} cache is the obvious optimization + * once wired into {@code ClaudeCodeLauncher}. * *

CB-185 split the fleet across two herdr daemons — lead operations on one, members on the * other ({@code memberHerdrSocket}). A caller's pane can live on either daemon (a lead's * MCP connection resolves against the lead daemon; a member's against the member daemon), so this * must be able to search more than one client. {@link #PaneLocator(HerdrClient, HerdrClient)} * searches the lead client first, then the member client, and collapses to a single scan when the - * two are the same object (the historical single-daemon deployment). + * two are the same object (the historical single-daemon deployment). The caller's ancestor set is + * computed once per {@link #terminalForPid} call and reused across every client searched — it + * does not depend on which daemon a pane happens to live on. */ public final class PaneLocator { + /** + * Bound on how many ancestor generations {@link #ancestorsOf} walks. This runs on every MCP + * call, so a cycle or a pathologically deep process tree must not hang identity resolution; + * 32 generations is far more than any real worker→helper process tree needs. + */ + private static final int MAX_ANCESTRY_DEPTH = 32; + private final List herdrs; + private final ParentResolver parentResolver; /** Search only this client — the single-daemon deployment. */ public PaneLocator(HerdrClient herdr) { + this(herdr, ParentResolver.PROCESS_HANDLE); + } + + /** Search only this client, resolving ancestry through {@code parentResolver} — for tests. */ + public PaneLocator(HerdrClient herdr, ParentResolver parentResolver) { this.herdrs = List.of(herdr); + this.parentResolver = parentResolver; } /** @@ -37,7 +63,13 @@ public final class PaneLocator { * collapses to one client and one scan, exactly {@link #PaneLocator(HerdrClient)}'s behaviour. */ public PaneLocator(HerdrClient lead, HerdrClient member) { + this(lead, member, ParentResolver.PROCESS_HANDLE); + } + + /** Two-daemon deployment, resolving ancestry through {@code parentResolver} — for tests. */ + public PaneLocator(HerdrClient lead, HerdrClient member, ParentResolver parentResolver) { this.herdrs = lead == member ? List.of(lead) : List.of(lead, member); + this.parentResolver = parentResolver; } /** @@ -49,8 +81,9 @@ public final class PaneLocator { if (pid <= 0) { return null; } + Set ancestry = ancestorsOf(pid); for (HerdrClient herdr : herdrs) { - String terminal = terminalForPid(herdr, pid); + String terminal = terminalForPid(herdr, ancestry); if (terminal != null) { return terminal; } @@ -58,28 +91,54 @@ public final class PaneLocator { return null; } - private static String terminalForPid(HerdrClient herdr, long pid) { + /** + * {@code pid} itself plus its ancestor chain, walked through {@link #parentResolver} up to + * {@link #MAX_ANCESTRY_DEPTH} generations or pid 1, whichever comes first. A vanished ancestor + * ({@link ParentResolver#parentOf} returning empty) ends the walk without error — it just means + * the chain is shorter than the bound. A cycle in a fake resolver is caught by the "already + * seen" check and also ends the walk, so this can never loop. + */ + private Set ancestorsOf(long pid) { + Set ancestry = new LinkedHashSet<>(); + long current = pid; + for (int depth = 0; depth < MAX_ANCESTRY_DEPTH; depth++) { + if (current <= 0 || !ancestry.add(current)) { + break; // vanished/invalid pid, or a cycle back to a pid already recorded + } + if (current == 1) { + break; // reached the root of the process tree + } + OptionalLong parent = parentResolver.parentOf(current); + if (parent.isEmpty()) { + break; // vanished ancestor — not an error, just the end of the chain + } + current = parent.getAsLong(); + } + return ancestry; + } + + private static String terminalForPid(HerdrClient herdr, Set ancestry) { for (JsonNode pane : herdr.call("pane.list", Map.of()).path("panes")) { String paneId = pane.path("pane_id").asText(null); - if (paneId != null && paneOwnsPid(herdr, paneId, pid)) { + if (paneId != null && paneOwnsAnyOf(herdr, paneId, ancestry)) { return pane.path("terminal_id").asText(null); } } return null; } - private static boolean paneOwnsPid(HerdrClient herdr, String paneId, long pid) { + private static boolean paneOwnsAnyOf(HerdrClient herdr, String paneId, Set ancestry) { JsonNode info; try { info = herdr.call("pane.process_info", Map.of("pane_id", paneId)).path("process_info"); } catch (HerdrException e) { return false; // pane vanished mid-scan — just skip it } - if (info.path("shell_pid").asLong(-1) == pid) { + if (ancestry.contains(info.path("shell_pid").asLong(-1))) { return true; } for (JsonNode p : info.path("foreground_processes")) { - if (p.path("pid").asLong(-1) == pid) { + if (ancestry.contains(p.path("pid").asLong(-1))) { return true; } } diff --git a/fleetd/src/main/java/dev/ltms/fleet/herdr/ParentResolver.java b/fleetd/src/main/java/dev/ltms/fleet/herdr/ParentResolver.java new file mode 100644 index 0000000..6cc8149 --- /dev/null +++ b/fleetd/src/main/java/dev/ltms/fleet/herdr/ParentResolver.java @@ -0,0 +1,21 @@ +package dev.ltms.fleet.herdr; + +import java.util.OptionalLong; + +/** + * Resolves a pid's parent pid — the seam {@link PaneLocator} walks a process's ancestry through, + * so its tests can drive the walk from a fake pid→parent map instead of spawning real processes. + * + *

{@link #PROCESS_HANDLE} is the production implementation, backed by {@link ProcessHandle}. + */ +public interface ParentResolver { + + /** The parent pid of {@code pid}, or empty if {@code pid} is gone or has no known parent. */ + OptionalLong parentOf(long pid); + + /** Production resolver: asks the JVM's {@link ProcessHandle} view of the OS process tree. */ + ParentResolver PROCESS_HANDLE = pid -> ProcessHandle.of(pid) + .flatMap(ProcessHandle::parent) + .map(parent -> OptionalLong.of(parent.pid())) + .orElse(OptionalLong.empty()); +} diff --git a/fleetd/src/test/java/dev/ltms/fleet/herdr/FakeParentResolver.java b/fleetd/src/test/java/dev/ltms/fleet/herdr/FakeParentResolver.java new file mode 100644 index 0000000..0dcf0a1 --- /dev/null +++ b/fleetd/src/test/java/dev/ltms/fleet/herdr/FakeParentResolver.java @@ -0,0 +1,27 @@ +package dev.ltms.fleet.herdr; + +import java.util.HashMap; +import java.util.Map; +import java.util.OptionalLong; + +/** + * Fake {@link ParentResolver} backed by an explicit pid→parent map — lets {@link PaneLocatorTest} + * drive {@link PaneLocator}'s ancestry walk (grandchild pids, cycles) without spawning real OS + * processes. + */ +final class FakeParentResolver implements ParentResolver { + + private final Map parents = new HashMap<>(); + + /** {@code pid}'s parent is {@code parentPid}. A pid with no entry here has no known parent. */ + FakeParentResolver parent(long pid, long parentPid) { + parents.put(pid, parentPid); + return this; + } + + @Override + public OptionalLong parentOf(long pid) { + Long parent = parents.get(pid); + return parent == null ? OptionalLong.empty() : OptionalLong.of(parent); + } +} diff --git a/fleetd/src/test/java/dev/ltms/fleet/herdr/PaneLocatorTest.java b/fleetd/src/test/java/dev/ltms/fleet/herdr/PaneLocatorTest.java index 929ddc1..7208f05 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/herdr/PaneLocatorTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/herdr/PaneLocatorTest.java @@ -1,7 +1,11 @@ package dev.ltms.fleet.herdr; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; import org.junit.jupiter.api.Test; +import java.util.concurrent.atomic.AtomicInteger; + import static org.junit.jupiter.api.Assertions.*; /** Unit tests for PID → pane resolution (the herdr half of connection-based MCP identity). */ @@ -63,4 +67,123 @@ class PaneLocatorTest { long paneListCalls = shared.calls.stream().filter(c -> c.method().equals("pane.list")).count(); assertEquals(1, paneListCalls, "same-object lead/member must scan exactly once, not twice"); } + + // --- ancestry walk (CB-161: grandchild pids matched no pane, resolving as primary) -------- + + @Test + void stillResolvesAPidThatIsExactlyThePaneShellPid() { + // Regression: a pid with no parent chain at all — no ancestry walk is needed to match it. + OnePaneHerdr pane = new OnePaneHerdr("term_x", "pX", 5000, 6000); + PaneLocator loc = new PaneLocator(pane, new FakeParentResolver()); + assertEquals("term_x", loc.terminalForPid(5000)); + } + + @Test + void stillResolvesAPidThatIsExactlyAForegroundPid() { + // Regression: same as above, but matching via the foreground-processes list. + OnePaneHerdr pane = new OnePaneHerdr("term_x", "pX", 5000, 6000); + PaneLocator loc = new PaneLocator(pane, new FakeParentResolver()); + assertEquals("term_x", loc.terminalForPid(6000)); + } + + @Test + void resolvesAGrandchildPidTwoLevelsBelowTheShellPid() { + // The bug: a helper process a worker spawns (python3, curl, ...) is a grandchild of the + // pane's shell — not the shell_pid and not a foreground pid directly. Before the fix, + // paneOwnsPid only checked direct pid equality, so this pid matched no pane and the + // caller fell through to loopback-trust as the primary. + OnePaneHerdr pane = new OnePaneHerdr("term_x", "pX", 5000, 6000); + FakeParentResolver parents = new FakeParentResolver() + .parent(7002, 7001) // grandchild -> child + .parent(7001, 5000); // child -> shell (the pane's shell_pid) + PaneLocator loc = new PaneLocator(pane, parents); + assertEquals("term_x", loc.terminalForPid(7002)); + } + + @Test + void nullForAPidWhoseAncestryMatchesNoPane() { + // Must not break the other direction: a pid that truly belongs to nothing here (e.g. the + // real primary) must still resolve to null. Resolving everything to a worker would demote + // the actual lead and refuse every orchestration call. + OnePaneHerdr pane = new OnePaneHerdr("term_x", "pX", 5000, 6000); + FakeParentResolver parents = new FakeParentResolver() + .parent(9002, 9001) + .parent(9001, 9000); // chain never reaches 5000 or 6000 + PaneLocator loc = new PaneLocator(pane, parents); + assertNull(loc.terminalForPid(9002)); + } + + @Test + void ancestryWalkTerminatesOnACycleInsteadOfHanging() { + // A fake (or corrupted) parent map that cycles must not hang identity resolution, which + // runs on every MCP call. The walk must still terminate and correctly resolve to null. + OnePaneHerdr pane = new OnePaneHerdr("term_x", "pX", 5000, 6000); + FakeParentResolver parents = new FakeParentResolver() + .parent(100, 101) + .parent(101, 100); // cycle, never reaches the pane's pids + PaneLocator loc = new PaneLocator(pane, parents); + assertNull(loc.terminalForPid(100)); + } + + @Test + void ancestrySetIsComputedOnceAcrossBothClientsInTheTwoDaemonConstructor() { + // CB-185: the two-daemon constructor searches lead then member. The ancestor set is + // per-caller, not per-client — it must be walked once and reused, not recomputed for + // each client searched. + OnePaneHerdr pane = new OnePaneHerdr("term_x", "pX", 5000, 6000); + FakeParentResolver parents = new FakeParentResolver() + .parent(7002, 7001) + .parent(7001, 5000); + AtomicInteger calls = new AtomicInteger(); + ParentResolver counting = pid -> { + calls.incrementAndGet(); + return parents.parentOf(pid); + }; + HerdrClient noPanes = new FakeHerdr().withNoPanes(); + PaneLocator two = new PaneLocator(noPanes, pane, counting); + assertEquals("term_x", two.terminalForPid(7002)); + assertEquals(3, calls.get(), "ancestry must be walked once (3 lookups: 7002, 7001, 5000), " + + "not re-walked per herdr client"); + } + + /** Minimal single-pane {@link HerdrClient} fake, purpose-built for the ancestry tests above. */ + private static final class OnePaneHerdr implements HerdrClient { + private final ObjectMapper mapper = new ObjectMapper(); + private final String terminalId; + private final String paneId; + private final long shellPid; + private final long foregroundPid; + + OnePaneHerdr(String terminalId, String paneId, long shellPid, long foregroundPid) { + this.terminalId = terminalId; + this.paneId = paneId; + this.shellPid = shellPid; + this.foregroundPid = foregroundPid; + } + + @Override + public JsonNode call(String method, Object params) { + try { + return switch (method) { + case "pane.list" -> mapper.readTree((""" + {"type":"pane_list","panes":[ + {"pane_id":"%s","terminal_id":"%s","workspace_id":"w1","tab_id":"w1:t1","agent":"claude"}]}""") + .formatted(paneId, terminalId)); + case "pane.process_info" -> mapper.readTree((""" + {"type":"pane_process_info","process_info":{"pane_id":"%s","shell_pid":%d, + "foreground_processes":[{"pid":%d,"name":"node","argv0":"claude"}]}}""") + .formatted(paneId, shellPid, foregroundPid)); + default -> throw new HerdrException("OnePaneHerdr has no canned response for " + method); + }; + } catch (HerdrException e) { + throw e; + } catch (Exception e) { + throw new HerdrException("OnePaneHerdr decode failed for " + method, e); + } + } + + @Override + public void close() { + } + } }