mirror of
https://github.com/zvx-echo6/meshai.git
synced 2026-08-26 17:31:34 +00:00
fix(config): warn on unknown config keys instead of silently dropping them
_dict_to_dataclass() silently continue'd past any key not in the target dataclass's field set -- an operator could set a config key, restart, and have it vanish with zero feedback (config.example.yaml's phantom mesh_intelligence keys are exactly this bug, fixed separately). Now logs a WARNING naming the key and the dataclass, hinting at a typo or a renamed/removed field, via the module's existing _config_logger. Traced every dynamic/free-form config path to rule out false positives: notifications.toggles, notifications.destinations, generic_sources, mesh_sources, and notifications.rules all route through explicit dict-of-dataclass or verbatim-passthrough handling and never spuriously warn. Two legitimate legacy shapes DO hit the strict field-check path with keys that were never (and will never be) dataclass fields: - notifications.channels (pre-v0.5 channel list), consumed directly from the raw dict by _migrate_legacy_channels - notifications.region_routes.enabled (pre mt/mc-split master switch), read directly by the explicit region_routes handler Both are allowlisted in _KNOWN_LEGACY_DROP_KEYS so users mid-migration don't get spurious noise on every load. Added tests/test_config_loader.py coverage: unknown key warns and does not raise, both legacy shapes stay silent, and the free-form/dynamic sections never warn for keys valid on their real target shape.
This commit is contained in:
parent
bbe97398bc
commit
6b71b23cf3
2 changed files with 119 additions and 0 deletions
|
|
@ -1241,6 +1241,29 @@ def _migrate_legacy_channels(notifications, data: dict):
|
|||
_config_logger.info("Migrated to %d self-contained rules", len(notifications.rules))
|
||||
|
||||
|
||||
# Keys that are legitimately present in raw config dicts but intentionally have
|
||||
# NO matching dataclass field on the target class -- they're consumed by
|
||||
# special-case logic elsewhere in _dict_to_dataclass (e.g. legacy-format
|
||||
# migration) rather than becoming a field. Warning about these would be a
|
||||
# false positive: the key isn't a typo, it's a known, still-supported legacy
|
||||
# shape. Keyed by (dataclass, key name).
|
||||
_KNOWN_LEGACY_DROP_KEYS = {
|
||||
# Pre-v0.5 notifications.channels list; migrated into self-contained
|
||||
# rules by _migrate_legacy_channels (reads straight from the raw dict,
|
||||
# not from the coerced NotificationsConfig).
|
||||
(NotificationsConfig, "channels"): (
|
||||
"legacy notifications.channels format, handled by _migrate_legacy_channels"
|
||||
),
|
||||
# Pre-region-routing-split master switch. The explicit region_routes
|
||||
# handler (in the "notifications" branch below) reads this directly via
|
||||
# rr.get("enabled", ...) as the default for mt_enabled; it never becomes
|
||||
# a RegionRouteMatrix field.
|
||||
(RegionRouteMatrix, "enabled"): (
|
||||
"legacy region_routes.enabled (pre-mt/mc split), mapped to mt_enabled"
|
||||
),
|
||||
}
|
||||
|
||||
|
||||
def _dict_to_dataclass(cls, data: dict):
|
||||
"""Recursively convert dict to dataclass, handling nested structures."""
|
||||
if data is None:
|
||||
|
|
@ -1253,6 +1276,13 @@ def _dict_to_dataclass(cls, data: dict):
|
|||
if key.startswith("_"):
|
||||
continue
|
||||
if key not in field_types:
|
||||
if (cls, key) not in _KNOWN_LEGACY_DROP_KEYS:
|
||||
_config_logger.warning(
|
||||
"Config key '%s' is not a recognized field on %s -- it will "
|
||||
"be IGNORED (dropped) on load. Check for a typo, or a "
|
||||
"renamed/removed field.",
|
||||
key, cls.__name__,
|
||||
)
|
||||
continue
|
||||
|
||||
field_type = field_types[key]
|
||||
|
|
|
|||
|
|
@ -6,10 +6,14 @@ cfg.notifications.rules as raw dicts (which crashed Dispatcher._matching_rules
|
|||
on rule.enabled). config_loader.load_config uses this same _dict_to_dataclass.
|
||||
"""
|
||||
|
||||
import logging
|
||||
|
||||
from meshai.config import (
|
||||
Config,
|
||||
MeshIntelligenceConfig,
|
||||
NotificationRuleConfig,
|
||||
NotificationToggle,
|
||||
RegionRouteMatrix,
|
||||
_dataclass_to_dict,
|
||||
_dict_to_dataclass,
|
||||
)
|
||||
|
|
@ -84,3 +88,88 @@ def test_toggle_meshcore_channel_name_round_trips():
|
|||
# Default stays None when unset.
|
||||
default = _dict_to_dataclass(NotificationToggle, {"name": "weather"})
|
||||
assert default.meshcore_channel is None
|
||||
|
||||
|
||||
def test_unknown_key_warns_but_does_not_raise(caplog):
|
||||
"""An unrecognized config key (typo, or a renamed/removed field) logs a
|
||||
WARNING naming the key and the dataclass, and the key is silently dropped
|
||||
(never raises). Regression guard for the silent-drop bug: previously an
|
||||
operator could set a bogus/typo'd key, restart, and get zero feedback."""
|
||||
data = {
|
||||
"mesh_intelligence": {
|
||||
"enabled": True,
|
||||
"region_radius_miles": 40.0, # never a real field -- see config.example.yaml fix
|
||||
}
|
||||
}
|
||||
with caplog.at_level(logging.WARNING, logger="meshai.config"):
|
||||
cfg = _dict_to_dataclass(Config, data)
|
||||
|
||||
assert cfg.mesh_intelligence.enabled is True
|
||||
assert not hasattr(cfg.mesh_intelligence, "region_radius_miles")
|
||||
warnings = [r for r in caplog.records if r.levelno == logging.WARNING]
|
||||
assert len(warnings) == 1
|
||||
msg = warnings[0].getMessage()
|
||||
assert "region_radius_miles" in msg
|
||||
assert "MeshIntelligenceConfig" in msg
|
||||
|
||||
|
||||
def test_legacy_region_routes_enabled_does_not_warn(caplog):
|
||||
"""region_routes.enabled is a pre-mt/mc-split legacy key, still read
|
||||
directly by the explicit region_routes handler in _dict_to_dataclass.
|
||||
It intentionally has no RegionRouteMatrix field and must NOT warn --
|
||||
warning here would be a false positive for every config still using the
|
||||
pre-split single master switch."""
|
||||
data = {"notifications": {"region_routes": {"enabled": True, "cells": {}}}}
|
||||
with caplog.at_level(logging.WARNING, logger="meshai.config"):
|
||||
cfg = _dict_to_dataclass(Config, data)
|
||||
|
||||
assert cfg.notifications.region_routes.mt_enabled is True
|
||||
assert not any(
|
||||
"region_routes" in r.getMessage() or "'enabled'" in r.getMessage()
|
||||
for r in caplog.records
|
||||
)
|
||||
|
||||
|
||||
def test_legacy_notifications_channels_does_not_warn(caplog):
|
||||
"""notifications.channels (pre-v0.5 format) has no NotificationsConfig
|
||||
field -- it's consumed directly from the raw dict by
|
||||
_migrate_legacy_channels. Must not warn for users mid-migration."""
|
||||
data = {
|
||||
"notifications": {
|
||||
"channels": [{"id": "c1", "type": "mesh_broadcast", "channel_index": 0}],
|
||||
"rules": [{"name": "r1", "channel_ids": ["c1"], "categories": ["fire"]}],
|
||||
}
|
||||
}
|
||||
with caplog.at_level(logging.WARNING, logger="meshai.config"):
|
||||
cfg = _dict_to_dataclass(Config, data)
|
||||
|
||||
assert len(cfg.notifications.rules) == 1
|
||||
assert cfg.notifications.rules[0].broadcast_channel == 0
|
||||
assert not any("channels" in r.getMessage() for r in caplog.records)
|
||||
|
||||
|
||||
def test_dynamic_sections_do_not_warn(caplog):
|
||||
"""Sections that are intentionally free-form/passthrough (generic_sources)
|
||||
or use explicit dict-of-dataclass coercion (toggles, destinations) must
|
||||
never warn for keys that ARE valid on their actual target shape."""
|
||||
data = {
|
||||
"notifications": {
|
||||
"toggles": {
|
||||
"weather": {"name": "weather", "enabled": True, "min_severity": "priority"},
|
||||
},
|
||||
"destinations": {
|
||||
"d1": {"name": "d1", "type": "mesh_broadcast", "broadcast_channel": 0},
|
||||
},
|
||||
},
|
||||
"mesh_sources": [
|
||||
{"name": "mv", "type": "meshview", "url": "http://x", "enabled": True},
|
||||
],
|
||||
"generic_sources": [
|
||||
{"name": "gs1", "enabled": True, "url": "http://x", "not_a_dataclass_field": "fine"},
|
||||
],
|
||||
}
|
||||
with caplog.at_level(logging.WARNING, logger="meshai.config"):
|
||||
cfg = _dict_to_dataclass(Config, data)
|
||||
|
||||
assert caplog.records == []
|
||||
assert cfg.generic_sources[0]["not_a_dataclass_field"] == "fine"
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue