From 21c539f22ed16a3c75d73894d672ab397f234dfe Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Thu, 3 Sep 2026 13:18:50 +0700 Subject: [PATCH] #113: derive the config guard from the record tree, both directions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The existing guard walks KNOWN_TOP_LEVEL_KEYS and anchors its regex at column 0, so it sees only top-level keys. Every nested key was outside its scope and nothing said so, which is the shape #113 collects: a checker narrower than it looks, whose green run stops anyone looking. Two derived guards replace the assumption: everyNestedConfigKeyIsDocumentedInTheExample walks FleetConfig's record components (17 records, 83 distinct key names) and requires each to be documented in the example. everyLiveKeyInTheExampleBindsToARecordComponent resolves every live key path in the example against the record tree, so a documented key that binds to nothing fails here instead of being silently ignored in production. Neither carries a list, so a key added to any nested record is covered the moment it compiles (criterion 2). Both mutations run through the real caller, not the helper (criterion 1, which asks for exactly that): removed every mention of paneProbeIntervalSeconds from the example -> FAILS, naming health.paneProbeIntervalSeconds added a live bind.totallyMadeUpKnob to the example -> FAILS, naming bind.totallyMadeUpKnob 0 compile errors in both; both reverted and confirmed with diff -q. The first attempt at mutation 1 removed only the `key:` line and the run stayed green — correctly, because the key was still documented in prose. An incomplete mutation proves nothing, so it was redone. Denominators (criterion 3): both guards print how many keys they checked, and the floor for "did the walk descend?" is derived from KNOWN_TOP_LEVEL_KEYS.size() rather than being a literal. Scope is stated in the javadoc rather than implied: the guards do not check a key sits at the right path, do not parse commented prose for the reverse direction, and do not prove a parsed key is read by anything. paneProbeIntervalSeconds is parsed and read by nothing, and these guards pass it -- the example already says so in its own text. everyOptionalKnobDocumentedInTheExampleBinds keeps its hand-written list but is re-documented as a value-binding spot check, explicitly not a coverage guard; coverage now comes from the two derived tests. broker.uri is documented only in the example's prose convention (`# uri -> ...`), never as a copy-pasteable `uri:` key, because writing it out invites pasting a password into a file -- the thing uriEnv exists to avoid. The matcher accepts that convention rather than pushing the file toward doing it. Full build: 1236 tests, 0 failures, 0 compile errors. --- .../ltms/fleet/config/FleetConfigTest.java | 246 +++++++++++++++++- 1 file changed, 243 insertions(+), 3 deletions(-) diff --git a/fleetd/src/test/java/dev/ltms/fleet/config/FleetConfigTest.java b/fleetd/src/test/java/dev/ltms/fleet/config/FleetConfigTest.java index 2ba37ec..bf07edf 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/config/FleetConfigTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/config/FleetConfigTest.java @@ -6,11 +6,18 @@ import dev.ltms.fleet.peer.MemberRole; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; +import java.lang.reflect.ParameterizedType; +import java.lang.reflect.RecordComponent; +import java.lang.reflect.Type; import java.nio.file.Files; import java.nio.file.Path; +import java.util.ArrayList; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.TreeMap; +import java.util.regex.Matcher; import java.util.regex.Pattern; import static org.junit.jupiter.api.Assertions.*; @@ -1366,9 +1373,18 @@ class FleetConfigTest { } /** - * Every optional knob the example documents must bind under the exact spelling used there. - * Keep this list in step with {@code fleetd.example.yaml}: a rename that updates the record - * but not the example (or vice versa) fails here instead of silently no-op'ing in production. + * A spot check that the knobs listed below bind under the exact spelling the example uses — + * it asserts real VALUES arrive in the record, which no name-matching guard can do. + * + *

