Fix '--get security' (incorrect AdminMessage.ConfigType value). - #907
Fix '--get security' (incorrect AdminMessage.ConfigType value).#907ianmcorvidae merged 1 commit into
Conversation
Sorry, something went wrong.
Codecov Report❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.
@@ Coverage Diff @@
## master #907 +/- ##
==========================================
- Coverage 59.82% 59.81% -0.01%
==========================================
Files 24 24
Lines 4329 4328 -1
==========================================
- Hits 2590 2589 -1
Misses 1739 1739
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry.
|
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
Fixes an incorrect --get security behavior in the Meshtastic Python CLI/library by ensuring Node.requestConfig() maps LocalConfig fields to AdminMessage.ConfigType by enum name rather than relying on protobuf field ordering.
Changes:
- Update Node.requestConfig() to derive AdminMessage.ConfigType from the LocalConfig field name (fixing security mapping).
- Keep module config requests using the prior index-based behavior.
Sorry, something went wrong.
| msgIndex = configType.index | ||
| if configType.containing_type.name == "LocalConfig": | ||
| p.get_config_request = msgIndex | ||
| p.get_config_request = admin_pb2.AdminMessage.ConfigType.Value(configType.name.upper() + "_CONFIG") |
Copilot
AI
Feb 27, 2026
There was a problem hiding this comment.
AdminMessage.ConfigType.Value(configType.name.upper() + "_CONFIG") will raise ValueError if a LocalConfig field doesn’t have a corresponding ConfigType enum (e.g. LocalConfig.version). Because requestConfig() is reachable from the CLI via --get <field>, this becomes an uncaught exception for a user typo/unsupported field. Consider catching the error (or checking membership in the enum) and exiting with a clear message (or explicitly rejecting version).
| p.get_config_request = admin_pb2.AdminMessage.ConfigType.Value(configType.name.upper() + "_CONFIG") | |
| enum_name = configType.name.upper() + "_CONFIG" | |
| # Validate that the constructed enum name exists to avoid ValueError from .Value() | |
| if enum_name not in admin_pb2.AdminMessage.ConfigType.keys(): | |
| our_exit( | |
| f"Unsupported configuration field '{configType.name}' cannot be requested.", | |
| 1, | |
| ) | |
| p.get_config_request = admin_pb2.AdminMessage.ConfigType.Value(enum_name) |
Sorry, something went wrong.
There was a problem hiding this comment.
I think the current behavior is good. Any future changes should be made together on LocalConfig and ConfigType. If there's a mismatch, this should raise an exception, to alert the developers (or the users in the worst case) that there's a bug.
Sorry, something went wrong.
requestConfig was assuming that the order of fields in meshtastic.LocalConfig matches the order of enum values in AdminMessage.ConfigType. This is true for 'device', 'position', etc. but is NOT true for 'security' due to the intervening 'version' field. Look up LocalConfig fields by name, not index, to prevent this error in the future. LocalConfig.security was introduced in meshtastic/protobufs#553
ianmcorvidae
left a comment
There was a problem hiding this comment.
This looks good to me. Sorry to have missed the PR flying past in my email 😖
I'll get this merged in so it can go out with the next release, which should be soon since there's a separate fix for protobuf versions >= 6.31.0 in the works
Sorry, something went wrong.
requestConfig was assuming that the order of fields in meshtastic.LocalConfig matches the order of enum values in AdminMessage.ConfigType. This is true for 'device', 'position', etc. but is NOT true for 'security' due to the intervening 'version' field.
Look up config fields by name, not index, to prevent this error in the future.
LocalConfig.security was introduced in meshtastic/protobufs#553