Skip to content

Fix configuration field-name resolution and reject invalid YAML imports - #995

Open
ftc2 wants to merge 1 commit into
meshtastic:masterfrom
ftc2:config-field-resolution
Open

ftc2 wants to merge 1 commit into
meshtastic:masterfrom
ftc2:config-field-resolution

Conversation

@ftc2

@ftc2 ftc2 commented Oct 16, 2026 •

Copy link
Copy Markdown

Exporting a configuration with the 12-hour clock enabled produces display.use12hClock: true. Reimporting that YAML currently converts the name to use12h_clock instead of use_12h_clock, silently omitting the setting.

Resolve configuration names using protobuf descriptors, accepting both canonical field names and JSON names while preserving previously accepted spellings through the existing conversion fallback. Share this lookup across YAML imports, --set, --get, and --ch-set. Also correct nested setter traversal, make assignment confirmations consistent, and preserve the original query in unknown-field errors.

YAML imports now reject unknown top-level keys, unknown configuration fields, and failed assignments before starting a settings transaction. Existing aliases remain supported. Profiles containing obsolete fields or extra metadata will now fail instead of silently skipping those entries. Update both shipped examples and add tests that import their actual contents.

Validation:

  • 224 focused tests passed, covering profile round trips, field-name aliases, CLI reads and writes, shipped examples, and import failures. The four clock round-trip regressions fail on the upstream base.
  • Upstream mypy and Pylint checks pass with Python 3.14.8 and locked dependencies.
  • One existing tunnel test was excluded because of a leaking reader thread; full-suite validation remains incomplete locally.
  • Verified clock imports using both spellings, export/reimport, and persistence after reboot on a LILYGO T-Beam S3 Core reporting firmware 2.8.2.a2a27f4.

This change does not address existing value-conversion failures or failures encountered while writing otherwise valid profiles to the device.

Summary by CodeRabbit

  • Configuration
    • Updated the example configuration with a US region and an always-on screen setting. The mesh SDS timeout setting is no longer included.
    • Configuration fields can be entered using protobuf field names, JSON names, or legacy camel-case names.
  • Bug Fixes
    • Invalid configuration fields and profile sections now produce clear errors before settings are written.
    • Nested configuration values are resolved and validated consistently across profile import and CLI commands.

Accept canonical protobuf field names and JSON names while retaining legacy
camel-to-snake lookup compatibility. Fix the use12hClock profile round trip,
share name resolution with CLI reads and writes, and keep nested assignments
on the current submessage.

Reject unknown YAML profile keys and failed assignments before starting
configuration writes. Preserve supported aliases, update shipped examples,
and provide consistent confirmations and original-input lookup errors.

Add offline regression coverage for imports, exports, CLI field aliases,
examples, nested paths, channel settings, and validation failures.
@CLAassistant

CLAassistant commented Oct 16, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@coderabbitai

coderabbitai Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: meshtastic/python/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: eb64feb9-a7d1-40ab-b670-a6bff6ad0d82
📥 Commits

Reviewing files that changed from the base of the PR and between 0a18357 and dbc3c08.

📒 Files selected for processing: 5
  • exampleConfig.yaml
  • example_config.yaml
  • meshtastic/__main__.py
  • meshtastic/tests/test_config_profile.py
  • meshtastic/tests/test_main.py
💤 Files with no reviewable changes: 1
  • example_config.yaml

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

📝 Walkthrough

The CLI now resolves configuration fields through protobuf descriptors, accepts protobuf, JSON, and legacy camel-case names, and validates nested assignments and YAML profile sections. Example configurations and offline tests cover aliases, profile loading, round-trips, and invalid input.

Changes: Configuration field resolution and YAML profiles

Layer / File(s) Summary
Resolve and set configuration fields
meshtastic/__main__.py, meshtastic/tests/test_config_profile.py, meshtastic/tests/test_main.py
Configuration lookup and assignment resolve protobuf, JSON, and legacy camel-case field names. Nested paths are validated, and CLI output uses resolved field names. Tests cover local and module configuration, --set, and --get behavior.
Validate and load YAML profiles
meshtastic/__main__.py, exampleConfig.yaml, example_config.yaml, meshtastic/tests/test_config_profile.py
YAML profile loading rejects unsupported top-level keys and invalid configuration sections. Example configurations and tests cover profile loading, alias handling, round-trips, and invalid input.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to dbc3c

This change makes configuration field lookup more consistent and rejects invalid YAML imports before any settings are written. Profiles that contain obsolete fields or extra metadata will now fail instead of being silently skipped, which is intended. No merge-blocking issue was found.

Pre-merge checks: Passed 5
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main changes: improved configuration field-name resolution and rejection of invalid YAML imports.
Docstring Coverage Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 26 functions across 3 files. (1 skipped: 1…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants