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

FleetdAssemblyFleetAppTest's javadoc states a conditional dead end as unconditional — it already cost a worker turn and nearly shipped 3 unnecessary public accessors #650

New Issue
Open
opened 2026-10-02 04:57:56 +02:00 by ltms · 0 comments
ltms commented 2026-10-02 04:57:56 +02:00
Owner
Copy Link
Copy Source

Summary

The class javadoc of fleetd/src/test/java/dev/ltms/fleet/FleetdAssemblyFleetAppTest.java (:62–:74) says a same-JVM test caller can never pass the Authz.Action.READ gate against a real assembly. That is true only under auth.mode: loopback-trust, which is what that test happens to use. The javadoc does not say so, and states it as a property of "the REAL assembly's real CallerResolver/ConnectionIdentity".

As of a42b124 the tree contains two merged tests that do the thing this javadoc calls impossible. A future session reading it will reach a false conclusion, because one already did.

The text

 * {@code Authz.Action.READ}, which — through the REAL assembly's real {@code
 * CallerResolver}/{@code ConnectionIdentity} (built with a hardcoded {@code
 * new LsofPeerPidLookup()}) — needs {@code Caller.resolved()}, i.e. a real positive pid from
 * {@code lsof}. {@code LsofPeerPidLookup} excludes its own pid (see its javadoc), and a JUnit
 * test's HTTP client and the daemon under test share one JVM pid, so the resolved pid is always
 * {@code -1} and every such request is refused as {@code ANONYMOUS} (fleetd #317's fail-closed
 * rule) before the route handler ... is ever reached. Verified
 * directly: driving {@code GET /sessions} here returns {@code 401 unauthenticated}

Two load-bearing words are wrong in general: READ needs Caller.resolved(), and every such request is refused.

Why it is false

CallerResolver.resolve(...), once c.terminal() == null:

if (tokenMode) {
    return presentedTokenMatches(authorizationHeader)
            ? Principal.primary(c.pid())
            : Principal.anonymous();
}

// loopback-trust below
return isLoopback(remoteAddr) && c.resolved() && c.scanComplete()
        ? Principal.primary(c.pid()) : Principal.anonymous();

c.resolved() and c.scanComplete() are in the loopback-trust branch only. Token mode returns before reaching them, so a valid bearer token resolves to PRIMARY with no pid lookup on the path at all. The LsofPeerPidLookup exclusion is real and is simply not consulted.

Why the javadoc's own evidence is consistent with this

FleetdAssemblyFleetAppTest has no auth: block, so it gets the default mode — loopback-trust (config/FleetConfig.java:71, :1643). Its 401 unauthenticated observation is therefore correct for its own config. Nothing it measured was wrong. The defect is purely that a result measured under one configuration is written down as a property of the assembly.

