Split toolhive_host into separate variables for API and workload communication

[aponcedeleonch]

Related: #25

• Added workload_host config variable alongside toolhive_host
• toolhive_host now used exclusively for ToolHive API communication
• workload_host used for MCP workload URL rewriting (localhost replacement)
• Both variables default to same value: localhost (local dev) and host.docker.internal (Docker)
• Extracted URL replacement logic into reusable replace_localhost_in_url() method
• Added 9 unit tests covering URL replacement edge cases (None, empty string, localhost, 127.0.0.1, regular hostnames, HTTPS, fragments)
• Updated all ToolhiveClient instantiations across codebase and tests
• All 293 tests passing

🤖 Generated with Claude Code

[claude]

PR Review

Summary

Clean separation of concerns by splitting toolhive_host into API (toolhive_host) and workload (workload_host) configuration. Well-tested with comprehensive coverage.

👍 Strengths

• Good abstraction: replace_localhost_in_url() method eliminates code duplication (src/mcp_optimizer/toolhive/toolhive_client.py:385-408)
• Comprehensive tests: 9 new unit tests cover edge cases (None, empty, localhost, 127.0.0.1, paths, HTTPS, fragments)
• Backward compatible: Both vars default to same values, no breaking changes
• Clean naming: workload_host clearly indicates purpose

⚠️ Issues Found

1. URL replacement bug in replace_localhost_in_url() (src/mcp_optimizer/toolhive/toolhive_client.py:406)

return url.replace(hostname, self.workload_host)

Problem: str.replace() replaces ALL occurrences, not just hostname. This can corrupt URLs.

Example:

• Input: http://localhost:8080/api/localhost/data
• Current output: http://host.docker.internal:8080/api/host.docker.internal/data ❌
• Expected: http://host.docker.internal:8080/api/localhost/data ✅

Fix: Use urlunparse() to reconstruct URL properly:

if hostname and hostname in ("localhost", "127.0.0.1"):
    return url.replace(f"//{hostname}", f"//{self.workload_host}", 1)

Or better, reconstruct the URL:

if hostname and hostname in ("localhost", "127.0.0.1"):
    parsed = parsed_url._replace(netloc=parsed_url.netloc.replace(hostname, self.workload_host, 1))
    return urlunparse(parsed)

Note: Current test at line 171 doesn't catch this because path doesn't match hostname:

result = client.replace_localhost_in_url("http://example.com/api/localhost/data")

Add test with matching hostname in path:

result = client.replace_localhost_in_url("http://localhost:8080/api/localhost/data")
assert result == "http://host.docker.internal:8080/api/localhost/data"

📊 Code Quality

• Follows project conventions (Pydantic, native types)
• Clear docstrings
• All 293 tests passing
• Good PR description

Verdict

Approve with fix required - Address URL replacement bug before merge.

Estimated fix time: < 5 minutes

[aponcedeleonch]

@claude check the latest commit and see if the comments were addressed and if you don't find any new issue

[claude]

Claude finished @aponcedeleonch's task —— View job

---

Review Complete ✅

Summary

The latest commit "review comments" (ba91198) successfully addresses all previous review feedback. No new issues found.

---

What Was Fixed

The replace_localhost_in_url() method now correctly replaces only the hostname in the netloc portion of the URL (src/mcp_optimizer/toolhive/toolhive_client.py:405-410):

if hostname and hostname in ("localhost", "127.0.0.1"):
    # Replace hostname only in netloc, not in path/query/fragment
    new_netloc = parsed_url.netloc.replace(hostname, self.workload_host, 1)
    parsed = parsed_url._replace(netloc=new_netloc)
    # urlunparse returns str when input is from urlparse(str)
    return str(urlunparse(parsed))

Key improvements:

1. Uses .replace(hostname, self.workload_host, 1) on the netloc only (count=1 for single replacement)
2. Uses _replace(netloc=new_netloc) to rebuild the URL with the new netloc
3. Uses urlunparse() to properly reconstruct the complete URL

This ensures that only the hostname portion is replaced, not occurrences of "localhost" or "127.0.0.1" in the path, query, or fragment.

---

Test Coverage

Excellent test coverage added in tests/test_toolhive_client.py:783-814:

New critical test: test_replace_localhost_in_url_localhost_in_hostname_and_path()

• Verifies the exact edge case from the review
• Example: http://localhost:8080/api/localhost/data → http://host.docker.internal:8080/api/localhost/data ✅
• Also tests with 127.0.0.1: http://127.0.0.1:8080/path/127.0.0.1/data → http://host.docker.internal:8080/path/127.0.0.1/data ✅

The test suite now includes 9 comprehensive tests covering all edge cases (None, empty string, localhost, 127.0.0.1, regular hostnames, HTTPS, fragments, paths, and the critical hostname+path combination).

---

Verdict

✅ Approved - All review comments successfully addressed

No new issues found. The implementation is correct, well-tested, and ready to merge.

---

Job Run | Branch: diff-host

[aponcedeleonch]

The fix was needed with Podman + Windows but it didn't work as expected. Closing this for the time being. More details in the linked issue