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

Five role-model comments describe the pre-CB-501 rule, and two of them say an unconfigured pane is the primary #759

New Issue
Open
opened 2026-10-05 10:12:17 +02:00 by ltms · 3 comments
ltms commented 2026-10-05 10:12:17 +02:00
Owner
Copy Link
Copy Source

A hunter sweep for comments that over-claim what their code does. Five findings, all five confirmed by me against the code, not taken on the worker's word. The sweep carried a working positive probe: its pattern matched the known seed at FleetMcp.java:827-832, so a zero elsewhere would have been a real zero.

They are one family. Role.PRIMARY used to be reached by failing every other check — "anything that is not a worker is the primary". CB-501 inverted that, CB-548 added architects, #703 added collaborators and #743 added observers. The code moved each time; these five comments did not.

1. auth/Role.java:15 — says an unconfigured pane is the primary

"Established either by being a loopback caller that is not a worker pane (under loopback-trust)"

CallerResolver.resolve:300 opens with if (c.terminal() != null), and every path inside that block returns: a spawned member, a lead, an architect slot, a collaborator, else Principal.observer(...) at :346. The loopback-trust promotion to primary is at :357, reachable only when c.terminal() == null.

So a loopback caller that is a pane but not a worker is an observer — never the primary. The comment states the opposite, and in the escalation direction: a reader would conclude an unconfigured tab may spawn, stop and drain. Highest severity of the five.

The same class javadoc already describes the inversion correctly two paragraphs up ("ANONYMOUS is the fallback, and PRIMARY must be established"). The enum constant's own comment contradicts its class's.

2. mcp/ConnectionIdentity.java:6-10 and :89-92 — the same wrong rule, second place

The hunter reported this as a null-handling inaccuracy. It is more than that. Line 90-92:

"The calling worker's terminal_id, or null if the caller is not a known on-host worker (treat as the primary)."

That parenthesis is finding 1 again, written where a reader of the identity layer will meet it. resolve:84 returns lookup.terminal() for any agent pane herdr maps — a lead's, an architect's, a collaborator's, an observer's. So "not a known on-host worker" does not imply a null terminal, and a null terminal does not imply the primary.

The class javadoc at :6-10 carries the same premise ("yielding the caller's worker terminal_id").

3. inject/MemberPresence.java:12-13 — presence is not worker-only

"any MCP request whose connection resolves to a worker terminal marks that worker present"

FleetMcp.markTrackedCallerPresent:888:

if (caller.isSpawnedMember() || caller.isObserver()) {
    presence.markPresent(caller.terminal());
}

isSpawnedMember() covers WORKER and ARCHITECT. So architects and observer panes enrol too. markTrackedCallerPresent's own javadoc states this correctly — "a worker, an architect, or the unconfigured-pane floor" — so the rule is written twice and the two copies disagree.

This one is load-bearing right now: #757 is about presence, and anyone reading MemberPresence first would conclude observer panes never enrol, which is the opposite of the mechanism #743 depends on.

4. mcp/FleetMcp.java:524-525 — fleet_reply is not worker-only

"the authz check is 'is this caller a worker at all'"

Authz:162 is case REPLY, ASK -> caller.ownsSession(targetSession);, and Principal.ownsSession:145 is terminal != null && terminal.equals(sessionId). Any peer with a terminal may answer for its own pane. Authz's own comment above that line says so explicitly and names CB-532 as the widening. A reader would wrongly withhold fleet_reply from a lead, architect, collaborator or observer that the gate accepts.

5. mcp/FleetMcp.java:2727 — the fleet_list tool description omits hunter

"each with sessionId, paneId, role (architect/dev/reviewer)"

MemberRole declares ARCHITECT, DEV, HUNTER, REVIEWER (peer/MemberRole.java:35-61), the spawn schema accepts hunter, and the row writes s.role().wireName(). Lowest harm of the five, but it reaches furthest: this is the live MCP schema, which CLAUDE.md names as the tool reference, so every agent on the bus reads it.

The pattern worth naming

