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

The herdr protocol number has no single home — 21 sites, no constant, and the fake and reality can disagree silently #454

New Issue
Open
opened 2026-09-10 12:37:20 +02:00 by ltms · 1 comment
ltms commented 2026-09-10 12:37:20 +02:00
Owner
Copy Link
Copy Source

Follow-up from #449. The worker there suggested a shared constant, saying the number lives "in 2-3 places". It is more than that, and the shape of the problem is not really duplication.

Measured

At 20c1094:

$ grep -rn 'protocol 19\|Protocol19\|(19,' fleetd/src '--include=*.java' | wc -l
21

$ grep -rn 'PROTOCOL' fleetd/src '--include=*.java'
(no output — no named constant anywhere)

Control for that second search: grep -rl 'herdr' fleetd/src/main '--include=*.java' gives 56 files, so the pattern does reach the code. The empty result is a real absence, not a broken search.

Of the 21 sites, 2 are live assertions and the rest are javadoc, comments and one test method name:

kind count examples
live assertion 2 HerdrContractTest:32 (against a real herdr), FleetAppTest:156 (against the fake)
test method name 1 pingReturnsProtocol19
javadoc / comment 18 HerdrClient:6, HerdrCodec:11, AgentControl:16, Tab.java:26, FakeHerdr:15, WorkspaceControl:80, …

Why a constant is only half an answer

A constant fixes the two assertions and none of the 18 comments — you cannot interpolate a Java constant into javadoc. So "extract a constant" would leave 18 sites that still drift, and would make the drift harder to see, because the two places that currently disagree loudly would agree by construction.

The real defect #449 exposed is not duplication. It is that the fake and reality are asserted separately, and only one of them was in CI. Before the fix:

site said
FakeHerdr 19
FleetAppTest:156 — unit test vs the fake 19
HerdrContractTest — the only test vs a real herdr 14

The unit suite was green and self-consistent. It was the fake agreeing with itself. The one assertion that could have caught reality was excluded from CI by name in the workflow, so the disagreement sat there unnoticed.

#452 fixed the CI half (the workflow now selects -Dgroups=contract, so the contract test runs whenever a socket exists, and skips cleanly when it does not). This ticket is about the other half.

What to build

Goal: there is exactly one place that declares which herdr protocol version fleetd is built against, and a disagreement between the fake and a real herdr cannot be green.

Invariants:

  1. One declaration. Both live assertions read it; neither carries a literal.
  2. FakeHerdr's default ping answer reads the same declaration. A fake that reports a version fleetd was not built against should not be reachable by accident. (FakeHerdr:67 already supports overriding the answer per-test, for CB-185 — that override must keep working, since testing the mismatch case is legitimate.)
  3. The 18 comments are the part a constant cannot fix. Do not try to fix them by mechanism. Either leave them and accept they are prose, or reduce them — several say the same thing about the same seam.

Explicitly NOT in scope, and please do not do it by reflex: do not add a build-time or startup check that refuses to run against a mismatched protocol. #449's lesson was that a test which never ran let the drift live, not that fleetd needed a runtime gate. A runtime gate is a different feature with its own failure mode (fleetd refusing to start after a herdr upgrade), and it needs its own argument. #453 has the same warning for the same reason: a winning argument from one ticket is not a licence in the next one.

Acceptance:

  • grep -rn 'PROTOCOL' fleetd/src '--include=*.java' finds the one declaration.
  • Neither live assertion contains a bare 19.
  • mvn -B clean test green, and mvn -B -Pcontract test -Dgroups=contract green on a host with a herdr socket. Report both totals.
  • A mutation to report, not just a pass: change the single declaration to a wrong number, then say which tests fail. If only the contract test fails, the fake is not reading the declaration and invariant 2 is not met. If nothing fails, the declaration is not load-bearing.

Priority

Low. Nothing is broken right now — #452 closed the path that let the drift survive, and this is about making the next drift impossible rather than merely visible. Filing it so the worker's suggestion is not lost, with the scope corrected to what the evidence supports.

