Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
cpatulea wants to merge 1 commit into
base: master
Choose a base branch
from
+2 −3
Open

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions meshtastic/node.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,11 +170,10 @@ def requestConfig(self, configType):
p.get_config_request = configType

else:
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.
else:
p.get_module_config_request = msgIndex
p.get_module_config_request = configType.index
Comment on lines 175 to +176
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
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.

self._sendAdmin(p, wantResponse=True, onResponse=onResponse)
if onResponse:
Expand Down
Loading