Four of the five are one rule written in two places, where one copy was updated and the other was not. Role vs its own class javadoc; ConnectionIdentity vs CallerResolver; MemberPresence vs markTrackedCallerPresent; FleetMcp:524 vs Authz:155-162. In each pair the copy nearest the enforcing line is right and the distant one is stale.

That is exactly what CLAUDE.md's "one fact, one place" rule predicts, and no build check sees it: rules 1, 2 and 3 check for tickets, dates, length and test-class names, never whether a sentence is true. The honest answer is that this family needs a sweep now and then, not a gate.

Sequencing

Findings 4 and 5 are in FleetMcp.java, which #756/#758 is editing right now. They must wait for that PR to merge, or the two changes collide. Findings 1, 2 and 3 are in Role.java, ConnectionIdentity.java and MemberPresence.java and can go immediately.

A `hunter` sweep for comments that over-claim what their code does. Five findings, **all five confirmed by me against the code**, not taken on the worker's word. The sweep carried a working positive probe: its pattern matched the known seed at `FleetMcp.java:827-832`, so a zero elsewhere would have been a real zero. They are one family. `Role.PRIMARY` used to be reached by *failing* every other check — "anything that is not a worker is the primary". CB-501 inverted that, CB-548 added architects, #703 added collaborators and #743 added observers. The code moved each time; these five comments did not. ## 1. `auth/Role.java:15` — says an unconfigured pane is the primary > *"Established either by being a loopback caller that is not a worker pane (under `loopback-trust`)"* `CallerResolver.resolve:300` opens with `if (c.terminal() != null)`, and **every path inside that block returns**: a spawned member, a lead, an architect slot, a collaborator, else `Principal.observer(...)` at `:346`. The loopback-trust promotion to primary is at `:357`, reachable only when `c.terminal() == null`. So a loopback caller that *is* a pane but not a worker is an observer — never the primary. The comment states the opposite, and in the escalation direction: a reader would conclude an unconfigured tab may spawn, stop and drain. Highest severity of the five. The same class javadoc already describes the inversion correctly two paragraphs up ("`ANONYMOUS` is the fallback, and `PRIMARY` must be established"). The enum constant's own comment contradicts its class's. ## 2. `mcp/ConnectionIdentity.java:6-10` and `:89-92` — the same wrong rule, second place The hunter reported this as a `null`-handling inaccuracy. It is more than that. Line 90-92: > *"The calling worker's `terminal_id`, or `null` if the caller is not a known on-host worker **(treat as the primary)**."* That parenthesis is finding 1 again, written where a reader of the identity layer will meet it. `resolve:84` returns `lookup.terminal()` for **any** agent pane herdr maps — a lead's, an architect's, a collaborator's, an observer's. So "not a known on-host worker" does not imply a null terminal, and a null terminal does not imply the primary. The class javadoc at `:6-10` carries the same premise ("yielding the caller's worker `terminal_id`"). ## 3. `inject/MemberPresence.java:12-13` — presence is not worker-only > *"any MCP request whose connection resolves to a worker terminal marks that worker present"* `FleetMcp.markTrackedCallerPresent:888`: ```java if (caller.isSpawnedMember() || caller.isObserver()) { presence.markPresent(caller.terminal()); } ``` `isSpawnedMember()` covers `WORKER` and `ARCHITECT`. So architects and observer panes enrol too. `markTrackedCallerPresent`'s own javadoc states this correctly — *"a worker, an architect, or the unconfigured-pane floor"* — so the rule is written twice and the two copies disagree. This one is load-bearing right now: #757 is about presence, and anyone reading `MemberPresence` first would conclude observer panes never enrol, which is the opposite of the mechanism #743 depends on. ## 4. `mcp/FleetMcp.java:524-525` — `fleet_reply` is not worker-only > *"the authz check is 'is this caller a worker at all'"* `Authz:162` is `case REPLY, ASK -> caller.ownsSession(targetSession);`, and `Principal.ownsSession:145` is `terminal != null && terminal.equals(sessionId)`. Any peer with a terminal may answer for its own pane. `Authz`'s own comment above that line says so explicitly and names CB-532 as the widening. A reader would wrongly withhold `fleet_reply` from a lead, architect, collaborator or observer that the gate accepts. ## 5. `mcp/FleetMcp.java:2727` — the `fleet_list` tool description omits `hunter` > *"each with sessionId, paneId, role (architect/dev/reviewer)"* `MemberRole` declares `ARCHITECT, DEV, HUNTER, REVIEWER` (`peer/MemberRole.java:35-61`), the spawn schema accepts hunter, and the row writes `s.role().wireName()`. Lowest harm of the five, but it reaches furthest: this is the live MCP schema, which `CLAUDE.md` names as *the* tool reference, so every agent on the bus reads it. ## The pattern worth naming Four of the five are **one rule written in two places, where one copy was updated and the other was not**. `Role` vs its own class javadoc; `ConnectionIdentity` vs `CallerResolver`; `MemberPresence` vs `markTrackedCallerPresent`; `FleetMcp:524` vs `Authz:155-162`. In each pair the copy nearest the enforcing line is right and the distant one is stale. That is exactly what `CLAUDE.md`'s "one fact, one place" rule predicts, and no build check sees it: rules 1, 2 and 3 check for tickets, dates, length and test-class names, never whether a sentence is true. The honest answer is that this family needs a sweep now and then, not a gate. ## Sequencing Findings 4 and 5 are in `FleetMcp.java`, which #756/#758 is editing right now. They must wait for that PR to merge, or the two changes collide. Findings 1, 2 and 3 are in `Role.java`, `ConnectionIdentity.java` and `MemberPresence.java` and can go immediately.
ltms commented 2026-10-05 10:27:45 +02:00
Author
Owner
Copy Link
Copy Source