Measured proof of the opposite, now on main

  • FleetdQuarantineOutageDualWindowAssemblyTest (PR #648) — real McpSyncClient callTool and a real HttpClient GET, token-authenticated. I ran it at 91792e1: Tests run: 4, Failures: 0, Errors: 0, Skipped: 0 / BUILD SUCCESS, and it asserts 200 on the GET.
  • FleetdListReportingSourcesAssemblyTest (PR #649) — real token-authenticated callTool("fleet_list"). I mutated FleetdAssembly.java:481 and got Tests run: 3, Failures: 1 with the live fleet_list JSON body in the failure message.

So both operator windows are reachable in-JVM today, and the suite proves it.

The cost already paid

PR #647 cited this javadoc, concluded the round-trip was impossible, and added three public accessors to FleetMcp.java (capacitySource(), healthCoverageSource(), coordinatorPeers()) to work around it. That PR was closed unmerged and the unit redone as #649 with zero production change. One worker turn lost, and a production API widening avoided only because the premise was checked.

Worth noting what made it expensive: the author did the honest thing and cited its source. The source was wrong.

Suggested fix

Qualify the claim where it is made, and point at the counter-example:

  1. In FleetdAssemblyFleetAppTest's javadoc, scope the statement to loopback-trust — say that this test runs in that mode and that READ is unreachable in that mode, not in general.
  2. Name the escape hatch in the same breath: under auth.mode: token a bearer header resolves to PRIMARY with no pid lookup, and FleetdQuarantineOutageDualWindowAssemblyTest / FleetdListReportingSourcesAssemblyTest do exactly that.

Do not "fix" this by weakening it to a vague warning. The specific mechanism is worth keeping — it is correct and useful. Only the scope is wrong.

For contrast, mcp/ConnectionIdentity.java:38–:41 makes a neighbouring claim correctly: it says a full HTTP round trip cannot exercise the PaneLocator, which is true and stays true under token mode, because the pid is still -1 and PaneLocator is still never called. That is the right altitude — a claim about the path it is actually about.

The shape, for the hunt

A result measured under one configuration, written down as a property of the system. The giveaway is a javadoc that says "needs" or "always" or "every" about a code path that has a branch the author's own test never took.

This is a documentation instance of a defect class already on record here: a guard or gate described by the direction it was observed from. It is worth a sweep rather than a one-line fix — any test javadoc asserting that something is unreachable is a candidate, because unreachability is exactly the claim that quietly depends on configuration. I have not run that sweep; this ticket fixes the one instance that cost us something and names the shape for whoever does.

Found while adjudicating the #647/#648 contradiction for #612.

## Summary The class javadoc of `fleetd/src/test/java/dev/ltms/fleet/FleetdAssemblyFleetAppTest.java` (`:62`–`:74`) says a same-JVM test caller can never pass the `Authz.Action.READ` gate against a real assembly. That is true **only under `auth.mode: loopback-trust`**, which is what that test happens to use. The javadoc does not say so, and states it as a property of "the REAL assembly's real `CallerResolver`/`ConnectionIdentity`". As of `a42b124` the tree contains **two merged tests that do the thing this javadoc calls impossible.** A future session reading it will reach a false conclusion, because one already did. ## The text ``` * {@code Authz.Action.READ}, which — through the REAL assembly's real {@code * CallerResolver}/{@code ConnectionIdentity} (built with a hardcoded {@code * new LsofPeerPidLookup()}) — needs {@code Caller.resolved()}, i.e. a real positive pid from * {@code lsof}. {@code LsofPeerPidLookup} excludes its own pid (see its javadoc), and a JUnit * test's HTTP client and the daemon under test share one JVM pid, so the resolved pid is always * {@code -1} and every such request is refused as {@code ANONYMOUS} (fleetd #317's fail-closed * rule) before the route handler ... is ever reached. Verified * directly: driving {@code GET /sessions} here returns {@code 401 unauthenticated} ``` Two load-bearing words are wrong in general: READ **needs** `Caller.resolved()`, and **every** such request is refused. ## Why it is false `CallerResolver.resolve(...)`, once `c.terminal() == null`: ```java if (tokenMode) { return presentedTokenMatches(authorizationHeader) ? Principal.primary(c.pid()) : Principal.anonymous(); } // loopback-trust below return isLoopback(remoteAddr) && c.resolved() && c.scanComplete() ? Principal.primary(c.pid()) : Principal.anonymous(); ``` `c.resolved()` and `c.scanComplete()` are in the **loopback-trust** branch only. Token mode returns before reaching them, so a valid bearer token resolves to `PRIMARY` with **no pid lookup on the path at all**. The `LsofPeerPidLookup` exclusion is real and is simply not consulted. ## Why the javadoc's own evidence is consistent with this `FleetdAssemblyFleetAppTest` has **no `auth:` block**, so it gets the default mode — `loopback-trust` (`config/FleetConfig.java:71`, `:1643`). Its `401 unauthenticated` observation is therefore correct *for its own config*. Nothing it measured was wrong. The defect is purely that a result measured under one configuration is written down as a property of the assembly. ## Measured proof of the opposite, now on `main` - `FleetdQuarantineOutageDualWindowAssemblyTest` (PR #648) — real `McpSyncClient` `callTool` **and** a real `HttpClient` GET, token-authenticated. I ran it at `91792e1`: `Tests run: 4, Failures: 0, Errors: 0, Skipped: 0` / `BUILD SUCCESS`, and it asserts `200` on the GET. - `FleetdListReportingSourcesAssemblyTest` (PR #649) — real token-authenticated `callTool("fleet_list")`. I mutated `FleetdAssembly.java:481` and got `Tests run: 3, Failures: 1` with the live `fleet_list` JSON body in the failure message. So both operator windows are reachable in-JVM today, and the suite proves it. ## The cost already paid PR #647 cited this javadoc, concluded the round-trip was impossible, and added three public accessors to `FleetMcp.java` (`capacitySource()`, `healthCoverageSource()`, `coordinatorPeers()`) to work around it. That PR was closed unmerged and the unit redone as #649 with **zero production change**. One worker turn lost, and a production API widening avoided only because the premise was checked. Worth noting what made it expensive: the author did the honest thing and cited its source. The source was wrong. ## Suggested fix Qualify the claim where it is made, and point at the counter-example: 1. In `FleetdAssemblyFleetAppTest`'s javadoc, scope the statement to `loopback-trust` — say that *this test* runs in that mode and that READ is unreachable **in that mode**, not in general. 2. Name the escape hatch in the same breath: under `auth.mode: token` a bearer header resolves to `PRIMARY` with no pid lookup, and `FleetdQuarantineOutageDualWindowAssemblyTest` / `FleetdListReportingSourcesAssemblyTest` do exactly that. Do **not** "fix" this by weakening it to a vague warning. The specific mechanism is worth keeping — it is correct and useful. Only the scope is wrong. For contrast, `mcp/ConnectionIdentity.java:38`–`:41` makes a neighbouring claim **correctly**: it says a full HTTP round trip cannot exercise the `PaneLocator`, which is true and stays true under token mode, because the pid is still `-1` and `PaneLocator` is still never called. That is the right altitude — a claim about the path it is actually about. ## The shape, for the hunt **A result measured under one configuration, written down as a property of the system.** The giveaway is a javadoc that says "needs" or "always" or "every" about a code path that has a branch the author's own test never took. This is a documentation instance of a defect class already on record here: a guard or gate described by the direction it was observed from. It is worth a sweep rather than a one-line fix — any test javadoc asserting that something is *unreachable* is a candidate, because unreachability is exactly the claim that quietly depends on configuration. I have **not** run that sweep; this ticket fixes the one instance that cost us something and names the shape for whoever does. Found while adjudicating the #647/#648 contradiction for #612.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
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#650
Delete Branch "%!s()"

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?