This is NOT a coverage guard, and must not be read as one (fleetd #113). The list + * inside it is hand-written, so it only ever covers what someone remembered to add. Coverage + * — "is every key the code reads documented, and does every documented key bind?" — comes + * from {@link #everyNestedConfigKeyIsDocumentedInTheExample} and + * {@link #everyLiveKeyInTheExampleBindsToARecordComponent}, both of which derive their key + * set from the record tree and therefore cannot drift. + * + *

Adding a knob here is optional. Leaving one out is not a coverage gap, because the two + * derived guards above already fail on an undocumented or unbindable key. */ @Test void everyOptionalKnobDocumentedInTheExampleBinds(@TempDir Path dir) throws Exception { @@ -1525,6 +1541,230 @@ class FleetConfigTest { return p.matcher(yaml).find(); } + /** + * The nested half of {@link #everyKnownTopLevelKeyIsDocumentedInTheExample} (fleetd #113). + * + *

That guard walks {@link FleetConfig#KNOWN_TOP_LEVEL_KEYS} and anchors its regex at + * column 0, so it sees ONLY top-level keys. Every nested key — {@code profiles..model}, + * {@code health.paneProbeIntervalSeconds} and a hundred others — is outside its scope, and + * neither its name nor its output says so. A green run then reads as "the example documents + * the schema" when most of the schema was never looked at. + * + *

This walks the record tree rather than a name list, so a key added to any nested record + * is covered the moment it compiles, with no edit here. That is the point: a hand-maintained + * second copy of a list always drifts from the thing it mirrors. + * + *

Scope, stated on purpose (fleetd #113 criterion 3 — every check reports what it + * did and did not look at): + *

+ */ + @Test + void everyNestedConfigKeyIsDocumentedInTheExample() throws Exception { + Path example = Path.of("fleetd.example.yaml"); + assertTrue(Files.exists(example), "fleetd.example.yaml must ship next to the pom"); + String text = Files.readString(example); + + Map pathByName = configKeyPaths(); + + // The denominator. An under-counting walk passes every subset check vacuously, which is + // the exact shape fleetd #113 collects — so the walk has to prove it descended at all. + // The floor is DERIVED, not a literal: the nested walk must find substantially more keys + // than the top-level set the old guard used, or it has not gone below the first level. + int topLevel = FleetConfig.KNOWN_TOP_LEVEL_KEYS.size(); + assertTrue(pathByName.size() > topLevel * 2, + "the record walk found " + pathByName.size() + " config key(s) against " + + topLevel + " top-level key(s) — it has stopped descending into the " + + "nested records, so this guard would pass vacuously. Fix the walk " + + "before trusting a green run."); + + List undocumented = pathByName.entrySet().stream() + .filter(e -> !keyDocumentedAnywhere(text, e.getKey())) + .map(Map.Entry::getValue) + .sorted() + .toList(); + + assertTrue(undocumented.isEmpty(), () -> "checked " + pathByName.size() + + " config key(s) that FleetConfig can bind; " + undocumented.size() + + " appear nowhere in fleetd.example.yaml: " + undocumented + + " — document each one there, commented out if optional. fleetd.yaml is " + + "gitignored, so the example is the only committed description of the schema."); + } + + /** + * The other direction: a LIVE key in the example that {@link FleetConfig} cannot bind. That is + * a key an operator would copy into {@code fleetd.yaml} expecting it to do something, where it + * would be silently ignored. + * + *

Scope, stated on purpose: only live (uncommented) keys are checked. Most of the + * example is commented-out prose, and that prose contains lines like {@code # mode: token} + * that are indistinguishable from keys by text alone. Parsing them would produce false + * failures, so they are deliberately out of scope — and saying so here is the point, rather + * than letting a reader assume the whole file was validated. + */ + @Test + void everyLiveKeyInTheExampleBindsToARecordComponent() throws Exception { + Path example = Path.of("fleetd.example.yaml"); + String text = Files.readString(example); + + List> paths = liveKeyPaths(text); + assertTrue(paths.size() >= 20, + "only " + paths.size() + " live key path(s) were parsed out of the example — the " + + "parser is not seeing the file, so this guard would pass vacuously."); + + List unbindable = paths.stream() + .filter(path -> !pathBinds(path)) + .map(path -> String.join(".", path)) + .distinct() + .sorted() + .toList(); + + assertTrue(unbindable.isEmpty(), () -> "checked " + paths.size() + + " live key path(s) in fleetd.example.yaml; " + unbindable.size() + + " bind to nothing in FleetConfig: " + unbindable + + " — an operator copying one of these into fleetd.yaml gets silence, not an error."); + } + + /** + * Every configuration key {@link FleetConfig} can bind, at every depth, as + * {@code name -> a dotted path to one place it appears}. Derived from the record components, + * so it cannot drift from the code. + */ + private static Map configKeyPaths() { + Map out = new TreeMap<>(); + collectConfigKeys(FleetConfig.class, "", new HashSet<>(), out); + return out; + } + + private static void collectConfigKeys(Class type, String prefix, Set seen, + Map out) { + if (!type.isRecord() || !seen.add(type.getName())) { + return; + } + for (RecordComponent rc : type.getRecordComponents()) { + String path = prefix.isEmpty() ? rc.getName() : prefix + "." + rc.getName(); + out.putIfAbsent(rc.getName(), path); + Class nested = rc.getType(); + if (nested.isRecord()) { + collectConfigKeys(nested, path, seen, out); + } else if (Map.class.isAssignableFrom(nested) || List.class.isAssignableFrom(nested)) { + Class element = elementRecord(rc); + if (element != null) { + String childPrefix = Map.class.isAssignableFrom(nested) + ? path + "." : path + "[]"; + collectConfigKeys(element, childPrefix, seen, out); + } + } + } + } + + /** The record type inside a {@code Map} or {@code List} component, else null. */ + private static Class elementRecord(RecordComponent rc) { + if (rc.getGenericType() instanceof ParameterizedType pt) { + Type[] args = pt.getActualTypeArguments(); + if (args.length > 0 && args[args.length - 1] instanceof Class c && c.isRecord()) { + return c; + } + } + return null; + } + + /** + * True when {@code key} is documented in the example, in either of the two conventions that + * file actually uses: + *

    + *
  1. as a YAML key at any indentation, live or commented out ({@code key:}); or
    2. + *
  2. in a prose block that describes a section's sub-keys, one per line, as + * {@code # key → what it does}.
    3. + *
+ * + *

The second form is not decoration. {@code broker.uri} is documented ONLY that way, on + * purpose: writing it out as a copy-pasteable {@code uri: amqp://user:pass@host} invites an + * operator to paste a password into a file, which is the very thing {@code uriEnv} exists to + * avoid. A guard that demanded the key form would push the file toward doing that. So this + * encodes the convention the example really uses rather than imposing a new one. + */ + private static boolean keyDocumentedAnywhere(String yaml, String key) { + String quoted = Pattern.quote(key); + Pattern asYamlKey = Pattern.compile("(?m)^\\s*(?:#\\s*)?" + quoted + ":"); + Pattern asProseEntry = Pattern.compile("(?m)^\\s*#\\s*" + quoted + "\\s+\u2192"); + return asYamlKey.matcher(yaml).find() || asProseEntry.matcher(yaml).find(); + } + + /** Every live (uncommented) key in {@code yaml}, as a path from the document root. */ + private static List> liveKeyPaths(String yaml) { + Pattern keyLine = Pattern.compile("^(\\s*)([A-Za-z][A-Za-z0-9_]*):(\\s.*)?$"); + List stack = new ArrayList<>(); + List indents = new ArrayList<>(); + List> paths = new ArrayList<>(); + for (String line : yaml.split("\n", -1)) { + if (line.isBlank() || line.stripLeading().startsWith("#")) { + continue; + } + Matcher m = keyLine.matcher(line); + if (!m.matches()) { + continue; + } + int indent = m.group(1).length(); + while (!indents.isEmpty() && indents.get(indents.size() - 1) >= indent) { + indents.remove(indents.size() - 1); + stack.remove(stack.size() - 1); + } + indents.add(indent); + stack.add(m.group(2)); + paths.add(List.copyOf(stack)); + } + return paths; + } + + /** True when a dotted YAML path resolves to something {@link FleetConfig} can bind. */ + private static boolean pathBinds(List path) { + Class type = FleetConfig.class; + boolean nextSegmentIsAFreeFormName = false; + for (int i = 0; i < path.size(); i++) { + if (nextSegmentIsAFreeFormName) { + nextSegmentIsAFreeFormName = false; + continue; + } + RecordComponent rc = componentNamed(type, path.get(i)); + if (rc == null) { + return false; + } + Class t = rc.getType(); + if (t.isRecord()) { + type = t; + } else if (Map.class.isAssignableFrom(t)) { + Class element = elementRecord(rc); + if (element == null) { + return true; // Map: its entries are data, not schema + } + type = element; + nextSegmentIsAFreeFormName = true; + } else { + // A scalar or a list of scalars: nothing may legitimately nest under it. + return i == path.size() - 1; + } + } + return true; + } + + private static RecordComponent componentNamed(Class type, String name) { + if (type == null || !type.isRecord()) { + return null; + } + for (RecordComponent rc : type.getRecordComponents()) { + if (rc.getName().equals(name)) { + return rc; + } + } + return null; + } + @Test void placementDefaultsToFixedForExistingConfigs(@TempDir Path dir) throws Exception { Path f = dir.resolve("no-placement.yaml");