Findings 1–3 are fixed — main at 39accf7

PR #760 plus one follow-up commit of mine. MVN_EXIT=0, surefire XML sum tests=2137 failures=0 errors=0 skipped=0, same as bare main — the right outcome for a comment-only change. Merged tree hash matched the tree I built, so the tested bytes are the shipped bytes.

A fourth copy existed, and my brief missed it

I named two places in ConnectionIdentity and there were three. The Caller record's own javadoc carried the same claim:

"its worker terminal (or null for the primary / an off-host client)"

The worker found it, left it alone as out of scope, and reported it — which is exactly right. I fixed it in the follow-up commit. So the count for this file was three, not two, and the wrong rule was written in four places overall, not three.

Worth recording why: I built the brief from the hunter's report, and the hunter cited the two spots its pattern matched. I treated its citation list as the extent of the defect instead of as the extent of its search. One grep answers "where did this pattern match", never "where else does this rule appear". For a defect whose whole shape is the same sentence copied around, the enumeration has to be mine and it has to be exhaustive before the brief goes out.

Two more instances, still open

Both in mcp/ConnectionIdentity.java, both reported by the #760 worker and correctly not touched:

  1. Caller#resolved's javadoc — carries fleetd #317, #305, and a narrative of what drifted and why the method was deliberately not widened for #505. Rule 1 bans tickets and history in comments; that reasoning belongs in the commit message or a docs/ page.
  2. isLoopback's javadoc — notebook-style: "used to say", a measurement date, and #305 removed the second copy.

These are rule-1 violations rather than false statements, so they are lower priority than findings 1–3 were. They are also a judgement call: the resolved() block explains a real two-states-one-sentinel trap that cost a ticket, and that knowledge is load-bearing. Rule 2's prescription applies — move it to docs/, never delete it. Whoever takes this should propose where it goes rather than cut it.

Findings 4 and 5 — unblocked soon

Both are in FleetMcp.java, which the #756/#758 worker still holds. They land once that PR merges. Finding 5 (fleet_list's tool description omitting hunter) is the one that reaches furthest, since CLAUDE.md names the live MCP schema as the tool reference.

