Fix workload discovery broken after toolhive removed tool_type field

[aponcedeleonch]

Fixes #167

After toolhive PR #2932 removed the tool_type field from the Workload API response, mcp-optimizer could not discover any MCP servers. All workloads were filtered out because the tool_type field was null.

Changes

• Remove tool_type filtering from workload discovery in ingestion.py (Docker and K8s modes)
• Remove tool_type filtering from toolhive_client.py and k8s_client.py
• Update logging to use remote field instead of tool_type for workload classification
• Update workload counting logic to use remote boolean field

The remote field now indicates workload type: remote=true for remote workloads, remote=false or null for container workloads.

Testing

• All 284 tests pass
• Linting and type checking pass with no new issues

🤖 Generated with Claude Code

[claude]

Code Review

Summary

Good fix addressing the breaking change from toolhive API. The approach of removing tool_type filtering and switching to the remote field is correct.

Issues Found

1. Inconsistent null handling (Critical)

In ingestion.py:1450-1451 and 1509-1510, the expression w.remote or False means remote=None is treated as container workload. But in the Workload model (core.py:40-43), the field description says Remote indicates whether this is a remote workload (true) or a container workload (false) - no mention of None.

Risk: If toolhive returns remote=None, workloads are silently categorized as containers. This could mask data quality issues.

Fix: Be explicit about None handling (w.remote is False or w.remote is None) or add validation/logging when remote=None.

2. Overly broad filtering removed (Moderate)

All three files removed filtering for non-MCP workloads. Now all running workloads are processed, regardless of type.

Question: Does toolhive return non-MCP workloads? If yes, removing this filter means mcp-optimizer will try to process them.

Recommendation: Verify toolhive only returns MCP workloads, or add a comment explaining why filtering is no longer needed.

3. Missing field validation (Low)

The tool_type field still exists in the Workload model (core.py:51-54) marked as Optional. Consider: Is this field deprecated? Should we log a warning if it is unexpectedly present?

Positive Points

• Tests pass (284 tests)
• Clean removal of deprecated field usage
• Consistent changes across all three files
• Good PR description explaining the change

Minor Suggestions

• Consider adding a unit test for remote=None behavior

[aponcedeleonch]

@claude check if your feedback was addressed. For 2. The answer is that ToolHive does not return workloads that are not MCP servers

[claude]

Claude finished @aponcedeleonch's task —— View job

---

Verification Complete ✓

I've reviewed the current code to check if the previous feedback was addressed. Here's my assessment:

• Read current code in changed files
• Verify issue Update python Docker tag to v3.14 #1 (null handling) resolution
• Verify issue Dependency Dashboard #2 (filtering) - confirmed by user that ToolHive only returns MCP workloads
• Verify issue Helm charts not released #3 (deprecated field validation)
• Provide final assessment

---

Assessment

Issue #1: Inconsistent null handling - ✅ RESOLVED

The code now explicitly handles remote=None cases in ingestion.py:1454-1462:

if workload.remote is None:
    logger.error(
        "Workload has remote=None, skipping (data quality issue)",
        workload_name=workload.name,
        ...
    )
    continue

This is much better! Instead of silently treating None as False (which was the critical issue), the code now:

• Explicitly detects when remote=None
• Logs it as a data quality error
• Skips the workload entirely
• Prevents silent misclassification

The previous w.remote or False pattern in lines 1450-1451 and 1509-1510 is gone, replaced with proper validation. Excellent fix!

Issue #2: Overly broad filtering - ✅ RESOLVED

Confirmed by user that ToolHive does not return non-MCP workloads, so removing the tool_type filter is safe. The code correctly processes all workloads returned by ToolHive.

Issue #3: Missing field validation - ⚠️ ACKNOWLEDGED

The tool_type field remains in the Workload model (core.py:51-54) as Optional[str] but is no longer used in the code. This is fine - it's a deprecated field that may still be present in API responses for backward compatibility. No action needed unless you want to explicitly document it as deprecated.

---

Summary

All critical and moderate issues have been properly addressed:

1. ✅ Null handling is now explicit and safe
2. ✅ Filtering approach confirmed correct
3. ⚠️ Deprecated field exists but harmless

The PR is in good shape. The fix properly addresses the breaking change from toolhive while adding better data validation. Great work! 🎉

---

Job Run | Branch: fix/workload-discovery-remote-field

[therealnb]

Looks good