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

A validate* method is wired by its NAME, so a rename silently removes a boot check — only one test stands in the way #740

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

Why this ticket exists

Found while verifying #734 (see that ticket's closing comment). It is not a defect today. It is a way to create one with a refactor that compiles and whose tests pass.

The shape

FleetConfig has config checks that run at boot. Some have no call site at all.

Measured 2026-10-04 on main at aabecce:

$ grep -rn 'validateLeadTabPrefixes' --include='*.java' fleetd/src/main
fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java:2722:    public void validateLeadTabPrefixes() {
fleetd/src/main/java/dev/ltms/fleet/herdr/LeadTabScanner.java:62:     * ... by {@code FleetConfig.validateLeadTabPrefixes} rather than documented here.

The declaration, and one javadoc mention. No caller. A search for callers returns nothing, so the method reads as dead code to anyone who looks that way.

It is not dead. It runs by name:

Fleetd.java:201        cfg.validateAll();
FleetConfig.java:3150  public void validateAll() { invokeAllValidators(this); }
FleetConfig.java:3173  ... && !m.getName().equals("validateAll")) {

invokeAllValidators reflects over the declared methods and invokes every one whose name begins with validate, skipping validateAll itself.

The hazard

Three edits unwire a boot check with no compile error:

  1. Renaming the method to something not starting with validate — for example checkLeadTabPrefixes.
  2. Making it non-public, or moving it to a helper class, depending on what the sweep's filter accepts.
  3. Changing the sweep's own filter.

In every case the method stops running. Nothing references it, so nothing breaks. The daemon boots, accepts a config it used to reject, and the first symptom is whatever that check existed to prevent.

The opposite direction is a hazard too: a new private helper accidentally named validateSomething gets invoked at boot, with whatever arguments the sweep passes.

What actually protects this

One test. FleetConfigValidateAllTest.java:195 asserts the exact set of swept method names:

assertEquals(new TreeSet<>(Set.of("validateAuthExposure", "validateLeadTabPrefixes", ...

So a rename does fail the build — but it fails it in a test that looks like bookkeeping. The obvious "fix" is to paste the new name into the expected set. That makes the build green and the boot check gone. Whoever does it gets no warning that they just deleted a startup guard.

This is the real problem: the guard is load-bearing and does not say so.

What to do

Not a code change in validateAll's logic — the sweep is a reasonable design and #734's verification showed it works. The gap is that the contract is invisible at all three places a person touches it.

  1. Say it at the sweep. invokeAllValidators' javadoc should state that the validate prefix is the wiring, and that a rename removes the check. It documents the order today but not the consequence.
  2. Say it at the test. FleetConfigValidateAllTest.java:195 needs a line saying what a failure here means: either you added a check (add the name) or you removed one (do not). Right now a reader cannot tell the two apart, and they need opposite responses — the same two-states-one-signal shape as fleetd #705.
  3. Consider making it structural instead of advisory, if it is cheap: an annotation (@ConfigValidator) that the sweep selects on, so the wiring survives a rename and an accidental name no longer joins in. Settle whether this is worth it before writing it — the naming convention may well be good enough once documented.

Item 3 is a judgment call and may be declined. Items 1 and 2 are small and should happen either way.

Acceptance

  • invokeAllValidators and the test each state the contract and the consequence.
  • A mutation proves the test is not vacuous: rename one validate* method, show the name-set assertion goes RED, revert, show GREEN.
  • Comments describe the code as it is now. No dates, no ticket numbers, no history — that goes in the commit message.
## Why this ticket exists Found while verifying #734 (see [that ticket's closing comment](https://git.ltms.dev/fleet/fleetd/issues/734#issuecomment-18995)). It is not a defect today. It is a way to create one with a refactor that compiles and whose tests pass. ## The shape `FleetConfig` has config checks that run at boot. Some have no call site at all. Measured 2026-10-04 on `main` at `aabecce`: ``` $ grep -rn 'validateLeadTabPrefixes' --include='*.java' fleetd/src/main fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java:2722: public void validateLeadTabPrefixes() { fleetd/src/main/java/dev/ltms/fleet/herdr/LeadTabScanner.java:62: * ... by {@code FleetConfig.validateLeadTabPrefixes} rather than documented here. ``` The declaration, and one javadoc mention. **No caller.** A search for callers returns nothing, so the method reads as dead code to anyone who looks that way. It is not dead. It runs by name: ``` Fleetd.java:201 cfg.validateAll(); FleetConfig.java:3150 public void validateAll() { invokeAllValidators(this); } FleetConfig.java:3173 ... && !m.getName().equals("validateAll")) { ``` `invokeAllValidators` reflects over the declared methods and invokes every one whose name begins with `validate`, skipping `validateAll` itself. ## The hazard Three edits unwire a boot check with **no compile error**: 1. Renaming the method to something not starting with `validate` — for example `checkLeadTabPrefixes`. 2. Making it non-`public`, or moving it to a helper class, depending on what the sweep's filter accepts. 3. Changing the sweep's own filter. In every case the method stops running. Nothing references it, so nothing breaks. The daemon boots, accepts a config it used to reject, and the first symptom is whatever that check existed to prevent. The opposite direction is a hazard too: a new private helper accidentally named `validateSomething` gets invoked at boot, with whatever arguments the sweep passes. ## What actually protects this One test. `FleetConfigValidateAllTest.java:195` asserts the **exact set** of swept method names: ```java assertEquals(new TreeSet<>(Set.of("validateAuthExposure", "validateLeadTabPrefixes", ... ``` So a rename does fail the build — but it fails it in a test that looks like bookkeeping. The obvious "fix" is to paste the new name into the expected set. That makes the build green and the boot check gone. Whoever does it gets no warning that they just deleted a startup guard. This is the real problem: the guard is load-bearing and does not say so. ## What to do Not a code change in `validateAll`'s logic — the sweep is a reasonable design and #734's verification showed it works. The gap is that the contract is invisible at all three places a person touches it. 1. Say it at the sweep. `invokeAllValidators`' javadoc should state that the `validate` prefix is the wiring, and that a rename removes the check. It documents the order today but not the consequence. 2. Say it at the test. `FleetConfigValidateAllTest.java:195` needs a line saying what a failure here means: either you added a check (add the name) or you removed one (do not). Right now a reader cannot tell the two apart, and they need opposite responses — the same two-states-one-signal shape as `fleetd` #705. 3. Consider making it structural instead of advisory, if it is cheap: an annotation (`@ConfigValidator`) that the sweep selects on, so the wiring survives a rename and an accidental name no longer joins in. Settle whether this is worth it before writing it — the naming convention may well be good enough once documented. Item 3 is a judgment call and may be declined. Items 1 and 2 are small and should happen either way. ## Acceptance - `invokeAllValidators` and the test each state the contract and the consequence. - A mutation proves the test is not vacuous: rename one `validate*` method, show the name-set assertion goes RED, revert, show GREEN. - Comments describe the code as it is now. No dates, no ticket numbers, no history — that goes in the commit message.
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#740
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?