Compare commits
7 Commits
| Author | SHA1 | Date | |
|---|---|---|---|
| 615af4ed0a | |||
| d89ae94a2e | |||
| 23ada1981e | |||
| a237fbff9d | |||
| 31d5516991 | |||
| 5ba05d0bdb | |||
| 25726a5ae7 |
@@ -38,6 +38,11 @@ public final class AgentControl {
|
||||
this.herdr = herdr;
|
||||
}
|
||||
|
||||
/** The herdr daemon this control object sends its agent calls to. */
|
||||
public HerdrClient herdr() {
|
||||
return herdr;
|
||||
}
|
||||
|
||||
/** One agent-targeted call, translating a terminal id to its pane id (retrying once fresh). */
|
||||
private JsonNode agentCall(String method, String target, Map<String, Object> extra) {
|
||||
String resolved = resolveTarget(target);
|
||||
|
||||
@@ -2,6 +2,7 @@ package dev.ltms.fleet.member;
|
||||
|
||||
import dev.ltms.fleet.config.FleetConfig;
|
||||
import dev.ltms.fleet.herdr.Agent;
|
||||
import dev.ltms.fleet.herdr.HerdrClient;
|
||||
import dev.ltms.fleet.peer.Capability;
|
||||
import dev.ltms.fleet.peer.MemberRole;
|
||||
import dev.ltms.fleet.peer.PeerHandle;
|
||||
@@ -21,6 +22,7 @@ import java.util.ArrayList;
|
||||
import java.util.Collections;
|
||||
import java.util.EnumSet;
|
||||
import java.util.HashSet;
|
||||
import java.util.IdentityHashMap;
|
||||
import java.util.LinkedHashMap;
|
||||
import java.util.List;
|
||||
import java.util.Map;
|
||||
@@ -44,12 +46,13 @@ import java.util.stream.Collectors;
|
||||
* the single adapter that declares it. Profiles partition cleanly across adapters: the
|
||||
* constructor rejects a name claimed by two.</li>
|
||||
* <li><strong>By pane id</strong> — {@link #stop} routes to the adapter that spawned that pane
|
||||
* (recorded at spawn time). A pane the composite never spawned (only real for a caller that
|
||||
* hand-rolls an id) falls back to the first delegate; teardown is pane-id addressed and
|
||||
* tab cleanup is single-occupant guarded, so it is safe either way.</li>
|
||||
* (recorded at spawn time). A pane the composite never spawned can use the fallback route
|
||||
* in a one-daemon fleet. With more than one herdr daemon, its owner is unknown, so stop refuses
|
||||
* the ambiguous id rather than closing a pane on an arbitrary herdr daemon.</li>
|
||||
* <li><strong>Fleet-wide</strong> — {@link #reapOrphanWorkers} and {@link #capabilities} fan out
|
||||
* and combine. {@link #list} is deduplicated by pane id because every herdr-backed delegate
|
||||
* shares one herdr connection and so reports the same global agent set.</li>
|
||||
* and combine. {@link #list} is deduplicated by (owning daemon, pane id): delegates that share
|
||||
* one herdr connection report the same global agent set, but two daemons can each hold a pane
|
||||
* called {@code w1:p1}, so the daemon has to be part of the key.</li>
|
||||
* </ul>
|
||||
*
|
||||
* <p>CB-518: an unqualified spawn is routed through a {@link PlacementPolicy}. The default
|
||||
@@ -435,12 +438,32 @@ public final class CompositePeerLauncher implements PeerLauncher {
|
||||
|
||||
@Override
|
||||
public void stop(String id) {
|
||||
HerdrPeerLauncher d = spawnedBy.remove(id);
|
||||
HerdrPeerLauncher d = spawnedBy.get(id);
|
||||
if (d == null) {
|
||||
log.debug("stop({}) — no recorded owner, routing to the first adapter (pane-addressed)", id);
|
||||
if (herdrDaemonCount() != 1) {
|
||||
throw new IllegalArgumentException("ambiguous paneId '" + id
|
||||
+ "': no owning herdr daemon was recorded");
|
||||
}
|
||||
log.debug("stop({}) — no recorded owner in a single-daemon fleet", id);
|
||||
d = delegates.getFirst();
|
||||
}
|
||||
// Drop the owner record only after the delegate accepted the stop. Removing it first meant a
|
||||
// delegate that threw left the pane alive with its owner forgotten, so the retry fell into
|
||||
// the ambiguous branch above and refused the id for good.
|
||||
d.stop(id);
|
||||
spawnedBy.remove(id);
|
||||
}
|
||||
|
||||
/**
|
||||
* Count actual herdr daemons, not peer adapter kinds. Identity is intentional: separate client
|
||||
* objects may represent different daemons even if a client later implements value equality.
|
||||
*/
|
||||
private int herdrDaemonCount() {
|
||||
Set<HerdrClient> daemons = Collections.newSetFromMap(new IdentityHashMap<>());
|
||||
for (HerdrPeerLauncher delegate : delegates) {
|
||||
daemons.add(delegate.herdr());
|
||||
}
|
||||
return daemons.size();
|
||||
}
|
||||
|
||||
@Override
|
||||
@@ -475,14 +498,24 @@ public final class CompositePeerLauncher implements PeerLauncher {
|
||||
return route(profileName).capabilities();
|
||||
}
|
||||
|
||||
/** Every herdr agent, deduplicated by pane id (all delegates share one herdr and list globally). */
|
||||
/**
|
||||
* Every herdr agent, deduplicated by (owning daemon, pane id).
|
||||
*
|
||||
* <p>Delegates that share one {@link HerdrClient} see the same global agent set, so listing them
|
||||
* both would report every agent twice — that is what the dedupe is for. But pane ids are
|
||||
* per-daemon counters, so two daemons really can both hold {@code w1:p1} on different panes.
|
||||
* Keying on the pane id alone would silently drop one of them from {@code fleet_list} and from
|
||||
* every status view built on it. The daemon is part of the key for exactly that reason.
|
||||
*/
|
||||
@Override
|
||||
public List<Agent> list() {
|
||||
Map<HerdrClient, Integer> daemonIndex = new IdentityHashMap<>();
|
||||
Map<String, Agent> byPane = new LinkedHashMap<>();
|
||||
for (HerdrPeerLauncher d : delegates) {
|
||||
int daemon = daemonIndex.computeIfAbsent(d.herdr(), _ -> daemonIndex.size());
|
||||
for (Agent a : d.list()) {
|
||||
if (a.paneId() != null) {
|
||||
byPane.putIfAbsent(a.paneId(), a);
|
||||
byPane.putIfAbsent(daemon + "\u0000" + a.paneId(), a);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -3,6 +3,7 @@ package dev.ltms.fleet.member;
|
||||
import dev.ltms.fleet.config.FleetConfig;
|
||||
import dev.ltms.fleet.herdr.Agent;
|
||||
import dev.ltms.fleet.herdr.AgentControl;
|
||||
import dev.ltms.fleet.herdr.HerdrClient;
|
||||
import dev.ltms.fleet.herdr.HerdrException;
|
||||
import dev.ltms.fleet.herdr.Tab;
|
||||
import dev.ltms.fleet.herdr.Workspace;
|
||||
@@ -520,6 +521,11 @@ public abstract class HerdrPeerLauncher implements PeerLauncher {
|
||||
req.sessionName(), spawned.agentSessionId(), spawned.receipt());
|
||||
}
|
||||
|
||||
/** The herdr daemon that owns this launcher's pane coordinates. */
|
||||
public HerdrClient herdr() {
|
||||
return agents.herdr();
|
||||
}
|
||||
|
||||
@Override
|
||||
public String effectiveCwd(SpawnRequest req) {
|
||||
return effectiveCwd(req.profileName(), req.requestedCwd(), req.callerCwd());
|
||||
@@ -1002,6 +1008,13 @@ public abstract class HerdrPeerLauncher implements PeerLauncher {
|
||||
* applied before the login shell runs and a sourced file can (and did) undo it. The control is
|
||||
* the ZDOTDIR scrub ({@link #applyEnvironmentAllowListPolicy}); {@code known}/{@code allow}
|
||||
* remain as reporting only via {@link #logCredentialGap}.
|
||||
*
|
||||
* <p>CB-633 follow-up (#192): under {@code allow-list} this method does NOT call {@link
|
||||
* #logCredentialGap} itself — at this point (called from {@link #baseEnv}, before {@link
|
||||
* #applyEnvironmentAllowListPolicy} runs) we do not yet know whether the pane's shell is zsh, so
|
||||
* we cannot yet pick correct wording. That decision, and the call, are deferred entirely to
|
||||
* {@link #applyEnvironmentAllowListPolicy}, which knows by then whether the scrub will actually
|
||||
* run.
|
||||
*/
|
||||
private void applyMemberCredentialPolicy(Map<String, String> workerEnv) {
|
||||
FleetConfig.MemberCredentials creds = memberCredentials == null ? null : memberCredentials.get();
|
||||
@@ -1010,8 +1023,8 @@ public abstract class HerdrPeerLauncher implements PeerLauncher {
|
||||
}
|
||||
if (!creds.isAllowList()) {
|
||||
overlayBlockedCredentials(workerEnv, creds);
|
||||
logCredentialGap(creds, null);
|
||||
}
|
||||
logCredentialGap(creds);
|
||||
}
|
||||
|
||||
/** Put {@link #BLOCKED_CREDENTIAL_SENTINEL} over every blocked name in the pane-creation env map. */
|
||||
@@ -1061,12 +1074,15 @@ public abstract class HerdrPeerLauncher implements PeerLauncher {
|
||||
// logCredentialGap's WARN (below) is the only signal for this path.
|
||||
warnNonZsh(loginShell);
|
||||
overlayBlockedCredentials(launch.env(), creds);
|
||||
logCredentialGap(creds);
|
||||
logCredentialGap(creds, null);
|
||||
return null;
|
||||
}
|
||||
// Only reached when the scrub is actually about to run — the count below describes that
|
||||
// scrub, so it must not be logged before this gate (see the non-zsh branch above).
|
||||
// scrub, so it must not be logged before this gate (see the non-zsh branch above). Same
|
||||
// reasoning gates logCredentialGap's wording: passing the derived `allowed` set (non-null)
|
||||
// here, and ONLY here, is what tells it the scrub will really blank an unkept name — #192.
|
||||
logAllowListCoverage(allowed);
|
||||
logCredentialGap(creds, allowed);
|
||||
Path dir = EnvAllowListScrub.generate(Path.of(System.getProperty("java.io.tmpdir")), allowed);
|
||||
launch.env().put("ZDOTDIR", dir.toAbsolutePath().toString());
|
||||
log.info("memberCredentials policy=allow-list: profile={} generated ZDOTDIR {} — derived "
|
||||
@@ -1171,18 +1187,57 @@ public abstract class HerdrPeerLauncher implements PeerLauncher {
|
||||
private static final Pattern CREDENTIAL_SHAPED_NAME =
|
||||
Pattern.compile("(?i).*(TOKEN|SECRET|_KEY|APIKEY|PASSWORD|CREDENTIAL|AUTH).*");
|
||||
|
||||
/** Guards {@link #logCredentialGap} to one WARN per launcher instance, not one per spawn. */
|
||||
private final AtomicBoolean credentialGapLogged = new AtomicBoolean();
|
||||
/**
|
||||
* Guards the {@code effectiveAllowed == null} branch of {@link #logCredentialGap} — the
|
||||
* genuinely-unprotected report (deny-by-default, and the allow-list non-zsh fallback) — to one
|
||||
* WARN per launcher instance, not one per spawn.
|
||||
*
|
||||
* <p>CB-633 follow-up (#192): kept SEPARATE from {@link #allowListGapLogged} on purpose.
|
||||
* {@code memberCredentials} is a live, re-read-per-spawn supplier, so the policy can change
|
||||
* between two spawns on the same launcher. A single shared flag would let a harmless allow-list
|
||||
* INFO on spawn 1 permanently suppress the real deny-by-default WARN a later spawn deserves —
|
||||
* the report that matters most getting hidden by the report that doesn't. Two flags mean each
|
||||
* report kind fires exactly once, independent of what the other kind already logged.
|
||||
*/
|
||||
private final AtomicBoolean unprotectedGapLogged = new AtomicBoolean();
|
||||
|
||||
/**
|
||||
* Guards the {@code effectiveAllowed != null} branch of {@link #logCredentialGap} — the
|
||||
* allow-list-scrub-covered report — to one INFO per launcher instance. See {@link
|
||||
* #unprotectedGapLogged}'s javadoc for why this is a separate flag rather than a shared one.
|
||||
*/
|
||||
private final AtomicBoolean allowListGapLogged = new AtomicBoolean();
|
||||
|
||||
/**
|
||||
* CB-596 criterion 4: a credential-shaped host env var name on neither {@code known} nor
|
||||
* {@code allow} is not silently allowed — it is reported. {@link #hostEnvNames} enumerates the
|
||||
* daemon's own environment (see that field's javadoc for why the daemon's env is read rather
|
||||
* than the spawned pane's, which the daemon has no channel to inspect at spawn time); this logs
|
||||
* every such NAME, at WARN, at most once per launcher instance — never a value, a prefix of a
|
||||
* value, or a hash of a value, so the log itself cannot leak anything.
|
||||
* every such NAME — never a value, a prefix of a value, or a hash of a value, so the log itself
|
||||
* cannot leak anything.
|
||||
*
|
||||
* <p>CB-633 follow-up (#192): {@code effectiveAllowed} picks the wording, and it must NOT be
|
||||
* picked from {@code creds.isAllowList()} — see {@link #applyEnvironmentAllowListPolicy}'s
|
||||
* javadoc for the reasoning this mirrors. {@code null} means no scrub-derived allow-list was
|
||||
* computed for this call — true on the deny-by-default path AND on the allow-list non-zsh
|
||||
* fallback, where nothing is ever scrubbed — so the whole gap is real and gets the WARN,
|
||||
* unchanged from before this fix. Non-null means this call came from the zsh branch of {@link
|
||||
* #applyEnvironmentAllowListPolicy}, reachable ONLY after that method's own zsh gate — but
|
||||
* {@code effectiveAllowed} is a SUPERSET of {@code known ∪ allow}: {@link MemberEnvAllowList#derive}
|
||||
* also unions in every profile's {@code gitTokenEnv}/{@code gitHostEnv}/{@code tokenEnv}/
|
||||
* {@code env:} keys, and {@link #derivedAllowedNames} further unions in this very spawn's own
|
||||
* env keys — so a name can be in the gap (uncovered by {@code known}/{@code allow}) AND still be
|
||||
* kept by the derived allow-list, in which case the scrub does NOT blank it and the member DOES
|
||||
* inherit it. Lead review on #192 caught this: the first cut of this fix reported the WHOLE gap
|
||||
* as scrub-blanked without checking that, which reported a real leak as safe — the exact
|
||||
* inversion #192 exists to remove. So on this path the gap is split with {@link
|
||||
* MemberEnvAllowList#keeps}, the SAME predicate the generated scrub itself evaluates, so this
|
||||
* split cannot drift from what the scrub actually does: the names it says are kept get the WARN
|
||||
* (same severity, and same guard, as the deny-by-default case — a name genuinely reaching a
|
||||
* member unprotected is equally serious whichever path put it there), and the names it says are
|
||||
* blanked keep the INFO.
|
||||
*/
|
||||
private void logCredentialGap(FleetConfig.MemberCredentials creds) {
|
||||
private void logCredentialGap(FleetConfig.MemberCredentials creds, Set<String> effectiveAllowed) {
|
||||
Set<String> covered = new HashSet<>(creds.known());
|
||||
covered.addAll(creds.allow());
|
||||
List<String> gap = hostEnvNames.get().stream()
|
||||
@@ -1193,7 +1248,37 @@ public abstract class HerdrPeerLauncher implements PeerLauncher {
|
||||
if (gap.isEmpty()) {
|
||||
return;
|
||||
}
|
||||
if (credentialGapLogged.compareAndSet(false, true)) {
|
||||
if (effectiveAllowed == null) {
|
||||
warnGapUnprotected(gap);
|
||||
return;
|
||||
}
|
||||
List<String> keptByDerivedList = gap.stream()
|
||||
.filter(name -> MemberEnvAllowList.keeps(effectiveAllowed, name))
|
||||
.toList();
|
||||
List<String> blankedByScrub = gap.stream()
|
||||
.filter(name -> !MemberEnvAllowList.keeps(effectiveAllowed, name))
|
||||
.toList();
|
||||
if (!keptByDerivedList.isEmpty() && unprotectedGapLogged.compareAndSet(false, true)) {
|
||||
log.warn("memberCredentials gap: {} credential-shaped env var name(s) are on neither "
|
||||
+ "known: nor allow: — the derived allow-list keeps them anyway (a profile's "
|
||||
+ "gitTokenEnv/gitHostEnv/tokenEnv/env: names one, or this spawn injects it), "
|
||||
+ "so every member pane inherits them UNBLOCKED — {}. Add each to "
|
||||
+ "memberCredentials.known (or .allow if a member legitimately needs it), or "
|
||||
+ "remove it from whatever profile setting derives it in.",
|
||||
keptByDerivedList.size(), keptByDerivedList);
|
||||
}
|
||||
if (!blankedByScrub.isEmpty() && allowListGapLogged.compareAndSet(false, true)) {
|
||||
log.info("memberCredentials gap: {} credential-shaped env var name(s) are on neither "
|
||||
+ "known: nor allow: — {}. The allow-list scrub blanks them anyway (they "
|
||||
+ "are not on the derived allow-list), so no member pane keeps them; add "
|
||||
+ "each to memberCredentials.known or .allow to make that explicit.",
|
||||
blankedByScrub.size(), blankedByScrub);
|
||||
}
|
||||
}
|
||||
|
||||
/** The deny-by-default (and allow-list non-zsh fallback) WARN — unchanged byte-for-byte by #192. */
|
||||
private void warnGapUnprotected(List<String> gap) {
|
||||
if (unprotectedGapLogged.compareAndSet(false, true)) {
|
||||
log.warn("memberCredentials gap: {} credential-shaped env var name(s) are on neither "
|
||||
+ "known: nor allow: — every member pane inherits them UNBLOCKED — {}. "
|
||||
+ "Add each to memberCredentials.known (blocked by default) or .allow "
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
package dev.ltms.fleet.member;
|
||||
|
||||
import ch.qos.logback.classic.Level;
|
||||
import ch.qos.logback.classic.Logger;
|
||||
import ch.qos.logback.classic.spi.ILoggingEvent;
|
||||
import ch.qos.logback.core.read.ListAppender;
|
||||
@@ -1055,6 +1056,288 @@ class ClaudeCodeLauncherTest {
|
||||
"a name already on allow: is covered, not a gap");
|
||||
}
|
||||
|
||||
/**
|
||||
* CB-633 follow-up (#192): the deny-by-default WARN wording is a promise an operator relies on —
|
||||
* pinned byte-for-byte so a future edit cannot drift it (e.g. while picking wording for the
|
||||
* allow-list path) without a test noticing.
|
||||
*/
|
||||
@Test
|
||||
void denyByDefaultKeepsTheExactCredentialGapWarn() {
|
||||
FakeHerdr herdr = new FakeHerdr();
|
||||
FleetConfig.Profile cfg = new FleetConfig.Profile(
|
||||
"ltms-local", "http://gx00.gw:8000", "coder", null, "FLEETD_WORKER_TOKEN",
|
||||
List.of("claude"), "tab", "fleetd-workers", "worker: {profile} #{n}", null, null, null);
|
||||
ClaudeCodeLauncher svc = new ClaudeCodeLauncher(new AgentControl(herdr), new WorkspaceControl(herdr),
|
||||
new SubscriptionGuard(Set.of("gx00.gw")), Map.of(cfg.profile(), cfg), cfg.profile(),
|
||||
_ -> null, 0, System::currentTimeMillis, () -> {}, null, () -> TEST_MEMBER_CREDENTIALS,
|
||||
() -> Set.of("A_BRAND_NEW_SECRET_TOKEN"));
|
||||
|
||||
Logger logger = (Logger) LoggerFactory.getLogger(HerdrPeerLauncher.class);
|
||||
ListAppender<ILoggingEvent> appender = new ListAppender<>();
|
||||
appender.start();
|
||||
logger.addAppender(appender);
|
||||
try {
|
||||
svc.spawn();
|
||||
} finally {
|
||||
logger.detachAppender(appender);
|
||||
}
|
||||
|
||||
assertTrue(appender.list.stream().anyMatch(e ->
|
||||
("memberCredentials gap: 1 credential-shaped env var name(s) are on neither "
|
||||
+ "known: nor allow: — every member pane inherits them UNBLOCKED — "
|
||||
+ "[A_BRAND_NEW_SECRET_TOKEN]. Add each to memberCredentials.known "
|
||||
+ "(blocked by default) or .allow (if a member legitimately needs it).")
|
||||
.equals(e.getFormattedMessage())),
|
||||
"the deny-by-default WARN text must not drift — got: "
|
||||
+ appender.list.stream().map(ILoggingEvent::getFormattedMessage).toList());
|
||||
}
|
||||
|
||||
/**
|
||||
* CB-633 follow-up (#192), defect 1: under {@code policy: allow-list} on a zsh login shell the
|
||||
* generated ZDOTDIR scrub genuinely blanks an unkept credential-shaped name, so the report must
|
||||
* not say the pane inherits it UNBLOCKED — that claim is exactly what PR #174 got wrong. This
|
||||
* goes through the real spawn path (not {@code HerdrPeerLauncherAllowListWiringTest}'s fixture,
|
||||
* which overrides {@code buildLaunch} and bypasses none of the logic under test here — the
|
||||
* shell-dependent branch lives in {@code applyEnvironmentAllowListPolicy}, which every spawn
|
||||
* still passes through).
|
||||
*/
|
||||
@Test
|
||||
void allowListPolicyOnZshReportsTheGapWithoutClaimingItIsUnblocked() {
|
||||
FakeHerdr herdr = new FakeHerdr();
|
||||
FleetConfig.Profile cfg = new FleetConfig.Profile(
|
||||
"ltms-local", "http://gx00.gw:8000", "coder", null, "FLEETD_WORKER_TOKEN",
|
||||
List.of("claude"), "tab", "fleetd-workers", "worker: {profile} #{n}", null, null, null);
|
||||
FleetConfig.MemberCredentials creds = new FleetConfig.MemberCredentials(
|
||||
FleetConfig.MemberCredentials.POLICY_ALLOW_LIST,
|
||||
List.of("AI_GATEWAY_TOKEN"), List.of());
|
||||
ClaudeCodeLauncher svc = new ClaudeCodeLauncher(new AgentControl(herdr), new WorkspaceControl(herdr),
|
||||
new SubscriptionGuard(Set.of("gx00.gw")), Map.of(cfg.profile(), cfg), cfg.profile(),
|
||||
name -> "SHELL".equals(name) ? "/bin/zsh" : null,
|
||||
0, System::currentTimeMillis, () -> {}, null, () -> creds,
|
||||
() -> Set.of("AI_GATEWAY_TOKEN", "A_BRAND_NEW_SECRET_TOKEN"));
|
||||
|
||||
Logger logger = (Logger) LoggerFactory.getLogger(HerdrPeerLauncher.class);
|
||||
Level original = logger.getLevel();
|
||||
logger.setLevel(Level.INFO);
|
||||
ListAppender<ILoggingEvent> appender = new ListAppender<>();
|
||||
appender.start();
|
||||
logger.addAppender(appender);
|
||||
try {
|
||||
svc.spawn();
|
||||
} finally {
|
||||
logger.detachAppender(appender);
|
||||
logger.setLevel(original);
|
||||
}
|
||||
|
||||
assertTrue(appender.list.stream().anyMatch(e ->
|
||||
e.getFormattedMessage().contains("memberCredentials gap")
|
||||
&& e.getFormattedMessage().contains("A_BRAND_NEW_SECRET_TOKEN")),
|
||||
"the unkept name must still be reported — got: "
|
||||
+ appender.list.stream().map(ILoggingEvent::getFormattedMessage).toList());
|
||||
assertFalse(appender.list.stream().anyMatch(e ->
|
||||
e.getFormattedMessage().contains("memberCredentials gap")
|
||||
&& e.getFormattedMessage().contains("UNBLOCKED")),
|
||||
"on zsh the scrub genuinely blanks the name, so the report must not claim it is "
|
||||
+ "inherited unblocked — got: "
|
||||
+ appender.list.stream().map(ILoggingEvent::getFormattedMessage).toList());
|
||||
}
|
||||
|
||||
/**
|
||||
* CB-633 follow-up (#192): the mirror of the zsh test above. On a non-zsh login shell {@code
|
||||
* ZDOTDIR} is ignored, so no scrub ever runs — the report must keep the WARN wording (a name here
|
||||
* really is inherited unblocked) rather than claiming a scrub protects it. This is the trap PR
|
||||
* #174 fell into the other direction: keying the wording on the shell, not on {@code
|
||||
* creds.isAllowList()}, is what keeps this branch correct.
|
||||
*/
|
||||
@Test
|
||||
void allowListPolicyOnNonZshKeepsTheWarnWording() {
|
||||
FakeHerdr herdr = new FakeHerdr();
|
||||
FleetConfig.Profile cfg = new FleetConfig.Profile(
|
||||
"ltms-local", "http://gx00.gw:8000", "coder", null, "FLEETD_WORKER_TOKEN",
|
||||
List.of("claude"), "tab", "fleetd-workers", "worker: {profile} #{n}", null, null, null);
|
||||
FleetConfig.MemberCredentials creds = new FleetConfig.MemberCredentials(
|
||||
FleetConfig.MemberCredentials.POLICY_ALLOW_LIST,
|
||||
List.of("AI_GATEWAY_TOKEN"), List.of());
|
||||
ClaudeCodeLauncher svc = new ClaudeCodeLauncher(new AgentControl(herdr), new WorkspaceControl(herdr),
|
||||
new SubscriptionGuard(Set.of("gx00.gw")), Map.of(cfg.profile(), cfg), cfg.profile(),
|
||||
name -> "SHELL".equals(name) ? "/bin/bash" : null,
|
||||
0, System::currentTimeMillis, () -> {}, null, () -> creds,
|
||||
() -> Set.of("AI_GATEWAY_TOKEN", "A_BRAND_NEW_SECRET_TOKEN"));
|
||||
|
||||
Logger logger = (Logger) LoggerFactory.getLogger(HerdrPeerLauncher.class);
|
||||
ListAppender<ILoggingEvent> appender = new ListAppender<>();
|
||||
appender.start();
|
||||
logger.addAppender(appender);
|
||||
try {
|
||||
svc.spawn();
|
||||
} finally {
|
||||
logger.detachAppender(appender);
|
||||
}
|
||||
|
||||
assertTrue(appender.list.stream().anyMatch(e ->
|
||||
e.getFormattedMessage().contains("memberCredentials gap")
|
||||
&& e.getFormattedMessage().contains("UNBLOCKED")
|
||||
&& e.getFormattedMessage().contains("A_BRAND_NEW_SECRET_TOKEN")),
|
||||
"no scrub runs on a non-zsh shell, so the WARN wording must be kept — got: "
|
||||
+ appender.list.stream().map(ILoggingEvent::getFormattedMessage).toList());
|
||||
assertFalse(appender.list.stream().anyMatch(e ->
|
||||
e.getFormattedMessage().contains("memberCredentials gap")
|
||||
&& e.getFormattedMessage().toLowerCase(java.util.Locale.ROOT).contains("scrub")),
|
||||
"nothing is scrubbed on this path, so the report must not claim otherwise — got: "
|
||||
+ appender.list.stream().map(ILoggingEvent::getFormattedMessage).toList());
|
||||
}
|
||||
|
||||
/**
|
||||
* CB-633 follow-up (#192), defect 2: {@code memberCredentials} is a live, re-read-per-spawn
|
||||
* supplier, so the policy can change between two spawns on the same launcher. Before this fix a
|
||||
* single {@code AtomicBoolean} guarded both report kinds, so the harmless allow-list INFO on the
|
||||
* first spawn would permanently suppress the real deny-by-default WARN a later spawn deserves.
|
||||
* This goes through {@link ClaudeCodeLauncher#buildLaunch}'s real {@code baseEnv()} path — the
|
||||
* WARN this test pins fires from {@code applyMemberCredentialPolicy}, which {@code
|
||||
* HerdrPeerLauncherAllowListWiringTest}'s fixture never reaches at all (see its class javadoc).
|
||||
*/
|
||||
@Test
|
||||
void secondSpawnStillWarnsAfterPolicyChangesFromAllowListToDenyByDefault() {
|
||||
FakeHerdr herdr = new FakeHerdr();
|
||||
FleetConfig.Profile cfg = new FleetConfig.Profile(
|
||||
"ltms-local", "http://gx00.gw:8000", "coder", null, "FLEETD_WORKER_TOKEN",
|
||||
List.of("claude"), "tab", "fleetd-workers", "worker: {profile} #{n}", null, null, null);
|
||||
AtomicReference<FleetConfig.MemberCredentials> creds = new AtomicReference<>(
|
||||
new FleetConfig.MemberCredentials(FleetConfig.MemberCredentials.POLICY_ALLOW_LIST,
|
||||
List.of("AI_GATEWAY_TOKEN"), List.of()));
|
||||
ClaudeCodeLauncher svc = new ClaudeCodeLauncher(new AgentControl(herdr), new WorkspaceControl(herdr),
|
||||
new SubscriptionGuard(Set.of("gx00.gw")), Map.of(cfg.profile(), cfg), cfg.profile(),
|
||||
name -> "SHELL".equals(name) ? "/bin/zsh" : null,
|
||||
0, System::currentTimeMillis, () -> {}, null, creds::get,
|
||||
() -> Set.of("AI_GATEWAY_TOKEN", "A_BRAND_NEW_SECRET_TOKEN"));
|
||||
|
||||
Logger logger = (Logger) LoggerFactory.getLogger(HerdrPeerLauncher.class);
|
||||
Level original = logger.getLevel();
|
||||
logger.setLevel(Level.INFO);
|
||||
ListAppender<ILoggingEvent> appender = new ListAppender<>();
|
||||
appender.start();
|
||||
try {
|
||||
logger.addAppender(appender);
|
||||
svc.spawn(); // allow-list + zsh: harmless INFO, sets the allow-list guard only
|
||||
|
||||
creds.set(new FleetConfig.MemberCredentials(FleetConfig.MemberCredentials.POLICY_DENY_BY_DEFAULT,
|
||||
List.of("AI_GATEWAY_TOKEN"), List.of()));
|
||||
svc.spawn(); // policy reloaded to deny-by-default: this WARN must NOT be suppressed
|
||||
} finally {
|
||||
logger.detachAppender(appender);
|
||||
logger.setLevel(original);
|
||||
}
|
||||
|
||||
assertTrue(appender.list.stream().anyMatch(e ->
|
||||
e.getFormattedMessage().contains("memberCredentials gap")
|
||||
&& e.getFormattedMessage().contains("UNBLOCKED")),
|
||||
"the second spawn's deny-by-default WARN must still fire even though the first "
|
||||
+ "spawn's allow-list INFO already logged the same underlying gap — got: "
|
||||
+ appender.list.stream().map(ILoggingEvent::getFormattedMessage).toList());
|
||||
}
|
||||
|
||||
/**
|
||||
* CB-633 follow-up (#192), lead-review fix: {@code effectiveAllowed} is a SUPERSET of
|
||||
* {@code known ∪ allow} — {@code MemberEnvAllowList.derive} also unions in every profile's
|
||||
* {@code tokenEnv} (among other fields), so a credential-shaped name can be uncovered by
|
||||
* {@code known:}/{@code allow:} and STILL survive the scrub because a profile's own
|
||||
* {@code tokenEnv} names it. Here {@code tokenEnv} is deliberately set to a credential-shaped
|
||||
* name the operator forgot to list — the misconfiguration this report exists to catch. The scrub
|
||||
* genuinely keeps it, so the report must WARN, not claim (as the pre-lead-review cut of this fix
|
||||
* did) that "no member pane keeps them".
|
||||
*/
|
||||
@Test
|
||||
void allowListWarnsWhenTheDerivedAllowListKeepsAnUncoveredName() {
|
||||
FakeHerdr herdr = new FakeHerdr();
|
||||
FleetConfig.Profile cfg = new FleetConfig.Profile(
|
||||
"ltms-local", "http://gx00.gw:8000", "coder", null, "SOME_LEAKY_TOKEN",
|
||||
List.of("claude"), "tab", "fleetd-workers", "worker: {profile} #{n}", null, null, null);
|
||||
FleetConfig.MemberCredentials creds = new FleetConfig.MemberCredentials(
|
||||
FleetConfig.MemberCredentials.POLICY_ALLOW_LIST, List.of(), List.of());
|
||||
ClaudeCodeLauncher svc = new ClaudeCodeLauncher(new AgentControl(herdr), new WorkspaceControl(herdr),
|
||||
new SubscriptionGuard(Set.of("gx00.gw")), Map.of(cfg.profile(), cfg), cfg.profile(),
|
||||
name -> "SHELL".equals(name) ? "/bin/zsh" : null,
|
||||
0, System::currentTimeMillis, () -> {}, null, () -> creds,
|
||||
() -> Set.of("SOME_LEAKY_TOKEN"));
|
||||
|
||||
Logger logger = (Logger) LoggerFactory.getLogger(HerdrPeerLauncher.class);
|
||||
Level original = logger.getLevel();
|
||||
logger.setLevel(Level.INFO);
|
||||
ListAppender<ILoggingEvent> appender = new ListAppender<>();
|
||||
appender.start();
|
||||
logger.addAppender(appender);
|
||||
try {
|
||||
svc.spawn();
|
||||
} finally {
|
||||
logger.detachAppender(appender);
|
||||
logger.setLevel(original);
|
||||
}
|
||||
|
||||
assertTrue(appender.list.stream().anyMatch(e ->
|
||||
e.getLevel() == Level.WARN
|
||||
&& e.getFormattedMessage().contains("memberCredentials gap")
|
||||
&& e.getFormattedMessage().contains("SOME_LEAKY_TOKEN")
|
||||
&& e.getFormattedMessage().contains("UNBLOCKED")),
|
||||
"a name kept by the derived allow-list (via this profile's tokenEnv) must still WARN "
|
||||
+ "— got: " + appender.list.stream().map(ILoggingEvent::getFormattedMessage).toList());
|
||||
assertFalse(appender.list.stream().anyMatch(e ->
|
||||
e.getFormattedMessage().contains("memberCredentials gap")
|
||||
&& e.getFormattedMessage().contains("SOME_LEAKY_TOKEN")
|
||||
&& e.getFormattedMessage().contains("blanks them anyway")),
|
||||
"the scrub does NOT blank this name, so the INFO wording must not claim it does — got: "
|
||||
+ appender.list.stream().map(ILoggingEvent::getFormattedMessage).toList());
|
||||
}
|
||||
|
||||
/**
|
||||
* CB-633 follow-up (#192), lead-review fix: a mixed gap — one name the derived allow-list keeps
|
||||
* (this profile's {@code tokenEnv}), one it does not — must split cleanly: the WARN names only
|
||||
* the kept one, the INFO names only the blanked one. Proves the split uses {@code
|
||||
* MemberEnvAllowList.keeps} per-name rather than an all-or-nothing decision for the whole gap.
|
||||
*/
|
||||
@Test
|
||||
void allowListSplitsAMixedGapBetweenTheWarnAndTheInfo() {
|
||||
FakeHerdr herdr = new FakeHerdr();
|
||||
FleetConfig.Profile cfg = new FleetConfig.Profile(
|
||||
"ltms-local", "http://gx00.gw:8000", "coder", null, "SOME_LEAKY_TOKEN",
|
||||
List.of("claude"), "tab", "fleetd-workers", "worker: {profile} #{n}", null, null, null);
|
||||
FleetConfig.MemberCredentials creds = new FleetConfig.MemberCredentials(
|
||||
FleetConfig.MemberCredentials.POLICY_ALLOW_LIST, List.of(), List.of());
|
||||
ClaudeCodeLauncher svc = new ClaudeCodeLauncher(new AgentControl(herdr), new WorkspaceControl(herdr),
|
||||
new SubscriptionGuard(Set.of("gx00.gw")), Map.of(cfg.profile(), cfg), cfg.profile(),
|
||||
name -> "SHELL".equals(name) ? "/bin/zsh" : null,
|
||||
0, System::currentTimeMillis, () -> {}, null, () -> creds,
|
||||
() -> Set.of("SOME_LEAKY_TOKEN", "A_BRAND_NEW_SECRET_TOKEN"));
|
||||
|
||||
Logger logger = (Logger) LoggerFactory.getLogger(HerdrPeerLauncher.class);
|
||||
Level original = logger.getLevel();
|
||||
logger.setLevel(Level.INFO);
|
||||
ListAppender<ILoggingEvent> appender = new ListAppender<>();
|
||||
appender.start();
|
||||
logger.addAppender(appender);
|
||||
try {
|
||||
svc.spawn();
|
||||
} finally {
|
||||
logger.detachAppender(appender);
|
||||
logger.setLevel(original);
|
||||
}
|
||||
|
||||
boolean warnNamesOnlyKept = appender.list.stream().anyMatch(e ->
|
||||
e.getLevel() == Level.WARN
|
||||
&& e.getFormattedMessage().contains("memberCredentials gap")
|
||||
&& e.getFormattedMessage().contains("SOME_LEAKY_TOKEN")
|
||||
&& !e.getFormattedMessage().contains("A_BRAND_NEW_SECRET_TOKEN"));
|
||||
boolean infoNamesOnlyBlanked = appender.list.stream().anyMatch(e ->
|
||||
e.getLevel() == Level.INFO
|
||||
&& e.getFormattedMessage().contains("memberCredentials gap")
|
||||
&& e.getFormattedMessage().contains("A_BRAND_NEW_SECRET_TOKEN")
|
||||
&& !e.getFormattedMessage().contains("SOME_LEAKY_TOKEN"));
|
||||
|
||||
assertTrue(warnNamesOnlyKept, "the WARN must name the derived-list-kept variable and only it "
|
||||
+ "— got: " + appender.list.stream().map(ILoggingEvent::getFormattedMessage).toList());
|
||||
assertTrue(infoNamesOnlyBlanked, "the INFO must name the scrub-blanked variable and only it "
|
||||
+ "— got: " + appender.list.stream().map(ILoggingEvent::getFormattedMessage).toList());
|
||||
}
|
||||
|
||||
/**
|
||||
* The half of CB-592 that can actually survive the pane's login shell. BRIDGED_MEMBER is a name
|
||||
* secrets.sh never exports, so nothing overwrites it — measured: GITEA_TOKEN is injected the
|
||||
|
||||
@@ -8,6 +8,7 @@ import dev.ltms.fleet.guard.SubscriptionGuard;
|
||||
import dev.ltms.fleet.herdr.Agent;
|
||||
import dev.ltms.fleet.herdr.AgentControl;
|
||||
import dev.ltms.fleet.herdr.FakeHerdr;
|
||||
import dev.ltms.fleet.herdr.HerdrException;
|
||||
import dev.ltms.fleet.herdr.WorkspaceControl;
|
||||
import dev.ltms.fleet.peer.Capability;
|
||||
import dev.ltms.fleet.peer.CharterReceipt;
|
||||
@@ -248,6 +249,112 @@ class CompositePeerLauncherTest {
|
||||
"stop routes to the spawning adapter and closes exactly that worker's pane");
|
||||
}
|
||||
|
||||
@Test
|
||||
void stopKeepsSameHerdrPaneIdSeparateByOwningAdapter() {
|
||||
// Separate herdr daemons can both issue w9:pRoot_1. The opaque handles identify their
|
||||
// spawning adapters, so each stop reaches only its recorded owner.
|
||||
FakeHerdr first = new FakeHerdr();
|
||||
FakeHerdr second = new FakeHerdr();
|
||||
PeerLauncher composite = new CompositePeerLauncher(
|
||||
List.of(claudeAdapter(first), opencodeAdapter(second)), "claude");
|
||||
|
||||
PeerHandle claude = composite.spawn(new SpawnRequest("claude", null, null));
|
||||
PeerHandle opencode = composite.spawn(new SpawnRequest("gemini", null, null));
|
||||
|
||||
assertNotEquals(claude.id(), opencode.id(), "each public paneId keeps its adapter owner");
|
||||
composite.stop(claude.id());
|
||||
assertTrue(first.calls.stream().anyMatch(c -> c.method().equals("pane.close")
|
||||
&& "w9:pRoot_1".equals(((Map<?, ?>) c.params()).get("pane_id"))),
|
||||
"the first daemon closes its own pane");
|
||||
assertFalse(second.called("pane.close"), "the matching pane on the second daemon stays live");
|
||||
|
||||
composite.stop(opencode.id());
|
||||
assertTrue(second.calls.stream().anyMatch(c -> c.method().equals("pane.close")
|
||||
&& "w9:pRoot_1".equals(((Map<?, ?>) c.params()).get("pane_id"))),
|
||||
"the second daemon then closes its own pane");
|
||||
}
|
||||
|
||||
@Test
|
||||
void stopAllowsALegacyBarePaneIdWithOneDaemon() {
|
||||
FakeHerdr herdr = new FakeHerdr();
|
||||
PeerLauncher composite = new CompositePeerLauncher(List.of(claudeAdapter(herdr)), "claude");
|
||||
|
||||
composite.stop("w9:pRoot_1");
|
||||
|
||||
assertTrue(herdr.calls.stream().anyMatch(c -> c.method().equals("pane.close")
|
||||
&& "w9:pRoot_1".equals(((Map<?, ?>) c.params()).get("pane_id"))),
|
||||
"one daemon keeps the legacy bare-pane routing behaviour");
|
||||
}
|
||||
|
||||
@Test
|
||||
void stopAllowsAnUnownedPaneIdWithTwoAdaptersSharingOneDaemon() {
|
||||
FakeHerdr herdr = new FakeHerdr();
|
||||
PeerLauncher composite = composite(herdr);
|
||||
|
||||
composite.stop("w9:pRoot_1");
|
||||
|
||||
assertTrue(herdr.calls.stream().anyMatch(c -> c.method().equals("pane.close")
|
||||
&& "w9:pRoot_1".equals(((Map<?, ?>) c.params()).get("pane_id"))),
|
||||
"two adapter kinds sharing one daemon keep the fallback route");
|
||||
}
|
||||
|
||||
@Test
|
||||
void listKeepsBothPanesWhenTwoDaemonsShareAPaneId() {
|
||||
// herdr pane ids are per-daemon counters, so two daemons really can both hold w1:p1 on
|
||||
// different panes. Deduplicating on the pane id alone dropped one of the two real agents.
|
||||
FakeHerdr first = new FakeHerdr().withAgent("x", "term_x", "w1:p1", "w1:t1");
|
||||
FakeHerdr second = new FakeHerdr().withAgent("y", "term_y", "w1:p1", "w1:t1");
|
||||
CompositePeerLauncher composite = new CompositePeerLauncher(
|
||||
List.of(claudeAdapter(first), opencodeAdapter(second)), "claude");
|
||||
|
||||
List<Agent> agents = composite.list();
|
||||
|
||||
assertEquals(2, agents.stream().filter(a -> "w1:p1".equals(a.paneId())).count(),
|
||||
"one w1:p1 per daemon survives — the pane id alone is not a unique key");
|
||||
assertTrue(agents.stream().anyMatch(a -> "term_x".equals(a.terminalId())));
|
||||
assertTrue(agents.stream().anyMatch(a -> "term_y".equals(a.terminalId())));
|
||||
}
|
||||
|
||||
@Test
|
||||
void listStillDeduplicatesTwoAdaptersSharingOneDaemon() {
|
||||
// Both adapters ask the SAME daemon, so both see the same agent set. Without the dedupe this
|
||||
// would report every agent twice; the daemon key must not break that.
|
||||
FakeHerdr herdr = new FakeHerdr().withAgent("x", "term_x", "w1:p1", "w1:t1");
|
||||
CompositePeerLauncher composite = composite(herdr);
|
||||
|
||||
List<Agent> agents = composite.list();
|
||||
|
||||
assertEquals(1, agents.stream().filter(a -> "w1:p1".equals(a.paneId())).count(),
|
||||
"one daemon still reports each of its agents once");
|
||||
}
|
||||
|
||||
@Test
|
||||
void stopKeepsTheOwnerRecordWhenTheDelegateRefusesTheStop() {
|
||||
// Removing the record before the delegate accepted the stop lost the owner on failure: the
|
||||
// pane was still alive, but the retry landed in the ambiguous branch and refused it for good.
|
||||
FakeHerdr first = new FakeHerdr().paneCloseFailsWith("pane_busy");
|
||||
CompositePeerLauncher composite = new CompositePeerLauncher(
|
||||
List.of(claudeAdapter(first), opencodeAdapter(new FakeHerdr())), "claude");
|
||||
PeerHandle claude = composite.spawn(new SpawnRequest("claude", null, null));
|
||||
|
||||
assertThrows(HerdrException.class, () -> composite.stop(claude.id()));
|
||||
|
||||
// The retry must still know its owner — a HerdrException, never "ambiguous paneId".
|
||||
assertThrows(HerdrException.class, () -> composite.stop(claude.id()),
|
||||
"the owner record survives a failed stop, so the retry is not ambiguous");
|
||||
}
|
||||
|
||||
@Test
|
||||
void stopRejectsAnUnownedPaneIdWhenMultipleDaemonsCouldOwnIt() {
|
||||
PeerLauncher composite = new CompositePeerLauncher(
|
||||
List.of(claudeAdapter(new FakeHerdr()), opencodeAdapter(new FakeHerdr())), "claude");
|
||||
|
||||
IllegalArgumentException error = assertThrows(IllegalArgumentException.class,
|
||||
() -> composite.stop("w1:p1"));
|
||||
|
||||
assertEquals("ambiguous paneId 'w1:p1': no owning herdr daemon was recorded", error.getMessage());
|
||||
}
|
||||
|
||||
@Test
|
||||
void opencodeContextResetIsANoOpAndWarnsOnlyOnce() {
|
||||
FakeHerdr herdr = new FakeHerdr();
|
||||
|
||||
Reference in New Issue
Block a user