## Findings 1–3 are fixed — `main` at `39accf7` PR #760 plus one follow-up commit of mine. `MVN_EXIT=0`, surefire XML sum `tests=2137 failures=0 errors=0 skipped=0`, same as bare `main` — the right outcome for a comment-only change. Merged tree hash matched the tree I built, so the tested bytes are the shipped bytes. ## A fourth copy existed, and my brief missed it I named two places in `ConnectionIdentity` and there were three. The `Caller` record's own javadoc carried the same claim: > *"its worker `terminal` (or `null` for the primary / an off-host client)"* The worker found it, left it alone as out of scope, and reported it — which is exactly right. I fixed it in the follow-up commit. So the count for this file was **three**, not two, and the wrong rule was written in **four** places overall, not three. Worth recording why: I built the brief from the hunter's report, and the hunter cited the two spots its pattern matched. I treated its citation list as the extent of the defect instead of as the extent of its search. One grep answers "where did this pattern match", never "where else does this rule appear". For a defect whose whole shape is *the same sentence copied around*, the enumeration has to be mine and it has to be exhaustive before the brief goes out. ## Two more instances, still open Both in `mcp/ConnectionIdentity.java`, both reported by the #760 worker and correctly not touched: 1. **`Caller#resolved`'s javadoc** — carries `fleetd #317`, `#305`, and a narrative of what drifted and why the method was deliberately not widened for `#505`. Rule 1 bans tickets and history in comments; that reasoning belongs in the commit message or a `docs/` page. 2. **`isLoopback`'s javadoc** — notebook-style: *"used to say"*, a measurement date, and `#305 removed the second copy`. These are rule-1 violations rather than false statements, so they are lower priority than findings 1–3 were. They are also a judgement call: the `resolved()` block explains a real two-states-one-sentinel trap that cost a ticket, and that knowledge is load-bearing. Rule 2's prescription applies — move it to `docs/`, never delete it. Whoever takes this should propose where it goes rather than cut it. ## Findings 4 and 5 — unblocked soon Both are in `FleetMcp.java`, which the #756/#758 worker still holds. They land once that PR merges. Finding 5 (`fleet_list`'s tool description omitting `hunter`) is the one that reaches furthest, since `CLAUDE.md` names the live MCP schema as *the* tool reference.
ltms commented 2026-10-05 10:46:22 +02:00
Author
Owner
Copy Link
Copy Source

Finding 6 — three {@link} references point at symbols that do not exist

Found while reading HerdrPeerLauncher for #763. All three are in that one file:

