From be8893a6e0a68ae9e9edea1d414f789ae81f825a Mon Sep 17 00:00:00 2001 From: Mike Date: Thu, 6 Aug 2026 18:51:09 -0500 Subject: [PATCH 1/5] fix: heal null location values when merging message user profiles `dict_update_keys` fills absent keys only, so a profile that carries location keys explicitly set to null can never inherit the configured default. Clients that serialize a full profile skeleton send `location: {"lat": None, ...}`, which permanently masks the core's configured location and breaks skills that require one. Apply the same null-coalescing `get_user_config_from_mycroft_conf` uses, scoped to the merge in `get_user_prefs`. `dict_update_keys` itself is left unchanged: its skill-settings callers persist merged results to disk, where replacing null would overwrite deliberately-cleared values. Co-Authored-By: Claude Opus 5 (1M context) --- neon_utils/user_utils.py | 9 ++++++- tests/user_util_tests.py | 54 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 1 deletion(-) diff --git a/neon_utils/user_utils.py b/neon_utils/user_utils.py index 03a62fa5..a2bdf155 100644 --- a/neon_utils/user_utils.py +++ b/neon_utils/user_utils.py @@ -104,7 +104,14 @@ def get_user_prefs(message: Message = None) -> dict: for profile in message.context.get(profile_key): try: if profile["user"]["username"] == username: - return dict(dict_update_keys(profile, default_user_config)) + merged = dict(dict_update_keys(profile, default_user_config)) + # `dict_update_keys` fills absent keys only, so location keys + # sent explicitly as null never inherit the configured default + default_location = default_user_config.get("location") or {} + location = merged.setdefault("location", {}) + for key, value in default_location.items(): + location[key] = location.get(key) or value + return merged except KeyError: LOG.error(f"Malformed profile in message context: {profile}") LOG.warning(f"No preferences found for {username} in {message.context}") diff --git a/tests/user_util_tests.py b/tests/user_util_tests.py index 196ef9c0..fdf6b944 100644 --- a/tests/user_util_tests.py +++ b/tests/user_util_tests.py @@ -228,6 +228,60 @@ def test_get_default_user_config(self): {'lat', 'lng', 'city', 'state', 'country', 'tz', 'utc'}) + @patch("ovos_config.config.Configuration") + def test_get_user_prefs_heals_null_location(self, config): + from ovos_config.models import LocalConf + test_config_dir = os.path.join(os.path.dirname(__file__), + "user_util_test_config") + config.return_value = LocalConf(join(test_config_dir, "mycroft", + "mycroft.conf")) + import importlib + from neon_utils import user_utils + importlib.reload(user_utils) + from neon_utils.user_utils import get_user_prefs + + null_location_profile = { + "user": {"username": "null_location_user"}, + "location": {"lat": None, "lng": None, "city": None, + "state": None, "country": None, "tz": None, + "utc": None}} + prefs = get_user_prefs(Message("test_message", {}, { + "username": "null_location_user", + "user_profiles": [null_location_profile]})) + self.assertEqual(prefs["location"], + {"lat": '38.971669', + "lng": '-95.23525', + "tz": 'America/Chicago', + "utc": '-6.0', + "city": 'Kirkland', + "state": 'Washington', + "country": "United States"}) + + @patch("ovos_config.config.Configuration") + def test_get_user_prefs_keeps_configured_location(self, config): + from ovos_config.models import LocalConf + test_config_dir = os.path.join(os.path.dirname(__file__), + "user_util_test_config") + config.return_value = LocalConf(join(test_config_dir, "mycroft", + "mycroft.conf")) + import importlib + from neon_utils import user_utils + importlib.reload(user_utils) + from neon_utils.user_utils import get_user_prefs + + user_location_profile = { + "user": {"username": "located_user"}, + "location": {"lat": '29.4241', "lng": '-98.4936', + "city": 'San Antonio', "state": 'Texas', + "country": "United States", "tz": 'America/Chicago', + "utc": '-6.0'}} + prefs = get_user_prefs(Message("test_message", {}, { + "username": "located_user", + "user_profiles": [user_location_profile]})) + self.assertEqual(prefs["location"]["city"], 'San Antonio') + self.assertEqual(prefs["location"]["lat"], '29.4241') + self.assertEqual(prefs["location"]["state"], 'Texas') + if __name__ == '__main__': unittest.main() From 8db7cea5dea083681f13cde465030ffd517c9986 Mon Sep 17 00:00:00 2001 From: Mike Date: Thu, 6 Aug 2026 18:54:48 -0500 Subject: [PATCH 2/5] test: cover null preservation outside location and in skill settings Adds the cases that guard the scoping decision: a null outside `location` is left alone, and `dict_update_keys` still preserves a deliberately nulled skill setting rather than inheriting the metadata default. Co-Authored-By: Claude Opus 5 (1M context) --- tests/user_util_tests.py | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/tests/user_util_tests.py b/tests/user_util_tests.py index fdf6b944..e9985c27 100644 --- a/tests/user_util_tests.py +++ b/tests/user_util_tests.py @@ -282,6 +282,39 @@ def test_get_user_prefs_keeps_configured_location(self, config): self.assertEqual(prefs["location"]["lat"], '29.4241') self.assertEqual(prefs["location"]["state"], 'Texas') + @patch("ovos_config.config.Configuration") + def test_get_user_prefs_keeps_null_outside_location(self, config): + from ovos_config.models import LocalConf + test_config_dir = os.path.join(os.path.dirname(__file__), + "user_util_test_config") + config.return_value = LocalConf(join(test_config_dir, "mycroft", + "mycroft.conf")) + import importlib + from neon_utils import user_utils + importlib.reload(user_utils) + from neon_utils.user_utils import get_user_prefs + + # Healing is scoped to `location`; a null elsewhere is left alone + null_email_profile = {"user": {"username": "null_email_user", + "email": None}} + prefs = get_user_prefs(Message("test_message", {}, { + "username": "null_email_user", + "user_profiles": [null_email_profile]})) + self.assertIsNone(prefs["user"]["email"]) + + def test_dict_update_keys_preserves_explicit_null(self): + from neon_utils.configuration_utils import dict_update_keys + + # Skill settings merge through this helper and persist to disk, so a + # deliberately-nulled setting must not inherit the metadata default + settings = {"api_key": None, "endpoint": "https://custom"} + merged = dict_update_keys(settings, {"api_key": "DEFAULT_KEY", + "endpoint": "https://default", + "added_key": "added_value"}) + self.assertIsNone(merged["api_key"]) + self.assertEqual(merged["endpoint"], "https://custom") + self.assertEqual(merged["added_key"], "added_value") + if __name__ == '__main__': unittest.main() From 98d4145e500e1285e30c8ba3c75294a2420e1a25 Mon Sep 17 00:00:00 2001 From: Mike Date: Fri, 7 Aug 2026 12:26:28 -0500 Subject: [PATCH 3/5] fix: heal null profile values for all keys, not just location Scoping the null-coalescing to `location` fixed only the case that surfaced first. `units` and `speech` carry real defaults that a profile serializing them as null masks in exactly the same way, so a client sending a full skeleton loses the configured measure, date format, and languages too. Drop null-valued keys from the profile before the merge and let `dict_update_keys` fill them, which is equivalent to coalescing after the merge but removes the special case. `dict_update_keys` itself is still unchanged: its skill-settings callers persist merged results to disk, where replacing null would overwrite deliberately-cleared values. Note the profile is modified in place rather than copied. `get_user_prefs` back-fills the message context profile with merged defaults, and callers depend on that. Co-Authored-By: Claude Opus 5 (1M context) --- neon_utils/user_utils.py | 28 ++++++++++++++++------- tests/user_util_tests.py | 48 ++++++++++++++++++++++++++++++++++------ 2 files changed, 61 insertions(+), 15 deletions(-) diff --git a/neon_utils/user_utils.py b/neon_utils/user_utils.py index a2bdf155..6edcb0fa 100644 --- a/neon_utils/user_utils.py +++ b/neon_utils/user_utils.py @@ -26,6 +26,7 @@ # NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS # SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. +from collections.abc import MutableMapping from os.path import isfile, join from ovos_bus_client import Message, MessageBusClient from ovos_utils.log import LOG @@ -72,6 +73,21 @@ def apply_local_user_profile_updates(updated_profile: dict, _DEFAULT_USER_CONFIG = user_config.content +def _drop_null_values(profile: MutableMapping) -> MutableMapping: + """ + Recursively remove keys with null values so a merge against default + configuration fills them, rather than treating null as a set value. + :param profile: user profile to modify and return + :returns: profile with all null-valued keys removed + """ + for key, value in list(profile.items()): + if isinstance(value, MutableMapping): + _drop_null_values(value) + elif value is None: + profile.pop(key) + return profile + + @resolve_message def get_user_prefs(message: Message = None) -> dict: """ @@ -104,14 +120,10 @@ def get_user_prefs(message: Message = None) -> dict: for profile in message.context.get(profile_key): try: if profile["user"]["username"] == username: - merged = dict(dict_update_keys(profile, default_user_config)) - # `dict_update_keys` fills absent keys only, so location keys - # sent explicitly as null never inherit the configured default - default_location = default_user_config.get("location") or {} - location = merged.setdefault("location", {}) - for key, value in default_location.items(): - location[key] = location.get(key) or value - return merged + # `dict_update_keys` fills absent keys only, so any key sent + # explicitly as null would mask the configured default + _drop_null_values(profile) + return dict(dict_update_keys(profile, default_user_config)) except KeyError: LOG.error(f"Malformed profile in message context: {profile}") LOG.warning(f"No preferences found for {username} in {message.context}") diff --git a/tests/user_util_tests.py b/tests/user_util_tests.py index e9985c27..1dd5ebb2 100644 --- a/tests/user_util_tests.py +++ b/tests/user_util_tests.py @@ -284,6 +284,36 @@ def test_get_user_prefs_keeps_configured_location(self, config): @patch("ovos_config.config.Configuration") def test_get_user_prefs_keeps_null_outside_location(self, config): + from ovos_config.models import LocalConf + test_config_dir = os.path.join(os.path.dirname(__file__), + "user_util_test_config") + config.return_value = LocalConf(join(test_config_dir, "mycroft", + "mycroft.conf")) + import importlib + from neon_utils import user_utils + importlib.reload(user_utils) + from neon_utils.user_utils import get_user_prefs, \ + get_default_user_config + + # Healing is not scoped to `location`; any null inherits its default + null_profile = {"user": {"username": "null_prefs_user", + "email": None}, + "units": {"measure": None, "date": "YMD"}, + "speech": {"tts_language": None}} + prefs = get_user_prefs(Message("test_message", {}, { + "username": "null_prefs_user", + "user_profiles": [null_profile]})) + default = get_default_user_config() + self.assertEqual(prefs["user"]["email"], default["user"]["email"]) + self.assertEqual(prefs["units"]["measure"], + default["units"]["measure"]) + self.assertEqual(prefs["speech"]["tts_language"], + default["speech"]["tts_language"]) + # A non-null value is still preferred over the default + self.assertEqual(prefs["units"]["date"], "YMD") + + @patch("ovos_config.config.Configuration") + def test_get_user_prefs_heals_null_in_message_context(self, config): from ovos_config.models import LocalConf test_config_dir = os.path.join(os.path.dirname(__file__), "user_util_test_config") @@ -294,13 +324,17 @@ def test_get_user_prefs_keeps_null_outside_location(self, config): importlib.reload(user_utils) from neon_utils.user_utils import get_user_prefs - # Healing is scoped to `location`; a null elsewhere is left alone - null_email_profile = {"user": {"username": "null_email_user", - "email": None}} - prefs = get_user_prefs(Message("test_message", {}, { - "username": "null_email_user", - "user_profiles": [null_email_profile]})) - self.assertIsNone(prefs["user"]["email"]) + # `get_user_prefs` back-fills the context profile in place, so the + # healed value must land there too rather than a null being restored + null_profile = {"user": {"username": "healed_user"}, + "units": {"measure": None}} + message = Message("test_message", {}, { + "username": "healed_user", + "user_profiles": [null_profile]}) + prefs = get_user_prefs(message) + self.assertEqual(message.context["user_profiles"][0], prefs) + self.assertIsNotNone( + message.context["user_profiles"][0]["units"]["measure"]) def test_dict_update_keys_preserves_explicit_null(self): from neon_utils.configuration_utils import dict_update_keys From d73b2eaf78700fe5482e0297e74a36d29c75145d Mon Sep 17 00:00:00 2001 From: Mike Date: Fri, 7 Aug 2026 12:28:40 -0500 Subject: [PATCH 4/5] test: rename null-healing test to match its assertion The test kept the name it had when healing was scoped to `location`, so it read as asserting the opposite of what it checks. Co-Authored-By: Claude Opus 5 (1M context) --- tests/user_util_tests.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/user_util_tests.py b/tests/user_util_tests.py index 1dd5ebb2..693ea19c 100644 --- a/tests/user_util_tests.py +++ b/tests/user_util_tests.py @@ -283,7 +283,7 @@ def test_get_user_prefs_keeps_configured_location(self, config): self.assertEqual(prefs["location"]["state"], 'Texas') @patch("ovos_config.config.Configuration") - def test_get_user_prefs_keeps_null_outside_location(self, config): + def test_get_user_prefs_heals_null_outside_location(self, config): from ovos_config.models import LocalConf test_config_dir = os.path.join(os.path.dirname(__file__), "user_util_test_config") @@ -295,7 +295,7 @@ def test_get_user_prefs_keeps_null_outside_location(self, config): from neon_utils.user_utils import get_user_prefs, \ get_default_user_config - # Healing is not scoped to `location`; any null inherits its default + # A null anywhere in the profile inherits its configured default null_profile = {"user": {"username": "null_prefs_user", "email": None}, "units": {"measure": None, "date": "YMD"}, From 7f3ca9dc492c87941689db61a826cb367f5a6785 Mon Sep 17 00:00:00 2001 From: Mike Date: Fri, 7 Aug 2026 12:32:23 -0500 Subject: [PATCH 5/5] test: cover falsy profile values that must not inherit defaults Mutation testing found a gap: replacing `value is None` with `not value` in `_drop_null_values` left the whole suite green. That mutation drops False, 0, and empty strings before the merge, so a profile with `privacy.save_audio: False` inherits the default True and silently reverts a user's opt-out. Add a test that pins False, 0, and '' as deliberate values. Also strengthen the null-healing assertions: `user.email` defaults to an empty string, so comparing against the default alone passed too easily. Co-Authored-By: Claude Opus 5 (1M context) --- tests/user_util_tests.py | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/tests/user_util_tests.py b/tests/user_util_tests.py index 693ea19c..22a767ed 100644 --- a/tests/user_util_tests.py +++ b/tests/user_util_tests.py @@ -304,14 +304,44 @@ def test_get_user_prefs_heals_null_outside_location(self, config): "username": "null_prefs_user", "user_profiles": [null_profile]})) default = get_default_user_config() - self.assertEqual(prefs["user"]["email"], default["user"]["email"]) + # These defaults are non-null, so the null cannot have survived self.assertEqual(prefs["units"]["measure"], default["units"]["measure"]) self.assertEqual(prefs["speech"]["tts_language"], default["speech"]["tts_language"]) + self.assertIsNotNone(prefs["units"]["measure"]) + self.assertIsNotNone(prefs["speech"]["tts_language"]) + # `user.email` defaults to an empty string, so assert the type changed + self.assertEqual(prefs["user"]["email"], default["user"]["email"]) + self.assertIsNotNone(prefs["user"]["email"]) # A non-null value is still preferred over the default self.assertEqual(prefs["units"]["date"], "YMD") + @patch("ovos_config.config.Configuration") + def test_get_user_prefs_keeps_falsy_values(self, config): + from ovos_config.models import LocalConf + test_config_dir = os.path.join(os.path.dirname(__file__), + "user_util_test_config") + config.return_value = LocalConf(join(test_config_dir, "mycroft", + "mycroft.conf")) + import importlib + from neon_utils import user_utils + importlib.reload(user_utils) + from neon_utils.user_utils import get_user_prefs + + # Only null is absent; False, 0, and '' are deliberate user values and + # must not inherit a truthy default (i.e. a privacy opt-out reverting) + falsy_profile = {"user": {"username": "falsy_user", "email": ""}, + "privacy": {"save_audio": False, "save_text": False}, + "units": {"time": 0}} + prefs = get_user_prefs(Message("test_message", {}, { + "username": "falsy_user", + "user_profiles": [falsy_profile]})) + self.assertFalse(prefs["privacy"]["save_audio"]) + self.assertFalse(prefs["privacy"]["save_text"]) + self.assertEqual(prefs["user"]["email"], "") + self.assertEqual(prefs["units"]["time"], 0) + @patch("ovos_config.config.Configuration") def test_get_user_prefs_heals_null_in_message_context(self, config): from ovos_config.models import LocalConf