CB-611: our checkers keep covering less than they look like they cover — three instances in one day #113

Closed
opened 2026-08-17 14:24:44 +02:00 by ltms · 3 comments
Owner

Filed on 2026-08-17 after a pre-tag scan. This is a pattern ticket, not a single defect. Three separate checkers were examined today and all three passed while the thing they exist to catch was present.

The three instances

1. CB-602's config guard — BridgedConfigTest#everyKnownTopLevelKeyIsDocumentedInTheExample, commit 32bf324.

It checks one direction only (code → example) and matches only keys anchored at column 0 (regex ^(?:#\s*)?<key>:, BridgedConfigTest.java:1308-1311). Consequences:

  • A nested key that the code reads but the example omits is invisible. That is exactly how CB-610 (profiles.<name>.subscription) got through — the test passes today with that gap open. Verified by running it: Tests run: 1, Failures: 0.
  • A key that exists only in the example and that no code reads is structurally out of scope. health.workingSuspectAfterSeconds and health.paneProbeIntervalSeconds are both parsed into the Health record and read by nothing.

The reverse direction is nominally covered by everyOptionalKnobDocumentedInTheExampleBinds (:1162), but that test carries a hand-written list of keys, so it has the same failure mode one level up.

2. scripts/probe-member-credentials.sh — already filed as CB-608 (#111). Hardcodes 31 names, never reads the policy, reported 26 blocked against a policy of 29, exit 0.

3. CB-586's unit tests — already closed, recorded here because it completes the shape. Every test called the seam directly and walked around the interval gate, so a sweep that never ran once had a fully green test suite.

The shape

In all three, the checker is narrower than the thing it is understood to check, and nothing says so. A green run is then read as "covered", which is worse than no checker, because it stops anyone looking.

Two recurring mechanisms:

  • A second hand-written list. The probe has one; everyOptionalKnobDocumentedInTheExampleBinds has one. A list written once drifts silently from the thing it mirrors — which is the same defect CB-596 was filed to fix in the credential policy itself.
  • A structural blind spot that reads as a complete sweep. "Every known top-level key" sounds exhaustive. It is exhaustive over top-level keys, and nothing in the name or the output says the nested ones were never looked at.

What to do

  1. Widen the config guard to both directions and to nested keys. Walk the record structure rather than a top-level name set; report a key the code reads that the example lacks, and a key the example carries that no code reads. It must fail on CB-610's subscription and on the two dead health.* keys.
  2. Delete the hand-written list in everyOptionalKnobDocumentedInTheExampleBinds, or derive it. A guard whose coverage is a literal is not a guard.
  3. Make every such check state its own scope in its output — how many items it examined, and out of how many. 26 blocked next to a policy of 29 is only detectable because someone knew to compare. A checker that prints "checked 26 of 29" reports its own gap.
  4. Fix CB-608's probe the same way (that ticket carries the detail).

Acceptance criteria

  1. The widened config guard fails on a deliberately introduced nested code-only key, and fails on a deliberately introduced doc-only key. Prove both by adding the key, watching it fail, and removing it — not by asserting the helper in isolation. A test that exercises the seam and skips the caller is precisely the defect in instance 3.
  2. No check in this family carries a hardcoded list of the items it is supposed to cover.
  3. Each prints its own denominator.

Milestone

2.0. Nothing is misbehaving on one host; these are gaps in how we verify, and the specific defect the config guard let through is filed separately as CB-610. Grouped here so the next person sees the pattern rather than three unrelated tickets.

Related

CB-608 (#111) · CB-610 (#112) · CB-586 (#67, closed) · CB-602 (#96, closed) · CB-596 (#82, closed)

Filed on 2026-08-17 after a pre-tag scan. This is a **pattern ticket**, not a single defect. Three separate checkers were examined today and all three passed while the thing they exist to catch was present. ## The three instances **1. CB-602's config guard** — `BridgedConfigTest#everyKnownTopLevelKeyIsDocumentedInTheExample`, commit `32bf324`. It checks **one direction only** (code → example) and matches **only keys anchored at column 0** (regex `^(?:#\s*)?<key>:`, `BridgedConfigTest.java:1308-1311`). Consequences: - A nested key that the code reads but the example omits is invisible. That is exactly how **CB-610** (`profiles.<name>.subscription`) got through — the test passes today with that gap open. Verified by running it: `Tests run: 1, Failures: 0`. - A key that exists only in the example and that no code reads is structurally out of scope. `health.workingSuspectAfterSeconds` and `health.paneProbeIntervalSeconds` are both parsed into the `Health` record and read by nothing. The reverse direction is nominally covered by `everyOptionalKnobDocumentedInTheExampleBinds` (`:1162`), but that test carries a **hand-written list** of keys, so it has the same failure mode one level up. **2. `scripts/probe-member-credentials.sh`** — already filed as **CB-608 (#111)**. Hardcodes 31 names, never reads the policy, reported 26 blocked against a policy of 29, exit 0. **3. CB-586's unit tests** — already closed, recorded here because it completes the shape. Every test called the seam directly and walked around the interval gate, so a sweep that never ran once had a fully green test suite. ## The shape In all three, the checker is **narrower than the thing it is understood to check**, and nothing says so. A green run is then read as "covered", which is worse than no checker, because it stops anyone looking. Two recurring mechanisms: - **A second hand-written list.** The probe has one; `everyOptionalKnobDocumentedInTheExampleBinds` has one. A list written once drifts silently from the thing it mirrors — which is the same defect CB-596 was filed to fix in the credential policy itself. - **A structural blind spot that reads as a complete sweep.** "Every known top-level key" sounds exhaustive. It is exhaustive *over top-level keys*, and nothing in the name or the output says the nested ones were never looked at. ## What to do 1. **Widen the config guard to both directions and to nested keys.** Walk the record structure rather than a top-level name set; report a key the code reads that the example lacks, **and** a key the example carries that no code reads. It must fail on CB-610's `subscription` and on the two dead `health.*` keys. 2. **Delete the hand-written list** in `everyOptionalKnobDocumentedInTheExampleBinds`, or derive it. A guard whose coverage is a literal is not a guard. 3. **Make every such check state its own scope in its output** — how many items it examined, and out of how many. `26 blocked` next to a policy of `29` is only detectable because someone knew to compare. A checker that prints "checked 26 of 29" reports its own gap. 4. Fix CB-608's probe the same way (that ticket carries the detail). ## Acceptance criteria 1. The widened config guard **fails** on a deliberately introduced nested code-only key, and **fails** on a deliberately introduced doc-only key. Prove both by adding the key, watching it fail, and removing it — not by asserting the helper in isolation. A test that exercises the seam and skips the caller is precisely the defect in instance 3. 2. No check in this family carries a hardcoded list of the items it is supposed to cover. 3. Each prints its own denominator. ## Milestone **2.0.** Nothing is misbehaving on one host; these are gaps in how we *verify*, and the specific defect the config guard let through is filed separately as CB-610. Grouped here so the next person sees the pattern rather than three unrelated tickets. ## Related CB-608 (#111) · CB-610 (#112) · CB-586 (#67, closed) · CB-602 (#96, closed) · CB-596 (#82, closed)
ltms added this to the 2.0 — one operation centre, many hosts milestone 2026-08-17 14:24:44 +02:00
Author
Owner

Three more instances from one session (2026-09-03), all found while verifying #201/#227 Units 1-4 and #234. Adding them because each is a different way a checker reports more coverage than it has.

1. A default overload makes a fix invisible to every existing lambda

#234's first round added onExhausted(target, reason, profile) as a default method. Fleetd.java:177 held a two-argument lambda, so it kept compiling, kept calling the old path, and the profile hint never reached production. The fix was dead and every test passed.

Round two was worse: the new tests built their own forwarder rather than using the production one. I mutated production back to a plain lambda and got 52 tests, 0 failures. The test could not fail, because it never held the object it claimed to test.

The fix that actually worked was to invert the interface — make the 3-argument method abstract and the 2-argument one default — so the compiler rejects the bad shape. The denominator lesson: when you add an overload, the unit of work is every implementation, and a default method is invisible to a lambda.

2. A compile error reads as a green mutation run

Twice I mutated production, saw Tests run: N, Failures: 0, and nearly recorded "not covered". Both times the build had actually failed to compile — a mutation left a dangling @Override. A compile failure prints zero test failures, so it is indistinguishable from a passing run if you only read the test line.

Every mutation run now prints its own denominator first:

compile errors: $(grep -cE 'COMPILATION ERROR|cannot find symbol' "$LOG")

A red is only a kill when that count is 0. This is the same shape as the ticket's core complaint, applied to the checking process itself.

3. A clean auto-merge is not a compiling merge

Units 1-4 were file-disjoint in production but shared CompletionResolverTest. Merging #234's interface inversion and then Unit 1 produced no conflict markers at all — git merge reported clean — and the build failed with exactly one error:

CompletionResolverTest.java:[874,41] incompatible types: incompatible parameter types in lambda expression

A merge tool compares text; lambda arity is a type fact. Only a compiler can see it. So "the merge was clean" is not evidence of anything, and each parallel unit now gets its own build after merging rather than one build at the end.

What I changed in how units are briefed

Every delegation now carries three lines, because each maps to one of the above:

  • Mutate production, never the test's own copy of a shape.
  • Check the compile-error count is 0 before calling a red a kill.
  • Mutation-test the lines you changed, not only the lines you added — Unit 4 shipped a changed line with no test, and reverting it left all 1129 tests green.

The third one caught a real gap in Unit 4 that the full suite could not see.

Three more instances from one session (2026-09-03), all found while verifying #201/#227 Units 1-4 and #234. Adding them because each is a *different* way a checker reports more coverage than it has. ### 1. A `default` overload makes a fix invisible to every existing lambda #234's first round added `onExhausted(target, reason, profile)` as a `default` method. `Fleetd.java:177` held a two-argument lambda, so it kept compiling, kept calling the old path, and the profile hint never reached production. The fix was dead and every test passed. Round two was worse: the new tests built *their own* forwarder rather than using the production one. I mutated production back to a plain lambda and got **52 tests, 0 failures**. The test could not fail, because it never held the object it claimed to test. The fix that actually worked was to invert the interface — make the 3-argument method abstract and the 2-argument one `default` — so the compiler rejects the bad shape. The denominator lesson: **when you add an overload, the unit of work is every implementation**, and a `default` method is invisible to a lambda. ### 2. A compile error reads as a green mutation run Twice I mutated production, saw `Tests run: N, Failures: 0`, and nearly recorded "not covered". Both times the build had actually failed to compile — a mutation left a dangling `@Override`. **A compile failure prints zero test failures**, so it is indistinguishable from a passing run if you only read the test line. Every mutation run now prints its own denominator first: ``` compile errors: $(grep -cE 'COMPILATION ERROR|cannot find symbol' "$LOG") ``` A red is only a kill when that count is `0`. This is the same shape as the ticket's core complaint, applied to the checking process itself. ### 3. A clean auto-merge is not a compiling merge Units 1-4 were file-disjoint in **production** but shared `CompletionResolverTest`. Merging #234's interface inversion and then Unit 1 produced *no conflict markers at all* — `git merge` reported clean — and the build failed with exactly one error: ``` CompletionResolverTest.java:[874,41] incompatible types: incompatible parameter types in lambda expression ``` A merge tool compares text; lambda arity is a type fact. Only a compiler can see it. So "the merge was clean" is not evidence of anything, and each parallel unit now gets its own build after merging rather than one build at the end. ### What I changed in how units are briefed Every delegation now carries three lines, because each maps to one of the above: - Mutate **production**, never the test's own copy of a shape. - Check the compile-error count is 0 **before** calling a red a kill. - Mutation-test the lines you **changed**, not only the lines you **added** — Unit 4 shipped a changed line with no test, and reverting it left all 1129 tests green. The third one caught a real gap in Unit 4 that the full suite could not see.
Author
Owner

Instance 1 (the config guard) is fixed on main — 21c539f.

First: two parts of this ticket had gone stale

The ticket was filed 2026-08-17. Checked today before starting:

  • CB-610 is already fixed. bf616e1 documented subscription:, so it is no longer available as the demonstration case criterion 1 asks for. I used a different key instead (below).
  • health.workingSuspectAfterSeconds is no longer dead. It is read by one file outside FleetConfig now. health.paneProbeIntervalSeconds is still read by nothing — and the example already says so in its own text ("parsed, but nothing reads it yet — changing it changes nothing"), so that one is documented honestly rather than hidden.

The structural defect was still entirely real, which is what got fixed.

What changed

The old guard walked KNOWN_TOP_LEVEL_KEYS with a regex anchored at column 0. It could see 22 top-level keys. The record tree holds 83 distinct key names across 17 records, so roughly three quarters of the schema was outside its scope, and neither its name nor its output said so.

Two derived guards now replace that:

Test Direction Derives its key set from
everyNestedConfigKeyIsDocumentedInTheExample code → example the record components, recursively
everyLiveKeyInTheExampleBindsToARecordComponent example → code resolving each live key path against the record tree

Neither carries a list, so a key added to any nested record is covered the moment it compiles.

Acceptance criteria

1 — both directions fail on an injected key, proved through the real caller. The ticket is explicit that asserting the helper in isolation is the defect, so both were run end to end:

removed every mention of paneProbeIntervalSeconds from the example
  -> FAILS: "checked 83 config key(s) ...; 1 appear nowhere ...:
             [health.paneProbeIntervalSeconds]"

added a live `bind.totallyMadeUpKnob: 42` to the example
  -> FAILS: "checked 38 live key path(s) ...; 1 bind to nothing in
             FleetConfig: [bind.totallyMadeUpKnob]"

0 compile errors in both runs. Both reverted, confirmed with diff -q.

Worth recording: my first attempt at mutation 1 stayed green, and was right to. I had removed only the paneProbeIntervalSeconds: line, and the key was still documented in a prose block, so it was still documented. An incomplete mutation proves nothing about the guard. Redone removing every mention, it failed as it should.

2 — no check in this family establishes coverage with a literal. everyOptionalKnobDocumentedInTheExampleBinds keeps its hand-written list, because it asserts real values bind, which no name-matching guard can do. It is now documented as a value-binding spot check that is explicitly not a coverage guard, with a pointer to the two derived tests that are. Leaving a knob out of that list is no longer a coverage gap.

3 — each prints its own denominator. Both report how many keys they examined. The "did the walk actually descend?" floor is itself derived (> KNOWN_TOP_LEVEL_KEYS.size() * 2) rather than a literal — an under-counting walk passes every subset check vacuously, which is this ticket's own failure mode aimed at the fix.

That floor earned its place immediately: I first guessed >= 100 and the guard failed at 83. I then counted the components by parsing the source independently — 83 distinct across 17 records — and the walk was complete. The guess was wrong, not the walk.

One finding worth keeping

The new guard's first real run flagged broker.uri as undocumented. It is not: it is documented in the example's prose convention (# uri → AMQP connection URI…) but never as a copy-pasteable uri: key.

That omission looks deliberate. Writing out uri: amqp://user:pass@host invites an operator to paste a password into a file, which is the thing uriEnv exists to avoid. A guard that demanded the key form would have pushed the file toward doing exactly that. So the matcher accepts both conventions — it encodes the convention the example really uses instead of imposing a new one.

Scope, stated rather than implied

Written into the javadoc so the next reader does not have to infer it:

  • Key names are matched, not paths. A key documented under the wrong parent still passes.
  • The reverse direction reads live keys only. Most of the example is commented prose containing lines like # mode: token that are indistinguishable from keys by text alone; parsing them would produce false failures.
  • "Parsed but read by nothing" is not covered. paneProbeIntervalSeconds passes both guards. Proving a key is live code needs a call graph.

Still open on this ticket

Instance 2 (scripts/probe-member-credentials.sh, #111) is in progress separately. Instance 3 is closed. Leaving this open until #111 lands.

Full build at 21c539f: 1236 tests, 0 failures, 0 compile errors.

Instance 1 (the config guard) is fixed on `main` — `21c539f`. ## First: two parts of this ticket had gone stale The ticket was filed 2026-08-17. Checked today before starting: - **CB-610 is already fixed.** `bf616e1` documented `subscription:`, so it is no longer available as the demonstration case criterion 1 asks for. I used a different key instead (below). - **`health.workingSuspectAfterSeconds` is no longer dead.** It is read by one file outside `FleetConfig` now. `health.paneProbeIntervalSeconds` is still read by nothing — and the example already says so in its own text ("parsed, but nothing reads it yet — changing it changes nothing"), so that one is documented honestly rather than hidden. The *structural* defect was still entirely real, which is what got fixed. ## What changed The old guard walked `KNOWN_TOP_LEVEL_KEYS` with a regex anchored at column 0. It could see 22 top-level keys. The record tree holds **83 distinct key names across 17 records**, so roughly three quarters of the schema was outside its scope, and neither its name nor its output said so. Two derived guards now replace that: | Test | Direction | Derives its key set from | |---|---|---| | `everyNestedConfigKeyIsDocumentedInTheExample` | code → example | the record components, recursively | | `everyLiveKeyInTheExampleBindsToARecordComponent` | example → code | resolving each live key path against the record tree | Neither carries a list, so a key added to any nested record is covered the moment it compiles. ## Acceptance criteria **1 — both directions fail on an injected key, proved through the real caller.** The ticket is explicit that asserting the helper in isolation is the defect, so both were run end to end: ``` removed every mention of paneProbeIntervalSeconds from the example -> FAILS: "checked 83 config key(s) ...; 1 appear nowhere ...: [health.paneProbeIntervalSeconds]" added a live `bind.totallyMadeUpKnob: 42` to the example -> FAILS: "checked 38 live key path(s) ...; 1 bind to nothing in FleetConfig: [bind.totallyMadeUpKnob]" ``` 0 compile errors in both runs. Both reverted, confirmed with `diff -q`. Worth recording: **my first attempt at mutation 1 stayed green, and was right to.** I had removed only the `paneProbeIntervalSeconds:` line, and the key was still documented in a prose block, so it was still documented. An incomplete mutation proves nothing about the guard. Redone removing every mention, it failed as it should. **2 — no check in this family establishes coverage with a literal.** `everyOptionalKnobDocumentedInTheExampleBinds` keeps its hand-written list, because it asserts real *values* bind, which no name-matching guard can do. It is now documented as a value-binding spot check that is **explicitly not a coverage guard**, with a pointer to the two derived tests that are. Leaving a knob out of that list is no longer a coverage gap. **3 — each prints its own denominator.** Both report how many keys they examined. The "did the walk actually descend?" floor is itself derived (`> KNOWN_TOP_LEVEL_KEYS.size() * 2`) rather than a literal — an under-counting walk passes every subset check vacuously, which is this ticket's own failure mode aimed at the fix. That floor earned its place immediately: I first guessed `>= 100` and the guard failed at 83. I then counted the components by parsing the source independently — 83 distinct across 17 records — and the walk was complete. The guess was wrong, not the walk. ## One finding worth keeping The new guard's first real run flagged `broker.uri` as undocumented. It is not: it is documented in the example's prose convention (`# uri → AMQP connection URI…`) but never as a copy-pasteable `uri:` key. That omission looks deliberate. Writing out `uri: amqp://user:pass@host` invites an operator to paste a password into a file, which is the thing `uriEnv` exists to avoid. A guard that demanded the key form would have pushed the file toward doing exactly that. So the matcher accepts both conventions — it encodes the convention the example really uses instead of imposing a new one. ## Scope, stated rather than implied Written into the javadoc so the next reader does not have to infer it: - Key **names** are matched, not paths. A key documented under the wrong parent still passes. - The reverse direction reads **live keys only**. Most of the example is commented prose containing lines like `# mode: token` that are indistinguishable from keys by text alone; parsing them would produce false failures. - **"Parsed but read by nothing" is not covered.** `paneProbeIntervalSeconds` passes both guards. Proving a key is live code needs a call graph. ## Still open on this ticket Instance 2 (`scripts/probe-member-credentials.sh`, #111) is in progress separately. Instance 3 is closed. Leaving this open until #111 lands. Full build at `21c539f`: 1236 tests, 0 failures, 0 compile errors.
Author
Owner

All three instances are now closed. Closing this pattern ticket.

Instance State
1 — the config guard (FleetConfigTest) fixed, 21c539f — walks the record tree, both directions, no list
2 — scripts/probe-member-credentials.sh (#111) fixed, 6b5f3f4, verified live: 34 checked / 29 blocked / 5 present
3 — CB-586's unit tests (#67) already closed

What the three had in common, restated now they are all fixed

Both live fixes worked the same way: delete the second copy, derive it instead. The config guard derives its key set from FleetConfig's record components; the probe derives its name list from the daemon. Neither can drift, because neither holds a copy any more.

And both now print a denominator — "checked 83 config key(s)", "policy contains 34 name(s); this run checked 34 — they match." That is the cheap half of the fix and it is what would have caught all three originally. 26 blocked was only detectable as wrong because somebody happened to know the policy said 29.

Two things this ticket got wrong by the time it was picked up

Recorded because a stale premise costs the next person a run:

  • CB-610 was already fixed (bf616e1), so subscription: was no longer available as criterion 1's demonstration case. A different key was used.
  • health.workingSuspectAfterSeconds is no longer dead. paneProbeIntervalSeconds still is — and the example already says so in its own text, so it is documented rather than hidden.

A caveat that stays open on purpose

Neither new guard proves a parsed key is read by anything. paneProbeIntervalSeconds passes both. That needs a call graph, and pretending otherwise would recreate this ticket's own defect — a checker that looks wider than it is. It is stated in the javadoc instead.

A fourth instance turned up while fixing this one, in my own tooling rather than the codebase: the find -newermt '-5 minutes' check used to tell whether a member is making progress matches nothing on macOS, because BSD find does not parse the GNU relative form. It does not error — it returns 0 every time, which reads as "this member is stalled". I reported two healthy members as dead before a one-line denominator probe (touch a file, then look for it) showed the check itself could not detect anything. Same shape, same fix: make the checker prove it can see something before believing its zero.

All three instances are now closed. Closing this pattern ticket. | Instance | State | |---|---| | 1 — the config guard (`FleetConfigTest`) | fixed, `21c539f` — walks the record tree, both directions, no list | | 2 — `scripts/probe-member-credentials.sh` (#111) | fixed, `6b5f3f4`, verified live: 34 checked / 29 blocked / 5 present | | 3 — CB-586's unit tests (#67) | already closed | ## What the three had in common, restated now they are all fixed Both live fixes worked the same way: **delete the second copy, derive it instead.** The config guard derives its key set from `FleetConfig`'s record components; the probe derives its name list from the daemon. Neither can drift, because neither holds a copy any more. And both now print a denominator — "checked 83 config key(s)", "policy contains 34 name(s); this run checked 34 — they match." That is the cheap half of the fix and it is what would have caught all three originally. `26 blocked` was only detectable as wrong because somebody happened to know the policy said 29. ## Two things this ticket got wrong by the time it was picked up Recorded because a stale premise costs the next person a run: - **CB-610 was already fixed** (`bf616e1`), so `subscription:` was no longer available as criterion 1's demonstration case. A different key was used. - **`health.workingSuspectAfterSeconds` is no longer dead.** `paneProbeIntervalSeconds` still is — and the example already says so in its own text, so it is documented rather than hidden. ## A caveat that stays open on purpose Neither new guard proves a *parsed* key is *read* by anything. `paneProbeIntervalSeconds` passes both. That needs a call graph, and pretending otherwise would recreate this ticket's own defect — a checker that looks wider than it is. It is stated in the javadoc instead. A fourth instance turned up while fixing this one, in my own tooling rather than the codebase: the `find -newermt '-5 minutes'` check used to tell whether a member is making progress **matches nothing on macOS**, because BSD `find` does not parse the GNU relative form. It does not error — it returns 0 every time, which reads as "this member is stalled". I reported two healthy members as dead before a one-line denominator probe (`touch` a file, then look for it) showed the check itself could not detect anything. Same shape, same fix: make the checker prove it can see something before believing its zero.
ltms closed this issue 2026-09-03 08:38:25 +02:00
Sign in to join this conversation.
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#113