Skip to content

Concurrent MCP discovery + robust schema normalization for Harmony tool injection #101

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mc936h
Copy link

@mc936h mc936h commented Aug 8, 2025

Concurrent MCP discovery + robust schema normalization for Harmony tool injection

Summary

  • Fetch MCP servers concurrently (no more asyncio.run per URL).
  • Normalize MCP JSON Schemas to Harmony’s variant:
    • Handles anyOf / oneOf / allOf / nullable
    • Deep-copies schemas (no in-place mutation)
    • Preserves enum, description, format
  • Resilient to partial failures — one bad server no longer breaks the build.
  • Deterministic UTC conversation_start_date.
  • Clearer variable naming, logging, and light type hints.
  • Centralized tool URL config for easier updates.

Why

  • Latency: Parallel discovery reduces startup time from the sum of all server latencies to the max of them.
  • Correctness: Tools with oneOf / nullable previously broke or were dropped; now they’re usable.
  • Reliability: Script continues even if one local tool server is down.
  • Maintainability: Easier to reason about, test, and extend.

Key changes

  • Added helpers:
    • _normalize_schema — deep-copy schema normalizer with extended support for common JSON Schema patterns.
    • _filter_tools — removes tools excluded from prompts with safe annotation checks.
    • gather_servers — fetches all MCP servers concurrently.
    • build_harmony_system_content — cleanly constructs SystemContent with all discovered tools.
  • Logging on exclusions & failures.
  • Uses UTC date via datetime.now(timezone.utc).
  • Avoids variable shadowing (no reusing system_message).
  • More explicit output variable: rendered_system_text.

Before / After

Before

  • Sequential SSE calls; one failure aborted the run.
  • Only anyOf support; oneOf / nullable unsupported.
  • Mutated schema in-place.
  • Local time in system message; ambiguous output variable.

After

  • Concurrency via asyncio.gather.
  • Broader schema support; deep-copy; preserves key metadata.
  • UTC date; explicit rendered_system_text output.

Tests

Unit (new)

  • _normalize_schema with:
    • anyOf: [{"type":"string"},{"type":"null"}]type: ["string"]
    • oneOf → flattened type list
    • allOf simple merges
    • nullable: truetype includes "null"
    • Recursion into properties / items
  • _filter_tools respects annotations.include_in_prompt

Manual (local)

  1. Start browser and python MCP servers at :8001 / :8000.
  2. Run script twice: once with both servers running; once with one stopped — verify output is still valid.
  3. Introduce a tool with oneOf and enum; ensure it appears correctly.

Rollout / Risk

  • Low risk: only changes how the system message is built; runtime consumers unchanged.
  • Possible limitation: highly nested/complex allOf merges are naive — logged as warnings.

Follow-ups

  • Add env/CLI config for server URLs (MCP_SSE_URLS) and REASONING_EFFORT.
  • Validate tool schemas against a “Harmony schema” using jsonschema or pydantic.
  • Add metrics: tools per namespace, excluded counts, build latency.
  • Golden-file test for rendered system text.

Reviewer checklist

  • Concurrency logic is correct — only one asyncio.run call.
  • Schema normalization does not drop meaningful fields.
  • Partial failures still yield usable SystemContent.
  • Logging is clear and helpful.

mc936h added 2 commits August 8, 2025 19:16
…ol injection

Summary
Fetch MCP servers concurrently (no more asyncio.run per URL).

Normalize MCP JSON Schemas to Harmony’s variant (handles anyOf/oneOf/allOf/nullable, deep-copies, preserves enums/descriptions).

Resilient to partial failures (one bad server doesn’t break the build).

Deterministic UTC conversation_start_date.

Clear variable naming, logging, light type hints.

Moves tool URLs to a single config list and makes it easy to plumb env/CLI later.

Why
Latency: Parallel discovery reduces startup from sum of server latencies to max of them.

Correctness: Tools with oneOf/nullable previously broke or were dropped; now they’re usable.

Reliability: Dev flow shouldn’t die because one local tool server is down.

Maintainability: Easier to reason about, test, and extend.

Key changes
New helpers: _normalize_schema, _filter_tools, gather_servers, build_harmony_system_content.

Safer schema handling: deep-copy, unionize types, recurse properties/items, naive allOf merge.

Logging on exclusions & failures; graceful skip of failing servers.

Avoids variable shadowing (no reusing system_message).

Uses UTC date via datetime.now(timezone.utc).

Before / After
Before

Sequential SSE calls; one failure aborted the run.

anyOf only; oneOf/nullable unsupported; in-place mutation.

Local time in system message; unclear printed output var.

After

Concurrency via asyncio.gather.

Broader schema support; deep-copy; preserves enum/description/format.

UTC date; explicit rendered_system_text output.

Tests
Unit (new):

_normalize_schema with:

anyOf: [{"type":"string"},{"type":"null"}] → type: ["string"]

oneOf types → flattened type list

allOf simple merges

nullable: true with type: "object" → type: ["object"]

properties/items recursion

_filter_tools respects annotations.include_in_prompt

Manual (local):

Start browser and python MCP servers at :8001 / :8000.

Run script twice: once with both up; once with one stopped — ensure it still prints a valid system message.

Introduce a tool with oneOf and enum; verify it appears in rendered output.

Rollout / Risk
Low: purely affects how we construct the system message; no runtime call sites changed.

If an MCP server returns highly nested/complex allOf, our naive merge could miss edge cases. Mitigation: we log a warning and still include the raw fields; follow-up below.

Follow-ups (separate PRs)
Env/CLI: read server URLs (MCP_SSE_URLS) and REASONING_EFFORT.

Add JSON Schema-based validation against a “Harmony schema” for tool parameters.

Metrics (count tools per namespace; excluded count; build latency).

Golden-file test for rendered system text.

Config / Docs
Config: central TOOL_SERVER_URLS list; documented at top of file.

Docs: Added module docstring explaining the pipeline and limitations of allOf merge.

Reviewer checklist
 Concurrency is correct (no blocking calls; one asyncio.run only).

 Schema normalization doesn’t drop meaningful fields (see tests).

 Partial-failure path still yields a usable SystemContent.

 Naming/readability and logs are clear.
Concurrent MCP discovery + robust schema normalization for Harmony tool injection
@mc936h
Copy link
Author

mc936h commented Aug 10, 2025

@romainhuet can you please review this PR and provide some feedback?

Thank you!

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.

1 participant