Repository navigation
fix: heal null location values when merging message user profiles - #565
Merged
NeonDaniel merged 5 commits intoAug 18, 2026
Merged
Conversation
Contributor
Author
|
#566 should address the failing tests |
NeonDaniel
pushed a commit
that referenced
this pull request
Aug 7, 2026
## Summary
The `unit_tests` workflow fails on `dev` at the Test Parse Utils step,
on Python 3.10, 3.11, and 3.12. The failed test is
`tests/parse_util_tests.py::ParseUtilTests::test_get_phonemes`.
The cause is a change in nltk 3.10, not a change in this repository.
nltk added `nltk/pathsec.py`, which authorizes a download target against
the directories in `nltk.data.path`. `get_phonemes`
(`neon_utils/parse_utils.py:115`) registered its download directory one
line after the download call:
```python
nltk.download('cmudict', download_dir=download_path)
nltk.data.path.append(download_path)
```
The download ran against a path that was not yet registered, so nltk
refused it:
```
[nltk_data] Error downloading 'cmudict' ... Security Violation
[nltk_data] [Downloader._download_package]: Unauthorized path
[nltk_data] /home/runner/.local/share/neon/corpora/cmudict.zip.tmp
```
`cmudict` did not install. The corpus lookup then raised `LookupError`.
`requirements/requirements.txt` pins `nltk~=3.5`, which permits 3.10.2.
The last `dev` run that passed was 2026-06-08, before this nltk release.
## Change
Register the download path before the download. The `in` test prevents
duplicate entries in `nltk.data.path` on repeat calls.
## Test plan
- Reproduced the fault alone: `nltk.download('cmudict',
download_dir=...)` returns `False` with the security violation above
when the directory is absent from `nltk.data.path`. It returns `True`
when the directory is present.
- Deleted the local corpus cache to get the same cold-cache state as CI.
`test_get_phonemes` fails before this change and passes after it.
- `tests/parse_util_tests.py`: 15 passed from a cold cache.
## Note
This change is independent of #565. #565 starts from the same commit and
shows the same CI fault, which this change corrects.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
NeonDaniel
reviewed
Aug 7, 2026
Member
Merged that one 🙂 This PR needs a rebase now |
mikejgray
force-pushed
the
FIX_HealNullProfileLocation
branch
from
August 7, 2026 17:21
290ceed to
6157088
Compare
`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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
mikejgray
force-pushed
the
FIX_HealNullProfileLocation
branch
from
August 18, 2026 21:33
ba8609e to
7f3ca9d
Compare
NeonDaniel
approved these changes
Aug 18, 2026
NeonDaniel
left a comment
NeonDaniel
left a comment
Member
There was a problem hiding this comment.
Looks good; tests indicate backwards-compat. with all cases previously covered
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
get_user_prefsmerges an inbound message profile against the defaults withdict_update_keys(neon_utils/user_utils.py:107). That helper fills absent keys only (configuration_utils.py:757). A key that is present with the valueNonenever inherits a default.Clients that serialize a full profile skeleton send
location: {"lat": None, "lng": None, ...}. These nulls mask the configured location of the core for every message. Skills that need a location then fail.skill-fallback_wolfram_alphaserializeslat=Noneto the string"None"and gets a 422 from the/proxy/wolframalphaendpoint.This change applies the null-coalescing that
get_user_config_from_mycroft_confalready uses (configuration_utils.py:797-810) after the merge, scoped tolocation.Why here and not in
dict_update_keysThree skill callers use
dict_update_keys:skills/neon_skill.py:646,skills/mycroft_skill.py:88, andskills/neon_fallback_skill.py:537. Each one mergessettings.jsonagainst the settingsmeta defaults and writes the result to disk. If the helper replacedNone, a skill setting that a user set to null on purpose would be overwritten on disk.dict_make_equal_keys(configuration_utils.py:716) also sends thetts,stt,hotwords, andlanguagesubtrees through the same helper. In those sections,Nonemeans "use the plugin default". The change is therefore scoped to the one call site in the message path.Why not call
get_user_config_from_mycroft_confdirectlyThat function assigns into
user_config["speech"]anduser_config["units"]without a guard, so a partial profile raisesKeyError. It also overwrites the speech and unit preferences of the user from the core config on every message. This change reuses the idiom of that function, not the function.locationis the only block indefault_user_conf.ymlthat usesNoneas the unset marker. All other fields use''or{}. Insidelocation, this change cannot tell a deliberateNonefrom an unset key, and treats both as unset.Test plan
Four new tests in
tests/user_util_tests.py:test_get_user_prefs_heals_null_location— a profile with alllocationvalues set toNoneinherits the configured location. This test fails without the source change.test_get_user_prefs_keeps_configured_location— a profile with a real location keeps it.test_get_user_prefs_keeps_null_outside_location— a null outsidelocation(user.email) stays null. This test fails if the change applies to all sections.test_dict_update_keys_preserves_explicit_null— a skill setting set to null does not inherit the metadata default.Results:
tests/user_util_tests.py: 10 passed.tests/configuration_util_tests.py: 50 passed, 4 failed. The 4 failures also occur on cleandev:test_make_loaded_config_safe,test_migrate_ngi_config,test_added_module_config, andtest_simultaneous_config_updates.This change is not yet tested against a live hub.
Replaces NeonGeckoCom/neon-hana#59, which corrected this on the client side.
🤖 Generated with Claude Code