Skip to content

Fix '--get security' (incorrect AdminMessage.ConfigType value).#907

Open
cpatulea wants to merge 1 commit intomeshtastic:masterfrom
cpatulea:get-security
Open

Fix '--get security' (incorrect AdminMessage.ConfigType value).#907
cpatulea wants to merge 1 commit intomeshtastic:masterfrom
cpatulea:get-security

Conversation

@cpatulea
Copy link

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

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
@codecov
Copy link

codecov bot commented Feb 27, 2026

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 59.81%. Comparing base (d0ccb1a) to head (918e30e).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
meshtastic/node.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            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              
Flag Coverage Δ
unittests 59.81% <50.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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")
Copy link

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
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)

Copilot uses AI. Check for mistakes.
Comment on lines 175 to +176
else:
p.get_module_config_request = msgIndex
p.get_module_config_request = configType.index
Copy link

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR description says config fields should be looked up by name (not index) to prevent future enum/field-order mismatches, but the module-config branch still uses configType.index for get_module_config_request. Either extend the name-based approach to ModuleConfigType as well (likely via an explicit mapping table, since some names differ like external_notification vs EXTNOTIF_CONFIG), or update the PR description to reflect that only LocalConfig is addressed.

Copilot uses AI. Check for mistakes.
Comment on lines 173 to +176
if configType.containing_type.name == "LocalConfig":
p.get_config_request = msgIndex
p.get_config_request = admin_pb2.AdminMessage.ConfigType.Value(configType.name.upper() + "_CONFIG")
else:
p.get_module_config_request = msgIndex
p.get_module_config_request = configType.index
Copy link

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change fixes a subtle mapping bug (LocalConfig.security vs ConfigType.SECURITY_CONFIG) but requestConfig() behavior isn’t covered by an active unit test (the existing requestConfig test in meshtastic/tests/test_node.py is commented out). Adding a unit test that asserts requesting the security field descriptor sets AdminMessage.get_config_request to SECURITY_CONFIG would prevent regressions as protobufs evolve.

Copilot generated this review using guidance from repository custom instructions.
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