fleet/fleetd
3
0
Fork 0
Code Issues 73 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

fleetd#341: per-name guard so a later spawns different unprotected credential name still warns #346

Closed
agent wants to merge 0 commits from worker/fleetd-341-af5a6b-24 into main
pull from: worker/fleetd-341-af5a6b-24
merge into: fleet:main
Conversation 0 Commits - Files Changed -
agent commented 2026-09-04 11:01:12 +02:00
Member
Copy Link
Copy Source

fleetd #341 — a per-name guard so a later spawn's different unprotected name still warns

What was wrong (reproduced before touching anything)

unprotectedGapLogged was a single AtomicBoolean guarding TWO WARN branches in
logCredentialGap that name DIFFERENT env var names:

  • HerdrPeerLauncher.java:1819 (old line numbers) — the allow-list branch: names the derived
    allow-list keeps anyway (keptByDerivedList), so "every member pane inherits them UNBLOCKED".
  • HerdrPeerLauncher.java:1839 (warnGapUnprotected) — the deny-by-default and allow-list
    non-zsh-fallback WARN.

Because memberCredentials is a live, re-read-per-spawn supplier, the policy can change between
two spawns on one launcher instance. I wrote a test with two spawns on one HerdrPeerLauncher:
spawn 1 under deny-by-default with gap {SPAWN_ONE_UNCOVERED_TOKEN} (trips the shared flag),
then a live policy reload to allow-list and spawn 2 with a DIFFERENT gap
{FLEETD_WORKER_TOKEN} (the test profile's own tokenEnv, kept by the derived allow-list).

Ran against unfixed code first, as required: spawn 1's WARN fired; spawn 2's WARN for
FLEETD_WORKER_TOKEN never fired — confirmed suppressed, exactly as the ticket describes. This
was a real bug, not a false alarm.

org.opentest4j.AssertionFailedError: spawn 2's gap names a DIFFERENT env var than spawn 1
(FLEETD_WORKER_TOKEN, not SPAWN_ONE_UNCOVERED_TOKEN) ... ==> expected: <true> but was: <false>

The fix

Replaced unprotectedGapLogged (AtomicBoolean) with unprotectedGapNamesWarned
(Set<String> via ConcurrentHashMap.newKeySet()), the same shape
OpenCodeLauncher.modelCheckSkippedWarned already uses one file over for the same
once-per-distinct-thing reason — followed the existing pattern rather than inventing a new one.
Both WARN call sites now do gap.stream().filter(unprotectedGapNamesWarned::add).toList() and
log only the names that filter kept (i.e. names never warned about before), instead of gating the
whole branch on one flag. allowListGapLogged (the separate INFO guard from #192) is untouched —
still a per-instance AtomicBoolean, still its own flag, per that ticket's reasoning, which is
correct and this ticket does not touch.

Wording of both WARNs is unchanged — same format strings, only the substituted name-list/count
arguments now come from the filtered (deduped) list instead of the raw gap.

The two questions the ticket asked me to answer explicitly

What bounds the set? Every name added first passed CREDENTIAL_SHAPED_NAME filtering over
hostEnvNames, i.e. it is a real environment variable name from the daemon's OWN process — a
small, OS-bounded source (a host environment realistically has tens to a few hundred entries), not
attacker- or request-controlled input. So the set's size is capped by however many distinct
credential-shaped names this host's environment has ever held across the launcher's lifetime,
which converges quickly and stays effectively fixed for the life of one daemon process. I judged
this "probably fine" reasoning in the ticket to be correct and did not add a separate cap.

When a WARN fires for {A,B} and a later spawn's gap is {B,C}, what gets logged? I chose to log
only the NEW names — {C} — not the whole current gap {B,C}. Reasoning: invariant 1 in the
ticket is explicit that the fix must be "once per distinct name, not once per spawn", and a
Set<String> guard with .add()-based filtering is what makes that literal — each name gets
exactly one WARN line, ever, across the launcher's whole life. Re-printing already-warned names
on every later spawn that happens to still contain them would turn the guard back into "once per
spawn" for any name that persists across policy reloads (the common case — most gap names don't
change between spawns), defeating the noise control the ticket said to keep. The tradeoff is that
an operator who only reads ONE log line will not see the complete current gap in that line; they
would need to also recall (or grep for) the earlier line for name B. I judged this acceptable
because the earlier WARN already told them about B in full, unsuppressed, and each name's WARN
still names memberCredentials.known/.allow as the fix, independent of when it was printed.

Reproduction / mutation proof

  1. Wrote the new test aDifferentUnprotectedGapOnALaterSpawnIsNotSuppressedByAnEarlierSpawnsWarn
    in HerdrPeerLauncherAllowListWiringTest.
  2. Ran it against unfixed code — FAILED as quoted above (proves the defect).
  3. Applied the fix, ran it — PASSED, along with all 21 tests in that class.
  4. Mutation proof: backed up the fixed file, git checkout -- reverted
    HerdrPeerLauncher.java to main's version (HEAD, unstaged — never committed), re-ran the
    single new test — it FAILED again with the identical assertion error quoted above. Restored the
    fixed file from the backup (cp back), confirmed unprotectedGapNamesWarned was back in the
    file. Re-ran the full class — 21/21 passed again.

Full build (unpiped, cd fleetd && mvn clean install)

[INFO] Tests run: 1356, Failures: 0, Errors: 0, Skipped: 0
...
[INFO] BUILD SUCCESS
[INFO] Total time:  49.037 s

(main's baseline, per the lead's own measurement today, was 1355 — my one new test accounts for
the +1.)

Shape survey — "find what else has this shape", report only, not fixed

Delegated a read-only sweep of member/ and inject/ for other single one-shot log guards
(AtomicBoolean/boolean) gating MULTIPLE log call sites with potentially different content.
Result: no other instance of this shape found.

Checked and ruled out (each gates exactly 1 call site):

  • HerdrPeerLauncher.resetUnsupportedLogged
  • HerdrPeerLauncher.nonZshShellWarned
  • HerdrPeerLauncher.cannotShareScrubDirWarned
  • HerdrPeerLauncher.unknownMemberEnvironmentWarned
  • HerdrPeerLauncher.allowListGapLogged (the sibling this fix deliberately left alone)
  • OpenCodeLauncher.discoveryUnavailableWarned
  • OpenCodeLauncher.modelMismatchReported (1 CAS site + 1 plain early-exit check, not a second
    log site)
  • OpenCodeSessionDiscovery.warnedMissingDatabase

inject/ (Injector, MemberPresence, StatusPoller, StatusRefiner, TurnListener,
CompletionResolver, BackendErrorSink, ExhaustionSink) has no boolean-based log guards at
all.

Files changed

  • src/main/java/dev/ltms/fleet/member/HerdrPeerLauncher.java
  • src/test/java/dev/ltms/fleet/member/HerdrPeerLauncherAllowListWiringTest.java

Caveat for review

None known. Both invariants (keep allowListGapLogged separate; don't change WARN wording) were
followed literally — please double check the wording diff is truly zero (only the argument
lists/variable names changed, not the format strings) since that was an explicit constraint.

## fleetd #341 — a per-name guard so a later spawn's different unprotected name still warns ### What was wrong (reproduced before touching anything) `unprotectedGapLogged` was a single `AtomicBoolean` guarding TWO WARN branches in `logCredentialGap` that name DIFFERENT env var names: - `HerdrPeerLauncher.java:1819` (old line numbers) — the allow-list branch: names the derived allow-list keeps anyway (`keptByDerivedList`), so "every member pane inherits them UNBLOCKED". - `HerdrPeerLauncher.java:1839` (`warnGapUnprotected`) — the deny-by-default and allow-list non-zsh-fallback WARN. Because `memberCredentials` is a live, re-read-per-spawn supplier, the policy can change between two spawns on one launcher instance. I wrote a test with two spawns on one `HerdrPeerLauncher`: spawn 1 under `deny-by-default` with gap `{SPAWN_ONE_UNCOVERED_TOKEN}` (trips the shared flag), then a live policy reload to `allow-list` and spawn 2 with a DIFFERENT gap `{FLEETD_WORKER_TOKEN}` (the test profile's own `tokenEnv`, kept by the derived allow-list). **Ran against unfixed code first**, as required: spawn 1's WARN fired; spawn 2's WARN for `FLEETD_WORKER_TOKEN` never fired — confirmed suppressed, exactly as the ticket describes. This was a real bug, not a false alarm. ``` org.opentest4j.AssertionFailedError: spawn 2's gap names a DIFFERENT env var than spawn 1 (FLEETD_WORKER_TOKEN, not SPAWN_ONE_UNCOVERED_TOKEN) ... ==> expected: <true> but was: <false> ``` ### The fix Replaced `unprotectedGapLogged` (`AtomicBoolean`) with `unprotectedGapNamesWarned` (`Set<String>` via `ConcurrentHashMap.newKeySet()`), the same shape `OpenCodeLauncher.modelCheckSkippedWarned` already uses one file over for the same once-per-distinct-thing reason — followed the existing pattern rather than inventing a new one. Both WARN call sites now do `gap.stream().filter(unprotectedGapNamesWarned::add).toList()` and log only the names that filter kept (i.e. names never warned about before), instead of gating the whole branch on one flag. `allowListGapLogged` (the separate INFO guard from #192) is untouched — still a per-instance `AtomicBoolean`, still its own flag, per that ticket's reasoning, which is correct and this ticket does not touch. **Wording of both WARNs is unchanged** — same format strings, only the substituted name-list/count arguments now come from the filtered (deduped) list instead of the raw gap. ### The two questions the ticket asked me to answer explicitly **What bounds the set?** Every name added first passed `CREDENTIAL_SHAPED_NAME` filtering over `hostEnvNames`, i.e. it is a real environment variable name from the daemon's OWN process — a small, OS-bounded source (a host environment realistically has tens to a few hundred entries), not attacker- or request-controlled input. So the set's size is capped by however many distinct credential-shaped names this host's environment has ever held across the launcher's lifetime, which converges quickly and stays effectively fixed for the life of one daemon process. I judged this "probably fine" reasoning in the ticket to be correct and did not add a separate cap. **When a WARN fires for {A,B} and a later spawn's gap is {B,C}, what gets logged?** I chose to log only the NEW names — `{C}` — not the whole current gap `{B,C}`. Reasoning: invariant 1 in the ticket is explicit that the fix must be "once per distinct name, not once per spawn", and a `Set<String>` guard with `.add()`-based filtering is what makes that literal — each name gets exactly one WARN line, ever, across the launcher's whole life. Re-printing already-warned names on every later spawn that happens to still contain them would turn the guard back into "once per spawn" for any name that persists across policy reloads (the common case — most gap names don't change between spawns), defeating the noise control the ticket said to keep. The tradeoff is that an operator who only reads ONE log line will not see the complete current gap in that line; they would need to also recall (or grep for) the earlier line for name B. I judged this acceptable because the earlier WARN already told them about B in full, unsuppressed, and each name's WARN still names memberCredentials.known/.allow as the fix, independent of when it was printed. ### Reproduction / mutation proof 1. Wrote the new test `aDifferentUnprotectedGapOnALaterSpawnIsNotSuppressedByAnEarlierSpawnsWarn` in `HerdrPeerLauncherAllowListWiringTest`. 2. Ran it against unfixed code — FAILED as quoted above (proves the defect). 3. Applied the fix, ran it — PASSED, along with all 21 tests in that class. 4. **Mutation proof**: backed up the fixed file, `git checkout --` reverted `HerdrPeerLauncher.java` to `main`'s version (HEAD, unstaged — never committed), re-ran the single new test — it FAILED again with the identical assertion error quoted above. Restored the fixed file from the backup (`cp` back), confirmed `unprotectedGapNamesWarned` was back in the file. Re-ran the full class — 21/21 passed again. ### Full build (unpiped, `cd fleetd && mvn clean install`) ``` [INFO] Tests run: 1356, Failures: 0, Errors: 0, Skipped: 0 ... [INFO] BUILD SUCCESS [INFO] Total time: 49.037 s ``` (main's baseline, per the lead's own measurement today, was 1355 — my one new test accounts for the +1.) ### Shape survey — "find what else has this shape", report only, not fixed Delegated a read-only sweep of `member/` and `inject/` for other single one-shot log guards (`AtomicBoolean`/boolean) gating MULTIPLE log call sites with potentially different content. Result: **no other instance of this shape found.** Checked and ruled out (each gates exactly 1 call site): - `HerdrPeerLauncher.resetUnsupportedLogged` - `HerdrPeerLauncher.nonZshShellWarned` - `HerdrPeerLauncher.cannotShareScrubDirWarned` - `HerdrPeerLauncher.unknownMemberEnvironmentWarned` - `HerdrPeerLauncher.allowListGapLogged` (the sibling this fix deliberately left alone) - `OpenCodeLauncher.discoveryUnavailableWarned` - `OpenCodeLauncher.modelMismatchReported` (1 CAS site + 1 plain early-exit check, not a second log site) - `OpenCodeSessionDiscovery.warnedMissingDatabase` `inject/` (`Injector`, `MemberPresence`, `StatusPoller`, `StatusRefiner`, `TurnListener`, `CompletionResolver`, `BackendErrorSink`, `ExhaustionSink`) has no boolean-based log guards at all. ### Files changed - `src/main/java/dev/ltms/fleet/member/HerdrPeerLauncher.java` - `src/test/java/dev/ltms/fleet/member/HerdrPeerLauncherAllowListWiringTest.java` ### Caveat for review None known. Both invariants (keep `allowListGapLogged` separate; don't change WARN wording) were followed literally — please double check the wording diff is truly zero (only the argument lists/variable names changed, not the format strings) since that was an explicit constraint.
agent added 1 commit 2026-09-04 11:01:13 +02:00
fleetd#341: a per-name guard so a later spawn's different unprotected name still warns
CI / contract (pull_request) Successful in 1m1s
Details
CI / build (pull_request) Successful in 1m29s
Details
464dbc0930 
unprotectedGapLogged was one AtomicBoolean guarding two WARN branches in
logCredentialGap that name different env var names (the allow-list
keptByDerivedList branch, and warnGapUnprotected's deny-by-default /
non-zsh-fallback branch). memberCredentials is a live, re-read-per-spawn
supplier, so between two spawns a policy reload can change which names are
in the gap: spawn 1 warns about name A and trips the shared flag, and
spawn 2's gap containing a different name B never gets its WARN.

Replace the AtomicBoolean with unprotectedGapNamesWarned, a
ConcurrentHashMap-backed Set<String> guard keyed per name (same shape as
OpenCodeLauncher.modelCheckSkippedWarned), so each distinct credential-shaped
name is warned about exactly once, ever, regardless of which branch or
which spawn first reports it. allowListGapLogged (the separate INFO guard,
#192) is untouched. Neither WARN's wording changed.
ltms closed this pull request 2026-09-04 11:14:53 +02:00
Some checks are pending
CI / contract (pull_request) Successful in 1m1s
Details
CI / build (pull_request) Successful in 1m29s
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
blocked
Cannot start until something else lands. The body says what.
 needs-live-proof
Merged and green, but never shown working on the running daemon. Not the same as done.
 ready-to-delegate
Scope, files and acceptance criteria are written. A worker can be briefed from the body alone.
 silent-default
A feature that compiles, passes tests, and ships turned off. Nine recurrences and counting.
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
agent kevin ltms
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#346
Delete Branch "worker/fleetd-341-af5a6b-24"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?