diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index cd9aba2..1848ac9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -5,7 +5,6 @@ on: branches: [main] tags: ["v*"] pull_request: - branches: [main] jobs: lint: @@ -16,8 +15,15 @@ jobs: steps: - uses: actions/checkout@v6 - uses: astral-sh/setup-uv@v7 - - run: uvx --python ${{ matrix.python-version }} ruff check src/ - - run: uvx --python ${{ matrix.python-version }} ruff format --check src/ + - run: uvx --python ${{ matrix.python-version }} ruff check src/ tests/ scripts/ + - run: uvx --python ${{ matrix.python-version }} ruff format --check src/ tests/ scripts/ + + guards: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v6 + - name: Run CI guards + run: bash scripts/ci_guards.sh type-check: runs-on: ubuntu-latest @@ -28,7 +34,34 @@ jobs: run: | sudo apt-get update sudo apt-get install -y libcairo2-dev libgirepository-2.0-dev libdbus-1-dev pkg-config - - run: uv run ty check src/ + - run: uv run ty check src/ tests/ scripts/ + + test: + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + # Skip 3.15 (alpha): rpds-py has no wheel and Rust source build fails on cpython-3.15.0a8. + python-version: ["3.12", "3.13", "3.14"] + steps: + - uses: actions/checkout@v6 + - uses: astral-sh/setup-uv@v7 + - name: Install system dependencies + run: | + sudo apt-get update + sudo apt-get install -y libcairo2-dev libgirepository-2.0-dev libdbus-1-dev pkg-config + - name: Run non-kwin pytest (tolerate exit 5 = no tests collected) + # Ignore tests/integration: those tests require a real KWin/Wayland session + # and import kwin_mcp.input which dlopens libei.so.1 (not available on GHA). + run: | + set +e + uv run --python ${{ matrix.python-version }} pytest -m "not kwin" --ignore=tests/integration + rc=$? + set -e + if [ "$rc" -eq 0 ] || [ "$rc" -eq 5 ]; then + exit 0 + fi + exit "$rc" build: runs-on: ubuntu-latest @@ -48,7 +81,7 @@ jobs: publish: if: startsWith(github.ref, 'refs/tags/v') - needs: [lint, type-check, build] + needs: [lint, type-check, test, build, guards] runs-on: ubuntu-latest environment: pypi permissions: diff --git a/.github/workflows/docs-seo.yml b/.github/workflows/docs-seo.yml index 97d4439..28b53c9 100644 --- a/.github/workflows/docs-seo.yml +++ b/.github/workflows/docs-seo.yml @@ -6,7 +6,6 @@ name: Docs & SEO Review # when the detected changes do not warrant a documentation review. on: pull_request: - branches: [main] paths: - "src/kwin_mcp/**" - "pyproject.toml" diff --git a/CHANGELOG.md b/CHANGELOG.md index 66eaada..e4d6b24 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- `dbus_call` MCP tool now accepts typed-JSON dicts (`{"type": "int32", "value": 42}`) alongside legacy dbus-send strings (`"int32:42"`); both shapes can mix freely in one `args` list. Container shapes supported: `array` (basic element types), `dict` (basic key + value types), `variant` (basic value type). +- New module `kwin_mcp.dbus_args` exposing `parse_dbus_send_arg`, `parse_typed_arg`, `parse_arg`, and `to_dbus_send_string` (80 unit tests). + +### Changed + +- `dbus_call` reply formatting: single-primitive returns are now the bare value (e.g. `org.freedesktop.DBus.GetId` returns just the UUID string instead of the multi-line `method return … string "…"` block); container and multi-value returns are JSON. Error replies use the format `D-Bus error: : ` instead of raw `dbus-send` stderr text. Callers that parse the structured value see equivalent data; callers grepping for the literal `method return` prefix must update. + +### Internal + +- Replaced `subprocess.run(["dbus-send", …])` inside `dbus_call` with in-process `dbus.bus.BusConnection` + `dbus.Interface`, removing one fork+exec per call and surfacing structured `dbus.DBusException` instead of CLI exit codes. +- Replaced per-call `subprocess.run([sys.executable, "-m", "kwin_mcp.accessibility", …])` for AT-SPI queries with a long-lived spawn-context `multiprocessing.Pool(processes=1)` worker. New module `kwin_mcp.accessibility_worker` is never imported by the parent process (CI guard 3 enforced); worker init validates `Atspi.get_desktop(0).get_child_count() >= 0` against the bus address. Cold start ≤ 1.5s, warm calls < 200ms (vs ~700ms per subprocess). Pool teardown protocol: `close → join 5s → terminate → join 2s → SIGKILL → join 1s` survives hung workers within 7s. External `SIGKILL` of the worker is recovered by lazy pool recreation on the next call. + ### Fixed - Segfault on Python 3.14 caused by missing `argtypes` on variadic `ei_seat_bind_capabilities` ctypes call diff --git a/CLAUDE.md b/CLAUDE.md index 7a8f062..141009c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -60,7 +60,11 @@ See ROADMAP.md. Key modules: | `src/kwin_mcp/server.py` | tool-registration, code-general | Check tool count, README tool tables, CONTRIBUTING structure, **integrations/*/skills/*/SKILL.md tool list freshness** | | `src/kwin_mcp/session.py` | session-api | Check README arch diagram, CONTRIBUTING session docs | | `src/kwin_mcp/core.py` | engine-api, code-general | Check README arch description, CONTRIBUTING structure | +| `src/kwin_mcp/dbus_args.py` | code-general | Check concrete numbers, CONTRIBUTING file listing, **integrations/*/skills/*/SKILL.md "30 capabilities" reference** | +| `src/kwin_mcp/window.py` | engine-api, code-general | Check README arch description, CONTRIBUTING structure | +| `src/kwin_mcp/accessibility_worker.py` | engine-api, code-general | Check README arch description, CONTRIBUTING structure | | `src/kwin_mcp/*.py` (any) | code-general | Check concrete numbers, CONTRIBUTING file listing, **integrations/*/skills/*/SKILL.md "30 capabilities" reference** | +| `docs/design/*.md` | code-general | Check docs/design architecture notes and related docs-seo guidance | | `pyproject.toml` | package-metadata | Sync keywords with `.claude/positioning.yml`; check CLAUDE.md keyword tiers; **run `python3 scripts/sync_plugin_version.py` to propagate version to integrations manifests** | | `CHANGELOG.md` | changelog-update | Sync docs-seo.md positioning; add new search intents | | `README.md` | readme-update | Sync docs-seo.md positioning; update CLAUDE.md keyword tiers | diff --git a/docs/design/dbus-call-call-sites.md b/docs/design/dbus-call-call-sites.md new file mode 100644 index 0000000..a360b2b --- /dev/null +++ b/docs/design/dbus-call-call-sites.md @@ -0,0 +1,70 @@ +# dbus_call Call Site Map + +## Summary + +- **Total internal code call sites**: 1 (`server.py` → `core.py`) +- **Hard-coded arg patterns in source**: 0 (all args are pass-through from external callers) +- **External API callers**: unlimited (LLM agents / MCP clients pass arbitrary dbus-send syntax) + +## Definition Sites + +| Location | Role | Current Args Type | +|---|---|---| +| `src/kwin_mcp/core.py:717` | `dbus_call()` method — implementation | `args: list[str] \| None` — passed directly to `dbus-send` CLI | +| `src/kwin_mcp/server.py:751` | `dbus_call` MCP tool — schema + wrapper | Annotated `list[str] \| None`, description mentions dbus-send format | + +## Internal Call Sites + +| Location | Caller | Service | Path | Interface | Method | Args | Type Complexity | +|---|---|---|---|---|---|---|---| +| `src/kwin_mcp/server.py:769` | MCP tool handler | pass-through | pass-through | pass-through | pass-through | pass-through from MCP user | varies | + +**There are no internal call sites with hard-coded arg patterns.** All args originate from external MCP clients (LLM agents). + +## External Arg Patterns (from MCP tool docstring) + +The `dbus_call` MCP tool description (`server.py:759-768`) documents the accepted format: +``` +Method arguments in dbus-send format (e.g. "string:value", "int32:42", "boolean:true") +``` + +Examples that MCP clients are known to supply (from plan QA scenarios and typical agent behavior): +- `[]` — no args (e.g., `org.freedesktop.DBus.GetId`, `org.kde.KWin.showDesktop`) +- `["string:hello"]` — single basic string arg +- `["int32:42"]` — integer arg +- `["boolean:true"]` — boolean arg +- `["string:hello", "int32:42"]` — mixed basic args + +## Required Parser Features + +Based on the zero internal hard-coded call sites and the MCP tool's documented usage, the parser in `dbus_args.py` **MUST support** all standard dbus-send basic types because external LLM agents may use any of them: + +| Type prefix | Example | Priority | +|---|---|---| +| `string:` | `string:hello` | **Required** — most common | +| `int32:` | `int32:42` | **Required** — most common integer | +| `boolean:` | `boolean:true` | **Required** — frequently used | +| `uint32:` | `uint32:1234` | **Required** — KWin D-Bus methods use uint32 | +| `uint64:` | `uint64:999` | **Required** — timestamps, PIDs | +| `int64:` | `int64:-1` | **Required** — standard type | +| `byte:` | `byte:255` | **Required** — protocol completeness | +| `int16:` | `int16:10` | **Required** — protocol completeness | +| `uint16:` | `uint16:10` | **Required** — protocol completeness | +| `double:` | `double:3.14` | **Required** — used by KWin geometry | +| `objpath:` | `objpath:/org/kde/KWin` | **Required** — object references | +| `signature:` | `signature:s` | **Required** — introspection | +| `array:` | `array:string:a,b,c` | **Required** — lists | +| `dict:` | `dict:string:int32:k:1` | **Required** — D-Bus dicts | +| `variant:` | `variant:string:hello` | **Required** — polymorphic values | + +**No types from the dbus-send man page are deferred** — the MCP tool is a public API and agents may use any valid dbus-send syntax. + +## Scope Impact on Task 7 (Parser) + +Task 7 must implement a full recursive-descent parser for the dbus-send argument syntax. The scope is NOT reducible based on current callers because: + +1. The tool is an external API with no internal call sites to scope-cap from +2. LLM agents generate diverse arg patterns based on D-Bus introspection output +3. Supporting only 2-3 types would break real-world agent workflows + +**Minimum viable parser scope**: all basic types + `array:`, `dict:`, `variant:` containers + nested containers. diff --git a/pyproject.toml b/pyproject.toml index 6a93f7d..49e4816 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -75,7 +75,21 @@ requires = ["uv_build>=0.10.3,<0.12.0"] build-backend = "uv_build" [dependency-groups] -dev = ["ruff>=0.11.0", "ty>=0.0.1"] +dev = [ + "ruff>=0.11.0", + "ty>=0.0.1", + "pytest>=8.0", + "pytest-asyncio>=0.23", + "pytest-timeout>=2.3.0", +] + +[tool.pytest.ini_options] +testpaths = ["tests"] +addopts = "-ra -q" +asyncio_mode = "auto" +timeout = 30 +timeout_method = "thread" +markers = ["kwin: marks tests requiring a real KWin virtual session (deselect with -m 'not kwin')"] [tool.ruff] target-version = "py312" @@ -108,7 +122,11 @@ python-version = "3.12" # gi.repository and dbus.bus are dynamic modules that cannot be resolved statically [[tool.ty.overrides]] -include = ["src/kwin_mcp/accessibility.py", "src/kwin_mcp/server.py"] +include = [ + "src/kwin_mcp/accessibility.py", + "src/kwin_mcp/accessibility_worker.py", + "src/kwin_mcp/server.py", +] [tool.ty.overrides.rules] unresolved-import = "ignore" diff --git a/scripts/ci_guards.sh b/scripts/ci_guards.sh new file mode 100755 index 0000000..9374307 --- /dev/null +++ b/scripts/ci_guards.sh @@ -0,0 +1,162 @@ +#!/usr/bin/env bash +# CI Guards - enforce backend overhaul invariants. +# +# Each guard prints a clear failure message identifying the rule and +# the offending location(s), then exits non-zero. On a clean tree, +# the script exits 0 silently (per-guard "ok" lines are emitted to +# stderr for visibility in CI logs). +# +# Guards: +# 1. No reachable `print()` in src/kwin_mcp/ (excluding __main__ blocks) +# 2. Every `kwin_wayland` invocation in tests/, scripts/, docs/ has --virtual +# 3. core.py does not import accessibility_worker at module top +# 4. No os.fork or raw multiprocessing.Process( in src/kwin_mcp/ + +set -euo pipefail + +REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +cd "$REPO_ROOT" + +fail=0 + +log_ok() { + echo "[ci_guards] OK: $1" >&2 +} + +log_fail() { + echo "[ci_guards] FAIL: $1" >&2 + fail=1 +} + +# --------------------------------------------------------------------------- +# Guard 1: No reachable `print()` in MCP server code paths under src/kwin_mcp/. +# Excludes: +# - __main__.py (module entrypoint guard blocks) +# - cli.py (interactive REPL whose contract IS to write to stdout) +# Rationale: any print() reachable from server.py would corrupt the MCP +# stdio JSON-RPC stream. +# --------------------------------------------------------------------------- +guard_1_no_print() { + local matches + matches="$(grep -rn -E '^[[:space:]]*print\(' src/kwin_mcp/ \ + --include='*.py' 2>/dev/null \ + | grep -v '__main__' \ + | grep -v 'src/kwin_mcp/cli.py:' || true)" + if [[ -n "$matches" ]]; then + log_fail "Guard 1 - reachable print() found in MCP server code paths (forbidden, use logging instead):" + echo "$matches" >&2 + return + fi + log_ok "Guard 1 - no reachable print() in MCP server code paths" +} + +# --------------------------------------------------------------------------- +# Guard 2: Every kwin_wayland reference in tests/, scripts/, docs/ uses +# --virtual on the same line. +# --------------------------------------------------------------------------- +guard_2_kwin_virtual() { + local matches="" + for d in tests scripts docs; do + if [[ -d "$d" ]]; then + local found + # Exclusions (non-launch references): + # - this guards script itself (documents kwin_wayland) + # - pkill cleanup lines (process kill, not a launch) + # - --version queries (introspection, not a launch) + # - lines whose first non-whitespace token after `path:lineno:` is `echo` + # (documentation strings inside shell scripts) + found="$(grep -rn 'kwin_wayland' "$d" 2>/dev/null \ + | grep -v -- '--virtual' \ + | grep -v 'scripts/ci_guards.sh' \ + | grep -v 'pkill' \ + | grep -v -- '--version' \ + | grep -Pv '^\S+:\s*echo\s' || true)" + if [[ -n "$found" ]]; then + matches+="$found"$'\n' + fi + fi + done + if [[ -n "$matches" ]]; then + log_fail "Guard 2 - kwin_wayland invocation without --virtual in tests/scripts/docs:" + printf '%s' "$matches" >&2 + return + fi + log_ok "Guard 2 - all kwin_wayland references in tests/scripts/docs use --virtual" +} + +# --------------------------------------------------------------------------- +# Guard 3: src/kwin_mcp/core.py must not import accessibility_worker at module +# top level. Uses Python AST so conditional/inline imports are tolerated. +# --------------------------------------------------------------------------- +guard_3_no_module_top_worker_import() { + local target="src/kwin_mcp/core.py" + if [[ ! -f "$target" ]]; then + log_ok "Guard 3 - $target absent, skipping" + return + fi + local result + result="$(python3 - "$target" <<'PYEOF' +import ast +import sys + +path = sys.argv[1] +with open(path, encoding="utf-8") as f: + tree = ast.parse(f.read(), filename=path) + +violations = [] +for node in tree.body: + if isinstance(node, ast.Import): + for alias in node.names: + if "accessibility_worker" in alias.name: + violations.append(f"line {node.lineno}: import {alias.name}") + elif isinstance(node, ast.ImportFrom): + mod = node.module or "" + if "accessibility_worker" in mod: + names = ", ".join(a.name for a in node.names) + violations.append(f"line {node.lineno}: from {mod} import {names}") + else: + for alias in node.names: + if "accessibility_worker" in alias.name: + violations.append( + f"line {node.lineno}: from {mod} import {alias.name}" + ) + +if violations: + print("\n".join(violations)) + sys.exit(1) +PYEOF +)" || { + log_fail "Guard 3 - core.py has module-top accessibility_worker import (must be lazy/local):" + echo "$result" >&2 + return + } + log_ok "Guard 3 - core.py has no module-top accessibility_worker import" +} + +# --------------------------------------------------------------------------- +# Guard 4: No os.fork or raw multiprocessing.Process( in src/kwin_mcp/. +# --------------------------------------------------------------------------- +guard_4_no_fork_or_process() { + local matches + matches="$(grep -rn -E 'os\.fork\b|multiprocessing\.Process\(' src/kwin_mcp/ \ + --include='*.py' 2>/dev/null || true)" + if [[ -n "$matches" ]]; then + log_fail "Guard 4 - os.fork or multiprocessing.Process( found in src/kwin_mcp/ (forbidden):" + echo "$matches" >&2 + return + fi + log_ok "Guard 4 - no os.fork or multiprocessing.Process( in src/kwin_mcp/" +} + +guard_1_no_print +guard_2_kwin_virtual +guard_3_no_module_top_worker_import +guard_4_no_fork_or_process + +if [[ "$fail" -ne 0 ]]; then + echo "[ci_guards] One or more guards failed." >&2 + exit 1 +fi + +echo "[ci_guards] All guards passed." >&2 +exit 0 diff --git a/src/kwin_mcp/accessibility_worker.py b/src/kwin_mcp/accessibility_worker.py new file mode 100644 index 0000000..ba8288f --- /dev/null +++ b/src/kwin_mcp/accessibility_worker.py @@ -0,0 +1,68 @@ +"""AT-SPI Pool worker entry points. + +Module-top imports MUST stay stdlib-only. ``gi.repository.Atspi`` caches +its D-Bus connection process-wide; if this module pulled it in at import +time, the parent (which imports this module to grab the Pool initializer ++ dispatcher callables) would taint every later worker. Both functions +therefore defer all gi / accessibility imports into their own bodies. +""" + +from __future__ import annotations + +import logging +import os +import signal +from typing import Any + +logger = logging.getLogger("kwin_mcp.atspi") + +# Bound AT-SPI startup so a stale registry cannot hang worker creation forever. +_ATSPI_INIT_TIMEOUT_SEC = 5 + + +def _init_atspi_worker(dbus_address: str) -> None: + """Pool initializer (spawn context). + + Pins ``DBUS_SESSION_BUS_ADDRESS`` for the worker process so AT-SPI + discovery hits the isolated session, then forces Atspi to bind by + reading the desktop's child count. ``-1`` means the registry is + unreachable; surface that as a clear init error. + """ + os.environ["DBUS_SESSION_BUS_ADDRESS"] = dbus_address + + def _alarm_handler(_signum: int, _frame: object) -> None: + raise RuntimeError( + "AT-SPI worker init failed: " + f"timed out after {_ATSPI_INIT_TIMEOUT_SEC}s (stale registry?) " + f"(DBUS={dbus_address})" + ) + + signal.signal(signal.SIGALRM, _alarm_handler) + signal.alarm(_ATSPI_INIT_TIMEOUT_SEC) + try: + from gi.repository import Atspi + + desktop = Atspi.get_desktop(0) + if desktop is None: + raise RuntimeError(f"AT-SPI worker init failed: desktop is None (DBUS={dbus_address})") + count = desktop.get_child_count() + if count < 0: + raise RuntimeError( + f"AT-SPI worker init failed: child_count={count} (DBUS={dbus_address})" + ) + finally: + signal.alarm(0) + logger.info("AT-SPI worker initialized for bus %s (child_count=%d)", dbus_address, count) + + +def do_atspi_op(op: str, **kwargs: Any) -> dict[str, Any]: + """Worker dispatcher. + + Delegates to ``kwin_mcp.accessibility._handle_request`` so the same + op handlers serve both the standalone ``__main__`` debug entry and + the Pool path. Imported here, never at module top. + """ + from kwin_mcp.accessibility import _handle_request + + request: dict[str, Any] = {"op": op, **kwargs} + return _handle_request(request) diff --git a/src/kwin_mcp/core.py b/src/kwin_mcp/core.py index bc9d65a..fd7b407 100644 --- a/src/kwin_mcp/core.py +++ b/src/kwin_mcp/core.py @@ -6,13 +6,18 @@ from __future__ import annotations +import contextlib import json +import logging +import multiprocessing +import multiprocessing.pool import os import shlex import shutil +import signal import subprocess -import sys import tempfile +import threading import time from pathlib import Path @@ -20,6 +25,8 @@ from kwin_mcp.screenshot import capture_frame_burst, capture_screenshot_to_file from kwin_mcp.session import LiveSession, Session, SessionConfig +_atspi_logger = logging.getLogger("kwin_mcp.atspi") + # Install hints for external binaries _INSTALL_HINTS: dict[str, str] = { "wl-paste": ( @@ -48,6 +55,56 @@ } +def _dbus_to_json(value: object) -> object: + import dbus + + if isinstance(value, dbus.Boolean): + return bool(value) + if isinstance(value, dbus.ObjectPath | dbus.Signature | dbus.String): + return str(value) + if isinstance( + value, + dbus.Byte | dbus.Int16 | dbus.UInt16 | dbus.Int32 | dbus.UInt32 | dbus.Int64 | dbus.UInt64, + ): + return int(value) + if isinstance(value, dbus.Double): + return float(value) + if isinstance(value, dbus.Array): + return [_dbus_to_json(x) for x in value] + if isinstance(value, dbus.Dictionary | dict): + return {_dbus_to_json(k): _dbus_to_json(v) for k, v in value.items()} + if isinstance(value, list | tuple): + return [_dbus_to_json(x) for x in value] + if isinstance(value, bool | int | float | str): + return value + return str(value) + + +def _format_dbus_result(result: object) -> str: + """Render a D-Bus reply for MCP clients. + + Returns the empty string for void replies, the bare value for single + primitives (so ``GetId`` returns just the UUID), and JSON for + containers and multi-value tuples. + """ + import dbus + + if result is None: + return "" + if isinstance(result, dbus.Boolean): + return "true" if bool(result) else "false" + if isinstance(result, dbus.ObjectPath | dbus.Signature | dbus.String | str): + return str(result) + if isinstance( + result, + dbus.Byte | dbus.Int16 | dbus.UInt16 | dbus.Int32 | dbus.UInt32 | dbus.Int64 | dbus.UInt64, + ): + return str(int(result)) + if isinstance(result, dbus.Double | float): + return str(float(result)) + return json.dumps(_dbus_to_json(result)) + + class AutomationEngine: """Core automation engine encapsulating all tool logic. @@ -61,6 +118,15 @@ def __init__(self) -> None: self._clipboard_enabled: bool = False self._wl_copy_proc: subprocess.Popen[bytes] | None = None self._keep_screenshots: bool = False + self._atspi_pool: multiprocessing.pool.Pool | None = None + self._atspi_worker_pids: tuple[int, ...] = () + + def __del__(self) -> None: + try: + if getattr(self, "_atspi_pool", None) is not None: + self._teardown_atspi_pool() + except Exception: + pass # ── Private helpers ─────────────────────────────────────────────────── @@ -96,47 +162,206 @@ def _session_env(self) -> dict[str, str]: env.pop("DISPLAY", None) return env - def _run_atspi(self, op: str, **kwargs: object) -> dict: - """Run an AT-SPI2 query in a subprocess with the isolated session's D-Bus address. - - The gi.repository.Atspi library caches the D-Bus connection process-wide, - so we must run queries in a fresh subprocess that inherits the correct - DBUS_SESSION_BUS_ADDRESS from the isolated dbus-run-session. + def _ensure_atspi_pool(self) -> multiprocessing.pool.Pool: + """Lazily build the spawn-context Pool that hosts AT-SPI ops. - Retries once on failure to handle transient AT-SPI2 bus instability. + The worker module is imported here, NOT at module top, so the parent + process never loads ``gi.repository.Atspi`` (which caches its D-Bus + connection process-wide). """ - env = self._session_env() - payload = json.dumps({"op": op, **kwargs}) + if self._atspi_pool is not None: + return self._atspi_pool - last_error = "" - for attempt in range(2): - if attempt > 0: - time.sleep(0.5) - try: - result = subprocess.run( - [sys.executable, "-m", "kwin_mcp.accessibility"], - input=payload, - env=env, - capture_output=True, - text=True, - timeout=30, - ) - except subprocess.TimeoutExpired: - last_error = f"AT-SPI2 query timed out after 30s (op={op})" - continue + from kwin_mcp.accessibility_worker import _init_atspi_worker - if result.returncode != 0: - last_error = f"AT-SPI2 query failed (exit {result.returncode}): {result.stderr}" - continue + dbus_addr = self._session_env().get("DBUS_SESSION_BUS_ADDRESS", "") + ctx = multiprocessing.get_context("spawn") + self._atspi_pool = ctx.Pool( + processes=1, + initializer=_init_atspi_worker, + initargs=(dbus_addr,), + ) + _atspi_logger.info("atspi pool created (DBUS=%s)", dbus_addr) + return self._atspi_pool - try: - return json.loads(result.stdout) - except json.JSONDecodeError: - last_error = f"AT-SPI2 query returned invalid JSON: {result.stdout[:200]}" - continue + @staticmethod + def _atspi_pool_worker_pids(pool: multiprocessing.pool.Pool) -> tuple[int, ...]: + return tuple(proc.pid for proc in getattr(pool, "_pool", []) if proc.pid is not None) + + @staticmethod + def _pid_alive(pid: int) -> bool: + try: + os.kill(pid, 0) + return True + except ProcessLookupError: + return False + + def _atspi_pool_has_dead_or_replaced_worker( + self, + pool: multiprocessing.pool.Pool, + recorded_pids: tuple[int, ...], + ) -> bool: + if not recorded_pids: + return False + + current_pids = self._atspi_pool_worker_pids(pool) + if current_pids != recorded_pids: + return True + + return any(not self._pid_alive(pid) for pid in recorded_pids) + + def _run_atspi(self, op: str, **kwargs: object) -> dict: + """Run an AT-SPI op via the spawn-context worker Pool. - msg = f"{last_error}. Retried once but still failed — the AT-SPI2 bus may be unstable." - raise RuntimeError(msg) + Long-lived worker amortises the ~700 ms PyGObject + Atspi startup; + warm calls land near the round-trip cost of the bus query itself. + + On TimeoutError or IPC death (the multiprocessing.pool surface for + dead workers — NOT ``concurrent.futures.process.BrokenProcessPool``), + tear down the Pool and retry exactly once. + """ + from kwin_mcp.accessibility_worker import do_atspi_op + + attempts = 0 + deadline = time.monotonic() + 30.0 + poll_quantum = 0.05 + health_check_grace = 2.0 + retry_backoff = 0.05 + call_worker_pids = self._atspi_worker_pids + while True: + attempts += 1 + attempt_started = time.monotonic() + pool = self._ensure_atspi_pool() + async_result = pool.apply_async(do_atspi_op, kwds={"op": op, **kwargs}) + timeout_count = 0 + while True: + remaining = deadline - time.monotonic() + if remaining <= 0: + _atspi_logger.warning( + "atspi call timeout (30s total, op=%s, attempt=%d)", + op, + attempts, + ) + self._teardown_atspi_pool() + if attempts >= 2: + raise multiprocessing.TimeoutError() from None + break + + wait_seconds = min(poll_quantum, remaining) + try: + result = async_result.get(timeout=wait_seconds) + except multiprocessing.TimeoutError: + timeout_count += 1 + if timeout_count < 3: + continue + + if not call_worker_pids: + continue + + if time.monotonic() - attempt_started < health_check_grace: + continue + + worker_failed = self._atspi_pool_has_dead_or_replaced_worker( + pool, + call_worker_pids, + ) + + if worker_failed: + _atspi_logger.warning( + "atspi worker died or was replaced (op=%s, attempt=%d, workers=%s)", + op, + attempts, + call_worker_pids, + ) + self._teardown_atspi_pool() + if attempts >= 2: + raise multiprocessing.TimeoutError( + "AT-SPI worker died or was replaced during call" + ) from None + break + continue + except (EOFError, BrokenPipeError, ConnectionResetError) as exc: + _atspi_logger.warning( + "atspi worker IPC death (%s, op=%s, attempt=%d)", + type(exc).__name__, + op, + attempts, + ) + self._teardown_atspi_pool() + if attempts >= 2: + raise + break + except OSError as exc: + if exc.errno in (32, 104): # EPIPE, ECONNRESET + _atspi_logger.warning( + "atspi worker IPC death (OSError errno=%d, op=%s, attempt=%d)", + exc.errno, + op, + attempts, + ) + self._teardown_atspi_pool() + if attempts >= 2: + raise + break + raise + else: + self._atspi_worker_pids = self._atspi_pool_worker_pids(pool) + return result + + if attempts < 2: + time.sleep(retry_backoff) + + @staticmethod + def _join_pool_with_timeout(pool: multiprocessing.pool.Pool, timeout: float) -> bool: + """Wrap ``Pool.join`` (which has no timeout kwarg) in a daemon thread.""" + t = threading.Thread(target=pool.join, daemon=True) + t.start() + t.join(timeout=timeout) + return not t.is_alive() + + @staticmethod + def _terminate_pool_with_timeout(pool: multiprocessing.pool.Pool, timeout: float) -> bool: + """Wrap ``Pool.terminate`` in a daemon thread so dead workers cannot hang teardown.""" + t = threading.Thread(target=pool.terminate, daemon=True) + t.start() + t.join(timeout=timeout) + return not t.is_alive() + + def _teardown_atspi_pool(self) -> None: + """Tear down the AT-SPI Pool with bounded shutdown latency. + + Escalation: ``close → join(5s) → terminate → join(2s) → SIGKILL → join(1s)``. + + ``multiprocessing.pool.Pool.join`` does not accept a timeout, so we + wrap it in a daemon thread. Worker PIDs are captured BEFORE shutdown + because ``terminate`` may reap them before SIGKILL escalation. + """ + pool = self._atspi_pool + if pool is None: + return + worker_pids = [p.pid for p in getattr(pool, "_pool", []) if p.pid is not None] + try: + pool.close() + if self._join_pool_with_timeout(pool, 5.0): + _atspi_logger.info("atspi pool torn down gracefully") + return + _atspi_logger.warning("atspi pool graceful shutdown timed out, terminating") + if self._terminate_pool_with_timeout(pool, 2.0) and self._join_pool_with_timeout( + pool, 2.0 + ): + _atspi_logger.warning("atspi pool terminated after graceful timeout") + return + _atspi_logger.error( + "atspi pool terminate timed out, escalating to SIGKILL pids=%s", + worker_pids, + ) + for pid in worker_pids: + with contextlib.suppress(ProcessLookupError): + os.kill(pid, signal.SIGKILL) + self._join_pool_with_timeout(pool, 1.0) + finally: + self._atspi_pool = None + self._atspi_worker_pids = () def _with_frame_capture( self, @@ -298,6 +523,9 @@ def session_stop(self) -> str: if self._input is not None: self._input.close() + # Pool worker holds a D-Bus connection to this session — tear down BEFORE session.stop(). + self._teardown_atspi_pool() + is_live = isinstance(self._session, LiveSession) if isinstance(self._session, LiveSession): self._session.stop(keep_screenshots=self._keep_screenshots) @@ -720,33 +948,40 @@ def dbus_call( path: str, interface: str, method: str, - args: list[str] | None = None, + args: list[str | dict] | None = None, ) -> str: - """Call a D-Bus method in the isolated session using dbus-send.""" - env = self._session_env() - cmd = [ - "dbus-send", - "--session", - "--print-reply", - f"--dest={service}", - f"{path}", - f"{interface}.{method}", - ] - if args: - cmd.extend(args) + """Call a D-Bus method in the isolated session. + + ``args`` accepts dbus-send strings (``"type:value"``) and/or + typed-JSON dicts (``{"type": ..., "value": ...}``); both shapes + may mix in one call. The reply value is rendered via + :func:`_format_dbus_result` (single primitives become bare strings, + containers and tuples become JSON). + """ + import dbus + import dbus.bus + + from kwin_mcp.dbus_args import parse_arg + + info = self._get_session().info + if info is None or not info.dbus_address: + return "D-Bus call failed: session has no D-Bus address" try: - result = subprocess.run( - cmd, - env=env, - capture_output=True, - timeout=10, - ) - except FileNotFoundError: - return _INSTALL_HINTS["dbus-send"] - if result.returncode != 0: - return f"D-Bus call failed: {result.stderr.decode(errors='replace')}" - return result.stdout.decode(errors="replace") + parsed_args = [parse_arg(a) for a in (args or [])] + except ValueError as exc: + return f"D-Bus call failed: {exc}" + + try: + bus = dbus.bus.BusConnection(info.dbus_address) + obj = bus.get_object(service, path) + iface = dbus.Interface(obj, interface) + result = iface.get_dbus_method(method)(*parsed_args) + except dbus.DBusException as exc: + name = exc.get_dbus_name() or type(exc).__name__ + msg = exc.get_dbus_message() or str(exc) + return f"D-Bus error: {name}: {msg}" + return _format_dbus_result(result) def read_app_log(self, pid: int, last_n_lines: int = 50) -> str: """Read stdout/stderr output of a launched app.""" diff --git a/src/kwin_mcp/dbus_args.py b/src/kwin_mcp/dbus_args.py new file mode 100644 index 0000000..0bab604 --- /dev/null +++ b/src/kwin_mcp/dbus_args.py @@ -0,0 +1,482 @@ +"""Parser for dbus-send-style argument strings. + +Converts the textual format used by ``dbus-send(1)`` into typed +``dbus.types.*`` instances suitable for passing through dbus-python's +``Interface.(...)`` calls. This replaces the historical reliance +on ``subprocess.run(["dbus-send", ...])`` for the generic ``dbus_call`` +MCP tool while preserving the wire format the agent already knows. + +Supported types follow the ``dbus-send(1)`` man page, plus recursive +container nesting: + + string, int16, uint16, int32, uint32, int64, uint64, double, byte, + boolean, objpath, signature + array:TYPE:V1,V2,... + dict:KTYPE:VTYPE:K1:V1,K2:V2,... + variant:TYPE:VALUE + +When a container's element type is itself a container, elements are +separated by ``':'`` instead of ``','`` to disambiguate the inner +comma-separated values. + +Examples: + >>> parse_dbus_send_arg("string:hello") + dbus.String('hello') + >>> parse_dbus_send_arg("int32:42") + dbus.Int32(42) + >>> parse_dbus_send_arg("array:string:a,b,c") + dbus.Array([dbus.String('a'), ...], signature=dbus.Signature('s')) + +All malformed input raises :class:`ValueError` with a message that +starts with ``"dbus-send arg:"``. +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING + +import dbus + +if TYPE_CHECKING: + from collections.abc import Mapping + +__all__ = ["parse_arg", "parse_dbus_send_arg", "parse_typed_arg", "to_dbus_send_string"] + + +# ── Type tables ──────────────────────────────────────────────────────────── + +_BASIC_SIGNATURES: dict[str, str] = { + "string": "s", + "int16": "n", + "uint16": "q", + "int32": "i", + "uint32": "u", + "int64": "x", + "uint64": "t", + "byte": "y", + "double": "d", + "boolean": "b", + "objpath": "o", + "signature": "g", +} + +_BASIC_TYPES: frozenset[str] = frozenset(_BASIC_SIGNATURES.keys()) +_CONTAINER_TYPES: frozenset[str] = frozenset({"array", "dict", "variant"}) + + +# ── Helpers ──────────────────────────────────────────────────────────────── + + +def _err(msg: str) -> ValueError: + return ValueError(f"dbus-send arg: {msg}") + + +def _split_once(s: str) -> tuple[str, str]: + parts = s.split(":", 1) + if len(parts) != 2: + raise _err(f"expected 'type:value' format, got {s!r}") + return parts[0], parts[1] + + +def _parse_int(value: str, type_name: str, *, signed: bool, bits: int) -> int: + """Parse an integer literal and bounds-check it against the dbus type.""" + try: + v = int(value) + except ValueError as exc: + raise _err(f"invalid {type_name} value {value!r}") from exc + if signed: + lo, hi = -(1 << (bits - 1)), (1 << (bits - 1)) - 1 + else: + lo, hi = 0, (1 << bits) - 1 + if not (lo <= v <= hi): + raise _err(f"{type_name} value {v} out of range [{lo}, {hi}]") + return v + + +def _parse_double(value: str) -> float: + try: + return float(value) + except ValueError as exc: + raise _err(f"invalid double value {value!r}") from exc + + +def _parse_boolean(value: str) -> bool: + lowered = value.lower() + if lowered == "true": + return True + if lowered == "false": + return False + raise _err(f"boolean must be 'true' or 'false', got {value!r}") + + +def _parse_basic(type_name: str, value: str) -> object: + if type_name == "string": + return dbus.String(value) + if type_name == "objpath": + if not value.startswith("/"): + raise _err(f"objpath must start with '/', got {value!r}") + return dbus.ObjectPath(value) + if type_name == "signature": + return dbus.Signature(value) + if type_name == "boolean": + return dbus.Boolean(_parse_boolean(value)) + if type_name == "double": + return dbus.Double(_parse_double(value)) + if type_name == "byte": + return dbus.Byte(_parse_int(value, "byte", signed=False, bits=8)) + if type_name == "int16": + return dbus.Int16(_parse_int(value, "int16", signed=True, bits=16)) + if type_name == "uint16": + return dbus.UInt16(_parse_int(value, "uint16", signed=False, bits=16)) + if type_name == "int32": + return dbus.Int32(_parse_int(value, "int32", signed=True, bits=32)) + if type_name == "uint32": + return dbus.UInt32(_parse_int(value, "uint32", signed=False, bits=32)) + if type_name == "int64": + return dbus.Int64(_parse_int(value, "int64", signed=True, bits=64)) + if type_name == "uint64": + return dbus.UInt64(_parse_int(value, "uint64", signed=False, bits=64)) + raise _err(f"unknown type {type_name!r}") + + +def _peel_element_type(s: str) -> tuple[str, str]: + """Peel the element type-prefix off the front of a container value-spec. + + Returns ``(type_prefix, remaining_values_str)`` where ``type_prefix`` + is the colon-joined type chain describing one element (e.g. + ``"string"``, ``"array:string"``, ``"dict:string:int32"``), and + ``remaining_values_str`` is whatever follows it. + """ + type_, rest = _split_once(s) + if type_ in _BASIC_TYPES: + return type_, rest + if type_ == "array": + sub, rest2 = _peel_element_type(rest) + return f"array:{sub}", rest2 + if type_ == "variant": + sub, rest2 = _peel_element_type(rest) + return f"variant:{sub}", rest2 + if type_ == "dict": + ktype, rest2 = _peel_element_type(rest) + if ktype not in _BASIC_TYPES: + raise _err(f"dict key type must be basic, got {ktype!r}") + vtype, rest3 = _peel_element_type(rest2) + return f"dict:{ktype}:{vtype}", rest3 + raise _err(f"unknown type {type_!r}") + + +def _signature_of(type_prefix: str) -> str: + return _signature_of_parts(type_prefix.split(":")) + + +def _signature_of_parts(parts: list[str]) -> str: + if not parts: + raise _err("empty type") + head = parts[0] + if head in _BASIC_SIGNATURES: + if len(parts) != 1: + raise _err(f"basic type {head!r} cannot have sub-types") + return _BASIC_SIGNATURES[head] + if head == "array": + return "a" + _signature_of_parts(parts[1:]) + if head == "dict": + if len(parts) < 3: + raise _err("dict needs key and value types") + if parts[1] not in _BASIC_SIGNATURES: + raise _err(f"dict key type must be basic, got {parts[1]!r}") + return "a{" + _BASIC_SIGNATURES[parts[1]] + _signature_of_parts(parts[2:]) + "}" + if head == "variant": + return "v" + raise _err(f"unknown type {head!r}") + + +def _is_container_prefix(prefix: str) -> bool: + return prefix.split(":", 1)[0] in _CONTAINER_TYPES + + +# ── Public API ───────────────────────────────────────────────────────────── + + +def parse_dbus_send_arg(s: str) -> object: + """Parse a single ``dbus-send``-style argument into a typed value. + + Args: + s: A string like ``"string:hello"`` or ``"array:int32:1,2,3"``. + + Returns: + A ``dbus.types.*`` instance. For ``variant:...`` the inner typed + value is returned directly; dbus-python infers the surrounding + variant from the introspection signature. + + Raises: + ValueError: If the input is malformed. The message always begins + with ``"dbus-send arg:"``. + """ + type_, rest = _split_once(s) + + if type_ in _BASIC_TYPES: + return _parse_basic(type_, rest) + if type_ == "array": + return _parse_array_rest(rest) + if type_ == "dict": + return _parse_dict_rest(rest) + if type_ == "variant": + return parse_dbus_send_arg(rest) + raise _err(f"unknown type {type_!r}") + + +# ── Container parsers ────────────────────────────────────────────────────── + + +def _parse_array_rest(rest: str) -> object: + """Parse the substring after ``array:`` into a ``dbus.Array``.""" + elem_prefix, values_str = _peel_element_type(rest) + sig = _signature_of(elem_prefix) + if values_str == "": + return dbus.Array([], signature=sig) + sep = ":" if _is_container_prefix(elem_prefix) else "," + pieces = values_str.split(sep) + items = [parse_dbus_send_arg(f"{elem_prefix}:{piece}") for piece in pieces] + return dbus.Array(items, signature=sig) + + +def _parse_dict_rest(rest: str) -> object: + """Parse the substring after ``dict:`` into a ``dbus.Dictionary``.""" + ktype, after_k = _split_once(rest) + if ktype not in _BASIC_TYPES: + raise _err(f"dict key type must be basic, got {ktype!r}") + vtype_prefix, values_str = _peel_element_type(after_k) + ksig = _BASIC_SIGNATURES[ktype] + vsig = _signature_of(vtype_prefix) + full_sig = f"{ksig}{vsig}" + if values_str == "": + return dbus.Dictionary({}, signature=full_sig) + if _is_container_prefix(vtype_prefix): + # The dbus-send man page only documents basic value types here, and + # mixing comma-separated pairs with comma-separated container values + # is ambiguous to parse. Reject explicitly so callers fall back to + # building dicts programmatically (or, in the future, JSON). + raise _err("dict with container value type is not supported") + pairs = values_str.split(",") + out: dict[object, object] = {} + for pair in pairs: + sub = pair.split(":", 1) + if len(sub) != 2: + raise _err(f"dict pair must be 'key:value', got {pair!r}") + key = _parse_basic(ktype, sub[0]) + val = _parse_basic(vtype_prefix, sub[1]) + out[key] = val + return dbus.Dictionary(out, signature=full_sig) + + +# ── Typed JSON args (option C: backward-compatible widening) ─────────────── +# +# Accepts arguments shaped as ``{"type": "", "value": }`` +# in addition to the legacy ``"type:value"`` strings. ``parse_arg`` is the +# top-level dispatcher that routes each arg to the right backend based on +# its Python type. +# +# Supported typed-JSON shapes: +# +# Basic: {"type": "", "value": } +# Array: {"type": "array", "element_type": "", +# "value": [, ...]} +# Dict: {"type": "dict", "key_type": "", +# "value_type": "", "value": {: , ...}} +# Variant: {"type": "variant", "value_type": "", +# "value": } +# +# Container element/key/value types are restricted to basic types in v1 +# (mirrors what the dbus-send string syntax supports cleanly). Nested +# containers can be added later without breaking compatibility. + + +def _coerce_basic(type_name: str, value: object) -> object: + """Validate-and-wrap a Python primitive for a basic D-Bus type.""" + if type_name == "string": + if not isinstance(value, str): + raise _err(f"string value must be str, got {type(value).__name__}") + return dbus.String(value) + if type_name == "objpath": + if not isinstance(value, str): + raise _err(f"objpath value must be str, got {type(value).__name__}") + if not value.startswith("/"): + raise _err(f"objpath must start with '/', got {value!r}") + return dbus.ObjectPath(value) + if type_name == "signature": + if not isinstance(value, str): + raise _err(f"signature value must be str, got {type(value).__name__}") + return dbus.Signature(value) + if type_name == "boolean": + if not isinstance(value, bool): + raise _err(f"boolean value must be bool, got {type(value).__name__}") + return dbus.Boolean(value) + if type_name == "double": + if not isinstance(value, (int, float)) or isinstance(value, bool): + raise _err(f"double value must be number, got {type(value).__name__}") + return dbus.Double(float(value)) + if type_name in _BASIC_TYPES: + if not isinstance(value, int) or isinstance(value, bool): + raise _err(f"{type_name} value must be int, got {type(value).__name__}") + return _parse_basic(type_name, str(value)) + raise _err(f"unknown type {type_name!r}") + + +def parse_typed_arg(d: Mapping[str, object]) -> object: + """Parse a single typed-JSON arg into a ``dbus.types.*`` instance.""" + if not isinstance(d, dict): + raise _err(f"typed-JSON arg must be a dict, got {type(d).__name__}") + if "type" not in d or "value" not in d: + raise _err( + f"typed-JSON arg must have 'type' and 'value' keys; got keys {sorted(d.keys())!r}" + ) + type_name = d["type"] + value = d["value"] + if not isinstance(type_name, str): + raise _err(f"'type' must be a string, got {type(type_name).__name__}") + + if type_name in _BASIC_TYPES: + return _coerce_basic(type_name, value) + + if type_name == "array": + elem_type = d.get("element_type") + if not isinstance(elem_type, str) or elem_type not in _BASIC_TYPES: + raise _err( + f"typed-JSON array requires 'element_type' = a basic type name; got {elem_type!r}" + ) + if not isinstance(value, list): + raise _err(f"typed-JSON array 'value' must be a list, got {type(value).__name__}") + items = [_coerce_basic(elem_type, v) for v in value] + return dbus.Array(items, signature=_BASIC_SIGNATURES[elem_type]) + + if type_name == "dict": + key_type = d.get("key_type") + val_type = d.get("value_type") + if not isinstance(key_type, str) or key_type not in _BASIC_TYPES: + raise _err(f"typed-JSON dict requires 'key_type' = a basic type name; got {key_type!r}") + if not isinstance(val_type, str) or val_type not in _BASIC_TYPES: + raise _err( + f"typed-JSON dict requires 'value_type' = a basic type name; got {val_type!r}" + ) + if not isinstance(value, dict): + raise _err(f"typed-JSON dict 'value' must be a dict, got {type(value).__name__}") + out: dict[object, object] = {} + for k, v in value.items(): + out[_coerce_basic(key_type, k)] = _coerce_basic(val_type, v) + sig = _BASIC_SIGNATURES[key_type] + _BASIC_SIGNATURES[val_type] + return dbus.Dictionary(out, signature=sig) + + if type_name == "variant": + val_type = d.get("value_type") + if not isinstance(val_type, str) or val_type not in _BASIC_TYPES: + raise _err( + f"typed-JSON variant requires 'value_type' = a basic type name; got {val_type!r}" + ) + return _coerce_basic(val_type, value) + + raise _err(f"unknown typed-JSON type {type_name!r}") + + +def parse_arg(arg: str | Mapping[str, object]) -> object: + """Top-level per-arg dispatcher: legacy string OR typed-JSON dict. + + Strings are routed to :func:`parse_dbus_send_arg`. Dicts are routed to + :func:`parse_typed_arg`. Anything else raises :class:`ValueError`. + """ + if isinstance(arg, str): + return parse_dbus_send_arg(arg) + if isinstance(arg, dict): + return parse_typed_arg(arg) + raise _err(f"arg must be str or dict, got {type(arg).__name__}") + + +def _to_dbus_send_basic(type_name: str, value: object) -> str: + """Render a Python primitive as the value half of a dbus-send string.""" + if type_name == "boolean": + if not isinstance(value, bool): + raise _err(f"boolean value must be bool, got {type(value).__name__}") + return "true" if value else "false" + if type_name == "double": + if not isinstance(value, (int, float)) or isinstance(value, bool): + raise _err(f"double value must be number, got {type(value).__name__}") + return repr(float(value)) + if type_name in {"string", "objpath", "signature"}: + if not isinstance(value, str): + raise _err(f"{type_name} value must be str, got {type(value).__name__}") + if "," in value or ":" in value: + # Comma/colon would corrupt container element splitting in + # dbus-send syntax. The dbus-send CLI itself has the same + # limitation, so we reject up-front rather than producing a + # subtly wrong CLI invocation. + raise _err( + f"{type_name} value contains ',' or ':', cannot be encoded " + "as a dbus-send string; use the in-process path" + ) + return value + if not isinstance(value, int) or isinstance(value, bool): + raise _err(f"{type_name} value must be int, got {type(value).__name__}") + return str(value) + + +def to_dbus_send_string(arg: str | Mapping[str, object]) -> str: + """Render an arg (legacy string or typed-JSON dict) as a dbus-send string. + + Used as a temporary bridge in ``core.py::dbus_call`` until the body is + refactored to call dbus-python directly. Round-trip identity holds: + ``parse_dbus_send_arg(to_dbus_send_string(parse_typed_arg(d)))`` produces + the same dbus type as ``parse_typed_arg(d)`` for every supported shape. + """ + if isinstance(arg, str): + parse_dbus_send_arg(arg) + return arg + if not isinstance(arg, dict): + raise _err(f"arg must be str or dict, got {type(arg).__name__}") + if "type" not in arg or "value" not in arg: + raise _err( + f"typed-JSON arg must have 'type' and 'value' keys; got keys {sorted(arg.keys())!r}" + ) + type_name = arg["type"] + value = arg["value"] + if not isinstance(type_name, str): + raise _err(f"'type' must be a string, got {type(type_name).__name__}") + + if type_name in _BASIC_TYPES: + return f"{type_name}:{_to_dbus_send_basic(type_name, value)}" + + if type_name == "array": + elem_type = arg.get("element_type") + if not isinstance(elem_type, str) or elem_type not in _BASIC_TYPES: + raise _err( + f"typed-JSON array requires 'element_type' = a basic type name; got {elem_type!r}" + ) + if not isinstance(value, list): + raise _err(f"typed-JSON array 'value' must be a list, got {type(value).__name__}") + rendered = ",".join(_to_dbus_send_basic(elem_type, v) for v in value) + return f"array:{elem_type}:{rendered}" + + if type_name == "dict": + key_type = arg.get("key_type") + val_type = arg.get("value_type") + if not isinstance(key_type, str) or key_type not in _BASIC_TYPES: + raise _err(f"typed-JSON dict requires 'key_type' = a basic type name; got {key_type!r}") + if not isinstance(val_type, str) or val_type not in _BASIC_TYPES: + raise _err( + f"typed-JSON dict requires 'value_type' = a basic type name; got {val_type!r}" + ) + if not isinstance(value, dict): + raise _err(f"typed-JSON dict 'value' must be a dict, got {type(value).__name__}") + pairs = ",".join( + f"{_to_dbus_send_basic(key_type, k)}:{_to_dbus_send_basic(val_type, v)}" + for k, v in value.items() + ) + return f"dict:{key_type}:{val_type}:{pairs}" + + if type_name == "variant": + val_type = arg.get("value_type") + if not isinstance(val_type, str) or val_type not in _BASIC_TYPES: + raise _err( + f"typed-JSON variant requires 'value_type' = a basic type name; got {val_type!r}" + ) + return f"variant:{val_type}:{_to_dbus_send_basic(val_type, value)}" + + raise _err(f"unknown typed-JSON type {type_name!r}") diff --git a/src/kwin_mcp/server.py b/src/kwin_mcp/server.py index b212f3d..ea51177 100644 --- a/src/kwin_mcp/server.py +++ b/src/kwin_mcp/server.py @@ -754,17 +754,25 @@ def dbus_call( interface: Annotated[str, Field(description='Interface name (e.g. "org.kde.KWin.Scripting").')], method: Annotated[str, Field(description="Method name to call.")], args: Annotated[ - list[str] | None, + list[str | dict] | None, Field( - description="Method arguments in dbus-send format " - '(e.g. ["string:hello", "int32:42", "boolean:true"]).' + description=( + "Method arguments. Two interchangeable shapes are accepted " + "and may be mixed in the same list: " + '(legacy) ["string:hello", "int32:42", "boolean:true"] OR ' + '(typed JSON) [{"type":"string","value":"hello"}, ' + '{"type":"int32","value":42}, ' + '{"type":"array","element_type":"string","value":["a","b"]}].' + ) ), ] = None, ) -> str: - """Call a D-Bus method in the isolated session using dbus-send. + """Call a D-Bus method in the isolated session. - Executes a D-Bus method call and returns the reply. Arguments must use - dbus-send type notation (e.g. "string:value", "int32:42", "boolean:true"). + Executes a D-Bus method call and returns the reply. Each entry in + ``args`` may use dbus-send notation (``"string:value"``, ``"int32:42"``, + ...) or the typed-JSON shape (``{"type":"string","value":"hello"}``). + Both shapes can mix in one call. """ return _engine.dbus_call( service=service, path=path, interface=interface, method=method, args=args diff --git a/tests/__init__.py b/tests/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/conftest.py b/tests/conftest.py new file mode 100644 index 0000000..fb4877d --- /dev/null +++ b/tests/conftest.py @@ -0,0 +1,29 @@ +from __future__ import annotations + +import logging +from typing import TYPE_CHECKING + +import pytest + +from kwin_mcp.session import Session, SessionConfig + +if TYPE_CHECKING: + from collections.abc import Iterator + + from kwin_mcp.session import SessionInfo + +if TYPE_CHECKING: + from collections.abc import Iterator + +logger = logging.getLogger(__name__) + + +@pytest.fixture(scope="session") +def virtual_session() -> Iterator[SessionInfo]: + session = Session() + info = session.start(SessionConfig(socket_name="wayland-test")) + try: + yield info + finally: + logger.info("Stopping virtual KWin session") + session.stop() diff --git a/tests/integration/__init__.py b/tests/integration/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py new file mode 100644 index 0000000..0372dbe --- /dev/null +++ b/tests/integration/conftest.py @@ -0,0 +1,20 @@ +from __future__ import annotations + +from typing import TYPE_CHECKING + +import pytest + +from kwin_mcp.core import AutomationEngine + +if TYPE_CHECKING: + from collections.abc import Iterator + + +@pytest.fixture(scope="session") +def engine() -> Iterator[AutomationEngine]: + eng = AutomationEngine() + eng.session_start(screen_width=1920, screen_height=1080) + try: + yield eng + finally: + eng.session_stop() diff --git a/tests/integration/test_atspi_pool.py b/tests/integration/test_atspi_pool.py new file mode 100644 index 0000000..0457586 --- /dev/null +++ b/tests/integration/test_atspi_pool.py @@ -0,0 +1,201 @@ +"""Tests for the AT-SPI multiprocessing.Pool worker integration in core.py.""" + +from __future__ import annotations + +import contextlib +import multiprocessing +import multiprocessing.pool +import os +import signal +import subprocess +import threading +import time +import uuid +from multiprocessing import get_context +from typing import TYPE_CHECKING + +import pytest + +from kwin_mcp.accessibility_worker import _init_atspi_worker + +if TYPE_CHECKING: + from kwin_mcp.core import AutomationEngine + +pytestmark = pytest.mark.kwin + + +def _pid_alive(pid: int) -> bool: + """Return True if ``pid`` still answers signal 0 (running or zombie not yet reaped). + + ProcessLookupError = pid is gone. Anything else (PermissionError) is unexpected + for our own children and is allowed to propagate. + """ + try: + os.kill(pid, 0) + except ProcessLookupError: + return False + return True + + +def _pgrep_has_worker_pid(pid: int) -> bool: + result = subprocess.run( + ["pgrep", "-f", "_init_atspi_worker"], + check=False, + stdout=subprocess.PIPE, + stderr=subprocess.DEVNULL, + text=True, + ) + return str(pid) in result.stdout.split() + + +def _join_worker_processes(pool: multiprocessing.pool.Pool) -> None: + for proc in getattr(pool, "_pool", []): + proc.join(timeout=0.2) + + +def _worker_pids(pool: multiprocessing.pool.Pool) -> list[int]: + procs = getattr(pool, "_pool", []) + return [p.pid for p in procs if p.pid is not None] + + +def _ensure_session(engine: AutomationEngine) -> None: + if engine._session is None or not engine._session.is_running: + engine.session_start(screen_width=1920, screen_height=1080) + + +def test_pool_cold_start_under_1500ms(engine: AutomationEngine) -> None: + """Cold first accessibility_tree call (spawn worker + bind AT-SPI) finishes under 1.5s.""" + _ensure_session(engine) + if engine._atspi_pool is not None: + engine._teardown_atspi_pool() + + t0 = time.perf_counter() + out = engine.accessibility_tree(app_name="") + cold_ms = (time.perf_counter() - t0) * 1000.0 + + assert isinstance(out, str) + assert cold_ms < 1500.0, f"cold start {cold_ms:.1f}ms >= 1500ms threshold" + + +def test_pool_warm_call_under_200ms(engine: AutomationEngine) -> None: + """Warm reuse of an already-spawned worker keeps each tree call under 200ms.""" + _ensure_session(engine) + t0 = time.perf_counter() + engine.accessibility_tree(app_name="") + cold_ms = (time.perf_counter() - t0) * 1000.0 + + warm_timings: list[float] = [] + for _ in range(2): + ts = time.perf_counter() + out = engine.accessibility_tree(app_name="") + warm_timings.append((time.perf_counter() - ts) * 1000.0) + assert isinstance(out, str) + + for idx, ms in enumerate(warm_timings, start=1): + assert ms < 200.0, f"warm call #{idx} took {ms:.1f}ms (>= 200ms threshold)" + assert min(warm_timings) <= cold_ms, ( + f"warm calls {warm_timings} not faster than cold {cold_ms:.1f}ms — pool not warming?" + ) + + +def test_pool_recovers_from_external_kill(engine: AutomationEngine) -> None: + """SIGKILL on the worker is caught by ``_run_atspi`` retry, which respawns the pool.""" + _ensure_session(engine) + out = engine.accessibility_tree(app_name="") + assert isinstance(out, str) + pool_before = engine._atspi_pool + assert pool_before is not None + pids = _worker_pids(pool_before) + assert pids, "expected at least one live worker pid" + worker_pid = pids[0] + + os.kill(worker_pid, signal.SIGKILL) + deadline = time.perf_counter() + 0.5 + while _pid_alive(worker_pid) and time.perf_counter() < deadline: + time.sleep(0.01) + + out = engine.accessibility_tree(app_name="") + assert isinstance(out, str) + pool_after = engine._atspi_pool + assert pool_after is not None + assert pool_after is not pool_before, "pool was not rebuilt after worker death" + + +def test_pool_init_failure_raises_with_bus_address() -> None: + """Spawn-context Pool with an unreachable DBUS bus address fails — never silently succeeds.""" + ctx = get_context("spawn") + bad_addr = f"unix:path=/tmp/nonexistent-bus-for-test-{uuid.uuid4().hex}" + pool = ctx.Pool(processes=1, initializer=_init_atspi_worker, initargs=(bad_addr,)) + try: + with pytest.raises(BaseException) as excinfo: + pool.apply_async(os.getpid).get(timeout=5) + # On Python 3.12, ``Atspi.get_desktop`` aborts the worker via dbind-ERROR before + # our RuntimeError is raised; the Pool respawns workers that all die, so .get() + # raises ``multiprocessing.TimeoutError`` after the 5s budget. On Python 3.13+ + # the original RuntimeError propagates instead. Accept either form so the test + # remains valid across versions, and at minimum verify that .get() did NOT + # silently return a pid (which would mean init succeeded). + msg = str(excinfo.value) + is_timeout = isinstance(excinfo.value, multiprocessing.TimeoutError) + is_init_err = "AT-SPI worker init failed" in msg and bad_addr in msg + assert is_timeout or is_init_err, ( + f"expected TimeoutError or AT-SPI init failure containing {bad_addr!r}; " + f"got {type(excinfo.value).__name__}: {msg[:200]!r}" + ) + finally: + pool.terminate() + pool.join() + + +def test_pool_terminate_within_7s_even_if_hung(engine: AutomationEngine) -> None: + """SIGSTOP'd worker can't shut down gracefully; session_stop must escalate within 7.5s.""" + _ensure_session(engine) + engine.accessibility_tree(app_name="") + pool = engine._atspi_pool + assert pool is not None + pids = _worker_pids(pool) + assert pids + worker_pid = pids[0] + + os.kill(worker_pid, signal.SIGSTOP) + cont_timer = threading.Timer(5.2, os.kill, args=(worker_pid, signal.SIGCONT)) + cont_timer.start() + try: + t0 = time.perf_counter() + engine.session_stop() + elapsed = time.perf_counter() - t0 + + assert elapsed < 7.5, f"session_stop took {elapsed:.2f}s (>= 7.5s threshold)" + assert not _pid_alive(worker_pid), ( + f"worker pid {worker_pid} still alive after session_stop " + f"(expected SIGKILL escalation to reap it)" + ) + finally: + cont_timer.cancel() + # Best-effort SIGCONT in case the test failed mid-flight before SIGKILL landed. + with contextlib.suppress(ProcessLookupError): + os.kill(worker_pid, signal.SIGCONT) + + +def test_no_zombie_after_session_stop(engine: AutomationEngine) -> None: + """``session_stop`` reaps the AT-SPI worker — no orphan PID may survive.""" + _ensure_session(engine) + engine.accessibility_tree(app_name="") + pool = engine._atspi_pool + assert pool is not None + pids = _worker_pids(pool) + assert pids + worker_pid = pids[0] + + engine.session_stop() + try: + assert not _pid_alive(worker_pid), ( + f"worker pid {worker_pid} still present after session_stop" + ) + assert not _pgrep_has_worker_pid(worker_pid), ( + f"pgrep still reports AT-SPI worker pid {worker_pid} after session_stop" + ) + finally: + # Restart the session so the session-scoped ``engine`` fixture can be reused + # by any test files that pytest collects after this one. + _ensure_session(engine) diff --git a/tests/integration/test_dbus_call_compat.py b/tests/integration/test_dbus_call_compat.py new file mode 100644 index 0000000..2a73393 --- /dev/null +++ b/tests/integration/test_dbus_call_compat.py @@ -0,0 +1,95 @@ +"""Backward-compat integration tests for dbus_call MCP tool. + +The dbus_call refactor (Task 9) replaces the dbus-send subprocess shim +with an in-process dbus-python call. These tests exercise the dual-shape +``args`` contract (legacy ``"type:value"`` strings + typed JSON dicts + +mixed lists) against a real virtual KWin session and verify the public +output format is preserved. +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING + +import pytest + +if TYPE_CHECKING: + from kwin_mcp.core import AutomationEngine + +pytestmark = pytest.mark.kwin + + +def test_legacy_get_id_no_args(engine: AutomationEngine) -> None: + out = engine.dbus_call( + service="org.freedesktop.DBus", + path="/org/freedesktop/DBus", + interface="org.freedesktop.DBus", + method="GetId", + ) + assert out + assert not out.startswith("D-Bus error:") + assert not out.startswith("Argument error:") + assert len(out) >= 32 + + +def test_typed_get_id_empty_array(engine: AutomationEngine) -> None: + out = engine.dbus_call( + service="org.freedesktop.DBus", + path="/org/freedesktop/DBus", + interface="org.freedesktop.DBus", + method="GetId", + args=[], + ) + assert out + assert not out.startswith("D-Bus error:") + + +def test_kwin_show_desktop_legacy_bool(engine: AutomationEngine) -> None: + out = engine.dbus_call( + service="org.kde.KWin", + path="/KWin", + interface="org.kde.KWin", + method="showDesktop", + args=["boolean:false"], + ) + assert out == "" + + +def test_kwin_show_desktop_typed_bool(engine: AutomationEngine) -> None: + out = engine.dbus_call( + service="org.kde.KWin", + path="/KWin", + interface="org.kde.KWin", + method="showDesktop", + args=[{"type": "boolean", "value": False}], + ) + assert out == "" + + +def test_mixed_legacy_and_typed_args(engine: AutomationEngine) -> None: + out_a = engine.dbus_call( + service="org.kde.KWin", + path="/KWin", + interface="org.kde.KWin", + method="showDesktop", + args=["boolean:true"], + ) + out_b = engine.dbus_call( + service="org.kde.KWin", + path="/KWin", + interface="org.kde.KWin", + method="showDesktop", + args=[{"type": "boolean", "value": True}], + ) + assert out_a == "" + assert out_b == "" + + +def test_unknown_service_returns_dbus_error(engine: AutomationEngine) -> None: + out = engine.dbus_call( + service="org.example.DefinitelyDoesNotExist", + path="/x", + interface="org.example.X", + method="Foo", + ) + assert out.startswith("D-Bus error:") diff --git a/tests/test_dbus_args.py b/tests/test_dbus_args.py new file mode 100644 index 0000000..da21707 --- /dev/null +++ b/tests/test_dbus_args.py @@ -0,0 +1,562 @@ +from __future__ import annotations + +import typing + +import dbus +import pytest + +from kwin_mcp.dbus_args import ( + parse_arg, + parse_dbus_send_arg, + parse_typed_arg, + to_dbus_send_string, +) + +# ── Basic types ──────────────────────────────────────────────────────────── + + +def test_string() -> None: + result = parse_dbus_send_arg("string:hello") + assert isinstance(result, dbus.String) + assert result == "hello" + + +def test_string_empty() -> None: + result = parse_dbus_send_arg("string:") + assert isinstance(result, dbus.String) + assert result == "" + + +def test_string_with_colons_kept_in_value() -> None: + result = parse_dbus_send_arg("string:hello:world:!") + assert isinstance(result, dbus.String) + assert result == "hello:world:!" + + +def test_int16() -> None: + result = parse_dbus_send_arg("int16:-32000") + assert isinstance(result, dbus.Int16) + assert result == -32000 + + +def test_int32() -> None: + result = parse_dbus_send_arg("int32:42") + assert isinstance(result, dbus.Int32) + assert result == 42 + + +def test_int64() -> None: + result = parse_dbus_send_arg("int64:-9223372036854775808") + assert isinstance(result, dbus.Int64) + assert result == -9223372036854775808 + + +def test_uint16() -> None: + result = parse_dbus_send_arg("uint16:65535") + assert isinstance(result, dbus.UInt16) + assert result == 65535 + + +def test_uint32() -> None: + result = parse_dbus_send_arg("uint32:4294967295") + assert isinstance(result, dbus.UInt32) + assert result == 4294967295 + + +def test_uint64() -> None: + result = parse_dbus_send_arg("uint64:18446744073709551615") + assert isinstance(result, dbus.UInt64) + assert result == 18446744073709551615 + + +def test_byte() -> None: + result = parse_dbus_send_arg("byte:255") + assert isinstance(result, dbus.Byte) + assert result == 255 + + +def test_double() -> None: + result = parse_dbus_send_arg("double:3.14") + assert isinstance(result, dbus.Double) + assert result == pytest.approx(3.14) + + +def test_boolean_true() -> None: + result = parse_dbus_send_arg("boolean:true") + assert isinstance(result, dbus.Boolean) + assert bool(result) is True + + +def test_boolean_false() -> None: + result = parse_dbus_send_arg("boolean:false") + assert isinstance(result, dbus.Boolean) + assert bool(result) is False + + +def test_boolean_uppercase() -> None: + result = parse_dbus_send_arg("boolean:TRUE") + assert isinstance(result, dbus.Boolean) + assert bool(result) is True + + +def test_boolean_mixed_case() -> None: + result = parse_dbus_send_arg("boolean:False") + assert isinstance(result, dbus.Boolean) + assert bool(result) is False + + +def test_objpath() -> None: + result = parse_dbus_send_arg("objpath:/org/kde/KWin") + assert isinstance(result, dbus.ObjectPath) + assert result == "/org/kde/KWin" + + +def test_signature() -> None: + result = parse_dbus_send_arg("signature:s") + assert isinstance(result, dbus.Signature) + assert result == "s" + + +# ── Arrays ───────────────────────────────────────────────────────────────── + + +def test_array_strings() -> None: + result = parse_dbus_send_arg("array:string:a,b,c") + assert isinstance(result, dbus.Array) + assert result.signature == "s" + assert list(result) == ["a", "b", "c"] + assert all(isinstance(item, dbus.String) for item in result) + + +def test_array_int32() -> None: + result = parse_dbus_send_arg("array:int32:1,2,3") + assert isinstance(result, dbus.Array) + assert result.signature == "i" + assert list(result) == [1, 2, 3] + assert all(isinstance(item, dbus.Int32) for item in result) + + +def test_array_int64() -> None: + result = parse_dbus_send_arg("array:int64:100,200,300") + assert isinstance(result, dbus.Array) + assert result.signature == "x" + assert list(result) == [100, 200, 300] + assert all(isinstance(item, dbus.Int64) for item in result) + + +def test_array_boolean() -> None: + result = parse_dbus_send_arg("array:boolean:true,false,true") + assert isinstance(result, dbus.Array) + assert result.signature == "b" + assert [bool(x) for x in result] == [True, False, True] + + +def test_array_empty() -> None: + result = parse_dbus_send_arg("array:string:") + assert isinstance(result, dbus.Array) + assert result.signature == "s" + assert list(result) == [] + + +def test_array_single_element() -> None: + result = parse_dbus_send_arg("array:string:only") + assert isinstance(result, dbus.Array) + assert list(result) == ["only"] + + +# ── Dictionaries ─────────────────────────────────────────────────────────── + + +def test_dict_string_string() -> None: + result = parse_dbus_send_arg("dict:string:string:k1:v1,k2:v2") + assert isinstance(result, dbus.Dictionary) + assert result.signature == "ss" + assert dict(result) == {"k1": "v1", "k2": "v2"} + + +def test_dict_string_int32() -> None: + result = parse_dbus_send_arg("dict:string:int32:key1:1,key2:2") + assert isinstance(result, dbus.Dictionary) + assert result.signature == "si" + assert dict(result) == {"key1": 1, "key2": 2} + for v in result.values(): + assert isinstance(v, dbus.Int32) + + +def test_dict_empty() -> None: + result = parse_dbus_send_arg("dict:string:int32:") + assert isinstance(result, dbus.Dictionary) + assert result.signature == "si" + assert dict(result) == {} + + +def test_dict_single_pair() -> None: + result = parse_dbus_send_arg("dict:string:string:only:one") + assert isinstance(result, dbus.Dictionary) + assert dict(result) == {"only": "one"} + + +# ── Variants ─────────────────────────────────────────────────────────────── + + +def test_variant_string() -> None: + result = parse_dbus_send_arg("variant:string:hello") + assert isinstance(result, dbus.String) + assert result == "hello" + + +def test_variant_int32() -> None: + result = parse_dbus_send_arg("variant:int32:42") + assert isinstance(result, dbus.Int32) + assert result == 42 + + +def test_variant_boolean() -> None: + result = parse_dbus_send_arg("variant:boolean:true") + assert isinstance(result, dbus.Boolean) + assert bool(result) is True + + +# ── Nested containers ────────────────────────────────────────────────────── + + +def test_nested_array_of_arrays() -> None: + result = parse_dbus_send_arg("array:array:string:a,b:c,d") + assert isinstance(result, dbus.Array) + assert result.signature == "as" + assert len(result) == 2 + assert list(result[0]) == ["a", "b"] + assert list(result[1]) == ["c", "d"] + + +def test_array_of_variants() -> None: + result = parse_dbus_send_arg("array:variant:string:one:two") + assert isinstance(result, dbus.Array) + assert result.signature == "v" + assert list(result) == ["one", "two"] + + +# ── Return type contract ─────────────────────────────────────────────────── + + +def test_returns_dbus_type_not_python_int() -> None: + result = parse_dbus_send_arg("int32:7") + assert type(result) is not int + assert isinstance(result, dbus.Int32) + + +def test_returns_dbus_type_not_python_str() -> None: + result = parse_dbus_send_arg("string:foo") + assert type(result) is not str + assert isinstance(result, dbus.String) + + +def test_returns_dbus_type_not_python_bool() -> None: + result = parse_dbus_send_arg("boolean:true") + assert type(result) is not bool + assert isinstance(result, dbus.Boolean) + + +# ── Errors ───────────────────────────────────────────────────────────────── + + +def test_error_unknown_type() -> None: + with pytest.raises(ValueError, match=r"dbus-send arg: unknown type 'foobar'"): + parse_dbus_send_arg("foobar:42") + + +def test_error_missing_colon() -> None: + with pytest.raises(ValueError, match=r"dbus-send arg: expected 'type:value' format"): + parse_dbus_send_arg("nocolon") + + +def test_error_invalid_int32() -> None: + with pytest.raises(ValueError, match=r"dbus-send arg: invalid int32 value 'notanint'"): + parse_dbus_send_arg("int32:notanint") + + +def test_error_invalid_int64() -> None: + with pytest.raises(ValueError, match=r"dbus-send arg: invalid int64 value"): + parse_dbus_send_arg("int64:abc") + + +def test_error_invalid_boolean() -> None: + with pytest.raises( + ValueError, + match=r"dbus-send arg: boolean must be 'true' or 'false', got 'yes'", + ): + parse_dbus_send_arg("boolean:yes") + + +def test_error_invalid_double() -> None: + with pytest.raises(ValueError, match=r"dbus-send arg: invalid double value"): + parse_dbus_send_arg("double:notafloat") + + +def test_error_int_out_of_range() -> None: + with pytest.raises(ValueError, match=r"dbus-send arg: byte value .* out of range"): + parse_dbus_send_arg("byte:300") + + +def test_error_uint_negative() -> None: + with pytest.raises(ValueError, match=r"dbus-send arg: uint32 value .* out of range"): + parse_dbus_send_arg("uint32:-1") + + +def test_error_objpath_no_leading_slash() -> None: + with pytest.raises(ValueError, match=r"dbus-send arg: objpath must start with '/'"): + parse_dbus_send_arg("objpath:no-slash") + + +def test_error_dict_pair_missing_colon() -> None: + with pytest.raises(ValueError, match=r"dbus-send arg: dict pair must be 'key:value'"): + parse_dbus_send_arg("dict:string:string:keyonly") + + +def test_error_dict_container_value_unsupported() -> None: + with pytest.raises(ValueError, match=r"dict with container value type is not supported"): + parse_dbus_send_arg("dict:string:array:string:k1:a,b") + + +def test_error_message_prefix_consistent() -> None: + cases = [ + "foobar:42", + "nocolon", + "int32:notanint", + "boolean:yes", + "byte:300", + "objpath:relative", + ] + for case in cases: + with pytest.raises(ValueError) as exc_info: + parse_dbus_send_arg(case) + assert str(exc_info.value).startswith("dbus-send arg:"), case + + +# ── Typed-JSON parser ────────────────────────────────────────────────────── + + +def test_typed_string() -> None: + result = parse_typed_arg({"type": "string", "value": "hello"}) + assert isinstance(result, dbus.String) + assert result == "hello" + + +def test_typed_int32() -> None: + result = parse_typed_arg({"type": "int32", "value": 42}) + assert isinstance(result, dbus.Int32) + assert result == 42 + + +def test_typed_boolean() -> None: + result = parse_typed_arg({"type": "boolean", "value": True}) + assert isinstance(result, dbus.Boolean) + assert bool(result) is True + + +def test_typed_double() -> None: + result = parse_typed_arg({"type": "double", "value": 3.14}) + assert isinstance(result, dbus.Double) + assert result == pytest.approx(3.14) + + +def test_typed_objpath() -> None: + result = parse_typed_arg({"type": "objpath", "value": "/org/kde/KWin"}) + assert isinstance(result, dbus.ObjectPath) + assert result == "/org/kde/KWin" + + +def test_typed_array() -> None: + result = parse_typed_arg({"type": "array", "element_type": "string", "value": ["a", "b", "c"]}) + assert isinstance(result, dbus.Array) + assert result.signature == "s" + assert list(result) == ["a", "b", "c"] + + +def test_typed_array_int32() -> None: + result = parse_typed_arg({"type": "array", "element_type": "int32", "value": [1, 2, 3]}) + assert isinstance(result, dbus.Array) + assert result.signature == "i" + assert list(result) == [1, 2, 3] + + +def test_typed_array_empty() -> None: + result = parse_typed_arg({"type": "array", "element_type": "string", "value": []}) + assert isinstance(result, dbus.Array) + assert result.signature == "s" + assert list(result) == [] + + +def test_typed_dict() -> None: + result = parse_typed_arg( + { + "type": "dict", + "key_type": "string", + "value_type": "int32", + "value": {"k1": 1, "k2": 2}, + } + ) + assert isinstance(result, dbus.Dictionary) + assert result.signature == "si" + assert dict(result) == {"k1": 1, "k2": 2} + + +def test_typed_variant() -> None: + result = parse_typed_arg({"type": "variant", "value_type": "string", "value": "hello"}) + assert isinstance(result, dbus.String) + assert result == "hello" + + +def test_typed_error_missing_value_key() -> None: + with pytest.raises(ValueError, match=r"must have 'type' and 'value'"): + parse_typed_arg({"type": "string"}) + + +def test_typed_error_missing_type_key() -> None: + with pytest.raises(ValueError, match=r"must have 'type' and 'value'"): + parse_typed_arg({"value": "hello"}) + + +def test_typed_error_unknown_type() -> None: + with pytest.raises(ValueError, match=r"unknown typed-JSON type 'mystery'"): + parse_typed_arg({"type": "mystery", "value": 1}) + + +def test_typed_error_string_with_int_value() -> None: + with pytest.raises(ValueError, match=r"string value must be str"): + parse_typed_arg({"type": "string", "value": 42}) + + +def test_typed_error_int_with_string_value() -> None: + with pytest.raises(ValueError, match=r"int32 value must be int"): + parse_typed_arg({"type": "int32", "value": "not-an-int"}) + + +def test_typed_error_boolean_rejects_int() -> None: + with pytest.raises(ValueError, match=r"boolean value must be bool"): + parse_typed_arg({"type": "boolean", "value": 1}) + + +def test_typed_error_array_missing_element_type() -> None: + with pytest.raises(ValueError, match=r"array requires 'element_type'"): + parse_typed_arg({"type": "array", "value": []}) + + +def test_typed_error_array_value_not_list() -> None: + with pytest.raises(ValueError, match=r"array 'value' must be a list"): + parse_typed_arg({"type": "array", "element_type": "string", "value": "not-a-list"}) + + +def test_typed_error_dict_value_not_dict() -> None: + with pytest.raises(ValueError, match=r"dict 'value' must be a dict"): + parse_typed_arg( + { + "type": "dict", + "key_type": "string", + "value_type": "int32", + "value": ["k", 1], + } + ) + + +# ── Dispatcher (parse_arg) ───────────────────────────────────────────────── + + +def test_dispatch_legacy_string() -> None: + assert type(parse_arg("string:hello")) is dbus.String + assert parse_arg("string:hello") == "hello" + + +def test_dispatch_legacy_int32() -> None: + assert type(parse_arg("int32:42")) is dbus.Int32 + assert parse_arg("int32:42") == 42 + + +def test_dispatch_typed_dict() -> None: + result = parse_arg({"type": "int32", "value": 42}) + assert type(result) is dbus.Int32 + assert result == 42 + + +def test_dispatch_mixed_per_arg() -> None: + args: list[str | dict] = [ + "string:hello", + {"type": "int32", "value": 42}, + "boolean:true", + ] + parsed = [parse_arg(a) for a in args] + assert isinstance(parsed[0], dbus.String) and parsed[0] == "hello" + assert isinstance(parsed[1], dbus.Int32) and parsed[1] == 42 + assert isinstance(parsed[2], dbus.Boolean) and bool(parsed[2]) is True + + +def test_dispatch_rejects_non_str_non_dict() -> None: + bad: object = 42 + with pytest.raises(ValueError, match=r"arg must be str or dict, got int"): + parse_arg(typing.cast("str", bad)) + + +def test_dispatch_rejects_list() -> None: + bad: object = ["string:hello"] + with pytest.raises(ValueError, match=r"arg must be str or dict, got list"): + parse_arg(typing.cast("str", bad)) + + +# ── Bridge (to_dbus_send_string) ─────────────────────────────────────────── + + +def test_bridge_passthrough_legacy() -> None: + assert to_dbus_send_string("string:hello") == "string:hello" + assert to_dbus_send_string("int32:42") == "int32:42" + + +def test_bridge_typed_basic() -> None: + assert to_dbus_send_string({"type": "string", "value": "hello"}) == "string:hello" + assert to_dbus_send_string({"type": "int32", "value": 42}) == "int32:42" + assert to_dbus_send_string({"type": "boolean", "value": True}) == "boolean:true" + assert to_dbus_send_string({"type": "boolean", "value": False}) == "boolean:false" + + +def test_bridge_typed_array() -> None: + out = to_dbus_send_string({"type": "array", "element_type": "string", "value": ["a", "b", "c"]}) + assert out == "array:string:a,b,c" + + +def test_bridge_typed_dict() -> None: + out = to_dbus_send_string( + { + "type": "dict", + "key_type": "string", + "value_type": "int32", + "value": {"k1": 1, "k2": 2}, + } + ) + assert out.startswith("dict:string:int32:") + parts = sorted(out[len("dict:string:int32:") :].split(",")) + assert parts == ["k1:1", "k2:2"] + + +def test_bridge_typed_variant() -> None: + assert ( + to_dbus_send_string({"type": "variant", "value_type": "string", "value": "x"}) + == "variant:string:x" + ) + + +def test_bridge_round_trip() -> None: + typed = {"type": "int32", "value": 7} + rendered = to_dbus_send_string(typed) + via_string = parse_dbus_send_arg(rendered) + via_typed = parse_typed_arg(typed) + assert type(via_string) is type(via_typed) is dbus.Int32 + assert via_string == via_typed == 7 + + +def test_bridge_rejects_string_with_comma() -> None: + with pytest.raises(ValueError, match=r"contains ',' or ':'"): + to_dbus_send_string({"type": "string", "value": "has,comma"}) + + +def test_bridge_rejects_invalid_legacy() -> None: + with pytest.raises(ValueError, match=r"unknown type"): + to_dbus_send_string("foobar:42") diff --git a/tests/test_smoke.py b/tests/test_smoke.py new file mode 100644 index 0000000..44decbc --- /dev/null +++ b/tests/test_smoke.py @@ -0,0 +1,14 @@ +from __future__ import annotations + +import dbus +import dbus.bus +import pytest + + +@pytest.mark.kwin +def test_virtual_session_reachable(virtual_session) -> None: + assert virtual_session.dbus_address + + bus = dbus.bus.BusConnection(virtual_session.dbus_address) + kwin_obj = bus.get_object("org.kde.KWin", "/org/kde/KWin") + dbus.Interface(kwin_obj, "org.freedesktop.DBus.Peer").Ping() diff --git a/uv.lock b/uv.lock index 869b219..f236ac5 100644 --- a/uv.lock +++ b/uv.lock @@ -234,6 +234,15 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/0e/61/66938bbb5fc52dbdf84594873d5b51fb1f7c7794e9c0f5bd885f30bc507b/idna-3.11-py3-none-any.whl", hash = "sha256:771a87f49d9defaf64091e6e6fe9c18d4833f140bd19464795bc32d966ca37ea", size = 71008, upload-time = "2025-10-12T14:55:18.883Z" }, ] +[[package]] +name = "iniconfig" +version = "2.3.0" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/72/34/14ca021ce8e5dfedc35312d08ba8bf51fdd999c576889fc2c24cb97f4f10/iniconfig-2.3.0.tar.gz", hash = "sha256:c76315c77db068650d49c5b56314774a7804df16fee4402c1f19d6d15d8c4730", size = 20503, upload-time = "2025-10-18T21:55:43.219Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/cb/b1/3846dd7f199d53cb17f49cba7e651e9ce294d8497c8c150530ed11865bb8/iniconfig-2.3.0-py3-none-any.whl", hash = "sha256:f631c04d2c48c52b84d0d0549c99ff3859c98df65b3101406327ecc7d53fbf12", size = 7484, upload-time = "2025-10-18T21:55:41.639Z" }, +] + [[package]] name = "jsonschema" version = "4.26.0" @@ -274,6 +283,9 @@ dependencies = [ [package.dev-dependencies] dev = [ + { name = "pytest" }, + { name = "pytest-asyncio" }, + { name = "pytest-timeout" }, { name = "ruff" }, { name = "ty" }, ] @@ -288,6 +300,9 @@ requires-dist = [ [package.metadata.requires-dev] dev = [ + { name = "pytest", specifier = ">=8.0" }, + { name = "pytest-asyncio", specifier = ">=0.23" }, + { name = "pytest-timeout", specifier = ">=2.3.0" }, { name = "ruff", specifier = ">=0.11.0" }, { name = "ty", specifier = ">=0.0.1" }, ] @@ -317,6 +332,15 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/fd/d9/eaa1f80170d2b7c5ba23f3b59f766f3a0bb41155fbc32a69adfa1adaaef9/mcp-1.26.0-py3-none-any.whl", hash = "sha256:904a21c33c25aa98ddbeb47273033c435e595bbacfdb177f4bd87f6dceebe1ca", size = 233615, upload-time = "2026-01-24T19:40:30.652Z" }, ] +[[package]] +name = "packaging" +version = "26.2" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/d7/f1/e7a6dd94a8d4a5626c03e4e99c87f241ba9e350cd9e6d75123f992427270/packaging-26.2.tar.gz", hash = "sha256:ff452ff5a3e828ce110190feff1178bb1f2ea2281fa2075aadb987c2fb221661", size = 228134, upload-time = "2026-04-24T20:15:23.917Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/df/b2/87e62e8c3e2f4b32e5fe99e0b86d576da1312593b39f47d8ceef365e95ed/packaging-26.2-py3-none-any.whl", hash = "sha256:5fc45236b9446107ff2415ce77c807cee2862cb6fac22b8a73826d0693b0980e", size = 100195, upload-time = "2026-04-24T20:15:22.081Z" }, +] + [[package]] name = "pillow" version = "12.1.1" @@ -386,6 +410,15 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/ec/d2/de599c95ba0a973b94410477f8bf0b6f0b5e67360eb89bcb1ad365258beb/pillow-12.1.1-cp314-cp314t-win_arm64.whl", hash = "sha256:7b03048319bfc6170e93bd60728a1af51d3dd7704935feb228c4d4faab35d334", size = 2546446, upload-time = "2026-02-11T04:22:50.342Z" }, ] +[[package]] +name = "pluggy" +version = "1.6.0" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/f9/e2/3e91f31a7d2b083fe6ef3fa267035b518369d9511ffab804f839851d2779/pluggy-1.6.0.tar.gz", hash = "sha256:7dcc130b76258d33b90f61b658791dede3486c3e6bfb003ee5c9bfb396dd22f3", size = 69412, upload-time = "2025-05-15T12:30:07.975Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/54/20/4d324d65cc6d9205fabedc306948156824eb9f0ee1633355a8f7ec5c66bf/pluggy-1.6.0-py3-none-any.whl", hash = "sha256:e920276dd6813095e9377c0bc5566d94c932c33b27a3e3945d8389c374dd4746", size = 20538, upload-time = "2025-05-15T12:30:06.134Z" }, +] + [[package]] name = "pycairo" version = "1.29.0" @@ -514,6 +547,15 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/00/4b/ccc026168948fec4f7555b9164c724cf4125eac006e176541483d2c959be/pydantic_settings-2.13.1-py3-none-any.whl", hash = "sha256:d56fd801823dbeae7f0975e1f8c8e25c258eb75d278ea7abb5d9cebb01b56237", size = 58929, upload-time = "2026-02-19T13:45:06.034Z" }, ] +[[package]] +name = "pygments" +version = "2.20.0" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/c3/b2/bc9c9196916376152d655522fdcebac55e66de6603a76a02bca1b6414f6c/pygments-2.20.0.tar.gz", hash = "sha256:6757cd03768053ff99f3039c1a36d6c0aa0b263438fcab17520b30a303a82b5f", size = 4955991, upload-time = "2026-03-29T13:29:33.898Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/f4/7e/a72dd26f3b0f4f2bf1dd8923c85f7ceb43172af56d63c7383eb62b332364/pygments-2.20.0-py3-none-any.whl", hash = "sha256:81a9e26dd42fd28a23a2d169d86d7ac03b46e2f8b59ed4698fb4785f946d0176", size = 1231151, upload-time = "2026-03-29T13:29:30.038Z" }, +] + [[package]] name = "pygobject" version = "3.54.5" @@ -537,6 +579,47 @@ crypto = [ { name = "cryptography" }, ] +[[package]] +name = "pytest" +version = "9.0.3" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "colorama", marker = "sys_platform == 'win32'" }, + { name = "iniconfig" }, + { name = "packaging" }, + { name = "pluggy" }, + { name = "pygments" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/7d/0d/549bd94f1a0a402dc8cf64563a117c0f3765662e2e668477624baeec44d5/pytest-9.0.3.tar.gz", hash = "sha256:b86ada508af81d19edeb213c681b1d48246c1a91d304c6c81a427674c17eb91c", size = 1572165, upload-time = "2026-04-07T17:16:18.027Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/d4/24/a372aaf5c9b7208e7112038812994107bc65a84cd00e0354a88c2c77a617/pytest-9.0.3-py3-none-any.whl", hash = "sha256:2c5efc453d45394fdd706ade797c0a81091eccd1d6e4bccfcd476e2b8e0ab5d9", size = 375249, upload-time = "2026-04-07T17:16:16.13Z" }, +] + +[[package]] +name = "pytest-asyncio" +version = "1.3.0" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "pytest" }, + { name = "typing-extensions", marker = "python_full_version < '3.13'" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/90/2c/8af215c0f776415f3590cac4f9086ccefd6fd463befeae41cd4d3f193e5a/pytest_asyncio-1.3.0.tar.gz", hash = "sha256:d7f52f36d231b80ee124cd216ffb19369aa168fc10095013c6b014a34d3ee9e5", size = 50087, upload-time = "2025-11-10T16:07:47.256Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/e5/35/f8b19922b6a25bc0880171a2f1a003eaeb93657475193ab516fd87cac9da/pytest_asyncio-1.3.0-py3-none-any.whl", hash = "sha256:611e26147c7f77640e6d0a92a38ed17c3e9848063698d5c93d5aa7aa11cebff5", size = 15075, upload-time = "2025-11-10T16:07:45.537Z" }, +] + +[[package]] +name = "pytest-timeout" +version = "2.4.0" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "pytest" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/ac/82/4c9ecabab13363e72d880f2fb504c5f750433b2b6f16e99f4ec21ada284c/pytest_timeout-2.4.0.tar.gz", hash = "sha256:7e68e90b01f9eff71332b25001f85c75495fc4e3a836701876183c4bcfd0540a", size = 17973, upload-time = "2025-05-05T19:44:34.99Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/fa/b6/3127540ecdf1464a00e5a01ee60a1b09175f6913f0644ac748494d9c4b21/pytest_timeout-2.4.0-py3-none-any.whl", hash = "sha256:c42667e5cdadb151aeb5b26d114aff6bdf5a907f176a007a30b940d3d865b5c2", size = 14382, upload-time = "2025-05-05T19:44:33.502Z" }, +] + [[package]] name = "python-dotenv" version = "1.2.1"