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

CB-602: nothing detects a new config key that never reaches bridged.example.yaml #96

New Issue
Closed
opened 2026-08-16 18:00:46 +02:00 by ltms · 1 comment
ltms commented 2026-08-16 18:00:46 +02:00
Owner
Copy Link
Copy Source

Split out of #85 (CB-597). I closed that ticket having delivered four of its six criteria, and this is the one I had called "the criterion that matters" — so it deserves its own ticket rather than a footnote on a closed one.

What already exists

More than #85 assumed. Two tests in BridgedConfigTest already cover part of this:

  • shippedExampleConfigParses — loads bridged.example.yaml through the real BridgedConfig.load and asserts a few resolved values. This satisfied #85's criterion 5 before the ticket was written. Its javadoc explains why it exists: the mapper is ignoreUnknown = true, so a misspelled key in the example is silently dropped and the operator gets a default they did not ask for. A spawn_ready_timeout_ms typo survived that way once already.
  • everyOptionalKnobDocumentedInTheExampleBinds — asserts every knob the example documents still binds under that exact spelling, so a record rename that misses the example fails here.

#85's criterion 6 (a stale line in docs/CB-307-Reliable-Delivery.md telling readers not to add a broker: block) is also already fixed — grep finds nothing on main.

What is still missing

Both existing tests protect the example → code direction: a key written in the example must bind. Nothing protects code → example: a brand-new config key that no one documents is invisible to the whole suite.

everyOptionalKnobDocumentedInTheExampleBinds is a hand-maintained list, and its own javadoc says so ("keep this list in step with bridged.example.yaml"). A hand-maintained list cannot notice a key nobody added to it. So the drift this ticket exists to stop is exactly the drift still unguarded.

This matters more here than in most projects because bridged/bridged.yaml is gitignored. Workers never see it, CI never checks it, and a new host has nothing to copy. bridged.example.yaml is the only committed description of the config schema — if a key is not in it, the key is undocumented, full stop.

Fix

A test that fails when a key the code reads is absent from the example.

The parser already knows the accepted set — BridgedConfig.KNOWN_TOP_LEVEL_KEYS backs the unknown-key warning. Comparing that set against the keys appearing in bridged.example.yaml is the cheap version and catches the case that actually happens: a whole new top-level section shipping undocumented.

Two things to get right, and they are the reason this is not a one-liner:

  1. Most of the example is commented out, deliberately — an optional section is documented as a commented block so the shipped file stays a working minimal config. So the check must scan the file as text, not as parsed YAML. Parsing it and reading the key set is what produced #85's wrong premise in the first place: it reported five sections missing that were all present as comments. Do not repeat that mistake.
  2. The failure message must be useful. "Key foo is read by BridgedConfig but appears nowhere in bridged.example.yaml — document it there, commented out if optional." A bare assertion failure will get an entry added with no explanation, which is half the value lost.

Nested keys are a bonus, not a requirement. Top-level coverage catches the expensive case. If you extend to nested, say how you bounded it — a naive recursive check will drown in false positives from example values.

Acceptance criteria

  1. Adding a new top-level key to BridgedConfig without documenting it in bridged.example.yaml fails the build.
  2. A key documented only as a comment counts as documented. Prove this — it is the case that makes the test correct rather than annoying, and getting it wrong makes the test unusable.
  3. The failure message names the key and says what to do.
  4. Both existing tests keep working unmodified.
  5. Prove the test fails when it should. Add a throwaway key to the parser, watch it fail, remove it, and quote what you saw. A drift guard that cannot detect drift is worse than none, because it makes people confident.
  6. mvn -f bridged/pom.xml clean install green, run unpiped.

Note

Do not use this ticket to also fix the cosmetic duplication in the example (primary: is documented twice, near the top and near the bottom). That was noticed during #85 and deliberately left; fold it in only if your change makes it trivial, and say so.

