Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends the tool registry to carry a per-tool “diagnostic hint” and surfaces that hint in hermes doctor output when a toolset is unavailable but has no missing env vars to report—specifically to improve setup guidance for image_gen.
Changes:
- Add
diagnostic_hintsupport toToolEntry+ToolRegistry.register(), and propagate it throughcheck_tool_availability(). - Attach an
image_gendiagnostic hint to the image generation tool registration. - Update
hermes_cli.doctor.run_doctor()to displaydiagnostic_hintwhenenv_varsis empty, and add tests for registry + doctor behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
tools/registry.py |
Adds diagnostic_hint field and includes it in unavailable toolset info returned by check_tool_availability(). |
tools/image_generation_tool.py |
Sets a diagnostic hint for the image_gen toolset via registry registration. |
hermes_cli/doctor.py |
Displays diagnostic_hint for unavailable toolsets when no env var guidance is available. |
tests/tools/test_registry.py |
Verifies diagnostic_hint storage/defaulting and inclusion/omission in check_tool_availability() output. |
tests/hermes_cli/test_doctor.py |
Verifies doctor prints diagnostic_hint when present and falls back otherwise. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ec530d6 to
ecd954b
Compare
|
I filed #9516 and traced the regression to the loss of actionable image_gen diagnostic metadata after managed Nous image generation support landed. This PR takes the registry-level diagnostic_hint approach so doctor stays generic instead of adding an image_gen-only special case in hermes_cli/doctor.py. I also pushed an additional regression test after the Copilot review:
That test verifies the missing-fal_client case does not hit this new diagnostic_hint path at all, because tools.image_generation_tool fails import and is skipped during discovery. So the diagnostic_hint remains specific to the setup/auth case this PR is intended to fix. Local verification: Result: |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating the doctor fallback and preserving the generic system-dependency path.
Problems
- Current image generation is no longer limited to FAL or managed Nous access:
tools/image_generation_tool.py:1086-1125probes registered providers too, while:1079-1081directs users to alternative providers. The proposed FAL/Nous-only hint would be incomplete. - The new discovery test assumes blocking
fal_clientprevents image-tool registration. Current main deliberately lazy-loads that dependency attools/image_generation_tool.py:31-56, then registersimage_generateat:1670-1680; this assertion must be replaced during salvage. - The registry availability mechanism was refactored to aggregate per-tool checks in
6e84257717f7ef43e642ec3c26a346955debb4cb, so the old registry hunk cannot be applied unchanged.
Suggested changes
- Port the diagnostic metadata through the current registry API and use provider-neutral setup guidance (or derive it from active provider state).
- Cover the actual registry-to-doctor path and the current lazy-import behavior.
Automated hermes-sweeper review.
| requires_env=[], | ||
| is_async=False, # Switched to sync fal_client API to fix "Event loop is closed" in gateway | ||
| emoji="🎨", | ||
| diagnostic_hint="missing FAL_KEY or Nous auth", |
There was a problem hiding this comment.
Current main also supports registered image providers (tools/image_generation_tool.py:1086-1125), so this FAL/Nous-only text is incomplete. Please use provider-neutral guidance or derive the hint from the configured provider state.
| check=True, | ||
| ) | ||
|
|
||
| payload = json.loads(result.stdout.strip().splitlines()[-1]) |
There was a problem hiding this comment.
This expectation is stale on current main: fal_client is lazy-loaded at call time (tools/image_generation_tool.py:31-56), while image_generate remains registered at module import (:1670-1680). Replace this with coverage of the lazy-import contract.
What does this PR do?
Fixes the misleading hermes doctor output reported in #9516.
Today, image_gen falls back to (system dependency not met) when the real blocker is configuration/authentication. After managed Nous image generation support was added, image_generate no longer exposed a simple requires_env=["FAL_KEY"], so doctor lost the information it needed to show an actionable message.
This PR introduces a reusable registry-level diagnostic_hint field so toolsets with multiple setup/auth paths can surface a user-facing hint without hard-coding special cases in hermes_cli/doctor.py.
For image_gen, doctor now reports an actionable setup/auth hint instead of a "system dependency" failure, while still preserving the generic fallback for toolsets that truly are missing a system dependency.
Related Issue
Fixes #9516
Type of Change
Changes Made
Files changed:
How to Test
source venv/bin/activate && python -m pytest tests/hermes_cli/test_doctor.py tests/tools/test_registry.py tests/hermes_cli/test_nous_subscription.py -q
Notes
This approach keeps doctor generic and reusable rather than introducing an image_gen-only hint map in doctor itself.