Follow-up from #449. The worker there suggested a shared constant, saying the number lives "in 2-3 places". It is more than that, and the shape of the problem is not really duplication. ## Measured At `20c1094`: ``` $ grep -rn 'protocol 19\|Protocol19\|(19,' fleetd/src '--include=*.java' | wc -l 21 $ grep -rn 'PROTOCOL' fleetd/src '--include=*.java' (no output — no named constant anywhere) ``` Control for that second search: `grep -rl 'herdr' fleetd/src/main '--include=*.java'` gives **56 files**, so the pattern does reach the code. The empty result is a real absence, not a broken search. Of the 21 sites, **2 are live assertions** and the rest are javadoc, comments and one test method name: | kind | count | examples | |---|---|---| | live assertion | 2 | `HerdrContractTest:32` (against a real herdr), `FleetAppTest:156` (against the fake) | | test method name | 1 | `pingReturnsProtocol19` | | javadoc / comment | 18 | `HerdrClient:6`, `HerdrCodec:11`, `AgentControl:16`, `Tab.java:26`, `FakeHerdr:15`, `WorkspaceControl:80`, … | ## Why a constant is only half an answer A constant fixes the two assertions and none of the 18 comments — you cannot interpolate a Java constant into javadoc. So "extract a constant" would leave 18 sites that still drift, and would make the drift *harder* to see, because the two places that currently disagree loudly would agree by construction. **The real defect #449 exposed is not duplication. It is that the fake and reality are asserted separately, and only one of them was in CI.** Before the fix: | site | said | |---|---| | `FakeHerdr` | 19 | | `FleetAppTest:156` — unit test vs the fake | 19 | | `HerdrContractTest` — the only test vs a real herdr | **14** | The unit suite was green and self-consistent. It was the fake agreeing with itself. The one assertion that could have caught reality was excluded from CI **by name** in the workflow, so the disagreement sat there unnoticed. #452 fixed the CI half (the workflow now selects `-Dgroups=contract`, so the contract test runs whenever a socket exists, and skips cleanly when it does not). This ticket is about the other half. ## What to build **Goal:** there is exactly one place that declares which herdr protocol version fleetd is built against, and a disagreement between the fake and a real herdr cannot be green. **Invariants:** 1. One declaration. Both live assertions read it; neither carries a literal. 2. `FakeHerdr`'s default ping answer reads the same declaration. A fake that reports a version fleetd was not built against should not be reachable by accident. (`FakeHerdr:67` already supports overriding the answer per-test, for CB-185 — that override must keep working, since testing the mismatch case is legitimate.) 3. The 18 comments are the part a constant cannot fix. Do not try to fix them by mechanism. Either leave them and accept they are prose, or reduce them — several say the same thing about the same seam. **Explicitly NOT in scope, and please do not do it by reflex:** do not add a build-time or startup check that refuses to run against a mismatched protocol. #449's lesson was that a *test* which never ran let the drift live, not that fleetd needed a runtime gate. A runtime gate is a different feature with its own failure mode (fleetd refusing to start after a herdr upgrade), and it needs its own argument. #453 has the same warning for the same reason: a winning argument from one ticket is not a licence in the next one. **Acceptance:** - `grep -rn 'PROTOCOL' fleetd/src '--include=*.java'` finds the one declaration. - Neither live assertion contains a bare `19`. - `mvn -B clean test` green, and `mvn -B -Pcontract test -Dgroups=contract` green on a host with a herdr socket. Report both totals. - **A mutation to report, not just a pass:** change the single declaration to a wrong number, then say which tests fail. If only the contract test fails, the fake is not reading the declaration and invariant 2 is not met. If nothing fails, the declaration is not load-bearing. ## Priority Low. Nothing is broken right now — #452 closed the path that let the drift survive, and this is about making the next drift impossible rather than merely visible. Filing it so the worker's suggestion is not lost, with the scope corrected to what the evidence supports.
ltms commented 2026-09-10 12:55:09 +02:00
Author
Owner
Copy Link
Copy Source

Qualifier from a peer review — this ticket must not be read as an argument against constants

A peer lead read the ticket and agreed with the scoping, then added a qualifier that changes what "done" looks like. Recording it, because without it this ticket could produce the wrong fix.

The general form of why I refused "extract a constant"

Their statement of it, which is sharper than mine:

A refactor that makes an inconsistency impossible to express also makes it impossible to detect — and when that inconsistency is the bug you are hunting, you have deleted your own evidence and called it cleanup.

That is exactly the risk here. The two currently-disagreeing sites (HerdrContractTest against a real herdr, FleetAppTest against the fake) are what made #449 findable at all. Forcing them to agree by construction removes the only place the disagreement could ever show up.

