Skip to content

Conversation

@therealnb
Copy link
Collaborator

@therealnb therealnb commented Nov 7, 2025

This PR fixes two issues in the ToolHive client:

  1. Non-semver version handling: Updates _parse_toolhive_version to gracefully handle development builds that don't follow semver (e.g., 'build-7c3a3077') by returning a default version '0.0.0-dev' instead of raising an error. This allows connections to succeed with development builds while still logging the issue.

  2. Configurable timeout: Replaces the hardcoded 1.0s timeout with self.timeout in _is_toolhive_available for better configurability.

Changes

  • Modified src/mcp_optimizer/toolhive/toolhive_client.py

Testing

  • Tested with development build versions
  • Verified timeout configuration is respected

Note

That code was inserted as part of #19 so we should consider if the patch above is consistent with that.

- Update _parse_toolhive_version to gracefully handle development builds
  that don't follow semver (e.g., 'build-7c3a3077') by returning a default
  version '0.0.0-dev' instead of raising an error
- Replace hardcoded 1.0s timeout with self.timeout in _is_toolhive_available
  for better configurability
@claude
Copy link

claude bot commented Nov 7, 2025

PR Review for #52

Summary

Good fixes that improve robustness. The changes handle non-semver versions gracefully and use the correct timeout configuration. Both changes are sensible improvements.

Code Quality & Best Practices ✅

  • Clean implementation following existing patterns
  • Proper error handling with logging
  • Good use of default fallback for non-semver versions

Issues & Suggestions 🔍

Critical:

  • Missing test coverage: The new behavior in _parse_toolhive_version (lines 159-167) isn't tested. Add a test case for non-semver versions like "build-7c3a3077" to ensure it returns Version.parse("0.0.0-dev") and logs appropriately.

Minor:

  • Line 166: Missing comma after default_version="0.0.0-dev" - this works in Python but inconsistent with multi-line function call style elsewhere in the codebase
  • Consider: Should we distinguish between different non-semver patterns? Current implementation treats all invalid versions the same way. This is probably fine for now but worth documenting if certain patterns need special handling in the future.

Performance & Security ✅

  • No performance concerns
  • No security issues
  • Using configurable timeout (line 177) is more flexible than hardcoded value

Regressions & Breaking Changes ✅

  • No breaking changes
  • Backward compatible: previously would raise error, now gracefully handles edge case
  • Existing tests should pass (though new test needed for new behavior)

Recommendations

  1. Add test case to test_toolhive_client.py for non-semver version parsing
  2. Fix formatting: add trailing comma on line 166

Estimated fix time: 5 minutes

ptelang
ptelang previously approved these changes Nov 7, 2025
Copy link
Collaborator

@ptelang ptelang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good find! Changes look good!

tgrunnagle
tgrunnagle previously approved these changes Nov 7, 2025
Signed-off-by: nigel brown <nigel@stacklok.com>
@therealnb therealnb dismissed stale reviews from tgrunnagle and ptelang via f0704f7 November 7, 2025 16:41
ptelang
ptelang previously approved these changes Nov 7, 2025
Signed-off-by: nigel brown <nigel@stacklok.com>
@therealnb therealnb enabled auto-merge (squash) November 7, 2025 17:18
@therealnb therealnb merged commit 6aee590 into main Nov 7, 2025
4 checks passed
@therealnb therealnb deleted the fix/toolhive-version-parsing-and-timeout branch November 7, 2025 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants