Compare commits

...

7 Commits

Author SHA1 Message Date
Dai Ha cf8da1d5fa fleetd #391: require reply caller role
CI / contract (pull_request) Successful in 1m21s
CI / build (pull_request) Successful in 1m45s
2026-09-10 09:13:16 +07:00
Dai Ha 3982ace544 fleetd #391: refuse lead fleet replies
CI / contract (pull_request) Successful in 1m11s
CI / build (pull_request) Successful in 1m31s
2026-09-10 08:43:54 +07:00
Dai Ha 48877315ca Merge #394: contain the fatal export that aborted the credential scrub
CI / contract (push) Successful in 50s
CI / build (push) Failing after 1m31s
The CB-633 allow-list scrub has been dying mid-loop on every fleet01 member
pane and saying nothing. `export UID=` in zsh is not a failed command — it is
a fatal parameter error that terminates the whole sourced file. The blanking
loop is wrapped in `{ ... } 2>/dev/null`, so the message was swallowed and the
report block after the loop never ran.

Root cause found by the fleet01 lead, with xtrace on a live pane's own ZDOTDIR:

    +scrub.zsh:28> _cb633_n=UID
    +scrub.zsh:28> export 'UID='
    +zsh:1> rc=126        <- file aborted

The severity is the INVERSION, and this is their finding, quoted:

  "env lists inherited names first and the names a startup file exports last.
   So the loop blanks the harmless inherited half and dies immediately before
   the operator's own exports — exactly the credentials the policy exists to
   remove. The selection is inverted, not merely partial."

Measured there: UID is name 42 of 57, and a ~/.zshrc decoy at 58 survived on
8 of 8 spawns. "Partial scrub" reads as "we got most of it"; it got precisely
the wrong half.

Fixed with `eval "export ${n}=" 2>/dev/null` rather than a skip-list of the
known-fatal names (UID EUID GID EGID PPID LINENO). A skip-list has to be
complete forever and this is a security control; eval needs no list. Measured:
plain export dies at UID and every later name keeps its value, while the eval
form completes the loop and blanks all of them. PR #396 proposed the skip-list
and is closed in favour of this; its claim that the abort happens "however the
assignment is wrapped" holds for a direct `if ! export` but not for eval,
which reparses in a nested context.

The report now carries `failed N` and `!`-prefixed unblankable names, and
HerdrPeerLauncher WARNs when any name could not be blanked. The old "no report"
WARN no longer claims the daemon knows what the member saw.

Why the suite stayed green: EnvAllowListScrubTest starts zsh from
pb.environment().clear(), and under a cleared parent UID is not an exported
name at all, so the abort could not reproduce in that harness.

Reviewed by mutation, which found a second gap now also closed: the eval is
only safe because names are filtered to ^[A-Za-z_][A-Za-z0-9_]*$. Replacing
that pattern with .* left the class green, so the line the security property
rests on was unpinned. The guard is now re-asserted at the eval site and pinned
by a test. The reachable vector is a VALUE with an embedded newline, not a
hostile name — measured: zsh strips non-identifier env names outright, while
MULTI=$'keep\njunk.fragment' forges 'junk.fragment' as a candidate name out of
its own value.

Closes #394. Refs #396, #388.
2026-09-10 08:30:15 +07:00
Dai Ha 5c08054533 #394 follow-up: re-assert the identifier guard at the eval call site
CI / contract (pull_request) Successful in 1m29s
CI / build (pull_request) Successful in 1m48s
EnvAllowListScrub's blanking loop splices each name into a string
handed to eval ("export ${n}="). That is only safe because every name
reaching _cb633_blank already passed an identifier check in the
enumeration loop -- 20 lines away, in a different loop. Before eval
was introduced a non-conforming name reaching plain `export "$n="`
was inert either way (the quoting neutralized it); eval removed that
safety net, so the enumeration loop's guard became the ONLY thing
standing between a non-identifier string and code execution in the
member's pane, with nothing at the eval site itself defending that
property.

Re-assert the same [A-Za-z_][A-Za-z0-9_]* check immediately before
the eval call, independent of the enumeration loop's own guard (left
untouched, not moved). A name that fails it is counted unblankable
rather than silently dropped, so a bypass of the upstream guard would
leave real evidence in the report.

New test exploits the "junk from multi-line values" gap the
enumeration loop's own comment already documents: a value with an
embedded newline makes `command env`'s text output split into a
spurious extra "name" line that was never a real variable. Runs the
real generated scrubScript() end-to-end under zsh and asserts the
non-conforming fragment is neither blanked nor counted unblankable.
The fragment used is merely non-conforming (contains a dot) --
never command-shaped.

Mutation-verified both guards. Weakening the enumeration guard alone
DOES break the new test (the fragment then reaches the new eval-site
guard and gets counted unblankable, failing the "not unblankable"
assertion). Removing the new eval-site guard alone, with the
enumeration guard intact, does NOT break it: _cb633_blank has exactly
one producer (the enumeration loop), so nothing can reach the eval
site without already having passed the identical check there. That is
expected given the single-source architecture, and it is exactly why
the eval-site guard is defense-in-depth against a future change that
adds a second path into _cb633_blank or decouples the two loops --
not a currently independently-observable divergence.
2026-09-10 08:24:46 +07:00
Dai Ha b6db9c31f5 charter: a peer lead is answered with fleet_send, not fleet_reply
CI / contract (push) Successful in 46s
CI / build (push) Successful in 2m0s
`fleet_reply` has no route to a peer lead. `AmqpReplyInbox` publishes to
`agent.<target>.inbox`, mandatory, and a lead's own terminal has no such
queue, so the publish is refused. `MessageService.reply()` has no peer
branch at all — `grep -c 'coord\|LeadMailbox'` on it returns 0. The charter
told every lead to use a tool that cannot work, and both leads here hit it.

Three edits to the canonical block, byte-identical with the wiki template
(pushed as 803726a; the in-sync check in this file reports True):

- the intent->tool row now says `fleet_send{coordId}`, or `{sessionId}` for
  a peer on the same host, and says plainly that `fleet_reply` is refused
- the prose says WHY: `fleet_reply` resolves a member's blocked `fleet_send`,
  while a peer's coord-id message is durable and non-blocking, so there is
  nothing for it to resolve
- lead<->lead item 3 gains the data-point rule: N observations are N data
  points only if they differ in the axis you are trusting

Wording for all three drafted by the fleet01 lead, who verified the missing
queue namespace independently in its own tree. The data-point rule has now
caught three separate errors in a day, in both directions: one cause blamed
for N failures, and N agreeing measurements that shared a single instrument.

The refusal message itself is still wrong — it says "queue not declared or
owned", which sends the reader to the broker instead of to this file. That
half stays open on #391.

Tracked as fleetd #391.
2026-09-10 08:22:35 +07:00
Dai Ha e3e403e5c8 #394: contain a fatal export error instead of letting it abort the scrub
CI / contract (pull_request) Successful in 1m23s
CI / build (pull_request) Successful in 1m52s
EnvAllowListScrub's blanking loop used a plain `export "$n="` on every
name not on the allow-list. For a zsh read-only/special parameter (e.g.
UID) that is a FATAL parameter error, and since the loop runs inside the
sourced startup file, the error aborts the whole file: every name still
to come is never blanked, and scrub-report.txt is never written at all
-- silently, because 2>/dev/null on the group swallows it.

Route each blanking attempt through `eval` instead, which contains the
error to that one iteration. The loop always finishes; a name it could
not blank is now counted separately ("failed" on the report's first
line) and listed !-prefixed rather than disappearing. No skip-list of
known-bad names is added -- every enumerated name is still attempted,
so a name nobody has thought of is still tried and, if it fails, still
counted.

