fix(config): distinguish disabled-plugin toolsets from typos in validation - #56731
fix(config): distinguish disabled-plugin toolsets from typos in validation#56731dante32683 wants to merge 2 commits into
Conversation
|
FYI for whoever reviews this, the linked mirror hashbender#601 (auto-backlinked above) carries this same fix plus one unrelated hunk in |
|
Filed #59547 for this with more background on the disabled-plugin-vs-typo confusion, in case that's useful context for review. |
|
Thanks for the focused fix. The current implementation still has the reported gap: Problems
Suggested changes
This is an automated hermes-sweeper review. |
…ation validate_platform_toolsets() (NousResearch#38798) already warns when a platform_toolsets entry is unknown, with a `hermes-<platform>` typo guess. That guess is wrong for the common case where the name is a real plugin toolset whose plugin is currently disabled or uninstalled — the three toolset-tracking lists (plugins.enabled, known_plugin_toolsets, platform_toolsets) can drift out of sync with no cross-check between them. known_plugin_toolsets (hermes_cli/tools_config.py's _save_platform_tools) already records, per platform, which plugin toolset keys were valid the last time `hermes tools` saved that platform. Cross-referencing it lets validate_platform_toolsets tell "this was a real plugin toolset, now unavailable" apart from "this was never a valid name," and point at the actual likely cause (plugins.enabled / installation) instead of a misleading typo suggestion. known_plugin_toolsets is a new optional third argument, threaded through from config.py's existing validate_platform_toolsets call site; omitting it preserves the prior generic-warning behavior.
Address review: the helper branches were covered but the production wiring at hermes_cli/config.py was not, so dropping the known_plugin_toolsets argument would silently restore the generic warning. Add temp-HERMES_HOME migrate_config() tests asserting the plugin-specific warning when the map is supplied, and the unknown-toolset warning when it is not.
da2542c to
0048883
Compare
|
Thanks — addressed in 0048883 (rebased onto current main; the propagation now sits at Added I verified the test actually guards the wiring: dropping the |
What changed
validate_platform_toolsets()(hermes_cli/toolset_validation.py, added for #38798) now accepts an optional third argument,known_plugin_toolsets, and cross-references it against unknown toolset names to produce a more accurate warning when the real cause is a disabled/uninstalled plugin rather than a typo.Why
Hermes tracks toolset names across three separate places:
plugins.enabled— which plugins are turned onknown_plugin_toolsets— per-platform snapshot of plugin toolset keys seen the last timehermes toolssaved that platform (hermes_cli/tools_config.py::_save_platform_tools)platform_toolsets— the actual per-platform toolset selectionvalidate_platform_toolsetsalready does excellent work catching corrupted/renamed entries (the #38798 incident:hermes-clisilently rewritten to the nonexistenthermes), including ahermes-<platform>"did you mean" guess. But that guess is actively misleading for a very common real-world case: aplatform_toolsetsentry that names a real plugin toolset whose plugin is now disabled (or its package uninstalled). Sinceis_valid_toolset(backed by the live toolset registry) only knows about currently-loaded plugins, a disabled plugin's toolset name looks identical to a typo to the existing check — same generic "unknown toolset ''" warning, with nohermes-<platform>hint either (since that guess doesn't happen to match), leaving the real cause unstated.known_plugin_toolsetsalready has exactly the information needed to tell these apart: if a name is inknown_plugin_toolsets[platform], it was a real, valid plugin toolset the last timehermes toolsran for that platform — so its current invalidity almost certainly means the plugin got disabled or uninstalled, not that the name was ever wrong.Approach
known_plugin_toolsetsparam, defaultNone(or any non-dict) → identical behavior to before this change, so existing callers/tests aren't broken.is_valid_toolsetAND is found inknown_plugin_toolsets[platform], emit a distinct, more actionable warning pointing atplugins.enabledand plugin installation, instead of the generic unknown-toolset message with its (here, irrelevant)hermes-<platform>guess.hermes_cli/config.py(read_raw_config().get("known_plugin_toolsets")), reusing the same try/except-wrapped, best-effort validation pass that already runs after every config migration.Test plan
tests/hermes_cli/test_toolset_validation.pywith:known_plugin_toolsets(None,[], a string,{}with a non-list value) all falling back to the pre-existing generic behaviorscripts/run_tests.sh tests/hermes_cli/test_toolset_validation.py tests/hermes_cli/test_config.py— 153 tests, all passing, no regressions in the config-migration suite that exercises this call site.