The qualifier: a single source of truth IS right, provided the detector moves to the boundary

This is the part I had scoped too defensively.

One constant plus a contract test asserting it against live herdr is strictly better than 21 copies, because drift is then caught where it actually originates — the external system — instead of between two internal copies that only happen to disagree today.

They are right. My ticket read as "do not consolidate", and that is not the correct goal. The correct goal is: consolidate, and keep the external check.

What you must not do is consolidate the copies AND drop the external check, which is the version that looks like a tidy-up and silently removes the only thing that noticed 14 vs 19.

Revised invariants, replacing invariant 1 and 2 above

  1. One declaration of the protocol fleetd is built against. Both live assertions read it; neither carries a literal. (Unchanged.)
  2. FakeHerdr's default ping answer reads the same declaration. (Unchanged.)
  3. NEW, and this is the load-bearing one: after consolidation, a test must still compare the declaration against a REAL herdr. HerdrContractTest is that test and it must keep asserting against the live socket, not against the constant it now reads. A test that reads the constant and asserts the constant is a tautology, and it is exactly the "fake agreeing with itself" failure this ticket was filed about — reintroduced one level up.

The failure mode to avoid, stated so a worker can check for it: if your change makes it impossible for HerdrContractTest to fail when herdr moves to protocol 20, you have made the codebase tidier and blinder. The mutation in the acceptance criteria is what catches this — change the single declaration to a wrong number and report which tests fail. If HerdrContractTest does not fail against a live herdr, invariant 3 is not met.

One thing this does not change

Still explicitly out of scope: a build-time or startup check that refuses to run against a mismatched protocol. #449's lesson was that a test which never ran let the drift live, not that fleetd needed a runtime gate. Consolidating the constant and keeping the contract test is the fix; refusing to boot is a different feature with its own failure mode.

## Qualifier from a peer review — this ticket must not be read as an argument against constants A peer lead read the ticket and agreed with the scoping, then added a qualifier that changes what "done" looks like. Recording it, because without it this ticket could produce the wrong fix. ### The general form of why I refused "extract a constant" Their statement of it, which is sharper than mine: > A refactor that makes an inconsistency impossible to **express** also makes it impossible to **detect** — and when that inconsistency is the bug you are hunting, you have deleted your own evidence and called it cleanup. That is exactly the risk here. The two currently-disagreeing sites (`HerdrContractTest` against a real herdr, `FleetAppTest` against the fake) are what made #449 findable at all. Forcing them to agree by construction removes the only place the disagreement could ever show up. ### The qualifier: a single source of truth IS right, provided the detector moves to the boundary This is the part I had scoped too defensively. > One constant plus a contract test asserting it against live herdr is strictly better than 21 copies, because drift is then caught where it actually originates — the external system — instead of between two internal copies that only happen to disagree today. They are right. My ticket read as "do not consolidate", and that is not the correct goal. The correct goal is: **consolidate, and keep the external check.** > What you must not do is consolidate the copies AND drop the external check, which is the version that looks like a tidy-up and silently removes the only thing that noticed 14 vs 19. ### Revised invariants, replacing invariant 1 and 2 above 1. **One declaration** of the protocol fleetd is built against. Both live assertions read it; neither carries a literal. (Unchanged.) 2. **`FakeHerdr`'s default ping answer reads the same declaration.** (Unchanged.) 3. **NEW, and this is the load-bearing one: after consolidation, a test must still compare the declaration against a REAL herdr.** `HerdrContractTest` is that test and it must keep asserting against the live socket, not against the constant it now reads. A test that reads the constant and asserts the constant is a tautology, and it is exactly the "fake agreeing with itself" failure this ticket was filed about — reintroduced one level up. **The failure mode to avoid, stated so a worker can check for it:** if your change makes it impossible for `HerdrContractTest` to fail when herdr moves to protocol 20, you have made the codebase tidier and blinder. The mutation in the acceptance criteria is what catches this — change the single declaration to a wrong number and report which tests fail. If `HerdrContractTest` does **not** fail against a live herdr, invariant 3 is not met. ### One thing this does not change Still explicitly out of scope: **a build-time or startup check that refuses to run against a mismatched protocol.** #449's lesson was that a test which never ran let the drift live, not that fleetd needed a runtime gate. Consolidating the constant and keeping the contract test is the fix; refusing to boot is a different feature with its own failure mode.
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#454
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?