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

validateLeadRollover has no validateAll-reachability test — the exact gap FleetConfigValidateAllTest was written to prevent #668

New Issue
Closed
opened 2026-10-03 19:45:33 +02:00 by ltms · 1 comment
ltms commented 2026-10-03 19:45:33 +02:00
Owner
Copy Link
Copy Source

Found on 2026-10-03 while reviewing PR #667 (fleetd #661). Pre-existing on main at 4b4a868; not caused by that PR.

The measurement

FleetConfigValidateAllTest.validateAllReachesEveryOneOfTodaysSixValidators enumerates one minimal failing config per validator and asserts each one also fails through validateAll(). It covers six:

$ sed -n '/void validateAllReachesEveryOneOfTodaysSixValidators/,/^    }/p' \
    fleetd/src/test/java/dev/ltms/fleet/config/FleetConfigValidateAllTest.java \
  | grep -oE '// validate[A-Za-z]+' | sort -u
// validateAuthExposure
// validateCharters
// validateLeadTabPrefixes
// validateMembers
// validateModels
// validateSubscriptionProfiles

But FleetConfig declares eight (excluding validateAll):

$ grep -oE 'public void validate[A-Za-z]+\(\)' fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java \
  | grep -v validateAll | sort -u | wc -l
8

Set difference — not covered by that enumeration:

  • validateLeadRollover
  • validatePanePlacementAgainstLeadTabs

validatePanePlacementAgainstLeadTabs is covered, by FleetConfigTest.validateAllAlsoRefusesPanePlacementAgainstALeadTab (FleetConfigTest.java:835), added in PR #667 because the brief required it.

validateLeadRollover is covered nowhere. I searched every test file that drives validateAll() or invokeAllValidators and counted leadRollover/handoverPath mentions in each:

test file driving validateAll leadRollover mentions
FleetdAssemblyRoleFallbackBoundaryTest 0
FleetdHealthCoverageSourceWiringTest 0
FleetdStartupReportTest 0
FleetdStartupValidationTest 0
FleetdSubscriptionGuardOrderingTest 0
ConfigRefTest 0
ConfigRefTopLevelCoverageTest 5
FleetConfigValidateAllTest 1

ConfigRefTopLevelCoverageTest is about key triage, not validator reachability. The single mention in FleetConfigValidateAllTest is the name inside the canary's Set.of literal. So nothing proves validateAll() actually reaches validateLeadRollover.

Why this is the interesting part

This is the failure the file was built to stop, in its own words:

A hand-maintained list of six names would have exactly the defect it replaces: the seventh validator someone adds next month has no reason to be added to it, and nothing would say so.

