CB-548: make duplicate-architect-slot detection top-level-only and depth-safe
CI / contract (pull_request) Successful in 42s
CI / build (pull_request) Successful in 1m13s

This commit is contained in:
Dai Ha
2026-08-13 18:31:29 +02:00
parent 6123576c68
commit f004a0c654
2 changed files with 127 additions and 16 deletions
@@ -593,7 +593,9 @@ public record BridgedConfig(
* have already collapsed last-wins — a duplicated slot name would silently drop one slot and the
* daemon would never know. Jackson's YAML parser does not fail on duplicate mapping keys by
* default, so duplicates are caught here, at parse time, before the map is built. Only the
* {@code architects:} block is walked, so parsing of the rest of the config is unaffected.
* <em>top-level</em> {@code architects:} block is considered, and only its direct child keys (the
* slot names) — a nested field elsewhere, even one also named {@code architects:}, is ignored, so
* parsing of the rest of the config is unaffected.
*
* @throws IllegalStateException when two {@code architects:} entries share a slot name, naming it
*/
@@ -602,31 +604,83 @@ public record BridgedConfig(
if (p.nextToken() != JsonToken.START_OBJECT) {
return; // not a mapping at top level — readValue reports the malformed file
}
// Scan the TOP-LEVEL mapping only. Every other field's value (however deep, including
// any nested field also literally named "architects") is consumed whole by skipValue, so
// the loop below can only ever see the top-level field names — a nested `architects:` can
// neither suppress the real block nor be misread as one.
JsonToken t;
while ((t = p.nextToken()) != null) {
if (t == JsonToken.FIELD_NAME && "architects".equals(p.getCurrentName())) {
if (p.nextToken() == JsonToken.START_OBJECT) {
Set<String> seen = new HashSet<>();
while ((t = p.nextToken()) != null && t != JsonToken.END_OBJECT) {
if (t == JsonToken.FIELD_NAME && !seen.add(p.getCurrentName())) {
throw new IllegalStateException("refusing to start: duplicate architect "
+ "slot name '" + p.getCurrentName() + "' — slot names must be "
+ "unique; a later entry would silently overwrite the earlier "
+ "one");
}
p.nextToken(); // the slot's value
p.skipChildren();
while ((t = p.nextToken()) != null && t != JsonToken.END_OBJECT) {
if (t == JsonToken.FIELD_NAME) {
String name = p.getCurrentName();
JsonToken value = p.nextToken();
if ("architects".equals(name)) {
if (value == JsonToken.START_OBJECT) {
rejectDuplicateChildSlotKeys(p);
}
return; // the single top-level architects block is handled; nothing more to check
}
return; // the architects block (or its absence) is handled; nothing more to check
skipValue(p, value);
}
p.skipChildren();
}
} catch (IOException e) {
// Not a duplicate-name condition — let readValue report the malformed file itself.
}
}
/**
* Reject a duplicated <em>direct child</em> key of the (already-positioned) {@code architects:}
* mapping — i.e. a duplicated {@code slot name}.
*
* <p>Each slot's value is consumed whole by {@link #skipValue}, so a duplicated field <em>inside</em>
* a slot (e.g. two {@code profile:} keys, or a duplicate nested {@code architects:}) is never seen
* here and cannot masquerade as a duplicated slot name.
*
* @throws IllegalStateException when two {@code architects:} entries share a slot name, naming it
*/
private static void rejectDuplicateChildSlotKeys(JsonParser p) throws IOException {
Set<String> seen = new HashSet<>();
JsonToken t;
while ((t = p.nextToken()) != null && t != JsonToken.END_OBJECT) {
if (t == JsonToken.FIELD_NAME) {
if (!seen.add(p.getCurrentName())) {
throw new IllegalStateException("refusing to start: duplicate architect slot name '"
+ p.getCurrentName() + "' — slot names must be unique; a later entry would "
+ "silently overwrite the earlier one");
}
skipValue(p, p.nextToken()); // the slot's entire value
}
}
}
/**
* Consume the whole value that starts at {@code start}, including every nested structure, and
* leave the parser positioned just past it. Used so depth is handled structurally rather than by
* a heuristic — a nested field is never interpreted as a top-level {@code architects:}.
*/
private static void skipValue(JsonParser p, JsonToken start) throws IOException {
switch (start) {
case START_OBJECT: {
JsonToken t;
while ((t = p.nextToken()) != null && t != JsonToken.END_OBJECT) {
if (t == JsonToken.FIELD_NAME) {
skipValue(p, p.nextToken());
}
}
return;
}
case START_ARRAY: {
JsonToken t;
while ((t = p.nextToken()) != null && t != JsonToken.END_ARRAY) {
skipValue(p, t);
}
return;
}
default:
// A scalar (VALUE_* / VALUE_NULL) is already fully consumed by the nextToken that
// returned it — nothing further to skip.
}
}
/**
* Log a WARN naming any top-level key this version does not understand (CB-530).
*
@@ -522,6 +522,63 @@ class BridgedConfigTest {
assertEquals(Set.of("sonnet"), BridgedConfig.load(f).workerProfiles().keySet());
}
@Test
void aNestedArchitectsFieldDoesNotSuppressRealDuplicateDetection(@TempDir Path dir) throws Exception {
// A field ALSO named `architects` nested under another block carries its own duplicate and
// sits BEFORE the real top-level block. Only the top-level block is ever inspected: the
// refusal must name the real slot (lead-designer), not the nested one (nested-slot).
Path f = dir.resolve("nested-arch.yaml");
Files.writeString(f, """
bind:
port: 8080
architects:
nested-slot:
k: v
nested-slot:
k: v
architects:
lead-designer:
profile: sonnet
lead-designer:
profile: sonnet
""");
IllegalStateException e =
assertThrows(IllegalStateException.class, () -> BridgedConfig.load(f));
assertTrue(e.getMessage().contains("lead-designer"),
"the real top-level duplicate must be reported, was: " + e.getMessage());
assertTrue(e.getMessage().contains("duplicate architect"),
"the refusal says the slot name is duplicated");
}
@Test
void nestedDuplicateFieldsInsideASlotAreNotDuplicateSlotNames(@TempDir Path dir) throws Exception {
// A duplicated field nested inside one slot's own value (here inside an ignored `extra:`
// sub-block) is not a duplicate SLOT name — it must not be rejected as one. Only the direct
// child keys of the architects mapping are slot names; whatever is deeper is the slot's
// business and must not masquerade as a duplicate slot.
Path f = dir.resolve("nested-dup-inside-slot.yaml");
Files.writeString(f, """
bind:
port: 8080
workers:
sonnet:
baseUrl: http://gx10.gw:8000
architects:
lead-designer:
profile: sonnet
extra:
a: 1
a: 1
""");
BridgedConfig cfg = assertDoesNotThrow(() -> BridgedConfig.load(f));
assertDoesNotThrow(cfg::validateArchitects,
"a nested duplicate inside a slot is not a duplicate slot and must not refuse startup");
assertEquals(Set.of("lead-designer"), cfg.architects().keySet());
assertEquals("sonnet", cfg.architects().get("lead-designer").profile());
}
@Test
void absentBrokerBlockLeavesInboxSoftState(@TempDir Path dir) throws Exception {
Path f = dir.resolve("no-broker.yaml");