Split out of #85 (CB-597). I closed that ticket having delivered four of its six criteria, and this is the one I had called "the criterion that matters" — so it deserves its own ticket rather than a footnote on a closed one. ## What already exists More than #85 assumed. Two tests in `BridgedConfigTest` already cover part of this: - `shippedExampleConfigParses` — loads `bridged.example.yaml` through the real `BridgedConfig.load` and asserts a few resolved values. This satisfied #85's criterion 5 before the ticket was written. Its javadoc explains why it exists: the mapper is `ignoreUnknown = true`, so a misspelled key in the example is silently dropped and the operator gets a default they did not ask for. A `spawn_ready_timeout_ms` typo survived that way once already. - `everyOptionalKnobDocumentedInTheExampleBinds` — asserts every knob the example documents still binds under that exact spelling, so a record rename that misses the example fails here. #85's criterion 6 (a stale line in `docs/CB-307-Reliable-Delivery.md` telling readers not to add a `broker:` block) is also already fixed — grep finds nothing on `main`. ## What is still missing Both existing tests protect the **example → code** direction: a key written in the example must bind. Nothing protects **code → example**: a brand-new config key that no one documents is invisible to the whole suite. `everyOptionalKnobDocumentedInTheExampleBinds` is a hand-maintained list, and its own javadoc says so ("keep this list in step with `bridged.example.yaml`"). A hand-maintained list cannot notice a key nobody added to it. So the drift this ticket exists to stop is exactly the drift still unguarded. This matters more here than in most projects because `bridged/bridged.yaml` is gitignored. Workers never see it, CI never checks it, and a new host has nothing to copy. `bridged.example.yaml` is the **only** committed description of the config schema — if a key is not in it, the key is undocumented, full stop. ## Fix A test that fails when a key the code reads is absent from the example. The parser already knows the accepted set — `BridgedConfig.KNOWN_TOP_LEVEL_KEYS` backs the unknown-key warning. Comparing that set against the keys appearing in `bridged.example.yaml` is the cheap version and catches the case that actually happens: a whole new top-level section shipping undocumented. Two things to get right, and they are the reason this is not a one-liner: 1. **Most of the example is commented out**, deliberately — an optional section is documented as a commented block so the shipped file stays a working minimal config. So the check must scan the file as **text**, not as parsed YAML. Parsing it and reading the key set is what produced #85's wrong premise in the first place: it reported five sections missing that were all present as comments. Do not repeat that mistake. 2. **The failure message must be useful.** "Key `foo` is read by BridgedConfig but appears nowhere in bridged.example.yaml — document it there, commented out if optional." A bare assertion failure will get an entry added with no explanation, which is half the value lost. Nested keys are a bonus, not a requirement. Top-level coverage catches the expensive case. If you extend to nested, say how you bounded it — a naive recursive check will drown in false positives from example values. ## Acceptance criteria 1. Adding a new top-level key to `BridgedConfig` without documenting it in `bridged.example.yaml` fails the build. 2. A key documented **only as a comment** counts as documented. Prove this — it is the case that makes the test correct rather than annoying, and getting it wrong makes the test unusable. 3. The failure message names the key and says what to do. 4. Both existing tests keep working unmodified. 5. **Prove the test fails when it should.** Add a throwaway key to the parser, watch it fail, remove it, and quote what you saw. A drift guard that cannot detect drift is worse than none, because it makes people confident. 6. `mvn -f bridged/pom.xml clean install` green, run unpiped. ## Note Do not use this ticket to also fix the cosmetic duplication in the example (`primary:` is documented twice, near the top and near the bottom). That was noticed during #85 and deliberately left; fold it in only if your change makes it trivial, and say so.
ltms added this to the 1.1 — single-host close-out milestone 2026-08-16 18:00:46 +02:00
ltms closed this issue 2026-08-16 18:17:13 +02:00
ltms commented 2026-08-16 18:17:24 +02:00
Author
Owner
Copy Link
Copy Source

Merged (PR #98). The only production change is widening KNOWN_TOP_LEVEL_KEYS from private to package-private, with a javadoc line saying why — the test reads the parser's own accepted set instead of duplicating it, which is the right call. A second hand-maintained list would have had the same drift problem the ticket is about.

I ran criterion 5 myself rather than taking it from the report:

  • Added leadProbeUndocumented to KNOWN_TOP_LEVEL_KEYS and ran BridgedConfigTest. It failed with:

    key(s) [leadProbeUndocumented] are read by BridgedConfig but appear nowhere in bridged.example.yaml — document each one there, commented out if optional. bridged.yaml is gitignored, so this file is the only committed description of the config schema an operator or a worker can see.

  • Then I documented that same key only as a comment in the example and re-ran: 85 tests, BUILD SUCCESS. So the commented-only case genuinely counts as documented — that is the half that makes this guard usable, since most of the example is commented on purpose.
  • Reverted both probes; no references left.
  • Trial merge onto main: Tests run: 829, Failures: 0, Errors: 0, Skipped: 0, BUILD SUCCESS, unpiped.

One gap left, deliberately not fixed here. KNOWN_TOP_LEVEL_KEYS is itself hand-maintained — its javadoc says "keep in step with the record components". A key added to the BridgedConfig record but never added to that set escapes this new test too, because the test starts from the set. It is a smaller hole than the one just closed: such a key produces a visible unknown-key warning at startup, so it fails loudly at runtime rather than silently. I am not filing a follow-up for it now; noting it here so the next person does not assume the direction is fully covered.

Merged (PR #98). The only production change is widening `KNOWN_TOP_LEVEL_KEYS` from `private` to package-private, with a javadoc line saying why — the test reads the parser's own accepted set instead of duplicating it, which is the right call. A second hand-maintained list would have had the same drift problem the ticket is about. I ran criterion 5 myself rather than taking it from the report: - Added `leadProbeUndocumented` to `KNOWN_TOP_LEVEL_KEYS` and ran `BridgedConfigTest`. It failed with: > `key(s) [leadProbeUndocumented] are read by BridgedConfig but appear nowhere in bridged.example.yaml — document each one there, commented out if optional. bridged.yaml is gitignored, so this file is the only committed description of the config schema an operator or a worker can see.` - Then I documented that same key **only as a comment** in the example and re-ran: 85 tests, BUILD SUCCESS. So the commented-only case genuinely counts as documented — that is the half that makes this guard usable, since most of the example is commented on purpose. - Reverted both probes; no references left. - Trial merge onto `main`: `Tests run: 829, Failures: 0, Errors: 0, Skipped: 0`, BUILD SUCCESS, unpiped. **One gap left, deliberately not fixed here.** `KNOWN_TOP_LEVEL_KEYS` is itself hand-maintained — its javadoc says "keep in step with the record components". A key added to the `BridgedConfig` record but never added to that set escapes this new test too, because the test starts from the set. It is a smaller hole than the one just closed: such a key produces a visible unknown-key warning at startup, so it fails loudly at runtime rather than silently. I am not filing a follow-up for it now; noting it here so the next person does not assume the direction is fully covered.
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 1.1 — single-host close-out
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#96
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?