diff --git a/SPECS/ARCHIVE/INDEX.md b/SPECS/ARCHIVE/INDEX.md index 42614fbe..105049dd 100644 --- a/SPECS/ARCHIVE/INDEX.md +++ b/SPECS/ARCHIVE/INDEX.md @@ -1,11 +1,12 @@ # mcpbridge-wrapper Tasks Archive -**Last Updated:** 2026-03-07 +**Last Updated:** 2026-03-10 ## Archived Tasks | Task ID | Folder | Archived | Verdict | |---------|--------|----------|---------| +| P2-T8 | [P2-T8_Gate_broker_tools_list_on_warmed_tool_catalog/](P2-T8_Gate_broker_tools_list_on_warmed_tool_catalog/) | 2026-03-10 | PASS | | P8-T1 | [P8-T1_Release_version_0.4.2_to_PyPI_and_MCP_Registry/](P8-T1_Release_version_0.4.2_to_PyPI_and_MCP_Registry/) | 2026-03-07 | PASS | | P7-T5 | [P7-T5_Document_broker_UX/](P7-T5_Document_broker_UX/) | 2026-03-07 | PASS | | P7-T4 | [P7-T4_Add_direct_local-status_fallback_for_TUI_when_dashboard_API_is_unavailable/](P7-T4_Add_direct_local-status_fallback_for_TUI_when_dashboard_API_is_unavailable/) | 2026-03-07 | PASS | @@ -205,6 +206,7 @@ | File | Description | |------|-------------| +| [REVIEW_p2_t8_tools_catalog_gate.md](_Historical/REVIEW_p2_t8_tools_catalog_gate.md) | Review report for P2-T8 | | [REVIEW_broker_ux_docs.md](_Historical/REVIEW_broker_ux_docs.md) | Review report for P7-T5 | | [REVIEW_tui_local_status_fallback.md](P7-T4_Add_direct_local-status_fallback_for_TUI_when_dashboard_API_is_unavailable/REVIEW_tui_local_status_fallback.md) | Review report for P7-T4 | | [REVIEW_broker_owned_listener_guidance.md](_Historical/REVIEW_broker_owned_listener_guidance.md) | Review report for FU-P7-T3-2 | @@ -636,3 +638,5 @@ | 2026-03-07 | P7-T2 | Archived REVIEW_broker_doctor_diagnostics report | | 2026-03-07 | P7-T3 | Archived Auto-recover_or_guide_on_dashboard_port_ownership_conflicts (PASS) | | 2026-03-07 | P7-T3 | Archived REVIEW_dashboard_port_ownership_conflicts report | +| 2026-03-10 | P2-T8 | Archived Gate_broker_tools_list_on_warmed_tool_catalog (PASS) | +| 2026-03-10 | P2-T8 | Archived REVIEW_p2_t8_tools_catalog_gate report | diff --git a/SPECS/ARCHIVE/P2-T8_Gate_broker_tools_list_on_warmed_tool_catalog/P2-T8_Gate_broker_tools_list_on_warmed_tool_catalog.md b/SPECS/ARCHIVE/P2-T8_Gate_broker_tools_list_on_warmed_tool_catalog/P2-T8_Gate_broker_tools_list_on_warmed_tool_catalog.md new file mode 100644 index 00000000..a568482b --- /dev/null +++ b/SPECS/ARCHIVE/P2-T8_Gate_broker_tools_list_on_warmed_tool_catalog/P2-T8_Gate_broker_tools_list_on_warmed_tool_catalog.md @@ -0,0 +1,91 @@ +# P2-T8: Gate broker tools/list on warmed tool catalog + +**Status:** In Progress +**Priority:** P0 +**Dependencies:** BUG-T9, P4-T2 +**Branch:** `codex/p2-t8-broker-tools-catalog-gate` + +--- + +## Problem Statement + +Strict MCP clients such as Cursor and Zed cache the first successful `tools/list` +response they receive from a server. In broker mode, `mcpbridge-wrapper` currently +lets external client `tools/list` requests pass as soon as the upstream +`initialize` round-trip finishes. + +That is too early. + +The broker still has a second startup phase after `initialize`: + +1. Send `notifications/initialized` to `xcrun mcpbridge` +2. Probe `tools/list` internally +3. Cache the resulting tool catalog for later clients + +If an external client sends `tools/list` during that warm-up gap, it can observe an +empty or invalid tools list and cache that broken success locally. The user then +sees a green MCP indicator but fewer than the full 20 Xcode tools until they toggle +the server multiple times. + +--- + +## Root Cause + +In `UnixSocketServer._process_client_line`, the only readiness gate for all +request/response traffic is `daemon.upstream_initialized`. That event becomes set +immediately after the broker receives the upstream `initialize` response. + +At that moment, however: + +- the broker's internal `tools/list` probe may still be in flight +- `_tools_list_cache` may still be `None` +- the upstream may still return an empty or malformed tools payload during cold-start + +Because `tools/list` follows the same gate as every other method, the first client +tool-discovery request can race ahead of the broker's own cache warm-up. + +--- + +## Fix + +Introduce a second broker readiness concept dedicated to tool discovery: + +- add a `tools_catalog_ready` event on the daemon +- set it only after the broker receives a non-empty, structurally valid + `tools/list` probe result +- clear it on reconnect or when the internal probe returns an empty/invalid catalog +- make external client `tools/list` wait on `tools_catalog_ready` instead of only + `upstream_initialized` + +This preserves existing behavior for non-`tools/list` methods while making the +first tool-discovery handshake safe for strict clients. + +--- + +## Deliverables + +| File | Change | +|------|--------| +| `src/mcpbridge_wrapper/broker/daemon.py` | Add explicit tools-catalog readiness event and reject empty/invalid warm-up results | +| `src/mcpbridge_wrapper/broker/transport.py` | Hold client `tools/list` until broker tool catalog is ready | +| `tests/unit/test_broker_daemon.py` | Cover ready/non-ready broker tool catalog transitions | +| `tests/unit/test_broker_transport.py` | Cover client `tools/list` wait/error/cache paths | +| `tests/integration/test_broker_multi_client.py` | Keep integration coverage aligned with the stronger broker contract | + +--- + +## Acceptance Criteria + +- [ ] Broker does not forward external `tools/list` while the internal tools cache is still cold +- [ ] Empty or invalid internal `tools/list` probe results do not open the client-facing readiness gate +- [ ] Client `tools/list` returns either a warmed catalog or a clear TTL error, never a premature empty success +- [ ] Existing non-`tools/list` broker traffic still flows after `upstream_initialized` +- [ ] `pytest` passes +- [ ] `ruff check src/` passes +- [ ] `mypy src/` passes +- [ ] `pytest --cov` remains at or above 90% + + +--- +**Archived:** 2026-03-10 +**Verdict:** PASS diff --git a/SPECS/ARCHIVE/P2-T8_Gate_broker_tools_list_on_warmed_tool_catalog/P2-T8_Validation_Report.md b/SPECS/ARCHIVE/P2-T8_Gate_broker_tools_list_on_warmed_tool_catalog/P2-T8_Validation_Report.md new file mode 100644 index 00000000..5f47e045 --- /dev/null +++ b/SPECS/ARCHIVE/P2-T8_Gate_broker_tools_list_on_warmed_tool_catalog/P2-T8_Validation_Report.md @@ -0,0 +1,77 @@ +# Validation Report: P2-T8 — Gate broker tools/list on warmed tool catalog + +**Date:** 2026-03-10 +**Verdict:** PASS + +--- + +## Acceptance Criteria + +| # | Criterion | Status | +|---|-----------|--------| +| 1 | Broker does not forward external `tools/list` while the internal tools cache is still cold | ✅ PASS | +| 2 | Empty or invalid internal `tools/list` probe results do not open the client-facing readiness gate | ✅ PASS | +| 3 | Client `tools/list` returns either a warmed catalog or a clear TTL error, never a premature empty success | ✅ PASS | +| 4 | Existing non-`tools/list` broker traffic still flows after `upstream_initialized` | ✅ PASS | +| 5 | `pytest` passes | ✅ PASS | +| 6 | `ruff check src/` passes | ✅ PASS | +| 7 | `mypy src/` passes | ✅ PASS | +| 8 | `pytest --cov` remains at or above 90% | ✅ PASS | + +--- + +## Evidence + +### Functional behavior + +- Added a dedicated `tools_catalog_ready` event in the broker daemon so tool discovery + is gated separately from the upstream `initialize` round-trip. +- The broker now treats only non-empty, structurally valid internal `tools/list` + probe results as a ready catalog; empty or invalid results keep the gate closed, + clear the cache, and schedule another broker-internal warm-up probe instead of + requiring a reconnect or manual restart. +- External client `tools/list` now waits on the warmed catalog gate and returns a + deterministic `-32001` TTL error if the broker never reaches a safe ready state. +- Non-`tools/list` methods still wait only on `upstream_initialized`, preserving the + existing broker contract for normal request forwarding. + +### Regression coverage + +- `tests/unit/test_broker_daemon.py` + - verifies catalog readiness opens only for a valid non-empty probe result + - verifies empty probe results keep the catalog gate closed + - verifies an empty first probe retries until a valid tool catalog becomes available + - verifies reconnect clears both cache and readiness state +- `tests/unit/test_broker_transport.py` + - verifies `tools/list` times out with a catalog-specific readiness error + - verifies non-`tools/list` requests still wait on upstream initialization only + - verifies `tools/list` resumes from the warmed cache instead of racing upstream +- `tests/integration/test_broker_multi_client.py` + - keeps concurrent multi-client coverage aligned with the stronger broker warm-up + contract by exercising a normal forwarded tool call path instead of the special + cached `tools/list` path + +### Validation environment hardening + +- Added `pythonpath = ["src"]` to `pyproject.toml` so pytest in a clean worktree + imports the local checkout instead of an unrelated editable install from another + repository path. This makes FLOW validation deterministic and fixes `pytest --cov` + reporting in multi-worktree setups. + +### Command results + +- `pytest` → **901 passed, 5 skipped, 2 warnings** +- `ruff check src/` → **All checks passed** +- `mypy src/` → **Success: no issues found in 20 source files** +- `pytest --cov` → **901 passed, 5 skipped, 2 warnings; coverage 91.58%** + +--- + +## Changed Files + +- `pyproject.toml` +- `src/mcpbridge_wrapper/broker/daemon.py` +- `src/mcpbridge_wrapper/broker/transport.py` +- `tests/integration/test_broker_multi_client.py` +- `tests/unit/test_broker_daemon.py` +- `tests/unit/test_broker_transport.py` diff --git a/SPECS/ARCHIVE/_Historical/REVIEW_p2_t8_tools_catalog_gate.md b/SPECS/ARCHIVE/_Historical/REVIEW_p2_t8_tools_catalog_gate.md new file mode 100644 index 00000000..9e21a88e --- /dev/null +++ b/SPECS/ARCHIVE/_Historical/REVIEW_p2_t8_tools_catalog_gate.md @@ -0,0 +1,41 @@ +## REVIEW REPORT — P2-T8 Tools Catalog Gate + +**Scope:** `origin/main..HEAD` +**Files:** 12 + +### Summary Verdict +- [x] Approve +- [ ] Approve with comments +- [ ] Request changes +- [ ] Block + +### Critical Issues + +- None. + +### Secondary Issues + +- None. + +### Architectural Notes + +- Separating `tools_catalog_ready` from `upstream_initialized` closes the broker's + startup race without delaying unrelated non-`tools/list` traffic. +- Review of the first P2-T8 revision uncovered one high-risk hole: an empty first + broker `tools/list` probe could leave the catalog gate closed indefinitely until + reconnect or restart. The final branch resolves that by retrying the broker-internal + warm-up probe until a valid non-empty catalog arrives or the daemon transitions. +- Adding `pythonpath = ["src"]` to `pytest` config is a valid repo-level hardening + step because this project is actively developed from multiple local worktrees and + otherwise can import an unrelated editable install. + +### Tests + +- `pytest` — PASS (`901 passed, 5 skipped, 2 warnings`) +- `ruff check src/` — PASS +- `mypy src/` — PASS +- `pytest --cov` — PASS (`91.58%`) + +### Next Steps + +- No actionable review findings. FOLLOW-UP is skipped. diff --git a/SPECS/INPROGRESS/next.md b/SPECS/INPROGRESS/next.md index 441423bd..8d81c421 100644 --- a/SPECS/INPROGRESS/next.md +++ b/SPECS/INPROGRESS/next.md @@ -1,28 +1,23 @@ -# Next Task: (none pending) +# Next Task: None -All current workplan tasks are complete. +## Selected Task + +- No active task selected. + +## Description + +All tasks currently tracked in `SPECS/Workplan.md` are archived or completed. Select a new +task only after a follow-up or new workplan entry is created. + +## Outputs + +- None. ## Recently Archived +- `2026-03-10` — `P2-T8` archived with verdict `PASS` - `2026-03-07` — `P8-T1` archived with verdict `PASS` - `2026-03-07` — `P7-T5` archived with verdict `PASS` - `2026-03-07` — `P7-T4` archived with verdict `PASS` - `2026-03-07` — `FU-P7-T3-2` archived with verdict `PASS` - `2026-03-07` — `FU-P7-T3-1` archived with verdict `PASS` - -## Next Step - -All tasks in the current workplan cycle have been completed. Add new tasks to -`SPECS/Workplan.md` to begin the next cycle. - -## Post-Merge Action Required - -After the P8-T1 PR merges to `main`, push the release tag to trigger publishing: - -```bash -git checkout main && git pull origin main -git tag v0.4.2 -git push origin v0.4.2 -``` - -Then verify GitHub Actions `publish-mcp.yml` completes successfully. diff --git a/SPECS/Workplan.md b/SPECS/Workplan.md index bb003c4a..6bcf58e2 100644 --- a/SPECS/Workplan.md +++ b/SPECS/Workplan.md @@ -274,6 +274,23 @@ Add new tasks using the canonical template in [TASK_TEMPLATE.md](TASK_TEMPLATE.m - [x] Broker mode guidance remains clear with `--broker` (proxy) and `--broker-daemon` (host) - [x] Required quality gates pass (`pytest`, `ruff check src/`, `mypy src/`, `pytest --cov` with coverage >=90%) +#### ✅ P2-T8: Gate broker tools/list on warmed tool catalog +- **Status:** ✅ Completed (2026-03-10) +- **Description:** Cursor and Zed can cache the first successful `tools/list` response they receive from `mcpbridge-wrapper`. Today the broker releases client `tools/list` requests immediately after upstream `initialize`, even if the broker has not yet completed its own `notifications/initialized` + `tools/list` warm-up and populated a stable tool cache. During cold-start or Xcode approval timing, that lets strict clients see an empty or invalid tool list and forces users to toggle the server several times before all 20 Xcode tools appear. Fix the broker so external `tools/list` waits for a warmed non-empty catalog instead of racing the warm-up path. +- **Priority:** P0 +- **Dependencies:** BUG-T9, P4-T2 +- **Parallelizable:** no +- **Outputs/Artifacts:** + - `src/mcpbridge_wrapper/broker/daemon.py` — explicit tool-catalog readiness gate and empty-catalog handling + - `src/mcpbridge_wrapper/broker/transport.py` — hold client `tools/list` until broker cache is ready + - `tests/unit/test_broker_daemon.py`, `tests/unit/test_broker_transport.py` — broker warm-up regression coverage + - `tests/integration/test_broker_multi_client.py` — integration coverage updated for the new broker contract +- **Acceptance Criteria:** + - [x] Broker does not forward client `tools/list` while its internal tool catalog is still cold + - [x] Empty or invalid broker `tools/list` probe results do not open the client-facing readiness gate + - [x] Cursor/Zed-style first `tools/list` requests receive either a warmed catalog or a clear TTL error, never a prematurely cached empty success + - [x] Required quality gates pass (`pytest`, `ruff check src/`, `mypy src/`, `pytest --cov` with coverage >=90%) + ### Phase 3: Web UI Controls #### ✅ P3-T11: Add Stop broker/service control button to Web UI diff --git a/pyproject.toml b/pyproject.toml index 4122e762..e1ca32b6 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -71,6 +71,7 @@ include-package-data = true [tool.pytest.ini_options] testpaths = ["tests"] +pythonpath = ["src"] python_files = ["test_*.py", "*_test.py"] python_classes = ["Test*"] python_functions = ["test_*"] diff --git a/src/mcpbridge_wrapper/broker/daemon.py b/src/mcpbridge_wrapper/broker/daemon.py index c02f823c..8ad21b2d 100644 --- a/src/mcpbridge_wrapper/broker/daemon.py +++ b/src/mcpbridge_wrapper/broker/daemon.py @@ -43,6 +43,8 @@ # broker_id is 1_048_576. These negative/zero values can never collide. _BROKER_INIT_ID = 0 _BROKER_TOOLS_ID = -1 +_TOOLS_PROBE_RETRY_BASE_DELAY_SECONDS = 0.25 +_TOOLS_PROBE_RETRY_MAX_DELAY_SECONDS = 2.0 class BrokerDaemon: @@ -90,6 +92,11 @@ def __init__( self._upstream_initialized: asyncio.Event = asyncio.Event() # Cached tools/list result (JSON string); None until first successful probe. self._tools_list_cache: str | None = None + # Set once a usable tools/list response has been cached for clients. + self._tools_catalog_ready: asyncio.Event = asyncio.Event() + # Background retry task for broker-internal tools/list warm-up probes. + self._tools_probe_retry_task: asyncio.Task[None] | None = None + self._tools_probe_retry_attempt: int = 0 # ------------------------------------------------------------------ # Public API @@ -111,6 +118,11 @@ def upstream_initialized(self) -> asyncio.Event: """ return self._upstream_initialized + @property + def tools_catalog_ready(self) -> asyncio.Event: + """Event that is set once a non-empty cached ``tools/list`` result exists.""" + return self._tools_catalog_ready + def status(self) -> dict[str, Any]: """Return a dictionary describing the current daemon status.""" upstream_pid: int | None = None @@ -243,6 +255,7 @@ async def stop(self) -> None: # Signal read loop to exit self._stop_event.set() + self._cancel_tools_probe_retry() # Cancel background read task if self._read_task is not None and not self._read_task.done(): @@ -339,6 +352,75 @@ async def _send_broker_probes(self) -> None: except Exception as exc: logger.warning("Failed to send broker initialize probe: %s", exc) + async def _send_tools_list_probe(self) -> None: + """Send the broker-internal ``tools/list`` probe to the upstream.""" + upstream = self._upstream + if upstream is None or upstream.stdin is None: + logger.warning("Failed to send tools/list probe: no upstream stdin available.") + return + + tools_probe = json.dumps( + { + "jsonrpc": "2.0", + "id": _BROKER_TOOLS_ID, + "method": "tools/list", + "params": {}, + }, + separators=(",", ":"), + ) + try: + upstream.stdin.write((tools_probe + "\n").encode()) + await upstream.stdin.drain() + logger.debug("Broker tools/list probe sent (id=%d)", _BROKER_TOOLS_ID) + except Exception as exc: + logger.warning("Failed to send tools/list probe: %s", exc) + + async def _retry_tools_list_probe_after_delay(self, delay: float) -> None: + """Retry broker warm-up probing after a short delay.""" + try: + if delay > 0: + await asyncio.sleep(delay) + if self._stop_event.is_set(): + return + await self._send_tools_list_probe() + except asyncio.CancelledError: + raise + finally: + if asyncio.current_task() is self._tools_probe_retry_task: + self._tools_probe_retry_task = None + + def _schedule_tools_list_probe(self, *, delay: float = 0.0) -> None: + """Ensure a broker-internal ``tools/list`` probe is scheduled.""" + if self._stop_event.is_set(): + return + task = self._tools_probe_retry_task + if task is not None and not task.done(): + return + self._tools_probe_retry_task = asyncio.create_task( + self._retry_tools_list_probe_after_delay(delay) + ) + + def _reset_tools_probe_retry_backoff(self) -> None: + """Reset retry state for broker-internal tools/list warm-up probing.""" + self._tools_probe_retry_attempt = 0 + + def _next_tools_probe_retry_delay(self) -> float: + """Return the next bounded backoff delay for broker tools/list retries.""" + delay = min( + _TOOLS_PROBE_RETRY_BASE_DELAY_SECONDS * (2**self._tools_probe_retry_attempt), + _TOOLS_PROBE_RETRY_MAX_DELAY_SECONDS, + ) + self._tools_probe_retry_attempt += 1 + return float(delay) + + def _cancel_tools_probe_retry(self) -> None: + """Cancel any pending retry for the broker-internal tools/list probe.""" + task = self._tools_probe_retry_task + if task is not None and not task.done(): + task.cancel() + self._tools_probe_retry_task = None + self._reset_tools_probe_retry_backoff() + async def _rollback_startup(self) -> None: """Roll back a failed :meth:`start` sequence. @@ -350,6 +432,7 @@ async def _rollback_startup(self) -> None: Safe to call even if the upstream was never launched (idempotent). """ logger.warning("Rolling back failed broker startup.") + self._cancel_tools_probe_retry() # Cancel background read task if it was already started if self._read_task is not None and not self._read_task.done(): @@ -498,30 +581,53 @@ async def _read_upstream_loop(self) -> None: # Now fetch tools/list for the cache. upstream = self._upstream if upstream is not None and upstream.stdin is not None: - tools_probe = json.dumps( - { - "jsonrpc": "2.0", - "id": _BROKER_TOOLS_ID, - "method": "tools/list", - "params": {}, - }, - separators=(",", ":"), - ) - try: - upstream.stdin.write((tools_probe + "\n").encode()) - await upstream.stdin.drain() - logger.debug("Broker tools/list probe sent (id=%d)", _BROKER_TOOLS_ID) - except Exception as exc: - logger.warning("Failed to send tools/list probe: %s", exc) + self._reset_tools_probe_retry_backoff() + self._schedule_tools_list_probe() continue if raw_id == _BROKER_TOOLS_ID: # Broker's own tools/list probe response received — cache it. if isinstance(msg, dict) and "result" in msg: - self._tools_list_cache = line - logger.info("tools/list cache populated (%d bytes).", len(line)) + result = msg.get("result") + tools = result.get("tools") if isinstance(result, dict) else None + if isinstance(tools, list) and tools: + self._cancel_tools_probe_retry() + self._tools_list_cache = line + self._tools_catalog_ready.set() + logger.info( + "tools/list cache populated with %d tool(s) (%d bytes).", + len(tools), + len(line), + ) + else: + self._tools_list_cache = None + self._tools_catalog_ready.clear() + delay = self._next_tools_probe_retry_delay() + log_fn = ( + logger.warning + if self._tools_probe_retry_attempt == 1 + else logger.debug + ) + log_fn( + "Broker tools/list probe returned an empty or invalid " + "tool catalog; retry %d in %.2fs.", + self._tools_probe_retry_attempt, + delay, + ) + self._schedule_tools_list_probe(delay=delay) else: - logger.warning("Broker tools/list probe returned no result; cache not set.") + self._tools_list_cache = None + self._tools_catalog_ready.clear() + delay = self._next_tools_probe_retry_delay() + log_fn = ( + logger.warning if self._tools_probe_retry_attempt == 1 else logger.debug + ) + log_fn( + "Broker tools/list probe returned no result; retry %d in %.2fs.", + self._tools_probe_retry_attempt, + delay, + ) + self._schedule_tools_list_probe(delay=delay) continue if self._transport is not None: @@ -535,6 +641,8 @@ async def _reconnect(self) -> None: # Invalidate readiness gate and cache so clients wait for the new upstream. self._upstream_initialized.clear() self._tools_list_cache = None + self._tools_catalog_ready.clear() + self._cancel_tools_probe_retry() cap = self._config.reconnect_backoff_cap while not self._stop_event.is_set(): diff --git a/src/mcpbridge_wrapper/broker/transport.py b/src/mcpbridge_wrapper/broker/transport.py index 16f1c137..a612ce6c 100644 --- a/src/mcpbridge_wrapper/broker/transport.py +++ b/src/mcpbridge_wrapper/broker/transport.py @@ -447,23 +447,41 @@ async def _process_client_line(self, session: ClientSession, line: str) -> None: local_alias: int | None = None if not is_notification: - # Gate: wait until the upstream has completed its initialize round-trip. - # This prevents clients from receiving empty or error responses during the - # Xcode approval window or other upstream restart scenarios. - if not self._daemon.upstream_initialized.is_set(): - try: - await asyncio.wait_for( - self._daemon.upstream_initialized.wait(), - timeout=float(self._config.queue_ttl), - ) - except asyncio.TimeoutError: - await self._send_error( - session, - raw_id, - -32001, - "Broker upstream not ready — request TTL exceeded", - ) - return + if method_name == "tools/list": + # Strict MCP clients cache the first tools/list result. Hold the + # request until the broker has its own warm cache populated. + if not self._daemon.tools_catalog_ready.is_set(): + try: + await asyncio.wait_for( + self._daemon.tools_catalog_ready.wait(), + timeout=float(self._config.queue_ttl), + ) + except asyncio.TimeoutError: + await self._send_error( + session, + raw_id, + -32001, + "Broker tools catalog not ready — request TTL exceeded", + ) + return + else: + # Gate: wait until the upstream has completed its initialize round-trip. + # This prevents clients from receiving empty or error responses during the + # Xcode approval window or other upstream restart scenarios. + if not self._daemon.upstream_initialized.is_set(): + try: + await asyncio.wait_for( + self._daemon.upstream_initialized.wait(), + timeout=float(self._config.queue_ttl), + ) + except asyncio.TimeoutError: + await self._send_error( + session, + raw_id, + -32001, + "Broker upstream not ready — request TTL exceeded", + ) + return if self._daemon.state not in (BrokerState.READY, BrokerState.RECONNECTING): await self._send_error( diff --git a/tests/integration/test_broker_multi_client.py b/tests/integration/test_broker_multi_client.py index c3e4fcdb..bb1f5ab7 100644 --- a/tests/integration/test_broker_multi_client.py +++ b/tests/integration/test_broker_multi_client.py @@ -155,7 +155,7 @@ async def _client(seq: int) -> dict[str, Any]: broker_config, request_id=request_id, seq=seq, - method="tools/list", + method="tools/call", ) client_count = 24 @@ -167,7 +167,7 @@ async def _client(seq: int) -> dict[str, Any]: for seq in range(client_count): request_id = f"req-{seq}" assert request_id in by_id - assert by_id[request_id]["result"]["method"] == "tools/list" + assert by_id[request_id]["result"]["method"] == "tools/call" assert by_id[request_id]["result"]["params"]["seq"] == seq await _wait_for_sessions_to_close(running_broker) diff --git a/tests/unit/test_broker_daemon.py b/tests/unit/test_broker_daemon.py index 4066f7ac..62eec0a3 100644 --- a/tests/unit/test_broker_daemon.py +++ b/tests/unit/test_broker_daemon.py @@ -1167,6 +1167,8 @@ async def _readline() -> bytes: call_count += 1 if call_count == 1: return (init_response + "\n").encode() + while sum(b'"id":-1' in line and b"tools/list" in line for line in written_lines) < 1: + await asyncio.sleep(0) daemon._stop_event.set() return b"" @@ -1238,11 +1240,138 @@ async def _readline() -> bytes: await asyncio.wait_for(daemon._read_task, timeout=1.0) assert daemon._tools_list_cache is not None + assert daemon.tools_catalog_ready.is_set() import json as _json cached = _json.loads(daemon._tools_list_cache) assert cached["result"]["tools"][0]["name"] == "BuildProject" + @pytest.mark.asyncio + async def test_empty_tools_list_probe_keeps_catalog_gate_closed(self, tmp_path: Path) -> None: + """Empty tool catalogs must not be cached as a valid broker warm-up result.""" + cfg = _make_config(tmp_path) + daemon = BrokerDaemon(cfg) + + init_response = ( + '{"jsonrpc":"2.0","id":0,"result":{"protocolVersion":"2024-11-05","capabilities":{}}}' + ) + empty_tools_response = '{"jsonrpc":"2.0","id":-1,"result":{"tools":[]}}' + + call_count = 0 + + async def _readline() -> bytes: + nonlocal call_count + call_count += 1 + if call_count == 1: + return (init_response + "\n").encode() + if call_count == 2: + return (empty_tools_response + "\n").encode() + daemon._stop_event.set() + return b"" + + proc = _make_mock_process() + proc.stdout.readline = _readline + proc.stdin.drain = AsyncMock() + proc.stdin.write = MagicMock() + + with patch( + "mcpbridge_wrapper.broker.daemon.asyncio.create_subprocess_exec", + new=AsyncMock(return_value=proc), + ): + await daemon.start() + if daemon._read_task: + with contextlib.suppress(asyncio.TimeoutError): + await asyncio.wait_for(daemon._read_task, timeout=1.0) + retry_task = daemon._tools_probe_retry_task + if retry_task is not None: + retry_task.cancel() + with contextlib.suppress(asyncio.CancelledError): + await retry_task + + assert daemon._tools_list_cache is None + assert not daemon.tools_catalog_ready.is_set() + + @pytest.mark.asyncio + async def test_empty_tools_list_probe_retries_until_catalog_ready(self, tmp_path: Path) -> None: + """An empty warm-up probe must trigger a retry instead of a permanent outage.""" + cfg = _make_config(tmp_path) + daemon = BrokerDaemon(cfg) + + init_response = ( + '{"jsonrpc":"2.0","id":0,"result":{"protocolVersion":"2024-11-05","capabilities":{}}}' + ) + empty_tools_response = '{"jsonrpc":"2.0","id":-1,"result":{"tools":[]}}' + valid_tools_response = ( + '{"jsonrpc":"2.0","id":-1,"result":{"tools":[{"name":"BuildProject"}]}}' + ) + + sent_messages: list[str] = [] + call_count = 0 + + async def _wait_for_tools_probe_count(expected: int) -> None: + while sum('"method":"tools/list"' in msg for msg in sent_messages) < expected: + await asyncio.sleep(0) + + async def _readline() -> bytes: + nonlocal call_count + call_count += 1 + if call_count == 1: + return (init_response + "\n").encode() + if call_count == 2: + await _wait_for_tools_probe_count(1) + return (empty_tools_response + "\n").encode() + if call_count == 3: + await _wait_for_tools_probe_count(2) + daemon._stop_event.set() + return (valid_tools_response + "\n").encode() + return b"" + + def _write(data: bytes) -> None: + sent_messages.append(data.decode().rstrip("\n")) + + proc = _make_mock_process() + proc.stdout.readline = _readline + proc.stdin.drain = AsyncMock() + proc.stdin.write = MagicMock(side_effect=_write) + + with patch( + "mcpbridge_wrapper.broker.daemon._TOOLS_PROBE_RETRY_BASE_DELAY_SECONDS", + 0.0, + ), patch( + "mcpbridge_wrapper.broker.daemon._TOOLS_PROBE_RETRY_MAX_DELAY_SECONDS", + 0.0, + ), patch( + "mcpbridge_wrapper.broker.daemon.asyncio.create_subprocess_exec", + new=AsyncMock(return_value=proc), + ): + await daemon.start() + if daemon._read_task: + with contextlib.suppress(asyncio.TimeoutError): + await asyncio.wait_for(daemon._read_task, timeout=1.0) + + assert daemon._tools_list_cache is not None + assert daemon.tools_catalog_ready.is_set() + assert sum('"method":"tools/list"' in msg for msg in sent_messages) == 2 + + def test_tools_list_probe_retry_backoff_is_bounded_and_resets(self, tmp_path: Path) -> None: + """Retry delays should back off and reset after cancellation/success transitions.""" + cfg = _make_config(tmp_path) + daemon = BrokerDaemon(cfg) + + with patch( + "mcpbridge_wrapper.broker.daemon._TOOLS_PROBE_RETRY_BASE_DELAY_SECONDS", + 0.25, + ), patch( + "mcpbridge_wrapper.broker.daemon._TOOLS_PROBE_RETRY_MAX_DELAY_SECONDS", + 1.0, + ): + assert daemon._next_tools_probe_retry_delay() == 0.25 + assert daemon._next_tools_probe_retry_delay() == 0.5 + assert daemon._next_tools_probe_retry_delay() == 1.0 + assert daemon._next_tools_probe_retry_delay() == 1.0 + daemon._cancel_tools_probe_retry() + assert daemon._next_tools_probe_retry_delay() == 0.25 + @pytest.mark.asyncio async def test_upstream_initialized_cleared_on_reconnect(self, tmp_path: Path) -> None: """_upstream_initialized is cleared at the start of _reconnect().""" @@ -1265,6 +1394,7 @@ async def test_upstream_initialized_cleared_on_reconnect(self, tmp_path: Path) - # Both should be cleared at the start of _reconnect. assert not daemon._upstream_initialized.is_set() assert daemon._tools_list_cache is None + assert not daemon.tools_catalog_ready.is_set() @pytest.mark.asyncio async def test_send_broker_probes_noop_when_no_upstream(self, tmp_path: Path) -> None: diff --git a/tests/unit/test_broker_transport.py b/tests/unit/test_broker_transport.py index 85a57a2a..2e566255 100644 --- a/tests/unit/test_broker_transport.py +++ b/tests/unit/test_broker_transport.py @@ -75,6 +75,9 @@ def _make_daemon_mock(state: BrokerState = BrokerState.READY) -> MagicMock: ready_event = _asyncio.Event() ready_event.set() daemon.upstream_initialized = ready_event + tools_ready_event = _asyncio.Event() + tools_ready_event.set() + daemon.tools_catalog_ready = tools_ready_event # No cached tools/list by default. daemon._tools_list_cache = None return daemon @@ -451,7 +454,7 @@ async def test_stop_sends_32001_for_pending_requests(self, tmp_path: Any) -> Non class TestQueueTTL: @pytest.mark.asyncio async def test_ttl_exceeded_returns_32001(self, tmp_path: Any) -> None: - """When upstream_initialized is not set and TTL=0, returns -32001 immediately.""" + """When tools/list cache is not ready and TTL=0, returns -32001 immediately.""" cfg = _make_config(tmp_path) cfg = BrokerConfig( socket_path=cfg.socket_path, @@ -462,11 +465,10 @@ async def test_ttl_exceeded_returns_32001(self, tmp_path: Any) -> None: graceful_shutdown_timeout=1, ) daemon = _make_daemon_mock(state=BrokerState.RECONNECTING) - # Simulate upstream not yet initialized (Xcode approval pending). + # Simulate broker tool catalog not yet warmed (cold-start / approval pending). import asyncio as _asyncio - not_ready = _asyncio.Event() # NOT set - daemon.upstream_initialized = not_ready + daemon.tools_catalog_ready = _asyncio.Event() # NOT set server = UnixSocketServer(cfg, daemon) session = _make_session(1) server._sessions[1] = session @@ -478,10 +480,11 @@ async def test_ttl_exceeded_returns_32001(self, tmp_path: Any) -> None: response = json.loads(call_bytes.rstrip(b"\n")) assert response["error"]["code"] == -32001 assert "TTL" in response["error"]["message"] + assert "tools catalog not ready" in response["error"]["message"] @pytest.mark.asyncio async def test_upstream_ready_proceeds(self, tmp_path: Any) -> None: - """When upstream_initialized becomes set, request is forwarded to upstream.""" + """Non-tools/list requests wait only for upstream initialize readiness.""" import asyncio as _asyncio cfg = _make_config(tmp_path) @@ -500,12 +503,49 @@ async def _set_event() -> None: # Set event slightly after _process_client_line starts waiting. _asyncio.ensure_future(_set_event()) - request = json.dumps({"jsonrpc": "2.0", "id": 5, "method": "tools/list"}) + request = json.dumps({"jsonrpc": "2.0", "id": 5, "method": "ping"}) await server._process_client_line(session, request) # Request should have been forwarded to upstream daemon._upstream.stdin.write.assert_called() + @pytest.mark.asyncio + async def test_tools_list_waits_for_catalog_cache(self, tmp_path: Any) -> None: + """tools/list resumes only after broker cache warm-up completes.""" + import asyncio as _asyncio + + cfg = _make_config(tmp_path) + daemon = _make_daemon_mock(state=BrokerState.READY) + catalog_ready = _asyncio.Event() + daemon.tools_catalog_ready = catalog_ready + server = UnixSocketServer(cfg, daemon) + session = _make_session(1) + server._sessions[1] = session + + cached_raw = json.dumps( + { + "jsonrpc": "2.0", + "id": -1, + "result": {"tools": [{"name": "BuildProject"}]}, + } + ) + + async def _warm_cache() -> None: + await _asyncio.sleep(0.01) + daemon._tools_list_cache = cached_raw + catalog_ready.set() + + _asyncio.ensure_future(_warm_cache()) + + request = json.dumps({"jsonrpc": "2.0", "id": 5, "method": "tools/list"}) + await server._process_client_line(session, request) + + daemon._upstream.stdin.write.assert_not_called() + call_bytes: bytes = session.writer.write.call_args[0][0] + response = json.loads(call_bytes.rstrip(b"\n")) + assert response["id"] == 5 + assert response["result"]["tools"][0]["name"] == "BuildProject" + # --------------------------------------------------------------------------- # _handle_client — session registration and cleanup @@ -1332,6 +1372,7 @@ async def test_cache_hit_returns_cached_response(self, tmp_path: Any) -> None: server = _make_server(tmp_path) daemon = server._daemon daemon._tools_list_cache = cached_raw + daemon.tools_catalog_ready.set() session = _make_session(1) server._sessions[1] = session @@ -1357,6 +1398,7 @@ async def test_cache_hit_with_string_id(self, tmp_path: Any) -> None: server = _make_server(tmp_path) daemon = server._daemon daemon._tools_list_cache = cached_raw + daemon.tools_catalog_ready.set() session = _make_session(1) server._sessions[1] = session @@ -1370,12 +1412,13 @@ async def test_cache_hit_with_string_id(self, tmp_path: Any) -> None: @pytest.mark.asyncio async def test_cache_miss_forwards_to_upstream(self, tmp_path: Any) -> None: - """When _tools_list_cache is None, tools/list is forwarded to upstream.""" + """Fallback forwards only when the catalog-ready gate is already open.""" import json as _json server = _make_server(tmp_path) daemon = server._daemon assert daemon._tools_list_cache is None # default fixture value + daemon.tools_catalog_ready.set() session = _make_session(1) server._sessions[1] = session