validateLeadRollover is that seventh validator. It was added (fleetd #480), and nothing said so.

The file also names its own safeguard:

The guarantee is fleetConfigDeclaresExactlyTheseValidatorsToday(): it fails the moment any validator is added or removed, which forces whoever changes the set to look at this file.

That canary works — it fired in PR #667 and the worker had to update it. But firing only brings a reader to the canary, and the canary's instruction does not send them on to the reachability enumeration twenty lines below. So the tripwire caught the change and still let the coverage gap through. A tripwire that does not name the second thing you must do is half a guard.

The ask

  1. Add a validateLeadRollover case to the reachability enumeration: a minimal config with a present leadRollover: block and a missing or blank handoverPath, asserted to fail through validateAll().
  2. Make the enumeration the single place for validateAll-reachability, and have the canary's failure message point at it by name, so the next person who adds a validator is told both things to do.
  3. Prove the new case is not vacuous: drop validateLeadRollover from the sweep (or typo its name) and show that case goes red.

Not verified by me

  • Whether any other validator is reachable in production but unproven by a test. I measured only this enumeration against the declared set, so a validator covered by some third route would not show up in my table.
  • Whether validateLeadRollover is in fact reached at runtime. The reflective sweep's filter should pick it up, so I expect it works and only the proof is missing. I did not run it.
Found on 2026-10-03 while reviewing PR #667 (fleetd #661). Pre-existing on `main` at `4b4a868`; not caused by that PR. ## The measurement `FleetConfigValidateAllTest.validateAllReachesEveryOneOfTodaysSixValidators` enumerates one minimal failing config per validator and asserts each one also fails through `validateAll()`. It covers **six**: ``` $ sed -n '/void validateAllReachesEveryOneOfTodaysSixValidators/,/^ }/p' \ fleetd/src/test/java/dev/ltms/fleet/config/FleetConfigValidateAllTest.java \ | grep -oE '// validate[A-Za-z]+' | sort -u // validateAuthExposure // validateCharters // validateLeadTabPrefixes // validateMembers // validateModels // validateSubscriptionProfiles ``` But `FleetConfig` declares **eight** (excluding `validateAll`): ``` $ grep -oE 'public void validate[A-Za-z]+\(\)' fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java \ | grep -v validateAll | sort -u | wc -l 8 ``` Set difference — not covered by that enumeration: - `validateLeadRollover` - `validatePanePlacementAgainstLeadTabs` **`validatePanePlacementAgainstLeadTabs` is covered**, by `FleetConfigTest.validateAllAlsoRefusesPanePlacementAgainstALeadTab` (`FleetConfigTest.java:835`), added in PR #667 because the brief required it. **`validateLeadRollover` is covered nowhere.** I searched every test file that drives `validateAll()` or `invokeAllValidators` and counted `leadRollover`/`handoverPath` mentions in each: | test file driving validateAll | leadRollover mentions | |---|---| | `FleetdAssemblyRoleFallbackBoundaryTest` | 0 | | `FleetdHealthCoverageSourceWiringTest` | 0 | | `FleetdStartupReportTest` | 0 | | `FleetdStartupValidationTest` | 0 | | `FleetdSubscriptionGuardOrderingTest` | 0 | | `ConfigRefTest` | 0 | | `ConfigRefTopLevelCoverageTest` | 5 | | `FleetConfigValidateAllTest` | 1 | `ConfigRefTopLevelCoverageTest` is about key triage, not validator reachability. The single mention in `FleetConfigValidateAllTest` is the name inside the canary's `Set.of` literal. So nothing proves `validateAll()` actually reaches `validateLeadRollover`. ## Why this is the interesting part This is the failure the file was built to stop, in its own words: > A hand-maintained list of six names would have exactly the defect it replaces: the seventh validator someone adds next month has no reason to be added to it, and nothing would say so. `validateLeadRollover` **is** that seventh validator. It was added (fleetd #480), and nothing said so. The file also names its own safeguard: > The guarantee is `fleetConfigDeclaresExactlyTheseValidatorsToday()`: it fails the moment any validator is added or removed, which forces whoever changes the set to look at this file. That canary works — it fired in PR #667 and the worker had to update it. But firing only brings a reader to the **canary**, and the canary's instruction does not send them on to the **reachability enumeration** twenty lines below. So the tripwire caught the change and still let the coverage gap through. A tripwire that does not name the second thing you must do is half a guard. ## The ask 1. Add a `validateLeadRollover` case to the reachability enumeration: a minimal config with a present `leadRollover:` block and a missing or blank `handoverPath`, asserted to fail through `validateAll()`. 2. Make the enumeration the single place for validateAll-reachability, and have the canary's failure message point at it by name, so the next person who adds a validator is told both things to do. 3. Prove the new case is not vacuous: drop `validateLeadRollover` from the sweep (or typo its name) and show that case goes red. ## Not verified by me - Whether any other validator is reachable in production but unproven by a test. I measured only this enumeration against the declared set, so a validator covered by some third route would not show up in my table. - Whether `validateLeadRollover` is in fact reached at runtime. The reflective sweep's filter should pick it up, so I expect it works and only the proof is missing. I did not run it.
ltms commented 2026-10-03 20:54:34 +02:00
Author
Owner
Copy Link
Copy Source

Merged as 283ccf8 (PR 674) plus 6f27522, pushed. Closing.

All three asks landed: the validateLeadRollover case, the canary pointing the reader at the reachability enumeration, and a red-then-green proof that the new case is not vacuous.

The vacuity proof is the part that matters

The worker renamed validateLeadRollover in FleetConfig.java so the reflective sweep stops finding it, and the new case went red for the right reason:

org.opentest4j.AssertionFailedError: lead-rollover.yaml: validateAll() must refuse this config
  ==> Expected java.lang.IllegalStateException to be thrown, but nothing was thrown.

That is the new case failing, not an unrelated assertion. The canary failed alongside it, which is expected collateral — reflection no longer finds a method of that name at all. After reverting, green.

I checked the set difference myself on the merged tree rather than hand-counting: the declared validate*() methods on FleetConfig and the cases in the enumeration now match exactly, with no leftovers on either side.

A correction this change made necessary — 6f27522

Adding the eighth case turned an existing comment false. The claim-2 section banner still said:

// ── Claim 2: FleetConfig.validateAll() is wired to that mechanism and reaches seven of eight today ──

It now reaches all of them. Two more lines said "today's six" and had already been stale since the seventh validator landed, and one carried (measured at review: 1491 tests, 0 failures) — a measurement in a comment, now far out of date against 1929.

I only caught this by grepping the whole file for spelled-out counts rather than reading the diff. The false line sat outside the worker's change, so a diff review would not have shown it. That is the same shape as this ticket: a tripwire fires, the reader fixes what it points at, and the thing twenty lines away that also needed updating goes quietly stale.

Still open in this file, and deliberately not fixed here

The class javadoc still narrates the six individual cfg.validateXxx() calls that the reflective sweep replaced (lines 26, 30, 32, 71). That is history in a code comment, which belongs in git rather than in the file. Rewriting it is a separate change from the one being merged, so I left it rather than widen this merge. Filed separately.

Build

Merged tree, throwaway worktree, never the main clone: Tests run: 1929, Failures: 0, Errors: 0, Skipped: 0, BUILD SUCCESS, 172 surefire report files, 0 lines matching <<< FAILURE|<<< ERROR.

The test count stays at 1929 because this change adds a case inside an existing test method rather than a new test method.

The wider sweep came back clean, with a caveat I am keeping attached

The worker ran a follow-up hunt for other tests of this shape — a reflective canary paired with a hand-maintained list — and found no second instance. ConfigRefTopLevelCoverageTest and its siblings looked like candidates but loop generically over the production key sets, so a new key is exercised with no second edit to remember. MemberRoleTest likewise loops over .values().

That hunt read source only and ran no build or mutation. The worker said so itself and did not promote it. I have not re-verified it either, so it stands as a source-read finding rather than a measured one.

## Merged as `283ccf8` (PR 674) plus `6f27522`, pushed. Closing. All three asks landed: the `validateLeadRollover` case, the canary pointing the reader at the reachability enumeration, and a red-then-green proof that the new case is not vacuous. ### The vacuity proof is the part that matters The worker renamed `validateLeadRollover` in `FleetConfig.java` so the reflective sweep stops finding it, and the new case went red for the right reason: ``` org.opentest4j.AssertionFailedError: lead-rollover.yaml: validateAll() must refuse this config ==> Expected java.lang.IllegalStateException to be thrown, but nothing was thrown. ``` That is the new case failing, not an unrelated assertion. The canary failed alongside it, which is expected collateral — reflection no longer finds a method of that name at all. After reverting, green. I checked the set difference myself on the merged tree rather than hand-counting: the declared `validate*()` methods on `FleetConfig` and the cases in the enumeration now match exactly, with no leftovers on either side. ### A correction this change made necessary — `6f27522` Adding the eighth case turned an existing comment **false**. The claim-2 section banner still said: > `// ── Claim 2: FleetConfig.validateAll() is wired to that mechanism and reaches seven of eight today ──` It now reaches all of them. Two more lines said "today's six" and had already been stale since the seventh validator landed, and one carried `(measured at review: 1491 tests, 0 failures)` — a measurement in a comment, now far out of date against 1929. I only caught this by grepping the whole file for spelled-out counts rather than reading the diff. The false line sat outside the worker's change, so a diff review would not have shown it. **That is the same shape as this ticket:** a tripwire fires, the reader fixes what it points at, and the thing twenty lines away that also needed updating goes quietly stale. ### Still open in this file, and deliberately not fixed here The class javadoc still narrates the six individual `cfg.validateXxx()` calls that the reflective sweep replaced (lines 26, 30, 32, 71). That is history in a code comment, which belongs in git rather than in the file. Rewriting it is a separate change from the one being merged, so I left it rather than widen this merge. Filed separately. ### Build Merged tree, throwaway worktree, never the main clone: `Tests run: 1929, Failures: 0, Errors: 0, Skipped: 0`, `BUILD SUCCESS`, 172 surefire report files, `0` lines matching `<<< FAILURE|<<< ERROR`. The test count stays at 1929 because this change adds a case inside an existing test method rather than a new test method. ### The wider sweep came back clean, with a caveat I am keeping attached The worker ran a follow-up hunt for other tests of this shape — a reflective canary paired with a hand-maintained list — and found no second instance. `ConfigRefTopLevelCoverageTest` and its siblings looked like candidates but loop generically over the production key sets, so a new key is exercised with no second edit to remember. `MemberRoleTest` likewise loops over `.values()`. **That hunt read source only and ran no build or mutation.** The worker said so itself and did not promote it. I have not re-verified it either, so it stands as a source-read finding rather than a measured one.
ltms closed this issue 2026-10-03 20:54:44 +02:00
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#668
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?