Line Dead reference Note
595 {@link #place} no place( member in the file
595 {@link #defaultProfileFor} the name appears exactly once in the file — in this link
1170 {@link #BLOCKED_GITEA_ACCESS_TOKEN} the constant is BLOCKED_CREDENTIAL_SENTINEL; this is its pre-rename name

This is rule 3's mechanism, one layer out. Rule 3 bans naming a test class in a main-source comment because the name is invisible to the compiler and rots in silence. The same is true of a {@link #X} to a renamed member: javac never looks inside a doc comment, and mvn clean install here does not run the javadoc tool, so all three have compiled green for as long as they have been wrong.

#BLOCKED_GITEA_ACCESS_TOKEN is the one that actually misleads. Its sentence says the marker "survives the login shell that wipes BLOCKED_GITEA_ACCESS_TOKEN" — but the thing the login shell wipes is the sentinel value, written over the variable GITEA_ACCESS_TOKEN. A reader chasing that link finds nothing and cannot tell which of the two the sentence means.

Fix

Point each link at the symbol that exists, or drop the link and name the thing in plain text. #place and #defaultProfileFor need a look at what line 595 was trying to say — both names are gone, so the sentence may need rewriting rather than relinking.

How I measured it, and the number I nearly published

Worth recording, because my first three attempts all gave a different answer:

  1. A regex for "declared as field, method or type in the same file" reported 24. Wrong: it missed every enum constant written NAME,.
  2. Adding the enum-constant shape gave 9. Still wrong: the last constant in an enum has no trailing comma, a record component is a declaration of a different shape, and enum AuthorizationMode { ENFORCED, UNENFORCED } is all on one line. Six of those nine were live symbols.
  3. Switching to "does the name appear anywhere outside a doc comment" gave 2. Wrong in the other direction: it dropped #place, because place is an ordinary English word and appears in the prose of a nearby comment.

The answer is 3, from a declaration-shape check run with six controls — three names that must resolve (MEMBER_MARKER, BLOCKED_CREDENTIAL_SENTINEL, baseEnv) and the three above that must not. All six came out as expected, in both directions.

A single-direction check would have passed at step 3 as well. The negative controls are what caught the English-word hole.

The sweep this belongs to

Three in one file is not a codebase sweep. A hunter pass should widen it in two directions this check does not cover: {@link Other#member} across files, and {@code X#method} which is plain text and resolves nothing by construction. The right long-term fix is a build check, so a rename cannot leave a dead link behind again — the same reasoning that gave rules 1, 2, 3 and 5 their gates.

Not in the running worker's scope: findings 4 and 5 only.

## Finding 6 — three `{@link}` references point at symbols that do not exist Found while reading `HerdrPeerLauncher` for #763. All three are in that one file: | Line | Dead reference | Note | |---|---|---| | 595 | `{@link #place}` | no `place(` member in the file | | 595 | `{@link #defaultProfileFor}` | the name appears exactly once in the file — in this link | | 1170 | `{@link #BLOCKED_GITEA_ACCESS_TOKEN}` | the constant is `BLOCKED_CREDENTIAL_SENTINEL`; this is its pre-rename name | This is rule 3's mechanism, one layer out. Rule 3 bans naming a *test class* in a main-source comment because the name is invisible to the compiler and rots in silence. The same is true of a `{@link #X}` to a renamed member: `javac` never looks inside a doc comment, and `mvn clean install` here does not run the javadoc tool, so all three have compiled green for as long as they have been wrong. `#BLOCKED_GITEA_ACCESS_TOKEN` is the one that actually misleads. Its sentence says the marker "survives the login shell that wipes `BLOCKED_GITEA_ACCESS_TOKEN`" — but the thing the login shell wipes is the sentinel *value*, written over the variable `GITEA_ACCESS_TOKEN`. A reader chasing that link finds nothing and cannot tell which of the two the sentence means. ### Fix Point each link at the symbol that exists, or drop the link and name the thing in plain text. `#place` and `#defaultProfileFor` need a look at what line 595 was trying to say — both names are gone, so the sentence may need rewriting rather than relinking. ### How I measured it, and the number I nearly published Worth recording, because my first three attempts all gave a different answer: 1. A regex for "declared as field, method or type in the same file" reported **24**. Wrong: it missed every enum constant written `NAME,`. 2. Adding the enum-constant shape gave **9**. Still wrong: the last constant in an enum has no trailing comma, a record component is a declaration of a different shape, and `enum AuthorizationMode { ENFORCED, UNENFORCED }` is all on one line. Six of those nine were live symbols. 3. Switching to "does the name appear anywhere outside a doc comment" gave **2**. Wrong in the other direction: it dropped `#place`, because *place* is an ordinary English word and appears in the prose of a nearby comment. The answer is **3**, from a declaration-shape check run with six controls — three names that must resolve (`MEMBER_MARKER`, `BLOCKED_CREDENTIAL_SENTINEL`, `baseEnv`) and the three above that must not. All six came out as expected, in both directions. A single-direction check would have passed at step 3 as well. The negative controls are what caught the English-word hole. ### The sweep this belongs to Three in one file is not a codebase sweep. A `hunter` pass should widen it in two directions this check does not cover: `{@link Other#member}` across files, and `{@code X#method}` which is plain text and resolves nothing by construction. The right long-term fix is a build check, so a rename cannot leave a dead link behind again — the same reasoning that gave rules 1, 2, 3 and 5 their gates. Not in the running worker's scope: findings 4 and 5 only.
ltms commented 2026-10-05 10:51:47 +02:00
Author
Owner
Copy Link
Copy Source

Findings 4 and 5 fixed and merged — main at 2289e94, PR #764. Verification is on that PR; one result from it belongs here instead.

The description fix is not pinned by any test

I mutated the shipped fix, putting the stale "architect/dev/reviewer" literal back into fleet_list's description, and ran FleetMcp*Test,MemberRoleTest: 185 tests, 0 failures, BUILD SUCCESS. The mutation survives.

MemberRole.wireNames() itself is pinned — MemberRoleTest.parseRejectsAnUnknownRoleAndListsTheValidOnes asserts the joined string, so that test now transitively proves the description would contain hunter if the description calls the method. Nothing proves it still calls it.

So the hole that lost hunter is still open in the same direction: a future author can type a literal back in and the build stays green.

listTool() is private static, so closing it means either widening it for a test — which is what code-quality rule 4 exists to prevent — or standing the MCP server up in a test. That is a design decision, so I am not deciding it inside a comment-fix ticket. It belongs with the remaining #748 build-gate work, where there is already a precedent to copy: McpContractDocTest reads FleetTool.wireNames() and carries its own positive control ("returned only N tool(s); the server registers eleven").

Status of this ticket's findings

# What Status
1–3 three role-model comments describing the pre-CB-501 rule fixed, 0f2ec7a
— the third copy in ConnectionIdentity's Caller record, which my brief missed fixed, 39accf7
4 the fleet_reply authz comment named the wrong predicate fixed, 2289e94
5 fleet_list's description omitted hunter fixed, 2289e94 — not pinned, see above
6 three dead {@link} refs in HerdrPeerLauncher open
— Caller#resolved and isLoopback javadoc: tickets, dates, "used to say" open — rule 1 breaches, but the knowledge is load-bearing, so rule 2 applies: move it to a page under docs/, never delete it

Leaving this ticket open for finding 6 and the two javadoc blocks.

Findings 4 and 5 fixed and merged — `main` at `2289e94`, PR #764. Verification is on that PR; one result from it belongs here instead. ## The description fix is not pinned by any test I mutated the shipped fix, putting the stale `"architect/dev/reviewer"` literal back into `fleet_list`'s description, and ran `FleetMcp*Test,MemberRoleTest`: **185 tests, 0 failures, BUILD SUCCESS**. The mutation survives. `MemberRole.wireNames()` itself is pinned — `MemberRoleTest.parseRejectsAnUnknownRoleAndListsTheValidOnes` asserts the joined string, so that test now transitively proves the description would contain `hunter` *if* the description calls the method. Nothing proves it still calls it. So the hole that lost `hunter` is still open in the same direction: a future author can type a literal back in and the build stays green. `listTool()` is `private static`, so closing it means either widening it for a test — which is what code-quality rule 4 exists to prevent — or standing the MCP server up in a test. That is a design decision, so I am not deciding it inside a comment-fix ticket. It belongs with the remaining #748 build-gate work, where there is already a precedent to copy: `McpContractDocTest` reads `FleetTool.wireNames()` and carries its own positive control ("returned only N tool(s); the server registers eleven"). ## Status of this ticket's findings | # | What | Status | |---|---|---| | 1–3 | three role-model comments describing the pre-CB-501 rule | fixed, `0f2ec7a` | | — | the third copy in `ConnectionIdentity`'s `Caller` record, which my brief missed | fixed, `39accf7` | | 4 | the `fleet_reply` authz comment named the wrong predicate | fixed, `2289e94` | | 5 | `fleet_list`'s description omitted `hunter` | fixed, `2289e94` — **not pinned**, see above | | 6 | three dead `{@link}` refs in `HerdrPeerLauncher` | open | | — | `Caller#resolved` and `isLoopback` javadoc: tickets, dates, "used to say" | open — rule 1 breaches, but the knowledge is load-bearing, so rule 2 applies: move it to a page under `docs/`, never delete it | Leaving this ticket open for finding 6 and the two javadoc blocks.
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#759
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?