diff --git a/SPECS/ARCHIVE/INDEX.md b/SPECS/ARCHIVE/INDEX.md index 69bf142..d7df7a0 100644 --- a/SPECS/ARCHIVE/INDEX.md +++ b/SPECS/ARCHIVE/INDEX.md @@ -1,11 +1,12 @@ # mcpbridge-wrapper Tasks Archive -**Last Updated:** 2026-03-04 (P1-T6 archived) +**Last Updated:** 2026-03-05 (P4-T1 archived) ## Archived Tasks | Task ID | Folder | Archived | Verdict | |---------|--------|----------|---------| +| P4-T1 | [P4-T1_Auto_restart_stale_broker_daemon_on_version_mismatch_after_upgrade/](P4-T1_Auto_restart_stale_broker_daemon_on_version_mismatch_after_upgrade/) | 2026-03-05 | PASS | | P1-T6 | [P1-T6_Update_webui_setup_and_DocC_mirror_to_use_broker_in_multi_agent_examples/](P1-T6_Update_webui_setup_and_DocC_mirror_to_use_broker_in_multi_agent_examples/) | 2026-03-04 | PASS | | P1-T5 | [P1-T5_Fix_missed_broker_spawn_references_in_troubleshooting/](P1-T5_Fix_missed_broker_spawn_references_in_troubleshooting/) | 2026-03-04 | PASS | | P1-T9 | [P1-T9_Add_direct_links_for_all_command_steps_in_FLOW/](P1-T9_Add_direct_links_for_all_command_steps_in_FLOW/) | 2026-03-03 | PASS | @@ -185,6 +186,7 @@ | File | Description | |------|-------------| +| [REVIEW_p4_t1_broker_version_restart.md](_Historical/REVIEW_p4_t1_broker_version_restart.md) | Review report for P4-T1 | | [REVIEW_p1_t6_webui_broker_examples.md](_Historical/REVIEW_p1_t6_webui_broker_examples.md) | Review report for P1-T6 | | [REVIEW_p1_t5_troubleshooting_broker.md](_Historical/REVIEW_p1_t5_troubleshooting_broker.md) | Review report for P1-T5 | | [REVIEW_p1_t9_flow_command_links.md](_Historical/REVIEW_p1_t9_flow_command_links.md) | Review report for P1-T9 | @@ -315,6 +317,8 @@ | Date | Task ID | Action | |------|---------|--------| +| 2026-03-05 | P4-T1 | Archived REVIEW_p4_t1_broker_version_restart report | +| 2026-03-05 | P4-T1 | Archived Auto_restart_stale_broker_daemon_on_version_mismatch_after_upgrade (PASS) | | 2026-03-04 | P1-T6 | Archived REVIEW_p1_t6_webui_broker_examples report | | 2026-03-04 | P1-T6 | Archived Update webui-setup.md and DocC mirror to use --broker in multi-agent examples (PASS) | | 2026-03-04 | P1-T5 | Archived REVIEW_p1_t5_troubleshooting_broker report | diff --git a/SPECS/ARCHIVE/P4-T1_Auto_restart_stale_broker_daemon_on_version_mismatch_after_upgrade/P4-T1_Auto_restart_stale_broker_daemon_on_version_mismatch_after_upgrade.md b/SPECS/ARCHIVE/P4-T1_Auto_restart_stale_broker_daemon_on_version_mismatch_after_upgrade/P4-T1_Auto_restart_stale_broker_daemon_on_version_mismatch_after_upgrade.md new file mode 100644 index 0000000..68ab54c --- /dev/null +++ b/SPECS/ARCHIVE/P4-T1_Auto_restart_stale_broker_daemon_on_version_mismatch_after_upgrade/P4-T1_Auto_restart_stale_broker_daemon_on_version_mismatch_after_upgrade.md @@ -0,0 +1,85 @@ +# P4-T1 PRD — Auto-restart stale broker daemon on version mismatch after upgrade + +## Task Metadata + +- **Task ID:** P4-T1 +- **Phase:** Phase 4: Broker Lifecycle Management +- **Priority:** P0 +- **Dependencies:** none +- **Source:** `SPECS/Workplan.md` open task entry + +## Objective Summary + +Prevent stale broker daemons from surviving wrapper upgrades and silently serving old behavior to new `--broker` clients. The wrapper must use a single version source of truth, persist daemon runtime version in broker state, detect mismatch before proxy reuse, and automatically restart stale daemons. The CLI must also expose explicit lifecycle control (`--broker-status`, `--broker-stop`) so users and scripts can inspect and remediate broker state without manual PID/socket file handling. + +The implementation scope includes runtime behavior, CLI surface, install/uninstall operational safety, docs, and regression tests. Backwards compatibility requirement: legacy daemons without a version file must still be accepted as compatible unless a mismatch can be proven. + +## Success Criteria + +1. `__version__` is derived from package metadata (`importlib.metadata`) instead of hard-coded constants. +2. Broker daemon writes `broker.version` on startup and removes it during shutdown/cleanup paths. +3. Proxy detects mismatched daemon version and auto-restarts stale daemon before connecting. +4. Missing `broker.version` file is treated as compatible (no forced restart). +5. `--broker-status` reports PID/status/version info and mismatch warning. +6. `--broker-stop` gracefully stops daemon, waits for exit, and removes pid/socket/version state files. +7. Install and uninstall scripts stop any running broker daemon before replacing/removing wrapper files. +8. Broker-mode docs describe new lifecycle commands and version management flow. +9. Required quality gates pass: `pytest`, `ruff check src/`, `mypy src/mcpbridge_wrapper`, `pytest --cov=src/mcpbridge_wrapper --cov-report=term-missing` (>= 90%). + +## Acceptance Tests + +- Unit tests for `BrokerConfig.version_file` path derivation. +- Unit tests for daemon version-file write/remove and status payload version field. +- Unit tests for proxy version mismatch detection and restart behavior. +- Unit tests for `_parse_broker_args` handling of `--broker-status` and `--broker-stop`. +- CLI behavior validation: + - `python -m mcpbridge_wrapper --broker-status` + - `python -m mcpbridge_wrapper --broker-stop` +- Script-level behavior validation for install/uninstall broker-stop logic (covered by shell-path checks and existing script tests if present). + +## Test-First Plan + +1. Add/extend tests for version file state in broker config and daemon lifecycle. +2. Add/extend proxy tests for mismatch detection, backward-compatible no-version behavior, and stale daemon restart. +3. Add/extend main/CLI tests for new broker control flags and argument parsing. +4. Implement runtime logic minimally to satisfy new failing tests. +5. Update docs and scripts after runtime tests pass. +6. Run full quality gates and capture outputs in validation report. + +## Implementation Plan (Hierarchical TODO) + +### Phase A — Version Source and Broker State + +- **Inputs:** Existing `__version__` declaration, `BrokerConfig`, daemon lifecycle logic. +- **Outputs:** Metadata-derived version string; `version_file` property; daemon writes/cleans version file; status includes version. +- **Verification:** New daemon/config unit tests pass and stale-lock cleanup removes version artifacts. + +### Phase B — Proxy and CLI Lifecycle Controls + +- **Inputs:** Proxy spawn/reuse flow, broker arg parsing and command dispatch in `__main__.py`. +- **Outputs:** Version mismatch detection + stale daemon restart; `--broker-status`; `--broker-stop` cleanup behavior. +- **Verification:** New proxy/main tests pass and manual status/stop commands return expected output/exit semantics. + +### Phase C — Operational Scripts, Docs, and Validation + +- **Inputs:** `scripts/install.sh`, `scripts/uninstall.sh`, `docs/broker-mode.md`, quality gate tooling. +- **Outputs:** Install/uninstall stop running daemon before file replacement/removal; docs updated for status/stop/version behavior; validation report. +- **Verification:** Required quality gates pass; acceptance criteria checklist fully satisfied. + +## Decision Points and Constraints + +- Use defensive behavior when metadata/version files are unavailable: prefer safe fallback over hard failure. +- Keep broker-stop idempotent (safe when daemon already dead or files are stale/corrupt). +- Avoid introducing behavior that depends on non-portable process APIs beyond existing macOS/Linux constraints. +- Keep implementation scoped to P4-T1 artifacts; do not refactor unrelated broker subsystems. + +## Notes (Post-Completion Docs/Artifacts) + +- Create `SPECS/INPROGRESS/P4-T1_Validation_Report.md` with command evidence and verdict. +- Archive PRD and validation report to `SPECS/ARCHIVE/P4-T1_Auto_restart_stale_broker_daemon_on_version_mismatch_after_upgrade/`. +- Mark `P4-T1` complete in `SPECS/Workplan.md` and update `SPECS/ARCHIVE/INDEX.md`. +- Create and archive `REVIEW_p4_t1_broker_version_restart.md` after review. + +--- +**Archived:** 2026-03-05 +**Verdict:** PASS diff --git a/SPECS/ARCHIVE/P4-T1_Auto_restart_stale_broker_daemon_on_version_mismatch_after_upgrade/P4-T1_Validation_Report.md b/SPECS/ARCHIVE/P4-T1_Auto_restart_stale_broker_daemon_on_version_mismatch_after_upgrade/P4-T1_Validation_Report.md new file mode 100644 index 0000000..0330c01 --- /dev/null +++ b/SPECS/ARCHIVE/P4-T1_Auto_restart_stale_broker_daemon_on_version_mismatch_after_upgrade/P4-T1_Validation_Report.md @@ -0,0 +1,72 @@ +# Validation Report — P4-T1 + +**Task:** P4-T1 — Auto-restart stale broker daemon on version mismatch after upgrade +**Date:** 2026-03-05 +**Executor:** Codex (`flow-run`) +**Verdict:** PASS + +## Summary + +Implemented and validated version-aware broker lifecycle handling across runtime code, CLI, scripts, documentation, and tests. + +Key outcomes: +- `__version__` now resolves from package metadata with safe fallback. +- Broker daemon writes/cleans `broker.version` and reports version in status payload. +- Proxy detects daemon version mismatch and restarts stale daemon before reuse. +- Added `--broker-status` and `--broker-stop` commands. +- Install/uninstall scripts now stop running broker daemons before file operations. +- Broker mode docs updated for new status/stop/version management workflow. +- Coverage restored above threshold after adding targeted tests for new paths. + +## Acceptance Criteria Check + +- [x] `__version__` derived from `importlib.metadata` (single source: `pyproject.toml`) +- [x] Daemon writes `~/.mcpbridge_wrapper/broker.version` on start and cleans on stop +- [x] Proxy auto-restarts daemon when version file mismatches current `__version__` +- [x] No version file (old daemon) treated as compatible +- [x] `--broker-status` prints daemon PID, version, mismatch warning +- [x] `--broker-stop` sends SIGTERM, waits, and cleans up state files +- [x] `scripts/install.sh` stops running broker daemon before writing new wrapper +- [x] `scripts/uninstall.sh` stops running broker daemon before removing files +- [x] `docs/broker-mode.md` documents `--broker-stop`, `--broker-status`, and version management +- [x] All quality gates pass (`pytest`, `ruff`, `mypy`, coverage >= 90%) + +## Quality Gate Evidence + +```bash +pytest +``` + +Result: `766 passed, 5 skipped, 2 warnings`. + +```bash +ruff check src/ +``` + +Result: `All checks passed!`. + +```bash +mypy src/mcpbridge_wrapper +``` + +Result: `Success: no issues found in 18 source files`. + +```bash +pytest --cov=src/mcpbridge_wrapper --cov-report=term-missing +``` + +Result: `766 passed, 5 skipped, 2 warnings`; total coverage `90.71%` (threshold `90%`). + +## Additional Test Coverage Added + +- `tests/unit/test_main.py`: + - New coverage for `--broker-status` and `--broker-stop` runtime branches. +- `tests/unit/test_broker_proxy.py`: + - New coverage for version-file read errors and `_stop_stale_daemon` cleanup paths. +- `tests/unit/test_init.py`: + - Metadata-derived version and fallback behavior. + +## Notes + +- Existing deprecation warnings from websocket dependencies remain unchanged and are non-blocking for this task. +- Ready for ARCHIVE step. diff --git a/SPECS/ARCHIVE/_Historical/REVIEW_p4_t1_broker_version_restart.md b/SPECS/ARCHIVE/_Historical/REVIEW_p4_t1_broker_version_restart.md new file mode 100644 index 0000000..9909539 --- /dev/null +++ b/SPECS/ARCHIVE/_Historical/REVIEW_p4_t1_broker_version_restart.md @@ -0,0 +1,32 @@ +## REVIEW REPORT — P4-T1 Broker version restart + +**Scope:** origin/main..HEAD +**Files:** 18 + +### Summary Verdict +- [x] Approve +- [ ] Approve with comments +- [ ] Request changes +- [ ] Block + +### Critical Issues +- None. + +### Secondary Issues +- None. + +### Architectural Notes +- Version source-of-truth now resolves from package metadata and is consistently reused across daemon/proxy status paths. +- Version mismatch handling is defensive and backward-compatible: no `broker.version` file does not force restart. +- Lifecycle command additions (`--broker-status`, `--broker-stop`) improve operability without changing normal stdio bridge behavior. + +### Tests +- Quality gates are documented in `SPECS/ARCHIVE/P4-T1_Auto_restart_stale_broker_daemon_on_version_mismatch_after_upgrade/P4-T1_Validation_Report.md`: + - `pytest` → `766 passed, 5 skipped` + - `ruff check src/` → pass + - `mypy src/mcpbridge_wrapper` → pass + - `pytest --cov=src/mcpbridge_wrapper --cov-report=term-missing` → `90.71%` + +### Next Steps +- No actionable review findings. +- FOLLOW-UP step is explicitly skipped. diff --git a/SPECS/INPROGRESS/next.md b/SPECS/INPROGRESS/next.md index 95522f3..7cafe0f 100644 --- a/SPECS/INPROGRESS/next.md +++ b/SPECS/INPROGRESS/next.md @@ -1,14 +1,14 @@ # No Active Task -**Status:** Idle — P1-T6 archived. Select the next task from `SPECS/Workplan.md`. +**Status:** Idle — P4-T1 archived. Select the next task from `SPECS/Workplan.md`. ## Recently Archived +- **P4-T1** — Auto-restart stale broker daemon on version mismatch after upgrade (2026-03-05, PASS) - **P1-T6** — Update webui-setup.md and DocC mirror to use --broker in multi-agent examples (2026-03-04, PASS) - **P1-T5** — Fix missed --broker-spawn references in troubleshooting.md "MCP tools are green" section (2026-03-04, PASS) - **P1-T9** — Add direct links for all command steps in FLOW.md (2026-03-03, PASS) - **P3-T11** — Add Stop broker/service control button to Web UI (2026-03-01, PASS) -- **P1-T8** — Update /config examples for broker setup first (2026-03-01, PASS) ## Suggested Next Tasks diff --git a/SPECS/Workplan.md b/SPECS/Workplan.md index 19ab6d4..b25dde1 100644 --- a/SPECS/Workplan.md +++ b/SPECS/Workplan.md @@ -265,3 +265,32 @@ Add new tasks using the canonical template in [TASK_TEMPLATE.md](TASK_TEMPLATE.m - [x] A proxy session forwarding `initialize` → `notifications/initialized` → `tools/list` returns 20 tools without the proxy exiting early - [x] All existing broker tests pass (715 passed, 5 skipped) - [x] MCP clients in broker mode (e.g. Zed with `--broker-spawn`) show the correct tool count + +### Phase 4: Broker Lifecycle Management + +#### ✅ P4-T1: Auto-restart stale broker daemon on version mismatch after upgrade +- **Status:** ✅ Completed (2026-03-05) +- **Description:** When users upgrade mcpbridge-wrapper, the old broker daemon keeps running with the old binary. New `--broker` clients silently connect to the stale daemon instead of using updated code. Fix by: (1) fixing version source of truth (`__init__.py` uses `importlib.metadata` from `pyproject.toml`), (2) daemon writes `broker.version` file on startup, (3) proxy checks version before connecting and auto-restarts mismatched daemons, (4) adding `--broker-stop` and `--broker-status` CLI commands, (5) install/uninstall scripts stop running daemons, (6) updating broker-mode docs. +- **Priority:** P0 +- **Dependencies:** none +- **Parallelizable:** yes +- **Outputs/Artifacts:** + - `src/mcpbridge_wrapper/__init__.py` — `importlib.metadata`-based `__version__` + - `src/mcpbridge_wrapper/broker/types.py` — `version_file` property on `BrokerConfig` + - `src/mcpbridge_wrapper/broker/daemon.py` — version file write/cleanup/status + - `src/mcpbridge_wrapper/broker/proxy.py` — `_check_version_mismatch()`, `_stop_stale_daemon()` + - `src/mcpbridge_wrapper/__main__.py` — `--broker-stop`, `--broker-status` CLI commands + - `scripts/install.sh` — stop running broker on install + - `scripts/uninstall.sh` — stop running broker on uninstall + - `docs/broker-mode.md` — CLI commands and version management section +- **Acceptance Criteria:** + - [x] `__version__` derived from `importlib.metadata` (single source: `pyproject.toml`) + - [x] Daemon writes `~/.mcpbridge_wrapper/broker.version` on start and cleans on stop + - [x] Proxy auto-restarts daemon when version file mismatches current `__version__` + - [x] No version file (old daemon) is treated as compatible (backwards-compatible) + - [x] `--broker-status` prints daemon PID, version, mismatch warning + - [x] `--broker-stop` sends SIGTERM, waits, and cleans up state files + - [x] `scripts/install.sh` stops running broker daemon before writing new wrapper + - [x] `scripts/uninstall.sh` stops running broker daemon before removing files + - [x] `docs/broker-mode.md` documents `--broker-stop`, `--broker-status`, and version management + - [x] All quality gates pass (`pytest`, `ruff`, `mypy`, coverage >= 90%) diff --git a/docs/broker-mode.md b/docs/broker-mode.md index 9505edc..968918c 100644 --- a/docs/broker-mode.md +++ b/docs/broker-mode.md @@ -71,22 +71,11 @@ uvx --from 'mcpbridge-wrapper[webui]' mcpbridge-wrapper \ ### Status ```bash -PID_FILE="$HOME/.mcpbridge_wrapper/broker.pid" -SOCK="$HOME/.mcpbridge_wrapper/broker.sock" - -if [ -f "$PID_FILE" ] && kill -0 "$(cat "$PID_FILE")" 2>/dev/null; then - echo "broker: running (pid $(cat "$PID_FILE"))" -else - echo "broker: stopped" -fi - -if [ -S "$SOCK" ]; then - echo "socket: present ($SOCK)" -else - echo "socket: missing ($SOCK)" -fi +mcpbridge-wrapper --broker-status ``` +Prints proxy version, daemon PID, daemon version, file paths, and warns on version mismatch. + ### Logs ```bash @@ -96,15 +85,24 @@ tail -f "$HOME/.mcpbridge_wrapper/broker.log" ### Stop ```bash -PID_FILE="$HOME/.mcpbridge_wrapper/broker.pid" -SOCK="$HOME/.mcpbridge_wrapper/broker.sock" - -if [ -f "$PID_FILE" ]; then - kill "$(cat "$PID_FILE")" 2>/dev/null || true -fi -rm -f "$PID_FILE" "$SOCK" +mcpbridge-wrapper --broker-stop ``` +Sends SIGTERM to the running daemon and waits up to 3 seconds for a clean exit. If the daemon exits, PID/socket/version files are removed. If it does not exit in time, the command returns an error and preserves state files for manual recovery. + +## Version management + +When upgrading `mcpbridge-wrapper` (via `pip install`, `uvx`, or `./scripts/install.sh`): + +1. The **install script** automatically stops any running broker daemon. +2. On next `--broker` launch, the proxy compares its version against the daemon's + version file (`~/.mcpbridge_wrapper/broker.version`). If versions differ, the + stale daemon is stopped and a fresh one is spawned automatically. +3. Use `--broker-status` to verify the running daemon matches the installed version. + +If an older daemon was started before the upgrade and you want to force an immediate +restart, run `mcpbridge-wrapper --broker-stop` followed by any `--broker` command. + ## Client configuration examples ### Unified single-config examples (recommended) @@ -190,13 +188,10 @@ If you prefer explicit host lifecycle management, start one `--broker-daemon --w 1. Remove `--broker` from MCP config args. 2. Restart the MCP client. -3. Stop any running broker process and remove stale files: +3. Stop any running broker process: ```bash -PID_FILE="$HOME/.mcpbridge_wrapper/broker.pid" -SOCK="$HOME/.mcpbridge_wrapper/broker.sock" -if [ -f "$PID_FILE" ]; then kill "$(cat "$PID_FILE")" 2>/dev/null || true; fi -rm -f "$PID_FILE" "$SOCK" +mcpbridge-wrapper --broker-stop ``` 4. Verify direct mode behavior by running one tool call and confirming no broker files are recreated. diff --git a/scripts/install.sh b/scripts/install.sh index 0760cef..15a01ff 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -99,6 +99,22 @@ else python3 -m pip install -e . --quiet fi +# Stop any running broker daemon so the new version takes over on next launch +BROKER_PID_FILE="$HOME/.mcpbridge_wrapper/broker.pid" +if [ -f "$BROKER_PID_FILE" ]; then + BROKER_PID=$(cat "$BROKER_PID_FILE" 2>/dev/null) + if [ -n "$BROKER_PID" ] && kill -0 "$BROKER_PID" 2>/dev/null; then + echo "Stopping running broker daemon (PID $BROKER_PID)..." + kill "$BROKER_PID" 2>/dev/null || true + sleep 1 + if kill -0 "$BROKER_PID" 2>/dev/null; then + sleep 2 + fi + fi + rm -f "$BROKER_PID_FILE" "$HOME/.mcpbridge_wrapper/broker.sock" "$HOME/.mcpbridge_wrapper/broker.version" + echo -e "${GREEN}✓ Old broker daemon stopped${NC}" +fi + # Create wrapper script in ~/bin that uses the venv Python cat > "$INSTALL_DIR/xcodemcpwrapper" << WRAPPER #!/bin/bash diff --git a/scripts/uninstall.sh b/scripts/uninstall.sh index 89ec78b..9b2ea0a 100755 --- a/scripts/uninstall.sh +++ b/scripts/uninstall.sh @@ -159,6 +159,22 @@ fi echo "Uninstalling..." echo "" +# Stop any running broker daemon before removing files +BROKER_PID_FILE="$HOME/.mcpbridge_wrapper/broker.pid" +if [ -f "$BROKER_PID_FILE" ]; then + BROKER_PID=$(cat "$BROKER_PID_FILE" 2>/dev/null) + if [ -n "$BROKER_PID" ] && kill -0 "$BROKER_PID" 2>/dev/null; then + echo "Stopping running broker daemon (PID $BROKER_PID)..." + kill "$BROKER_PID" 2>/dev/null || true + sleep 1 + if kill -0 "$BROKER_PID" 2>/dev/null; then + sleep 2 + fi + fi + rm -f "$BROKER_PID_FILE" "$HOME/.mcpbridge_wrapper/broker.sock" "$HOME/.mcpbridge_wrapper/broker.version" + echo -e "${GREEN}✓ Broker daemon stopped${NC}" +fi + # Remove pip package(s) if [ "$PIP_PACKAGE_EXISTS" = true ]; then for pkg in "${DETECTED_PIP_PACKAGES[@]}"; do diff --git a/src/mcpbridge_wrapper/__init__.py b/src/mcpbridge_wrapper/__init__.py index 1be0fc6..9597058 100644 --- a/src/mcpbridge_wrapper/__init__.py +++ b/src/mcpbridge_wrapper/__init__.py @@ -21,7 +21,12 @@ process_response_line, ) -__version__ = "1.0.0" +try: + from importlib.metadata import version as _meta_version + + __version__ = _meta_version("mcpbridge-wrapper") +except Exception: # PackageNotFoundError or any import issue + __version__ = "0.0.0+unknown" __all__ = [ # Bridge functions "create_bridge", diff --git a/src/mcpbridge_wrapper/__main__.py b/src/mcpbridge_wrapper/__main__.py index 45bcea4..9ae83e0 100644 --- a/src/mcpbridge_wrapper/__main__.py +++ b/src/mcpbridge_wrapper/__main__.py @@ -280,12 +280,12 @@ def _has_error(line: str) -> bool: def _parse_broker_args( args: list, -) -> Tuple[bool, bool, bool, list]: +) -> Tuple[bool, bool, bool, bool, bool, list]: """Parse broker arguments from command-line args. - Extracts ``--broker-daemon`` and ``--broker`` flags and returns them - along with the remaining args to forward to the bridge. Broker-only flags - are *never* forwarded to + Extracts ``--broker-daemon``, ``--broker``, ``--broker-status``, and + ``--broker-stop`` flags and returns them along with the remaining args + to forward to the bridge. Broker-only flags are *never* forwarded to ``xcrun mcpbridge``. ``--broker`` is the recommended flag: it auto-detects whether a daemon is @@ -295,11 +295,14 @@ def _parse_broker_args( args: Command-line arguments list. Returns: - Tuple of (broker_daemon, broker_connect, broker_spawn, remaining_args). + Tuple of (broker_daemon, broker_connect, broker_spawn, + broker_status, broker_stop, remaining_args). """ broker_daemon = False broker_connect = False broker_spawn = False + broker_status = False + broker_stop = False remaining = [] for arg in args: @@ -309,10 +312,14 @@ def _parse_broker_args( # Recommended flag: auto-detect (spawn if needed, then connect). broker_spawn = True broker_connect = True + elif arg == "--broker-status": + broker_status = True + elif arg == "--broker-stop": + broker_stop = True else: remaining.append(arg) - return broker_daemon, broker_connect, broker_spawn, remaining + return broker_daemon, broker_connect, broker_spawn, broker_status, broker_stop, remaining def _track_pending_method( @@ -488,7 +495,9 @@ def main() -> int: print(f"Error: {exc}", file=sys.stderr) return 2 - broker_daemon, broker_connect, broker_spawn, bridge_args = _parse_broker_args(after_webui_args) + broker_daemon, broker_connect, broker_spawn, broker_status, broker_stop, bridge_args = ( + _parse_broker_args(after_webui_args) + ) if web_ui_only and (broker_daemon or broker_connect): print( @@ -497,6 +506,104 @@ def main() -> int: ) return 2 + # --broker-status: print broker daemon status and exit + if broker_status: + from mcpbridge_wrapper import __version__ + from mcpbridge_wrapper.broker.types import BrokerConfig + + broker_config = BrokerConfig.default() + print(f"Proxy version: {__version__}") + print(f"PID file: {broker_config.pid_file}") + print(f"Socket: {broker_config.socket_path}") + print(f"Version file: {broker_config.version_file}") + + pid: Optional[int] = None + if broker_config.pid_file.exists(): + try: + pid = int(broker_config.pid_file.read_text().strip()) + os.kill(pid, 0) + print(f"Daemon PID: {pid} (running)") + except (ValueError, ProcessLookupError): + print("Daemon PID: (not running)") + pid = None + except PermissionError: + print(f"Daemon PID: {pid} (running, different user)") + else: + print("Daemon PID: (not running)") + + daemon_version: Optional[str] = None + if broker_config.version_file.exists(): + with contextlib.suppress(OSError): + daemon_version = broker_config.version_file.read_text().strip() + if daemon_version: + print(f"Daemon version: {daemon_version}") + if daemon_version != __version__: + print(f"WARNING: version mismatch! proxy={__version__}, daemon={daemon_version}") + else: + print("Daemon version: (unknown)") + return 0 + + # --broker-stop: stop running broker daemon and exit + if broker_stop: + from mcpbridge_wrapper.broker.types import BrokerConfig + + broker_config = BrokerConfig.default() + pid_file = broker_config.pid_file + if not pid_file.exists(): + print("Broker is not running (no PID file).") + return 0 + try: + pid_val = int(pid_file.read_text().strip()) + except (ValueError, OSError): + print("Corrupt PID file; cleaning up.", file=sys.stderr) + for p in (pid_file, broker_config.socket_path, broker_config.version_file): + p.unlink(missing_ok=True) + return 0 + + stopped = False + try: + os.kill(pid_val, signal.SIGTERM) + print(f"Sent SIGTERM to broker (PID {pid_val}).") + except ProcessLookupError: + print(f"Broker (PID {pid_val}) is not running; cleaning up files.") + stopped = True + except PermissionError: + print( + f"Error: Cannot stop broker (PID {pid_val}): permission denied.", + file=sys.stderr, + ) + return 1 + + # Wait up to 3 seconds for clean exit. + if not stopped: + deadline = time.monotonic() + 3.0 + while time.monotonic() < deadline: + try: + os.kill(pid_val, 0) + except ProcessLookupError: + stopped = True + break + time.sleep(0.1) + + # Final probe in case process exited just after timeout boundary. + if not stopped: + try: + os.kill(pid_val, 0) + except ProcessLookupError: + stopped = True + + if not stopped: + print( + "Error: Broker did not stop within 3 seconds; state files were left intact.", + file=sys.stderr, + ) + return 1 + + for p in (pid_file, broker_config.socket_path, broker_config.version_file): + p.unlink(missing_ok=True) + print("Broker stopped and files cleaned up.") + return 0 + # Broker daemon mode: long-lived upstream + Unix socket server if broker_daemon: import asyncio diff --git a/src/mcpbridge_wrapper/broker/daemon.py b/src/mcpbridge_wrapper/broker/daemon.py index 7632c5b..c40a778 100644 --- a/src/mcpbridge_wrapper/broker/daemon.py +++ b/src/mcpbridge_wrapper/broker/daemon.py @@ -30,6 +30,7 @@ from asyncio.subprocess import PIPE from typing import TYPE_CHECKING, Any +from mcpbridge_wrapper import __version__ from mcpbridge_wrapper.broker.types import BrokerConfig, BrokerState if TYPE_CHECKING: @@ -99,6 +100,7 @@ def status(self) -> dict[str, Any]: "state": self._state.value, "pid": os.getpid(), "upstream_pid": upstream_pid, + "version": __version__, } def request_shutdown(self) -> None: @@ -150,6 +152,10 @@ async def start(self) -> None: self._config.pid_file.write_text(str(os.getpid())) logger.debug("PID file written: %s", self._config.pid_file) + # Write version stamp so proxy clients can detect version mismatches. + self._config.version_file.write_text(__version__) + logger.debug("Version file written: %s", self._config.version_file) + # Background reader self._stop_event.clear() self._stopped_event.clear() @@ -307,11 +313,13 @@ def _check_and_clear_stale_lock(self) -> None: """ pid_file = self._config.pid_file sock_file = self._config.socket_path + ver_file = self._config.version_file if not pid_file.exists(): - # No lock file — clear any orphaned socket and proceed + # No lock file — clear any orphaned socket/version and proceed if sock_file.exists(): sock_file.unlink(missing_ok=True) + ver_file.unlink(missing_ok=True) return raw = pid_file.read_text().strip() @@ -321,6 +329,7 @@ def _check_and_clear_stale_lock(self) -> None: logger.warning("Corrupt PID file (%r); removing.", raw) pid_file.unlink(missing_ok=True) sock_file.unlink(missing_ok=True) + ver_file.unlink(missing_ok=True) return try: @@ -335,6 +344,7 @@ def _check_and_clear_stale_lock(self) -> None: logger.info("Stale lock found (PID %d dead); cleaning up.", pid) pid_file.unlink(missing_ok=True) sock_file.unlink(missing_ok=True) + ver_file.unlink(missing_ok=True) except PermissionError as err: # Process exists but owned by another user — treat as running raise RuntimeError( @@ -426,8 +436,8 @@ async def _reconnect(self) -> None: self._state = BrokerState.STOPPING def _cleanup_files(self) -> None: - """Remove PID file and socket file.""" - for path in (self._config.pid_file, self._config.socket_path): + """Remove PID file, socket file, and version file.""" + for path in (self._config.pid_file, self._config.socket_path, self._config.version_file): try: path.unlink(missing_ok=True) logger.debug("Removed %s", path) diff --git a/src/mcpbridge_wrapper/broker/proxy.py b/src/mcpbridge_wrapper/broker/proxy.py index 887f075..c549c20 100644 --- a/src/mcpbridge_wrapper/broker/proxy.py +++ b/src/mcpbridge_wrapper/broker/proxy.py @@ -20,9 +20,13 @@ import json import logging import os +import signal import socket +import subprocess import sys +import time +from mcpbridge_wrapper import __version__ from mcpbridge_wrapper.broker.types import BrokerConfig logger = logging.getLogger(__name__) @@ -197,6 +201,87 @@ def _warn_web_ui_mismatch(self) -> None: file=sys.stderr, ) + def _check_version_mismatch(self) -> bool: + """Return True if the running daemon's version differs from this proxy's. + + Returns False (no mismatch) when the version file does not exist — this + handles the backwards-compatible case where an older daemon did not + write a version file. + """ + version_file = self._config.version_file + if not version_file.exists(): + return False + try: + daemon_version = version_file.read_text().strip() + except OSError: + return False + if daemon_version == __version__: + return False + logger.warning( + "Broker version mismatch: daemon=%s, proxy=%s", + daemon_version, + __version__, + ) + return True + + def _pid_belongs_to_broker(self, pid: int) -> bool: + """Return True when PID command line matches broker daemon shape.""" + try: + cmdline = subprocess.check_output( + ["ps", "-p", str(pid), "-o", "command="], + stderr=subprocess.DEVNULL, + text=True, + ).strip() + except (OSError, subprocess.CalledProcessError): + return False + return "mcpbridge_wrapper" in cmdline and "--broker-daemon" in cmdline + + def _stop_stale_daemon(self) -> None: + """Stop a running broker daemon via SIGTERM + wait + file cleanup.""" + pid_file = self._config.pid_file + if not pid_file.exists(): + return + try: + pid = int(pid_file.read_text().strip()) + except (ValueError, OSError): + return + + if not self._pid_belongs_to_broker(pid): + logger.warning( + "PID %d from %s is not a broker daemon; cleaning stale files without SIGTERM.", + pid, + pid_file, + ) + self._cleanup_broker_files() + return + + try: + os.kill(pid, signal.SIGTERM) + except (ProcessLookupError, PermissionError): + # Already dead or not ours — just clean up files. + self._cleanup_broker_files() + return + + # Wait up to 3 seconds for the process to exit. + deadline = time.monotonic() + 3.0 + while time.monotonic() < deadline: + try: + os.kill(pid, 0) + except ProcessLookupError: + break + time.sleep(0.1) + + self._cleanup_broker_files() + + def _cleanup_broker_files(self) -> None: + """Remove broker PID, socket, and version files.""" + for path in ( + self._config.pid_file, + self._config.socket_path, + self._config.version_file, + ): + path.unlink(missing_ok=True) + async def _spawn_broker_if_needed(self) -> None: """Spawn the broker daemon if not already running. @@ -233,8 +318,14 @@ async def _spawn_broker_if_needed(self) -> None: try: pid = int(pid_file.read_text().strip()) os.kill(pid, 0) - logger.debug("Broker already running (PID %d); skipping spawn.", pid) - return + # Daemon is alive — check for version mismatch. + if self._check_version_mismatch(): + logger.info("Stopping stale broker (version mismatch)…") + await loop.run_in_executor(None, self._stop_stale_daemon) + # Fall through to spawn a new daemon. + else: + logger.debug("Broker already running (PID %d); skipping spawn.", pid) + return except (ValueError, ProcessLookupError, PermissionError): logger.debug("Stale PID file; will spawn broker.") @@ -255,6 +346,7 @@ async def _spawn_broker_if_needed(self) -> None: ) socket_path.unlink(missing_ok=True) pid_file.unlink(missing_ok=True) + self._config.version_file.unlink(missing_ok=True) # Fall through to spawn. logger.info("Spawning broker daemon…") diff --git a/src/mcpbridge_wrapper/broker/types.py b/src/mcpbridge_wrapper/broker/types.py index 299fdad..a203b75 100644 --- a/src/mcpbridge_wrapper/broker/types.py +++ b/src/mcpbridge_wrapper/broker/types.py @@ -44,6 +44,11 @@ class BrokerConfig: queue_ttl: int = 60 graceful_shutdown_timeout: int = 5 + @property + def version_file(self) -> Path: + """Path to the version stamp file, derived from pid_file's directory.""" + return self.pid_file.parent / "broker.version" + @classmethod def default(cls) -> BrokerConfig: """Return config with default paths under ~/.mcpbridge_wrapper/.""" diff --git a/tests/unit/test_broker_daemon.py b/tests/unit/test_broker_daemon.py index cc76c1c..97d9dd7 100644 --- a/tests/unit/test_broker_daemon.py +++ b/tests/unit/test_broker_daemon.py @@ -984,6 +984,76 @@ async def test_transport_start_failure_sets_stop_events(self, tmp_path: Path) -> assert daemon._stopped_event.is_set() +# --------------------------------------------------------------------------- +# Version file handling (P4-T1) +# --------------------------------------------------------------------------- + + +class TestBrokerDaemonVersionFile: + @pytest.mark.asyncio + async def test_start_writes_version_file(self, tmp_path: Path) -> None: + """start() writes a version file alongside the PID file.""" + cfg = _make_config(tmp_path) + daemon = BrokerDaemon(cfg) + proc = _make_mock_process() + + with patch( + "mcpbridge_wrapper.broker.daemon.asyncio.create_subprocess_exec", + new=AsyncMock(return_value=proc), + ): + await daemon.start() + + assert cfg.version_file.exists() + version_text = cfg.version_file.read_text().strip() + assert len(version_text) > 0 + + daemon._stop_event.set() + if daemon._read_task and not daemon._read_task.done(): + daemon._read_task.cancel() + + @pytest.mark.asyncio + async def test_stop_removes_version_file(self, tmp_path: Path) -> None: + """stop() removes the version file.""" + cfg = _make_config(tmp_path) + daemon = BrokerDaemon(cfg) + proc = _make_mock_process() + + with patch( + "mcpbridge_wrapper.broker.daemon.asyncio.create_subprocess_exec", + new=AsyncMock(return_value=proc), + ): + await daemon.start() + assert cfg.version_file.exists() + await daemon.stop() + + assert not cfg.version_file.exists() + + @pytest.mark.asyncio + async def test_status_includes_version(self, tmp_path: Path) -> None: + """status() includes a 'version' key.""" + cfg = _make_config(tmp_path) + daemon = BrokerDaemon(cfg) + status = daemon.status() + assert "version" in status + assert isinstance(status["version"], str) + + def test_stale_lock_cleanup_removes_version_file(self, tmp_path: Path) -> None: + """_check_and_clear_stale_lock removes version file for dead processes.""" + cfg = _make_config(tmp_path) + cfg.pid_file.write_text("99999999") + cfg.socket_path.write_text("leftover") + cfg.version_file.write_text("0.1.0") + + daemon = BrokerDaemon(cfg) + with patch( + "mcpbridge_wrapper.broker.daemon.os.kill", + side_effect=ProcessLookupError, + ): + daemon._check_and_clear_stale_lock() + + assert not cfg.version_file.exists() + + # --------------------------------------------------------------------------- # P2-T2: atexit cleanup registration # --------------------------------------------------------------------------- diff --git a/tests/unit/test_broker_proxy.py b/tests/unit/test_broker_proxy.py index 32215a7..fc3f208 100644 --- a/tests/unit/test_broker_proxy.py +++ b/tests/unit/test_broker_proxy.py @@ -17,6 +17,8 @@ import asyncio import inspect import os +import signal +import subprocess import sys from pathlib import Path from unittest.mock import AsyncMock, MagicMock, patch @@ -762,6 +764,236 @@ def test_warning_is_actionable(self, tmp_path: Path) -> None: assert "broker.sock" in stderr_output or "Restart" in stderr_output +# --------------------------------------------------------------------------- +# Version mismatch detection (P4-T1) +# --------------------------------------------------------------------------- + + +class TestBrokerProxyVersionMismatch: + def test_no_version_file_returns_false(self, tmp_path: Path) -> None: + """No version file (old daemon) → no mismatch → backwards-compatible.""" + cfg = _make_config(tmp_path) + proxy = BrokerProxy(cfg) + assert proxy._check_version_mismatch() is False + + def test_matching_version_returns_false(self, tmp_path: Path) -> None: + """Same version → no mismatch.""" + from mcpbridge_wrapper import __version__ + + cfg = _make_config(tmp_path) + cfg.version_file.write_text(__version__) + proxy = BrokerProxy(cfg) + assert proxy._check_version_mismatch() is False + + def test_different_version_returns_true(self, tmp_path: Path) -> None: + """Different version → mismatch detected.""" + cfg = _make_config(tmp_path) + cfg.version_file.write_text("0.0.0-old") + proxy = BrokerProxy(cfg) + assert proxy._check_version_mismatch() is True + + def test_version_file_read_error_returns_false(self, tmp_path: Path) -> None: + """Unreadable version file is treated as no mismatch.""" + cfg = _make_config(tmp_path) + cfg.version_file.write_text("ignored") + proxy = BrokerProxy(cfg) + with patch.object(Path, "read_text", side_effect=OSError): + assert proxy._check_version_mismatch() is False + + def test_pid_belongs_to_broker_true_for_expected_command(self, tmp_path: Path) -> None: + cfg = _make_config(tmp_path) + proxy = BrokerProxy(cfg) + with patch( + "mcpbridge_wrapper.broker.proxy.subprocess.check_output", + return_value="python -m mcpbridge_wrapper --broker-daemon --web-ui", + ): + assert proxy._pid_belongs_to_broker(123) is True + + def test_pid_belongs_to_broker_false_on_ps_failure(self, tmp_path: Path) -> None: + cfg = _make_config(tmp_path) + proxy = BrokerProxy(cfg) + with patch( + "mcpbridge_wrapper.broker.proxy.subprocess.check_output", + side_effect=subprocess.CalledProcessError(1, ["ps"]), + ): + assert proxy._pid_belongs_to_broker(123) is False + + def test_pid_belongs_to_broker_false_for_unrelated_command(self, tmp_path: Path) -> None: + cfg = _make_config(tmp_path) + proxy = BrokerProxy(cfg) + with patch( + "mcpbridge_wrapper.broker.proxy.subprocess.check_output", + return_value="python -m http.server 8000", + ): + assert proxy._pid_belongs_to_broker(123) is False + + def test_stop_stale_daemon_no_pid_file_noop(self, tmp_path: Path) -> None: + """No PID file means there is nothing to stop.""" + cfg = _make_config(tmp_path) + proxy = BrokerProxy(cfg) + with patch("mcpbridge_wrapper.broker.proxy.os.kill") as mock_kill: + proxy._stop_stale_daemon() + mock_kill.assert_not_called() + + def test_stop_stale_daemon_invalid_pid_text_returns(self, tmp_path: Path) -> None: + """Corrupt PID file short-circuits without touching broker files.""" + cfg = _make_config(tmp_path) + cfg.pid_file.write_text("not-a-pid") + cfg.socket_path.write_text("sock") + cfg.version_file.write_text("version") + proxy = BrokerProxy(cfg) + + with patch("mcpbridge_wrapper.broker.proxy.os.kill") as mock_kill: + proxy._stop_stale_daemon() + + mock_kill.assert_not_called() + assert cfg.socket_path.exists() + assert cfg.version_file.exists() + + def test_stop_stale_daemon_skips_unrelated_pid(self, tmp_path: Path) -> None: + """Unrelated PID in stale pid file is never signaled.""" + cfg = _make_config(tmp_path) + cfg.pid_file.write_text("987") + cfg.socket_path.write_text("stale") + cfg.version_file.write_text("old") + proxy = BrokerProxy(cfg) + + with patch.object(proxy, "_pid_belongs_to_broker", return_value=False), patch( + "mcpbridge_wrapper.broker.proxy.os.kill" + ) as mock_kill: + proxy._stop_stale_daemon() + + mock_kill.assert_not_called() + assert not cfg.pid_file.exists() + assert not cfg.socket_path.exists() + assert not cfg.version_file.exists() + + def test_stop_stale_daemon_cleanup_when_process_missing(self, tmp_path: Path) -> None: + """ProcessLookupError triggers stale file cleanup.""" + cfg = _make_config(tmp_path) + cfg.pid_file.write_text("321") + cfg.socket_path.write_text("stale") + cfg.version_file.write_text("old") + proxy = BrokerProxy(cfg) + + with patch( + "mcpbridge_wrapper.broker.proxy.os.kill", + side_effect=ProcessLookupError, + ): + proxy._stop_stale_daemon() + + assert not cfg.pid_file.exists() + assert not cfg.socket_path.exists() + assert not cfg.version_file.exists() + + def test_stop_stale_daemon_permission_error_cleans_files(self, tmp_path: Path) -> None: + """PermissionError on SIGTERM still triggers stale file cleanup.""" + cfg = _make_config(tmp_path) + cfg.pid_file.write_text("432") + cfg.socket_path.write_text("stale") + cfg.version_file.write_text("old") + proxy = BrokerProxy(cfg) + + with patch.object(proxy, "_pid_belongs_to_broker", return_value=True), patch( + "mcpbridge_wrapper.broker.proxy.os.kill", + side_effect=PermissionError, + ): + proxy._stop_stale_daemon() + + assert not cfg.pid_file.exists() + assert not cfg.socket_path.exists() + assert not cfg.version_file.exists() + + def test_stop_stale_daemon_waits_for_exit_then_cleans(self, tmp_path: Path) -> None: + """SIGTERM path waits for exit probe and removes broker files.""" + cfg = _make_config(tmp_path) + cfg.pid_file.write_text("654") + cfg.socket_path.write_text("stale") + cfg.version_file.write_text("old") + proxy = BrokerProxy(cfg) + + probes = {"count": 0} + + def fake_kill(pid: int, sig: int) -> None: + assert pid == 654 + if sig == signal.SIGTERM: + return None + probes["count"] += 1 + if probes["count"] > 1: + raise ProcessLookupError + return None + + with patch.object(proxy, "_pid_belongs_to_broker", return_value=True), patch( + "mcpbridge_wrapper.broker.proxy.os.kill", + side_effect=fake_kill, + ), patch("mcpbridge_wrapper.broker.proxy.time.sleep", return_value=None): + proxy._stop_stale_daemon() + + assert probes["count"] == 2 + assert not cfg.pid_file.exists() + assert not cfg.socket_path.exists() + assert not cfg.version_file.exists() + + @pytest.mark.asyncio + async def test_connect_with_timeout_returns_streams_on_success(self, tmp_path: Path) -> None: + """Successful unix connect returns the opened stream pair.""" + cfg = _make_config(tmp_path) + proxy = BrokerProxy(cfg, connect_timeout=0.2) + expected_reader = asyncio.StreamReader() + expected_writer = MagicMock() + + with patch( + "mcpbridge_wrapper.broker.proxy.asyncio.open_unix_connection", + AsyncMock(return_value=(expected_reader, expected_writer)), + ): + reader, writer = await proxy._connect_with_timeout() + + assert reader is expected_reader + assert writer is expected_writer + + @pytest.mark.asyncio + async def test_version_mismatch_triggers_restart(self, tmp_path: Path) -> None: + """When version mismatches, old daemon is stopped and new one spawned.""" + cfg = _make_config(tmp_path) + # Write a live PID (our own) and a mismatched version + cfg.pid_file.write_text(str(os.getpid())) + cfg.version_file.write_text("0.0.0-old") + + proxy = BrokerProxy(cfg, auto_spawn=True, connect_timeout=0.3) + + stop_called = [] + + def fake_stop() -> None: + stop_called.append(True) + # Clean up files so spawn proceeds + cfg.pid_file.unlink(missing_ok=True) + cfg.socket_path.unlink(missing_ok=True) + cfg.version_file.unlink(missing_ok=True) + + with patch.object(proxy, "_stop_stale_daemon", fake_stop), patch( + "subprocess.Popen" + ), pytest.raises(TimeoutError): + await proxy._spawn_broker_if_needed() + + assert stop_called == [True] + + @pytest.mark.asyncio + async def test_version_match_reuses_daemon(self, tmp_path: Path) -> None: + """When versions match, existing daemon is reused (no stop/spawn).""" + from mcpbridge_wrapper import __version__ + + cfg = _make_config(tmp_path) + cfg.pid_file.write_text(str(os.getpid())) + cfg.version_file.write_text(__version__) + + proxy = BrokerProxy(cfg, auto_spawn=True, connect_timeout=0.3) + + with patch("subprocess.Popen") as mock_popen: + await proxy._spawn_broker_if_needed() + + mock_popen.assert_not_called() + + # --------------------------------------------------------------------------- # _parse_broker_args # --------------------------------------------------------------------------- @@ -769,56 +1001,96 @@ def test_warning_is_actionable(self, tmp_path: Path) -> None: class TestParseBrokerArgs: def test_no_flags_leaves_all_args(self) -> None: - daemon, connect, spawn, remaining = _parse_broker_args(["--some-flag", "value"]) + daemon, connect, spawn, status, stop, remaining = _parse_broker_args( + ["--some-flag", "value"] + ) assert daemon is False assert connect is False assert spawn is False + assert status is False + assert stop is False assert remaining == ["--some-flag", "value"] def test_broker_flag_sets_spawn_and_connect(self) -> None: - daemon, connect, spawn, remaining = _parse_broker_args(["--broker"]) + daemon, connect, spawn, status, stop, remaining = _parse_broker_args(["--broker"]) assert daemon is False assert connect is True assert spawn is True + assert status is False + assert stop is False assert remaining == [] def test_legacy_broker_connect_flag_is_forwarded(self) -> None: - daemon, connect, spawn, remaining = _parse_broker_args(["--broker-connect"]) + daemon, connect, spawn, status, stop, remaining = _parse_broker_args(["--broker-connect"]) assert daemon is False assert connect is False assert spawn is False assert remaining == ["--broker-connect"] def test_legacy_broker_spawn_flag_is_forwarded(self) -> None: - daemon, connect, spawn, remaining = _parse_broker_args(["--broker-spawn"]) + daemon, connect, spawn, status, stop, remaining = _parse_broker_args(["--broker-spawn"]) assert daemon is False assert connect is False assert spawn is False assert remaining == ["--broker-spawn"] def test_unknown_flags_pass_through(self) -> None: - daemon, connect, spawn, remaining = _parse_broker_args(["--other-flag", "val"]) + daemon, connect, spawn, status, stop, remaining = _parse_broker_args( + ["--other-flag", "val"] + ) assert daemon is False assert connect is False assert spawn is False + assert status is False + assert stop is False assert remaining == ["--other-flag", "val"] def test_empty_args(self) -> None: - daemon, connect, spawn, remaining = _parse_broker_args([]) + daemon, connect, spawn, status, stop, remaining = _parse_broker_args([]) assert daemon is False assert connect is False assert spawn is False + assert status is False + assert stop is False assert remaining == [] def test_broker_daemon_flag(self) -> None: - daemon, connect, spawn, remaining = _parse_broker_args(["--broker-daemon"]) + daemon, connect, spawn, status, stop, remaining = _parse_broker_args(["--broker-daemon"]) assert daemon is True assert connect is False assert spawn is False assert remaining == [] def test_broker_daemon_not_in_remaining(self) -> None: - daemon, connect, spawn, remaining = _parse_broker_args(["--broker-daemon", "--other-flag"]) + daemon, connect, spawn, status, stop, remaining = _parse_broker_args( + ["--broker-daemon", "--other-flag"] + ) assert daemon is True assert "--broker-daemon" not in remaining assert remaining == ["--other-flag"] + + def test_broker_status_flag(self) -> None: + daemon, connect, spawn, status, stop, remaining = _parse_broker_args(["--broker-status"]) + assert status is True + assert daemon is False + assert remaining == [] + + def test_broker_stop_flag(self) -> None: + daemon, connect, spawn, status, stop, remaining = _parse_broker_args(["--broker-stop"]) + assert stop is True + assert daemon is False + assert remaining == [] + + def test_broker_status_not_in_remaining(self) -> None: + daemon, connect, spawn, status, stop, remaining = _parse_broker_args( + ["--broker-status", "--other"] + ) + assert status is True + assert "--broker-status" not in remaining + + def test_broker_stop_not_in_remaining(self) -> None: + daemon, connect, spawn, status, stop, remaining = _parse_broker_args( + ["--broker-stop", "--other"] + ) + assert stop is True + assert "--broker-stop" not in remaining diff --git a/tests/unit/test_broker_stubs.py b/tests/unit/test_broker_stubs.py index 5b68e94..06e28de 100644 --- a/tests/unit/test_broker_stubs.py +++ b/tests/unit/test_broker_stubs.py @@ -88,6 +88,19 @@ def test_field_names(self) -> None: assert "pid_file" in field_names assert "upstream_cmd" in field_names + def test_version_file_derived_from_pid_file(self) -> None: + cfg = BrokerConfig.default() + assert cfg.version_file.name == "broker.version" + assert cfg.version_file.parent == cfg.pid_file.parent + + def test_version_file_custom_path(self) -> None: + cfg = BrokerConfig( + socket_path=Path("/tmp/test.sock"), + pid_file=Path("/tmp/custom/test.pid"), + upstream_cmd=["my-bridge"], + ) + assert cfg.version_file == Path("/tmp/custom/broker.version") + # --------------------------------------------------------------------------- # ClientSession diff --git a/tests/unit/test_init.py b/tests/unit/test_init.py new file mode 100644 index 0000000..07d6110 --- /dev/null +++ b/tests/unit/test_init.py @@ -0,0 +1,23 @@ +"""Unit tests for package version initialization.""" + +import importlib +from unittest.mock import patch + +import mcpbridge_wrapper + + +def test_version_comes_from_importlib_metadata() -> None: + with patch("importlib.metadata.version", return_value="9.9.9"): + reloaded = importlib.reload(mcpbridge_wrapper) + + assert reloaded.__version__ == "9.9.9" + + +def test_version_fallback_when_metadata_unavailable() -> None: + with patch("importlib.metadata.version", side_effect=Exception("missing metadata")): + reloaded = importlib.reload(mcpbridge_wrapper) + + assert reloaded.__version__ == "0.0.0+unknown" + + # Restore module state for the remaining test session. + importlib.reload(mcpbridge_wrapper) diff --git a/tests/unit/test_main.py b/tests/unit/test_main.py index e752542..b3ca516 100644 --- a/tests/unit/test_main.py +++ b/tests/unit/test_main.py @@ -899,16 +899,18 @@ class TestParseBrokerArgs: def test_no_flags_returns_all_as_remaining(self): from mcpbridge_wrapper.__main__ import _parse_broker_args - daemon, connect, spawn, remaining = _parse_broker_args(["--some-flag"]) + daemon, connect, spawn, status, stop, remaining = _parse_broker_args(["--some-flag"]) assert daemon is False assert connect is False assert spawn is False + assert status is False + assert stop is False assert remaining == ["--some-flag"] def test_legacy_broker_connect_flag_is_forwarded(self): from mcpbridge_wrapper.__main__ import _parse_broker_args - daemon, connect, spawn, remaining = _parse_broker_args(["--broker-connect"]) + daemon, connect, spawn, status, stop, remaining = _parse_broker_args(["--broker-connect"]) assert daemon is False assert connect is False assert spawn is False @@ -917,7 +919,7 @@ def test_legacy_broker_connect_flag_is_forwarded(self): def test_legacy_broker_spawn_flag_is_forwarded(self): from mcpbridge_wrapper.__main__ import _parse_broker_args - daemon, connect, spawn, remaining = _parse_broker_args(["--broker-spawn"]) + daemon, connect, spawn, status, stop, remaining = _parse_broker_args(["--broker-spawn"]) assert daemon is False assert connect is False assert spawn is False @@ -926,7 +928,7 @@ def test_legacy_broker_spawn_flag_is_forwarded(self): def test_broker_daemon_flag(self): from mcpbridge_wrapper.__main__ import _parse_broker_args - daemon, connect, spawn, remaining = _parse_broker_args(["--broker-daemon"]) + daemon, connect, spawn, status, stop, remaining = _parse_broker_args(["--broker-daemon"]) assert daemon is True assert connect is False assert spawn is False @@ -935,7 +937,7 @@ def test_broker_daemon_flag(self): def test_broker_daemon_not_in_remaining(self): from mcpbridge_wrapper.__main__ import _parse_broker_args - daemon, connect, spawn, remaining = _parse_broker_args( + daemon, connect, spawn, status, stop, remaining = _parse_broker_args( ["--broker-daemon", "--some-bridge-arg"] ) assert daemon is True @@ -945,7 +947,7 @@ def test_broker_daemon_not_in_remaining(self): def test_broker_flag_sets_spawn_and_connect(self): from mcpbridge_wrapper.__main__ import _parse_broker_args - daemon, connect, spawn, remaining = _parse_broker_args(["--broker"]) + daemon, connect, spawn, status, stop, remaining = _parse_broker_args(["--broker"]) assert daemon is False assert connect is True assert spawn is True @@ -954,7 +956,9 @@ def test_broker_flag_sets_spawn_and_connect(self): def test_broker_flag_not_forwarded_to_bridge(self): from mcpbridge_wrapper.__main__ import _parse_broker_args - daemon, connect, spawn, remaining = _parse_broker_args(["--broker", "--some-bridge-arg"]) + daemon, connect, spawn, status, stop, remaining = _parse_broker_args( + ["--broker", "--some-bridge-arg"] + ) assert "--broker" not in remaining assert remaining == ["--some-bridge-arg"] @@ -1063,6 +1067,166 @@ def test_main_broker_with_webui_propagates_spawn_args(self): ] +class TestMainBrokerLifecycleCommands: + """Tests for --broker-status and --broker-stop command branches.""" + + @staticmethod + def _make_config(tmp_path): + from mcpbridge_wrapper.broker.types import BrokerConfig + + state_dir = tmp_path / "broker-state" + state_dir.mkdir(parents=True, exist_ok=True) + return BrokerConfig( + socket_path=state_dir / "broker.sock", + pid_file=state_dir / "broker.pid", + upstream_cmd=["xcrun", "mcpbridge"], + ) + + def test_main_broker_status_reports_running_pid_and_version_mismatch(self, tmp_path): + cfg = self._make_config(tmp_path) + cfg.pid_file.write_text("1234") + cfg.version_file.write_text("0.0.1-old") + + with patch( + "mcpbridge_wrapper.__main__.sys.argv", + ["mcpbridge-wrapper", "--broker-status"], + ), patch( + "mcpbridge_wrapper.broker.types.BrokerConfig.default", + return_value=cfg, + ), patch( + "mcpbridge_wrapper.__main__.os.kill", + return_value=None, + ), patch( + "mcpbridge_wrapper.__version__", + "9.9.9", + ), patch("builtins.print") as mock_print: + result = main() + + assert result == 0 + printed = "\n".join( + " ".join(str(arg) for arg in call.args) for call in mock_print.call_args_list + ) + assert "Proxy version: 9.9.9" in printed + assert "Daemon PID: 1234 (running)" in printed + assert "WARNING: version mismatch! proxy=9.9.9, daemon=0.0.1-old" in printed + + def test_main_broker_stop_cleans_corrupt_pid_files(self, tmp_path): + cfg = self._make_config(tmp_path) + cfg.pid_file.write_text("not-a-number") + cfg.socket_path.write_text("stale") + cfg.version_file.write_text("old") + + with patch( + "mcpbridge_wrapper.__main__.sys.argv", + ["mcpbridge-wrapper", "--broker-stop"], + ), patch( + "mcpbridge_wrapper.broker.types.BrokerConfig.default", + return_value=cfg, + ), patch("builtins.print") as mock_print: + result = main() + + assert result == 0 + assert not cfg.pid_file.exists() + assert not cfg.socket_path.exists() + assert not cfg.version_file.exists() + printed = "\n".join( + " ".join(str(arg) for arg in call.args) for call in mock_print.call_args_list + ) + assert "Corrupt PID file; cleaning up." in printed + + def test_main_broker_stop_permission_error_returns_1(self, tmp_path): + cfg = self._make_config(tmp_path) + cfg.pid_file.write_text("4321") + + with patch( + "mcpbridge_wrapper.__main__.sys.argv", + ["mcpbridge-wrapper", "--broker-stop"], + ), patch( + "mcpbridge_wrapper.broker.types.BrokerConfig.default", + return_value=cfg, + ), patch( + "mcpbridge_wrapper.__main__.os.kill", + side_effect=PermissionError, + ), patch("builtins.print"): + result = main() + + assert result == 1 + + def test_main_broker_stop_successfully_terminates_and_cleans_files(self, tmp_path): + cfg = self._make_config(tmp_path) + cfg.pid_file.write_text("4321") + cfg.socket_path.write_text("sock") + cfg.version_file.write_text("ver") + + def fake_kill(pid, sig): + assert pid == 4321 + if sig == signal.SIGTERM: + return None + raise ProcessLookupError + + with patch( + "mcpbridge_wrapper.__main__.sys.argv", + ["mcpbridge-wrapper", "--broker-stop"], + ), patch( + "mcpbridge_wrapper.broker.types.BrokerConfig.default", + return_value=cfg, + ), patch( + "mcpbridge_wrapper.__main__.os.kill", + side_effect=fake_kill, + ), patch( + "mcpbridge_wrapper.__main__.time.monotonic", + side_effect=[100.0, 100.1], + ), patch("builtins.print") as mock_print: + result = main() + + assert result == 0 + assert not cfg.pid_file.exists() + assert not cfg.socket_path.exists() + assert not cfg.version_file.exists() + printed = "\n".join( + " ".join(str(arg) for arg in call.args) for call in mock_print.call_args_list + ) + assert "Sent SIGTERM to broker (PID 4321)." in printed + assert "Broker stopped and files cleaned up." in printed + + def test_main_broker_stop_timeout_returns_1_and_preserves_state(self, tmp_path): + cfg = self._make_config(tmp_path) + cfg.pid_file.write_text("4321") + cfg.socket_path.write_text("sock") + cfg.version_file.write_text("ver") + + def fake_kill(pid, sig): + assert pid == 4321 + return None + + with patch( + "mcpbridge_wrapper.__main__.sys.argv", + ["mcpbridge-wrapper", "--broker-stop"], + ), patch( + "mcpbridge_wrapper.broker.types.BrokerConfig.default", + return_value=cfg, + ), patch( + "mcpbridge_wrapper.__main__.os.kill", + side_effect=fake_kill, + ), patch( + "mcpbridge_wrapper.__main__.time.monotonic", + side_effect=[100.0, 100.5, 103.6], + ), patch( + "mcpbridge_wrapper.__main__.time.sleep", + return_value=None, + ), patch("builtins.print") as mock_print: + result = main() + + assert result == 1 + assert cfg.pid_file.exists() + assert cfg.socket_path.exists() + assert cfg.version_file.exists() + printed = "\n".join( + " ".join(str(arg) for arg in call.args) for call in mock_print.call_args_list + ) + assert "did not stop within 3 seconds" in printed + + class TestMainBrokerDaemonMode: """Tests for main() --broker-daemon mode."""