HerdrPeerLauncher: log a WARN when a pane's report carries a nonzero
failed count, and reword the "no report at all" WARN so it no longer
claims the daemon knows the member "saw the full host environment" --
a partial vs. a missing scrub are different situations and only the
first is now distinguishable from the report alone.
2026-09-10 08:08:41 +07:00
Dai Ha 799014e99d Merge #388: scrub a pane shell that is neither login nor interactive
CI / contract (push) Successful in 1m8s
CI / build (push) Successful in 1m38s
2026-09-10 07:21:48 +07:00
6 changed files with 287 additions and 36 deletions
+10 -4
View File
@@ -137,7 +137,7 @@ the merge — and merging on a reviewer's word is delegating it by proxy.
| Answer a member's `fleet_ask` | `fleet_send{turnId, content}` — **not** `sessionId` |
| Message a **peer lead** on this host | `fleet_send{sessionId: <their terminal>, content}` — `fleet_list` → `leads` reports it. Coordination only, **never** a task |
| Message a **peer lead** on another daemon or host | `fleet_send{coordId: <their coord-id>, content}` — needs a `coordinator:` block; your own coord-id is in `fleet_list`. Coordination only, **never** a task |
| Answer a peer lead that messaged you | `fleet_reply{content}` — the one case a lead replies |
| Answer a peer lead that messaged you | `fleet_send{coordId}` — or `{sessionId}` if they are on this host. **Not** `fleet_reply`: it has no peer route and the publish is refused |
| Collect a held reply | `fleet_poll{target}` · then `fleet_ack{target, msgId}` |
| Tear down a member | `fleet_stop{paneId}` |
@@ -163,15 +163,21 @@ The traffic between leads is coordination and nothing else:
3. **Verify a peer exactly as you verify yourself.** Peer status buys nothing: check the claim
against the code, and re-run the build. A peer's correction gets the same treatment — right or
wrong on the evidence, not on who said it. Neither of you merges the other's work unreviewed.
**N observations are N data points only if they differ in the axis you are trusting.** This cuts
both ways. N *failures* blamed on one cause are one data point when the cases share what you are
not varying. N *agreeing measurements* are also one data point when they share an instrument —
two hosts, two operators and the same formula is one formula, not two confirmations.
4. **Ask a peer to read your project addendum.** Your addendum is instruction surface: every future
session on your host obeys it, and a wrong one is obeyed just as faithfully as a right one. The
author is the worst reader of their own qualifier placement — measured here, one addendum carried
two defects and a non-author found both. If you have no peer, at least re-read it asking "which
sentence goes false first, and would a reader reach the caveat before acting?"
Being messaged by a peer does not make you its worker: answer with `fleet_reply`, and push back on
the substance if it is wrong. A peer that simply complies has thrown away the reason there are two of
you.
Being messaged by a peer does not make you its worker: answer the way you would open —
`fleet_send{coordId}` for another daemon, `fleet_send{sessionId}` on this host — and push back on
the substance if it is wrong. `fleet_reply` resolves a member's blocked `fleet_send`; a peer's
coord-id message is durable and non-blocking, so there is nothing for it to resolve. A peer that
simply complies has thrown away the reason there are two of you.
### Member (worker or architect) — the turn contract
@@ -346,7 +346,7 @@ public final class FleetMcp {
String self = callerTerminal(exchange);
McpSchema.CallToolResult denied = deny(exchange, toolAction("fleet_reply", req.arguments()), self);
if (denied != null) return denied;
return reply(messages, self, str(req.arguments(), "content"));
return reply(messages, self, principal(exchange).role(), str(req.arguments(), "content"));
};
// fleet_ask (CB-205): a worker's mid-turn question — identity from the CONNECTION.
BiFunction<McpSyncServerExchange, McpSchema.CallToolRequest, McpSchema.CallToolResult> askHandler =
@@ -868,19 +868,26 @@ public final class FleetMcp {
/**
* {@code fleet_reply}: the worker returns its structured answer, resolving the awaiting send
* or — when no send is open — queueing the reply in the inbox for later drain (CB-307).
* {@code callerTerminal} is resolved from the connection (never an argument); a {@code null}
* means the caller is not a known worker (e.g. the primary called it by mistake).
* {@code callerTerminal} and {@code callerRole} are resolved from the connection (never an
* argument). A {@code null} terminal means the caller is not a known worker. A PRIMARY with a
* terminal is a lead and must use {@code fleet_send}, because reply has no peer-lead route.
*
* <p>fleetd #365: the result text names which of those actually happened
* ({@link MessageService.ReplyOutcome#description()}) instead of the single word "delivered"
* for both — a queued reply is a real success, but it is not the same fact as one that resolved
* a live waiter, and the caller could not previously tell them apart.
*/
static McpSchema.CallToolResult reply(MessageService messages, String callerTerminal, String content) {
static McpSchema.CallToolResult reply(MessageService messages, String callerTerminal, Role callerRole, String content) {
if (callerTerminal == null) {
return error("fleet_reply is for workers only — could not identify the calling worker "
+ "from the connection");
}
if (callerRole == Role.PRIMARY) {
return error("fleet_reply has no route to a peer lead. Use fleet_send{coordId: ...} for a peer on another "
+ "daemon or fleet_send{sessionId: ...} for a peer on this host. fleet_reply resolves a member's "
+ "blocked fleet_send, and a peer's coord-id message is durable and non-blocking, so there is "
+ "nothing for it to resolve.");
}
// fleetd #302: isBlank, not == null, to match fleet_send's own guard above. MessageService
// .reply now REJECTS blank content, and this handler is a bare BiFunction with no try/catch
// around it — so a whitespace-only fleet_reply would leave here as an uncaught
@@ -75,9 +75,28 @@ import java.util.stream.Stream;
* never to the control.
*
* <p>The scrub also writes {@code scrub-report.txt} into its own directory: one {@code allowed N of
* M} line (N = exports left untouched, M = exports present when the scrub ran), then the blanked
* NAMES — never values. The launcher reads this back at teardown and logs it, because a blocked
* count next to an unknown denominator is not a finding.
* M failed F} line (N = exports left untouched, M = exports present when the scrub ran, F = names
* the scrub attempted to blank but could not), then the NAMES — blanked ones bare, unblankable ones
* {@code !}-prefixed — never values. The launcher reads this back at teardown and logs it, because a
* blocked count next to an unknown denominator is not a finding.
*
* <p><b>fleetd #394:</b> plain {@code export "$n="} is a FATAL error for a zsh read-only or special
* parameter (for example {@code UID}) — it aborts the whole sourced file, so every name still to
* come is never blanked and the report above is never written at all. The blanking loop instead
* routes each attempt through {@code eval}, which contains that error to the single iteration: the
* loop always finishes, and a name that could not be blanked is counted as {@code failed} and
* listed {@code !}-prefixed rather than silently disappearing. This is deliberately not a skip-list
* of known-bad names — every enumerated name is still attempted, so a name nobody has thought of
* yet still gets tried and, if it fails, still gets counted.
*
* <p>The blanking loop also re-asserts, on its own, the same {@code [A-Za-z_][A-Za-z0-9_]*} shape
* check the enumeration loop already applied. Before {@code eval} was introduced a non-conforming
* name reaching {@code export "$n="} was harmless either way — the quoting made it inert. With
* {@code eval}, the name is spliced into a string and interpreted as shell syntax, so the enumeration
* loop's check is no longer sufficient on its own to keep that call site safe — it is a guard on a
* different loop, and the two must not silently drift apart. Re-checking right before the
* {@code eval} keeps that call site safe by its own reading, independent of whatever the enumeration
* loop does or stops doing in a later change.
*/
public final class EnvAllowListScrub {
@@ -132,10 +151,16 @@ public final class EnvAllowListScrub {
}
/**
* A parsed {@code scrub-report.txt}: how many exported variables existed when the scrub ran,
* how many were left untouched (allowed), and the NAMES that were blanked. Values never appear.
* A parsed {@code scrub-report.txt}: how many exported variables existed when the scrub ran
* ({@code total}), how many were left untouched ({@code allowed}), how many the scrub attempted
* to blank but could not ({@code failed} — fleetd #394: a zsh read-only/special parameter such
* as {@code UID} fatally errors on plain {@code export NAME=}, so those attempts go through
* {@code eval} instead so the loop keeps going and the failure is counted rather than left
* invisible), and the NAMES in each of the latter two categories. {@code allowed +
* blanked.size() + unblankable.size() == total}, and {@code unblankable.size() == failed}.
* Values never appear.
*/
record ScrubReport(int allowed, int total, List<String> blanked) {
record ScrubReport(int allowed, int total, int failed, List<String> blanked, List<String> unblankable) {
}
/**
@@ -334,15 +359,46 @@ public final class EnvAllowListScrub {
_cb633_blank+=("$_cb633_n")
done
{ for _cb633_n in "${_cb633_blank[@]}"; do export "$_cb633_n="; done; } 2>/dev/null
# fleetd #394: plain `export "$n="` is FATAL for a zsh read-only/special parameter
# (e.g. UID) and aborts this whole sourced file — every name still to come is never
# blanked, and the report below is never written, silently. `eval` contains that
# error to the single iteration instead: it still fails for that one name, but the
# loop continues and we can tell allowed / blanked / unblankable apart afterwards.
# This is not a skip-list of known-bad names (that would miss the next one nobody
# thought of) — every name in _cb633_blank is still attempted, unconditionally.
# Every name reaching this loop already passed the identical identifier check in the
# enumeration loop above — but that guard is 20 lines away in a different loop, and
# this line is about to splice the name into a string handed to `eval`. Before this
# fix the name only ever reached `export` quoted ("$n="), which is inert on a
# non-identifier string either way; `eval` makes THIS line the only thing standing
# between such a string and code execution in the member's pane, so it re-asserts the
# same check on its own rather than trusting a guard it does not own. Under normal
# operation this can never fire (the enumeration guard already filtered everything
# reaching _cb633_blank), so a name caught here is counted as unblankable rather than
# silently dropped — it is real evidence that the upstream guard was bypassed.
typeset -a _cb633_ok _cb633_unblankable
_cb633_ok=()
_cb633_unblankable=()
for _cb633_n in "${_cb633_blank[@]}"; do
if [[ ! "$_cb633_n" =~ ^[A-Za-z_][A-Za-z0-9_]*$ ]]; then
_cb633_unblankable+=("$_cb633_n")
continue
fi
if eval "export ${_cb633_n}=" 2>/dev/null; then
_cb633_ok+=("$_cb633_n")
else
_cb633_unblankable+=("$_cb633_n")
fi
done
integer _cb633_kept=$(( _cb633_total - ${#_cb633_blank} ))
{
print -r -- "allowed $_cb633_kept of $_cb633_total"
for _cb633_n in "${_cb633_blank[@]}"; do print -r -- "$_cb633_n"; done
print -r -- "allowed $_cb633_kept of $_cb633_total failed ${#_cb633_unblankable}"
for _cb633_n in "${_cb633_ok[@]}"; do print -r -- "$_cb633_n"; done
for _cb633_n in "${_cb633_unblankable[@]}"; do print -r -- "!$_cb633_n"; done
} > "$ZDOTDIR/%s" 2>/dev/null
unset _cb633_allowed _cb633_names _cb633_blank _cb633_n _cb633_total _cb633_kept
unset _cb633_allowed _cb633_names _cb633_blank _cb633_ok _cb633_unblankable _cb633_n _cb633_total _cb633_kept
""".formatted(names, MemberEnvAllowList.zshCasePattern(), REPORT_FILE);
}
@@ -356,6 +412,11 @@ public final class EnvAllowListScrub {
* Read and parse {@link #REPORT_FILE} out of a generated ZDOTDIR directory. Returns {@code null}
* when absent or unreadable (the pane may have been torn down before its login shell ever got to
* the scrub) — callers treat that as "no measurement available", never as success.
*
* <p>First line is {@code "allowed <N> of <M> failed <F>"} (fleetd #394 added the trailing
* {@code failed <F>} — a count of names the scrub attempted to blank but could not, e.g. a zsh
* read-only/special parameter). Every following non-blank line is a name: a bare name was
* blanked, a {@code !}-prefixed name was attempted and failed. Values never appear on either.
*/
static ScrubReport readReport(Path zdotdir) {
Path report = zdotdir.resolve(REPORT_FILE);
@@ -368,17 +429,24 @@ public final class EnvAllowListScrub {
return null;
}
String[] parts = lines.getFirst().substring("allowed ".length()).trim().split("\\s+");
if (parts.length != 3 || !"of".equals(parts[1])) {
if (parts.length != 5 || !"of".equals(parts[1]) || !"failed".equals(parts[3])) {
return null;
}
List<String> blanked = new ArrayList<>();
List<String> unblankable = new ArrayList<>();
for (int i = 1; i < lines.size(); i++) {
if (!lines.get(i).isBlank()) {
blanked.add(lines.get(i));
String line = lines.get(i);
if (line.isBlank()) {
continue;
}
if (line.startsWith("!")) {
unblankable.add(line.substring(1));
} else {
blanked.add(line);
}
}
return new ScrubReport(Integer.parseInt(parts[0]), Integer.parseInt(parts[2]),
List.copyOf(blanked));
Integer.parseInt(parts[4]), List.copyOf(blanked), List.copyOf(unblankable));
} catch (IOException | NumberFormatException e) {
return null;
}
@@ -1643,11 +1643,19 @@ public abstract class HerdrPeerLauncher implements PeerLauncher {
log.warn("memberCredentials allow-list: pane {} left no scrub report in {} — the "
+ "environment scrub cannot be confirmed to have run. Either the pane ended "
+ "before its shell finished starting, or its shell never read our generated "
+ "startup files, in which case that member saw the full host environment.",
+ "startup files. Either way, we cannot tell from here whether the scrub ran, "
+ "so we do not know what that member's environment contained.",
paneId, dir);
} else {
log.info("memberCredentials allow-list: pane {} allowed {} of {} environment variables",
paneId, report.allowed(), report.total());
if (report.failed() > 0) {
log.warn("memberCredentials allow-list: pane {} could not blank {} environment "
+ "variable(s) — {} (likely a zsh read-only/special parameter) — those "
+ "names were left in the member's environment. Confirm none of them is a "
+ "credential.",
paneId, report.failed(), report.unblankable());
}
List<String> shaped = report.blanked().stream()
.filter(name -> CREDENTIAL_SHAPED_NAME.matcher(name).matches())
.toList();
@@ -3,6 +3,7 @@ package dev.ltms.fleet.mcp;
import dev.ltms.fleet.auth.CallerResolver;
import dev.ltms.fleet.auth.MemberRegistry;
import dev.ltms.fleet.auth.Principal;
import dev.ltms.fleet.auth.Role;
import dev.ltms.fleet.config.FleetConfig;
import dev.ltms.fleet.guard.SubscriptionGuard;
import dev.ltms.fleet.herdr.AgentControl;
@@ -28,6 +29,7 @@ import dev.ltms.fleet.placement.BackendQuarantine;
import dev.ltms.fleet.placement.PlacementPolicies;
import io.modelcontextprotocol.spec.McpSchema;
import dev.ltms.fleet.msg.InMemoryReplyInbox;
import dev.ltms.fleet.msg.ReplyInbox;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
@@ -77,7 +79,7 @@ class FleetMcpTest {
Thread.sleep(5);
}
assertTrue(rendezvous.isWaiting(target), "send should be accepted for " + target);
FleetMcp.reply(messages, target, "received");
FleetMcp.reply(messages, target, Role.WORKER, "received");
assertEquals("received", textOf(send.get(6, TimeUnit.SECONDS)));
}
@@ -110,7 +112,7 @@ class FleetMcpTest {
// fleetd #365: a resolved live send must read distinctly from a merely-queued reply —
// see replyWithNoPendingSendIsQueuedNotError below for the other case.
McpSchema.CallToolResult reply = FleetMcp.reply(messages, "term_a", "LGTM");
McpSchema.CallToolResult reply = FleetMcp.reply(messages, "term_a", Role.WORKER, "LGTM");
assertEquals(MessageService.ReplyOutcome.RESOLVED_SEND.description(), textOf(reply));
McpSchema.CallToolResult res = send.get(6, TimeUnit.SECONDS);
@@ -136,7 +138,7 @@ class FleetMcpTest {
}
assertTrue(rendezvous.isWaiting("term_a"), "send should have opened its waiter");
McpSchema.CallToolResult reply = FleetMcp.reply(messages, "term_a", "async LGTM");
McpSchema.CallToolResult reply = FleetMcp.reply(messages, "term_a", Role.WORKER, "async LGTM");
assertEquals(MessageService.ReplyOutcome.RESOLVED_SEND.description(), textOf(reply));
// Poll until the async send completes and reports the reply.
@@ -183,7 +185,7 @@ class FleetMcpTest {
Thread.sleep(5);
}
assertTrue(rendezvous.isWaiting("term_a"));
FleetMcp.reply(messages, "term_a", "done");
FleetMcp.reply(messages, "term_a", Role.WORKER, "done");
assertEquals("done", textOf(answer.get(6, TimeUnit.SECONDS)));
McpSchema.CallToolResult done = FleetMcp.poll(messages, ticket, null);
@@ -215,7 +217,7 @@ class FleetMcpTest {
// fleet_poll{ticket} stuck PENDING forever and later force-failed with a false "session
// released before it replied" reason. This used to land in the inbox instead (see the old
// assertion this replaced: messages.drainReplies("term_a").getFirst()...) — that was the bug.
FleetMcp.reply(messages, "term_a", "finished after timeout");
FleetMcp.reply(messages, "term_a", Role.WORKER, "finished after timeout");
assertEquals("finished after timeout", textOf(FleetMcp.poll(messages, ticket, null)));
assertTrue(messages.drainReplies("term_a").isEmpty(),
"the reply completed its own ticket directly and never touched the inbox");
@@ -269,7 +271,7 @@ class FleetMcpTest {
}
assertEquals(MessageService.Phase.FAILED, second.phase());
FleetMcp.reply(messages, "term_a", "late reply");
FleetMcp.reply(messages, "term_a", Role.WORKER, "late reply");
assertEquals("late reply", messages.drainReplies("term_a").getFirst().content());
CompletableFuture<McpSchema.CallToolResult> answer = CompletableFuture.supplyAsync(
@@ -278,7 +280,7 @@ class FleetMcpTest {
while (!rendezvous.isWaiting("term_a") && System.currentTimeMillis() < deadline) {
Thread.sleep(5);
}
FleetMcp.reply(messages, "term_a", "done");
FleetMcp.reply(messages, "term_a", Role.WORKER, "done");
assertEquals("done", textOf(answer.get(6, TimeUnit.SECONDS)));
}
@@ -331,7 +333,7 @@ class FleetMcpTest {
void replyWithNoPendingSendIsQueuedNotError() {
// CB-307: a reply with no open send is now queued in the inbox, not an error.
// fleetd #365: it must also no longer claim "delivered" — nothing was waiting for it.
McpSchema.CallToolResult res = FleetMcp.reply(messages, "term_a", "orphan");
McpSchema.CallToolResult res = FleetMcp.reply(messages, "term_a", Role.WORKER, "orphan");
assertNotEquals(Boolean.TRUE, res.isError(), "a queued reply is not an error");
assertEquals(MessageService.ReplyOutcome.QUEUED.description(), textOf(res));
@@ -341,6 +343,40 @@ class FleetMcpTest {
assertEquals("orphan", drained.getFirst().content());
}
@Test
void replyFromLeadIsRefusedBeforeItCanPublishToTheWorkerInbox() {
ReplyInbox inboxThatRejectsPublishes = new ReplyInbox() {
@Override public void own(String target) { }
@Override public void release(String target) { }
@Override public void publish(String target, String msgId, String content) {
fail("a lead fleet_reply must not publish to the worker inbox");
}
@Override public List<InboxMessage> peek(String target) { return List.of(); }
@Override public void ack(String target, String msgId) { }
};
MessageService leadMessages = new MessageService(agents, new Injector(agents), new Rendezvous(),
inboxThatRejectsPublishes);
McpSchema.CallToolResult res = assertDoesNotThrow(
() -> FleetMcp.reply(leadMessages, "term_lead", Role.PRIMARY, "peer reply"));
assertTrue(res.isError());
assertEquals("fleet_reply has no route to a peer lead. Use fleet_send{coordId: ...} for a peer on another "
+ "daemon or fleet_send{sessionId: ...} for a peer on this host. fleet_reply resolves a member's "
+ "blocked fleet_send, and a peer's coord-id message is durable and non-blocking, so there is "
+ "nothing for it to resolve.",
textOf(res));
}
@Test
void replyFromUnidentifiedCallerKeepsItsOwnError() {
McpSchema.CallToolResult res = FleetMcp.reply(messages, null, Role.PRIMARY, "reply");
assertTrue(res.isError());
assertEquals("fleet_reply is for workers only — could not identify the calling worker from the connection",
textOf(res));
}
@Test
void replyWithBlankContentIsACleanToolErrorNotAnUncaughtException() {
// fleetd #302: MessageService.reply now REJECTS blank content by throwing. fleet_reply's
@@ -351,7 +387,7 @@ class FleetMcpTest {
// has always used isBlank for exactly this reason.
for (String blank : new String[] {null, "", " ", "\n\t"}) {
McpSchema.CallToolResult res = assertDoesNotThrow(
() -> FleetMcp.reply(messages, "term_a", blank),
() -> FleetMcp.reply(messages, "term_a", Role.WORKER, blank),
"blank content must be refused as a tool error, never thrown out of the handler");
assertEquals(Boolean.TRUE, res.isError(), "blank content is an error result");
assertTrue(textOf(res).contains("content is required"),
@@ -364,7 +400,7 @@ class FleetMcpTest {
@Test
void bridgePollWithTargetDrainsReplies() {
// A reply with no open send queues it in the inbox.
FleetMcp.reply(messages, "term_a", "queued-msg");
FleetMcp.reply(messages, "term_a", Role.WORKER, "queued-msg");
// fleet_poll with target drains the inbox.
McpSchema.CallToolResult res = FleetMcp.poll(messages, null, "term_a");
@@ -415,7 +451,7 @@ class FleetMcpTest {
Thread.sleep(5);
}
assertTrue(rendezvous.isWaiting("term_a"), "the answer should have reopened a waiter");
McpSchema.CallToolResult reply = FleetMcp.reply(messages, "term_a", "done");
McpSchema.CallToolResult reply = FleetMcp.reply(messages, "term_a", Role.WORKER, "done");
assertEquals(MessageService.ReplyOutcome.RESOLVED_SEND.description(), textOf(reply));
assertEquals("done", textOf(answer.get(6, TimeUnit.SECONDS)));
}
@@ -1254,13 +1290,13 @@ class FleetMcpTest {
@Test
void bridgeAckRemovesSpecificReply() {
// Queue a reply and capture its msgId.
FleetMcp.reply(messages, "term_a", "orphan");
FleetMcp.reply(messages, "term_a", Role.WORKER, "orphan");
var before = messages.drainReplies("term_a");
assertEquals(1, before.size(), "one reply in the inbox");
String msgId = before.getFirst().msgId();
// Publish the same reply again and ack it via fleet_ack surface.
FleetMcp.reply(messages, "term_a", "orphan-again");
FleetMcp.reply(messages, "term_a", Role.WORKER, "orphan-again");
var peeked = messages.drainReplies("term_a");
assertEquals(1, peeked.size(), "one fresh reply in the inbox");
@@ -18,6 +18,7 @@ import java.util.regex.Matcher;
import java.util.regex.Pattern;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
@@ -118,6 +119,131 @@ class EnvAllowListScrubTest {
"allowed N of M with N <= M — the denominator is always reported");
}
/**
* fleetd #394: the actual defect. Plain {@code export "$n="} is FATAL for a zsh read-only or
* special parameter (e.g. {@code UID}) and aborts the whole sourced file — every name still to
* come is never blanked, and the {@code scrub-report.txt} below is never written at all,
* silently ({@code 2>/dev/null} swallows the error). This plants an unblankable, exported,
* read-only variable in the MIDDLE of the names the scrub attempts to blank, with two more
* names after it, and asserts that both of those later names are STILL blanked and the report
* is STILL written with the failure counted — a test that only checked names BEFORE the failure
* point would pass today and prove nothing.
*
* <p>The planted name is a made-up one ({@code FLEETD_TEST_UNBLANKABLE}), not {@code UID} or
* any other name a skip-list might already know about — invariant 1 is that the loop survives
* ANY unblankable name, not a known one, so the test must not lean on one either.
*
* <p>Exercises the real artefact: {@link EnvAllowListScrub#scrubScript} is run verbatim under a
* real {@code /bin/zsh}, not just asserted on as a Java string. The four planted names are
* exported one at a time via {@code typeset -x}/{@code typeset -rx} immediately before the
* script runs, in a fixed order — zsh's {@code export}/{@code typeset -x} appends to the
* process's environment table in call order (verified empirically: a freshly-exported name
* always sorts after every inherited one and after every earlier freshly-exported name in
* {@code command env}'s own output), which is what makes the "middle" position deterministic
* here, unlike relying on the OS's own inherited-environment order.
*/
@Test
void unblankableNameInTheMiddleDoesNotAbortNamesAfterIt(@TempDir Path tmp) throws Exception {
assumeTrue(Files.isExecutable(ZSH), "/bin/zsh not present — nothing to prove here");
// Only ZDOTDIR is allowed — it must survive the scrub itself, since the report is written
// to "$ZDOTDIR/..." AFTER the blanking loop runs; if ZDOTDIR were blanked as a side effect,
// the report write would silently go to the wrong place instead of testing anything.
String script = EnvAllowListScrub.scrubScript(Set.of("ZDOTDIR"));
String setup = """
typeset -x FLEETD_TEST_BEFORE=1
typeset -rx FLEETD_TEST_UNBLANKABLE=1
typeset -x FLEETD_TEST_AFTER_A=1
typeset -x FLEETD_TEST_AFTER_B=1
""";
ProcessBuilder pb = new ProcessBuilder("/bin/zsh");
pb.environment().clear();
pb.environment().put("PATH", "/usr/bin:/bin");
pb.environment().put("ZDOTDIR", tmp.toAbsolutePath().toString());
pb.redirectError(ProcessBuilder.Redirect.DISCARD);
Process zsh = pb.start();
zsh.getOutputStream().write((setup + script).getBytes(StandardCharsets.UTF_8));
zsh.getOutputStream().flush();
zsh.getOutputStream().close();
assertTrue(zsh.waitFor(60, java.util.concurrent.TimeUnit.SECONDS),
"the scrub script did not exit within 60s");
assertEquals(0, zsh.exitValue(),
"the scrub script itself must never abort — an unblankable name must not kill the "
+ "sourced file");
EnvAllowListScrub.ScrubReport report = EnvAllowListScrub.readReport(tmp);
assertNotNull(report, "the report must still be written even though one name could not be "
+ "blanked — a report that silently never appears is the #394 bug");
assertTrue(report.blanked().contains("FLEETD_TEST_BEFORE"),
"sanity: the name before the unblankable one must be blanked");
assertTrue(report.blanked().contains("FLEETD_TEST_AFTER_A"),
"the FIRST name AFTER the unblankable one must still be blanked — before the fix, "
+ "the whole loop aborted at the unblankable name and every later name was "
+ "silently left untouched");
assertTrue(report.blanked().contains("FLEETD_TEST_AFTER_B"),
"the SECOND name after the unblankable one must also still be blanked");
assertEquals(1, report.failed(),
"exactly one attempted name could not be blanked, and that count must be visible "
+ "without reading the member's environment");
assertEquals(List.of("FLEETD_TEST_UNBLANKABLE"), report.unblankable(),
"the unblankable name is reported by name, not silently dropped");
}
/**
* fleetd #394 follow-up: the blanking loop's {@code eval "export ${n}="} splices {@code n} into
* a string that zsh then interprets as shell syntax. That is only safe because every name
* reaching {@code _cb633_blank} already passed an identifier check in the ENUMERATION loop
* (20 lines away, in a different loop) — so the fix re-asserts the identical check immediately
* before the {@code eval} call, rather than trusting that distant guard to keep holding.
*
* <p>This test plants a value with an embedded newline, exploiting the exact "junk from
* multi-line values" gap the enumeration loop's own comment already documents: {@code command
* env}'s text output is read line-by-line, so a value's second line becomes a spurious extra
* "name" that was never a real exported variable. The fragment used here ({@code
* junk.fragment}) is merely non-conforming (it contains a dot) — never command-shaped; this
* test must never demonstrate command execution and plants no command-shaped payload.
*
* <p>Exercises the real artefact end-to-end: {@link EnvAllowListScrub#scrubScript} runs
* verbatim under a real {@code /bin/zsh}, exactly as {@code generate()} would produce it — this
* is not a synthetic call into just the blanking loop.
*/
@Test
void nonIdentifierJunkFromAMultilineValueIsSkippedNotBlankedOrUnblankable(@TempDir Path tmp)
throws Exception {
assumeTrue(Files.isExecutable(ZSH), "/bin/zsh not present — nothing to prove here");
String script = EnvAllowListScrub.scrubScript(Set.of("ZDOTDIR"));
ProcessBuilder pb = new ProcessBuilder("/bin/zsh");
pb.environment().clear();
pb.environment().put("PATH", "/usr/bin:/bin");
pb.environment().put("ZDOTDIR", tmp.toAbsolutePath().toString());
// Embedded newline: `command env`'s own text output splits this into two lines, and the
// second ("junk.fragment") has no "=" at all, so `cut -d= -f1` returns it unchanged as a
// spurious candidate "name" — it was never an actual exported variable by that name.
pb.environment().put("FLEETD_TEST_MULTILINE", "keep\njunk.fragment");
pb.redirectError(ProcessBuilder.Redirect.DISCARD);
Process zsh = pb.start();
zsh.getOutputStream().write(script.getBytes(StandardCharsets.UTF_8));
zsh.getOutputStream().flush();
zsh.getOutputStream().close();
assertTrue(zsh.waitFor(60, java.util.concurrent.TimeUnit.SECONDS),
"the scrub script did not exit within 60s");
assertEquals(0, zsh.exitValue(), "the scrub script must reach its end");
EnvAllowListScrub.ScrubReport report = EnvAllowListScrub.readReport(tmp);
assertNotNull(report, "the report must still be written");
assertTrue(report.blanked().contains("FLEETD_TEST_MULTILINE"),
"sanity: the real, identifier-shaped variable must still be blanked normally");
assertFalse(report.blanked().contains("junk.fragment"),
"a non-identifier fragment is not a real variable and must never be blanked");
assertFalse(report.unblankable().contains("junk.fragment"),
"a non-identifier fragment must never even become a candidate the blanking loop "
+ "attempts — it must be filtered before either guard has to catch it, so "
+ "it is neither blanked nor counted as a failed attempt");
}
/** A group-shared ZDOTDIR still lets the member truncate and write its pre-created receipt. */
@Test
void groupSharedScrubWritesAndReadsItsReport(@TempDir Path tmp) throws Exception {