Add version-based ToolHive instance selection

[ptelang]

When multiple ToolHive instances are running on different ports, the optimizer now automatically selects the one with the latest version. This ensures compatibility with newer features and bug fixes.

Changes:

• Modified _is_toolhive_available to return version along with availability status
• Updated port scanning logic to collect and compare versions using semver
• Added error handling for invalid semver strings to gracefully skip incompatible instances
• Updated all test mocks to match new tuple return signature (version, bool)
• Fixed 52 test cases across test_toolhive_client.py, test_toolhive_retry.py, and test_polling_manager.py

Closes #18

🤖 Generated with Claude Code

[claude]

Code Review

This PR adds version-based ToolHive instance selection. Implementation is solid with good test coverage.

Strengths

• Clean semver implementation with proper error handling
• Comprehensive test updates (52 cases)
• Good logging throughout

Issues

1. Code duplication (lines 154-205 and 396-428)
The async and sync versions of _scan_for_toolhive contain nearly identical version parsing logic (~30 duplicate lines). Extract into a shared helper method.

2. Inefficiency (lines 176, 406)
Version string normalization .replace(v, ) happens at different points in async vs sync. Consolidate this into the shared helper.

3. Verbosity (lines 422-423, 199-200)
Use if thv_version_port_parsed: instead of if len(thv_version_port_parsed) > 0:

4. Inconsistency
_is_toolhive_available returns tuple[str, bool] but _is_toolhive_available_async returns bool. Consider aligning both approaches.

Performance

Sequential port scanning could be slow with large ranges. Consider using asyncio.gather for concurrent scanning in the async version.

Verdict

✅ No security concerns or regressions. Recommend addressing code duplication before merge.

[aponcedeleonch]

@claude check the changes in the latest commit. Pay special attention for potential bugs or regressions with respect to main. Make your review precise and short

[claude]

Claude finished @aponcedeleonch's task —— View job

---

Code Review

Reviewed commit 577e799 - consolidation to async-only functions.

Findings

✅ Good Changes

• Eliminated ~130 lines of sync/async duplication
• Unified return types: _is_toolhive_available now consistently returns tuple[Version, int]
• Used asyncio.gather for concurrent port scanning (lines 182-188) - major performance improvement
• Proper exception-based flow control with new ToolhiveScanError

⚠️ Critical Bug
Line 229: self._discover_port(self._initial_port) called from async _rediscover_port() - this will execute asyncio.run() inside an already running event loop, causing RuntimeError: asyncio.run() cannot be called from a running event loop

Fix: Should be await self._discover_port_async() or refactor _discover_port to be async

Minor Issues

1. Line 197: Use if thv_version_port: instead of if len(thv_version_port) > 0:
2. Lines 166, 170: Exception chaining in _is_toolhive_available creates verbose tracebacks - consider simpler raises for common cases

Regression Risk

The bug in _rediscover_port will cause crashes during reconnection attempts. This is high severity since it affects error recovery - the optimizer won't be able to recover from ToolHive restarts.

Recommendation

Fix the event loop nesting bug before merge. Otherwise the refactoring successfully addresses all previous review feedback.

---

Job run | Branch: use-latest-thv-version

[aponcedeleonch]

@claude check the changes in the latest commit to see if the previous comments were addressed. Pay special attention for potential bugs or regressions with respect to main. Make your review precise and short

[claude]

Claude finished @aponcedeleonch's task —— View job

---

Code Review - Latest Commit

Reviewed commit 5ebbef6 - all previous issues addressed.

Todos:

• Read latest toolhive_client.py implementation
• Check git diff for latest commit changes
• Verify critical bug fix in _rediscover_port()
• Verify other previous feedback items
• Look for new bugs or regressions
• Provide final assessment

---

Summary

✅ Critical bug fixed: Line 252 now correctly calls await self._discover_port_async() instead of self._discover_port(), preventing the RuntimeError from nested event loops.

✅ All previous feedback addressed:

• Eliminated sync/async code duplication (~130 lines removed)
• Unified return types: _is_toolhive_available now consistently returns tuple[Version, int]
• Added asyncio.gather for concurrent port scanning (lines 205-211) - significant performance improvement
• Proper exception handling with new ToolhiveScanError
• Removed time import (unused after async consolidation)

✅ No regressions detected: Error recovery flow now works correctly, allowing reconnection after ToolHive restarts.

Minor observation: Line 220 still uses if thv_version_port: (not if len(thv_version_port) > 0:), which is the cleaner approach - good.

Verdict

✅ Ready to merge - all issues from previous review resolved, no new concerns identified.

---

Job run | Branch: use-latest-thv-version

[therealnb]

Nice.

I think this is all a very good argument for using another method for communicating the thv serve port to the MCP servers.