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

The deferred key set still has no reporting coverage — measured, a dropped comparison is invisible #337

New Issue
Closed
opened 2026-09-04 10:44:09 +02:00 by ltms · 1 comment
ltms commented 2026-09-04 10:44:09 +02:00
Owner
Copy Link
Copy Source

Follow-up to #333. The #333 worker declared this gap in its own test's javadoc rather than closing
over it, which is why it was findable in one command. I have now measured it.

The gap

ConfigRefTopLevelReportingCoverageTest (new in #333) proves that every member of COLD_KEYS and
SPLIT_KEYS really has a comparison branch behind it. It deliberately does not cover
DEFERRED_TOP_LEVEL_KEYS — 11 of the 22 top-level keys, the largest bucket of the four.

So a deferred key can sit in the set with no comparison in changedDeferredKeys, and nothing fails.

Measured

On main at b4f9d7f. I deleted guard's comparison from changedDeferredKeys and left guard
in the deferred set:

// removed:
if (!Objects.equals(old.guard(), fresh.guard())) {
    changed.add("guard");
}

Full suite, unpiped:

Tests run: 1355, Failures: 0, Errors: 0, Skipped: 0
BUILD SUCCESS

Nothing failed. Not the top-level coverage test, not the reporting coverage test, not the
assert. An operator changing guard: would get a bare "config reloaded" and no word that the
change needs a restart. Reverted; main is unchanged.

For contrast, the same mutation against the covered half — dropping memberHerdrSocket's branch
from changedColdKeys while leaving it in COLD_KEYS — fails immediately and names the key:

ConfigRefTopLevelReportingCoverageTest.everyColdKeyIsActuallyReportedByChangedColdKeys:185
these keys are in ConfigRef.COLD_KEYS but mutating them alone produces no matching entry from
changedColdKeys — a set entry with no comparison behind it: [memberHerdrSocket]

The mechanism works. It is only pointed at half the keys.

Which keys are actually exposed

The #333 worker's own note says most deferred keys already have an individual behavioural test in
ConfigRefTest naming them: worktreeGroup, primary, configReload, the profile launch
settings, exhaustedPattern, errorPattern, ideProjectDir.

It lists these as having none: guard, lifecycle, leadHeartbeat, the spawnReady* settings,
quarantineCooldownSeconds. My guard measurement is one confirmed instance of that list.

Do not take that list as the answer. It is the worker's reading and I have only measured one
entry of it. The first job on this ticket is to re-derive which deferred keys have no individual
test, by mutation, one key at a time — not by reading ConfigRefTest's method names.

Goal and invariant

Goal: dropping any deferred key's comparison must fail the build and name that key, exactly as
it already does for a cold or split key.

Invariant: the existing individual behavioural tests in ConfigRefTest stay. They assert the
message wording an operator sees, which a reflection test cannot check. This is added coverage,
not a replacement.

Candidate mechanism, as a candidate only: extend
ConfigRefTopLevelReportingCoverageTest to DEFERRED_TOP_LEVEL_KEYS the same way it already
covers the other two sets. Decide it yourself and justify it. Watch for the reason #333 narrowed
the scope in the first place: some deferred keys are nested records where "differ in exactly one
top-level component" is harder to synthesise than for a scalar. If a key genuinely cannot be
covered this way, put it in a named, pinned exclusion set with a one-line reason each — never
a silent skip, and never a growing hatch.

Why this keeps happening

Fourth time the same shape ships: worktreeGroup (#323), primary and configReload (#326),
fleet (#333), and now the deferred set's own checker. Each time a reload reported success for a
change the daemon never picked up. Every checker added so far has closed one direction and left the
other open. Ask which states still OPEN this one before calling it done.

Follow-up to #333. The #333 worker declared this gap in its own test's javadoc rather than closing over it, which is why it was findable in one command. I have now measured it. ## The gap `ConfigRefTopLevelReportingCoverageTest` (new in #333) proves that every member of `COLD_KEYS` and `SPLIT_KEYS` really has a comparison branch behind it. It deliberately does **not** cover `DEFERRED_TOP_LEVEL_KEYS` — 11 of the 22 top-level keys, the largest bucket of the four. So a deferred key can sit in the set with no comparison in `changedDeferredKeys`, and nothing fails. ## Measured On `main` at `b4f9d7f`. I deleted `guard`'s comparison from `changedDeferredKeys` and left `guard` in the deferred set: ```java // removed: if (!Objects.equals(old.guard(), fresh.guard())) { changed.add("guard"); } ``` Full suite, unpiped: ``` Tests run: 1355, Failures: 0, Errors: 0, Skipped: 0 BUILD SUCCESS ``` **Nothing failed.** Not the top-level coverage test, not the reporting coverage test, not the `assert`. An operator changing `guard:` would get a bare "config reloaded" and no word that the change needs a restart. Reverted; `main` is unchanged. For contrast, the same mutation against the covered half — dropping `memberHerdrSocket`'s branch from `changedColdKeys` while leaving it in `COLD_KEYS` — fails immediately and names the key: ``` ConfigRefTopLevelReportingCoverageTest.everyColdKeyIsActuallyReportedByChangedColdKeys:185 these keys are in ConfigRef.COLD_KEYS but mutating them alone produces no matching entry from changedColdKeys — a set entry with no comparison behind it: [memberHerdrSocket] ``` The mechanism works. It is only pointed at half the keys. ## Which keys are actually exposed The #333 worker's own note says most deferred keys already have an individual behavioural test in `ConfigRefTest` naming them: `worktreeGroup`, `primary`, `configReload`, the profile launch settings, `exhaustedPattern`, `errorPattern`, `ideProjectDir`. It lists these as having none: `guard`, `lifecycle`, `leadHeartbeat`, the `spawnReady*` settings, `quarantineCooldownSeconds`. My `guard` measurement is one confirmed instance of that list. **Do not take that list as the answer.** It is the worker's reading and I have only measured one entry of it. The first job on this ticket is to re-derive which deferred keys have no individual test, by mutation, one key at a time — not by reading `ConfigRefTest`'s method names. ## Goal and invariant **Goal:** dropping any deferred key's comparison must fail the build and name that key, exactly as it already does for a cold or split key. **Invariant:** the existing individual behavioural tests in `ConfigRefTest` stay. They assert the *message wording* an operator sees, which a reflection test cannot check. This is added coverage, not a replacement. **Candidate mechanism, as a candidate only:** extend `ConfigRefTopLevelReportingCoverageTest` to `DEFERRED_TOP_LEVEL_KEYS` the same way it already covers the other two sets. Decide it yourself and justify it. Watch for the reason #333 narrowed the scope in the first place: some deferred keys are nested records where "differ in exactly one top-level component" is harder to synthesise than for a scalar. If a key genuinely cannot be covered this way, put it in a **named, pinned** exclusion set with a one-line reason each — never a silent skip, and never a growing hatch. ## Why this keeps happening Fourth time the same shape ships: `worktreeGroup` (#323), `primary` and `configReload` (#326), `fleet` (#333), and now the deferred set's own checker. Each time a reload reported success for a change the daemon never picked up. Every checker added so far has closed one direction and left the other open. Ask which states still OPEN this one before calling it done.
ltms commented 2026-09-04 11:19:36 +02:00
Author
Owner
Copy Link
Copy Source

Merged to main as 591df91 (--no-ff; the branch was behind main).

Build: Tests run: 1363, Failures: 0, Errors: 0, Skipped: 0, BUILD SUCCESS, unpiped.

The list in this issue was wrong, and the implementer proved it

This ticket told the implementer not to trust my list of uncovered deferred keys and to re-derive it
by mutation, one key at a time. It did, and it came back with two corrections:

  • lifecycle — I listed it as uncovered. It is covered.
  • worktreeRoot — I did not list it at all. It is uncovered.

I checked both myself rather than take the report on trust.

Mutation AA — drop lifecycle's comparison:

ConfigRefTest.aDeferredChangeIsAppliedAndReported:235   expected: <[lifecycle]> but was: <[]>
ConfigRefTest.aSplitChangeAndADeferredChangeCoexist:787 expected: <[lifecycle]> but was: <[]>

Two pre-existing tests fail. It was already covered. My list was wrong; the measurement was right.

Mutation Z — drop worktreeRoot's comparison (a different key than the implementer used for its
own proof):

ConfigRefTopLevelReportingCoverageTest.everyDeferredKeyIsActuallyReportedByChangedDeferredKeys:278
these keys are in ConfigRef.DEFERRED_KEYS but mutating them alone produces no matching entry from
changedDeferredKeys — a set entry with no comparison behind it ... [worktreeRoot]

Caught, and it names the key. Both reverted; main is unchanged.

The real answer is 6 of 11 uncovered: guard, leadHeartbeat, worktreeRoot,
spawnReadyTimeoutMs, spawnReadyPollMs, quarantineCooldownSeconds. All now covered.

This is the sixth time a list I supplied as a convenience turned out to be wrong. The difference
here is that the brief said in so many words to re-derive it from a command and to report
disagreement — so the error cost nothing instead of shipping.

No escape hatch was needed

Every DEFERRED_KEYS component turned out to be a scalar or a simple record, so the reflective
one-field-differs trick worked for all 11 with no exclusion set. That was the risk this ticket
called out, and it did not materialise. Better than a small, well-documented hatch.

Promoting the test-side copy of the deferred set into ConfigRef.DEFERRED_KEYS was the right call:
the checker now reads the same set the production method is compared against, instead of a second
hand-maintained copy that could drift. Visibility of changedDeferredKeys went private →
package-private, matching the other two changed*Keys methods. No behaviour change.

On the git checkout -- snag

The implementer reported that a plain git checkout -- ConfigRef.java after a mutation reverted the
file all the way to main, wiping its own uncommitted work, and that it caught this from a
BUILD FAILURE on the final full build, redid the edits, and re-ran the mutation proof against the
correctly restored file.

Reporting that unprompted is the right behaviour, and the final diff and build are correct — I
checked. It is also a good argument for copying a file aside before mutating it rather than relying
on git checkout --, which cannot tell your mutation from your fix.

Closing. The four reload buckets now each prove their own reporting coverage.

Merged to `main` as `591df91` (`--no-ff`; the branch was behind main). Build: `Tests run: 1363, Failures: 0, Errors: 0, Skipped: 0`, `BUILD SUCCESS`, unpiped. ## The list in this issue was wrong, and the implementer proved it This ticket told the implementer not to trust my list of uncovered deferred keys and to re-derive it by mutation, one key at a time. It did, and it came back with two corrections: - **`lifecycle`** — I listed it as uncovered. It is **covered**. - **`worktreeRoot`** — I did not list it at all. It is **uncovered**. I checked both myself rather than take the report on trust. **Mutation AA — drop `lifecycle`'s comparison:** ``` ConfigRefTest.aDeferredChangeIsAppliedAndReported:235 expected: <[lifecycle]> but was: <[]> ConfigRefTest.aSplitChangeAndADeferredChangeCoexist:787 expected: <[lifecycle]> but was: <[]> ``` Two pre-existing tests fail. It was already covered. My list was wrong; the measurement was right. **Mutation Z — drop `worktreeRoot`'s comparison** (a different key than the implementer used for its own proof): ``` ConfigRefTopLevelReportingCoverageTest.everyDeferredKeyIsActuallyReportedByChangedDeferredKeys:278 these keys are in ConfigRef.DEFERRED_KEYS but mutating them alone produces no matching entry from changedDeferredKeys — a set entry with no comparison behind it ... [worktreeRoot] ``` Caught, and it names the key. Both reverted; main is unchanged. The real answer is **6 of 11 uncovered**: `guard`, `leadHeartbeat`, `worktreeRoot`, `spawnReadyTimeoutMs`, `spawnReadyPollMs`, `quarantineCooldownSeconds`. All now covered. This is the sixth time a list I supplied as a convenience turned out to be wrong. The difference here is that the brief said in so many words to re-derive it from a command and to report disagreement — so the error cost nothing instead of shipping. ## No escape hatch was needed Every `DEFERRED_KEYS` component turned out to be a scalar or a simple record, so the reflective one-field-differs trick worked for all 11 with no exclusion set. That was the risk this ticket called out, and it did not materialise. Better than a small, well-documented hatch. Promoting the test-side copy of the deferred set into `ConfigRef.DEFERRED_KEYS` was the right call: the checker now reads the same set the production method is compared against, instead of a second hand-maintained copy that could drift. Visibility of `changedDeferredKeys` went `private` → package-private, matching the other two `changed*Keys` methods. No behaviour change. ## On the `git checkout --` snag The implementer reported that a plain `git checkout -- ConfigRef.java` after a mutation reverted the file all the way to `main`, wiping its own uncommitted work, and that it caught this from a `BUILD FAILURE` on the final full build, redid the edits, and re-ran the mutation proof against the correctly restored file. Reporting that unprompted is the right behaviour, and the final diff and build are correct — I checked. It is also a good argument for copying a file aside before mutating it rather than relying on `git checkout --`, which cannot tell your mutation from your fix. Closing. The four reload buckets now each prove their own reporting coverage.
ltms closed this issue 2026-09-04 11:19:43 +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#337
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?