Skip to content

test(config): cover enabled plugin toolset discovery during migration - #86233

Open
francip wants to merge 1 commit into
NousResearch:mainfrom
kortexa-ai:kortexa-ai/fix-config-plugin-toolset-validation
Open

francip wants to merge 1 commit into
NousResearch:mainfrom
kortexa-ai:kortexa-ai/fix-config-plugin-toolset-validation

Conversation

@francip

@francip francip commented Aug 14, 2026 •

Copy link
Copy Markdown

Config migration should recognize a toolset contributed by an enabled plugin before warning that the saved selection is invalid. Upstream commit 361dc5cf69fb9b2677e8b2c1e1ac95a92e40cfd1 now provides the runtime behavior, so this rebased PR retains only regression coverage for #86231.

The test creates a real temporary plugin through ctx.register_tool, runs migration against its saved toolset selection, and verifies that the plugin name is accepted while an unrelated unknown name still produces a warning. It lives in a dedicated test module to respect the current code-health file limit.

Validation on Python 3.14.7: the four config, plugin-validation, toolset-validation and doctor modules pass (257 passed, 4 platform skips); python scripts/check passes all 11 checks. Rebased onto dce1e9b37581dd62e480a9064dc04a709c2940d3.

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cli CLI entry point, hermes_cli/, setup wizard comp/plugins Plugin system and bundled plugins area/config Config system, migrations, profiles sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Aug 14, 2026
@francip
francip marked this pull request as ready for review August 14, 2026 17:55
@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference, author can ignore or act on any point.

fix(config): validate plugin toolsets after discovery

  1. The retry only runs when the first pass produced warnings; a plugin-declared toolset will warn in pass 1 (unknown toolset), so the trigger is correct. One edge: ts_warnings from pass 2 fully replaces pass-1 output, so any pass-1-only warning (unrelated to plugins) would be dropped if pass 2 yields none. Validation is deterministic, so pass 2 would normally reproduce it — but merging both passes' warnings would be more robust than overwriting.
  2. discover_plugins() failure is swallowed at debug level and pass-1 warnings are kept — good fail-safe. The retry only runs once, which is fine for idempotent discovery.
  3. The test mutates the process-global TOOLSETS dict and restores it in finally. If discovery in migrate_config ever registers the plugin toolset via its own path, the TOOLSETS.pop could remove a toolset another test registered — currently safe, just a fragility note for future tests in the same file.
  4. Minor: raw_platform_toolsets is read once before the retry; nothing in migrate_config writes config between the passes, so this is only a note.

@francip
francip force-pushed the kortexa-ai/fix-config-plugin-toolset-validation branch 3 times, most recently from 780fe7f to 0e7c3e8 Compare August 31, 2026 15:56
@francip
francip force-pushed the kortexa-ai/fix-config-plugin-toolset-validation branch from 0e7c3e8 to 7c47675 Compare September 8, 2026 05:16
@francip
francip force-pushed the kortexa-ai/fix-config-plugin-toolset-validation branch 4 times, most recently from 2310b3a to 6d2039a Compare September 14, 2026 16:52
@btorbich

Copy link
Copy Markdown

Independent confirmation of the cold-registry bug addressed here. An enabled custom plugin toolset was reported as unknown during configuration validation even though hermes tools list showed it enabled and its tool was available at runtime.

In a fresh process, validate_toolset(name) returned False before discover_plugins() and True afterward.

I independently tested a minimal local fix that calls the existing idempotent discover_plugins() before _warn_invalid_platform_toolsets() validates the configuration. Real subprocess regression tests used temporary HERMES_HOME directories and actual plugin registration, with enabled/disabled cases and a genuinely unknown-name control:

  • Before the fix: enabled-plugin case failed; disabled-plugin case passed.
  • After the fix: both cases passed, with unknown/disabled toolset warnings preserved.
  • On upstream base a982d2c882, the existing toolset-validation tests plus these local regression cases passed: 14 passed, 0 failed.

These results validate the root cause and discovery-before-validation approach, not this PR exact head; I have not run this PR branch or completed the full repository suite. The retry-on-warning and discovery-failure handling here are more complete than my minimal local patch. Posting the evidence here instead of opening a duplicate PR.

@francip francip changed the title fix(config): validate plugin toolsets after discovery test(config): cover enabled plugin toolset discovery during migration Oct 10, 2026
@francip
francip force-pushed the kortexa-ai/fix-config-plugin-toolset-validation branch from a7511e8 to a5faa8c Compare October 10, 2026 05:35

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

area/config Config system, migrations, profiles comp/cli CLI entry point, hermes_cli/, setup wizard comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants