From 5de08da6dc189f65d031ab25a6001e3b4aaa3f43 Mon Sep 17 00:00:00 2001 From: Byeonghoon Yoo Date: Tue, 5 May 2026 22:33:24 +0900 Subject: [PATCH 1/5] ci: run pull request workflows on all bases Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus --- .github/workflows/ci.yml | 1 - .github/workflows/docs-seo.yml | 1 - 2 files changed, 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index cd9aba2..4a9d1d1 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: 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" From 5e9bb9616ef85f9189ca858d24ab29ca8da466c9 Mon Sep 17 00:00:00 2001 From: Byeonghoon Yoo Date: Tue, 5 May 2026 00:19:32 +0900 Subject: [PATCH 2/5] chore(tests): bootstrap pytest infra + CI guards --- .github/workflows/ci.yml | 42 +++++++++- CLAUDE.md | 4 + pyproject.toml | 8 +- scripts/ci_guards.sh | 162 +++++++++++++++++++++++++++++++++++++++ tests/__init__.py | 0 tests/conftest.py | 24 ++++++ tests/test_smoke.py | 14 ++++ uv.lock | 69 +++++++++++++++++ 8 files changed, 318 insertions(+), 5 deletions(-) create mode 100755 scripts/ci_guards.sh create mode 100644 tests/__init__.py create mode 100644 tests/conftest.py create mode 100644 tests/test_smoke.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4a9d1d1..1848ac9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -15,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 @@ -27,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 @@ -47,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/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/pyproject.toml b/pyproject.toml index 6a93f7d..088431f 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -75,7 +75,13 @@ 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"] + +[tool.pytest.ini_options] +testpaths = ["tests"] +addopts = "-ra -q" +asyncio_mode = "auto" +markers = ["kwin: marks tests requiring a real KWin virtual session (deselect with -m 'not kwin')"] [tool.ruff] target-version = "py312" 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/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..d6a40b5 --- /dev/null +++ b/tests/conftest.py @@ -0,0 +1,24 @@ +from __future__ import annotations + +import logging +from typing import TYPE_CHECKING + +import pytest + +from kwin_mcp.session import Session, SessionConfig, 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/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..d13c38d 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,8 @@ dependencies = [ [package.dev-dependencies] dev = [ + { name = "pytest" }, + { name = "pytest-asyncio" }, { name = "ruff" }, { name = "ty" }, ] @@ -288,6 +299,8 @@ requires-dist = [ [package.metadata.requires-dev] dev = [ + { name = "pytest", specifier = ">=8.0" }, + { name = "pytest-asyncio", specifier = ">=0.23" }, { name = "ruff", specifier = ">=0.11.0" }, { name = "ty", specifier = ">=0.0.1" }, ] @@ -317,6 +330,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 +408,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 +545,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 +577,35 @@ 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 = "python-dotenv" version = "1.2.1" From d3c357362fedb139ef260ae073d8764f29d0b97e Mon Sep 17 00:00:00 2001 From: Byeonghoon Yoo Date: Tue, 5 May 2026 01:48:23 +0900 Subject: [PATCH 3/5] refactor(dbus): replace dbus-send CLI with in-process dbus-python - Add dbus_args.py: full dbus-send recursive-descent parser (12 basic types + array/dict/variant containers) returning dbus-python types. - Add typed-JSON args dispatcher: args list now accepts both legacy 'type:value' strings and {type, value} dicts, mixed in one call. Schema widened (additive): list[str] -> list[str | dict]. - core.py:dbus_call now uses dbus.bus.BusConnection + Interface in-process (no subprocess). Errors surface as 'D-Bus error: : '. - _format_dbus_result: void -> empty, primitive -> bare, container -> JSON. - 80 unit tests for the parser; 6 integration tests against virtual KWin. - docs/design/dbus-call-call-sites.md documents the public-tool contract. Pre-commit: ruff/ty clean, ci_guards pass, 87 tests green. --- CHANGELOG.md | 13 + docs/design/dbus-call-call-sites.md | 70 +++ src/kwin_mcp/core.py | 105 +++- src/kwin_mcp/dbus_args.py | 482 ++++++++++++++++++ src/kwin_mcp/server.py | 20 +- tests/conftest.py | 7 +- tests/integration/__init__.py | 0 tests/integration/conftest.py | 20 + tests/integration/test_dbus_call_compat.py | 95 ++++ tests/test_dbus_args.py | 562 +++++++++++++++++++++ 10 files changed, 1343 insertions(+), 31 deletions(-) create mode 100644 docs/design/dbus-call-call-sites.md create mode 100644 src/kwin_mcp/dbus_args.py create mode 100644 tests/integration/__init__.py create mode 100644 tests/integration/conftest.py create mode 100644 tests/integration/test_dbus_call_compat.py create mode 100644 tests/test_dbus_args.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 66eaada..d9b0cdd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,19 @@ 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. + ### Fixed - Segfault on Python 3.14 caused by missing `argtypes` on variadic `ei_seat_bind_capabilities` ctypes call 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/src/kwin_mcp/core.py b/src/kwin_mcp/core.py index bc9d65a..8fe11a7 100644 --- a/src/kwin_mcp/core.py +++ b/src/kwin_mcp/core.py @@ -48,6 +48,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. @@ -720,33 +770,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/conftest.py b/tests/conftest.py index d6a40b5..fb4877d 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -5,7 +5,12 @@ import pytest -from kwin_mcp.session import Session, SessionConfig, SessionInfo +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 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_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") From 3f248bc377ae3b87acb9d40e8da3920535d296f6 Mon Sep 17 00:00:00 2001 From: Byeonghoon Yoo Date: Tue, 5 May 2026 02:50:44 +0900 Subject: [PATCH 4/5] refactor(atspi): replace per-call subprocess with multiprocessing.Pool worker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces per-call `subprocess.run([sys.executable, '-m', 'kwin_mcp.accessibility', ...])` with a long-lived spawn-context multiprocessing.Pool(processes=1) worker. Key changes: - New module `kwin_mcp.accessibility_worker` containing picklable top-level callables `_init_atspi_worker` and `do_atspi_op`. Module is NEVER imported at module-top by core.py (CI guard 3 enforces this); deferred imports inside `_ensure_atspi_pool` and `_run_atspi` only. - Worker init validates `Atspi.get_desktop(0).get_child_count() >= 0` against the bus address and raises RuntimeError with the bus address on failure. - Pool teardown protocol survives hung workers within 7s: close → join 5s → terminate → join 2s → SIGKILL → join 1s. - IPC error handling catches `(EOFError, BrokenPipeError, ConnectionResetError)` and `OSError errno in (32, 104)` for transparent recovery; one retry, then re-raise. - Defensive `__del__` teardown swallows exceptions for partial-init safety. - AGENT_EXEC_APPROVAL=1 prefix used for all uv invocations during this change per session permission. Performance: - Cold start ≤ 1.5s (verified) - Warm calls < 200ms (verified, plan-mandated threshold) - vs ~700ms per subprocess on the previous design Tests: - tests/integration/test_atspi_pool.py — 6 tests, all passing under `AGENT_EXEC_APPROVAL=1 uv run pytest -m kwin`: cold start <1.5s; warm call <200ms; recovers from external SIGKILL; init failure surfaces RuntimeError with bus address; teardown within 7s even with SIGSTOPped worker; no zombie processes after session_stop. Plan tasks: 11, 12, 13, 14, 15. --- CHANGELOG.md | 1 + pyproject.toml | 16 +- src/kwin_mcp/accessibility_worker.py | 68 ++++++++ src/kwin_mcp/core.py | 250 +++++++++++++++++++++++---- tests/integration/test_atspi_pool.py | 201 +++++++++++++++++++++ uv.lock | 14 ++ 6 files changed, 512 insertions(+), 38 deletions(-) create mode 100644 src/kwin_mcp/accessibility_worker.py create mode 100644 tests/integration/test_atspi_pool.py diff --git a/CHANGELOG.md b/CHANGELOG.md index d9b0cdd..e4d6b24 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### 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 diff --git a/pyproject.toml b/pyproject.toml index 088431f..49e4816 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -75,12 +75,20 @@ requires = ["uv_build>=0.10.3,<0.12.0"] build-backend = "uv_build" [dependency-groups] -dev = ["ruff>=0.11.0", "ty>=0.0.1", "pytest>=8.0", "pytest-asyncio>=0.23"] +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] @@ -114,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/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 8fe11a7..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": ( @@ -111,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 ─────────────────────────────────────────────────── @@ -146,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. + def _ensure_atspi_pool(self) -> multiprocessing.pool.Pool: + """Lazily build the spawn-context Pool that hosts AT-SPI ops. - 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. - - 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) - msg = f"{last_error}. Retried once but still failed — the AT-SPI2 bus may be unstable." - raise RuntimeError(msg) + @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. + + 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, @@ -348,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) 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/uv.lock b/uv.lock index d13c38d..f236ac5 100644 --- a/uv.lock +++ b/uv.lock @@ -285,6 +285,7 @@ dependencies = [ dev = [ { name = "pytest" }, { name = "pytest-asyncio" }, + { name = "pytest-timeout" }, { name = "ruff" }, { name = "ty" }, ] @@ -301,6 +302,7 @@ requires-dist = [ 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" }, ] @@ -606,6 +608,18 @@ 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" From d49118e321c3705ab9222dff784b96314f875fa5 Mon Sep 17 00:00:00 2001 From: Byeonghoon Yoo Date: Tue, 5 May 2026 03:18:09 +0900 Subject: [PATCH 5/5] feat(screenshot): add capability probe + ScreenShot2 D-Bus primary backend PR 4 lands the screenshot-backend overhaul as a single atomic unit. What changed: - New `_probe_screenshot_capability(dbus_address, wayland_socket)` runs at session_start. Tries a 1x1 `CaptureArea` via in-process ScreenShot2 D-Bus, falls back to a real `spectacle` capture, returns one of: "screenshot2_dbus" | "spectacle_cli" | "unavailable". 10s wall-clock cap; bus address GUID redacted in logs. - `SessionInfo.screenshot_backend: str = "unavailable"` carries the probe result; `Session.start` and `LiveSession.__init__` populate it via a helper that swallows probe exceptions and warns on failure. - `capture_screenshot_to_file` and `capture_frame_burst` now accept `screenshot_backend` as an explicit keyword argument (no global state). Dispatch is a `match` on the three values; "unavailable" raises `RuntimeError("No screenshot backend available; install spectacle or fix KWin EglBackend")`. - Two `core.py` call sites pass `screenshot_backend=info.screenshot_backend`. Tests: - New `tests/integration/test_screenshot_backends.py` (4 tests, marked `@pytest.mark.kwin`): 1. probe returns one of the three documented strings 2. screenshot writes a real PNG (signature byte-checked) 3. burst capture preserves backend invariant + valid PNGs per frame 4. forcing backend="unavailable" raises with the exact message All 4 pass against a real virtual KWin session. Plan tasks: 16, 17, 18, 19. --- CHANGELOG.md | 1 + src/kwin_mcp/core.py | 2 + src/kwin_mcp/screenshot.py | 152 +++++++++++++++--- src/kwin_mcp/session.py | 18 +++ tests/integration/test_screenshot_backends.py | 139 ++++++++++++++++ 5 files changed, 291 insertions(+), 21 deletions(-) create mode 100644 tests/integration/test_screenshot_backends.py diff --git a/CHANGELOG.md b/CHANGELOG.md index e4d6b24..38fa620 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - 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. +- Screenshot backend selection now runs an eager capability probe at session_start. `_probe_screenshot_capability(dbus_address, wayland_socket)` tries a 1×1 `CaptureArea` via in-process ScreenShot2 D-Bus first; on any failure it falls back to a real `spectacle` capture. The result (`screenshot2_dbus` / `spectacle_cli` / `unavailable`) is stored on `SessionInfo.screenshot_backend` and consumed by `capture_screenshot_to_file` and `capture_frame_burst` via an explicit keyword argument — no global state. `unavailable` raises `RuntimeError("No screenshot backend available; install spectacle or fix KWin EglBackend").` ### Fixed diff --git a/src/kwin_mcp/core.py b/src/kwin_mcp/core.py index fd7b407..ff78065 100644 --- a/src/kwin_mcp/core.py +++ b/src/kwin_mcp/core.py @@ -382,6 +382,7 @@ def _with_frame_capture( output_dir=info.screenshot_dir, delays_ms=screenshot_after_ms, wayland_socket=info.wayland_socket, + screenshot_backend=info.screenshot_backend, ) lines = [action_result, f"Captured {len(frames)} frames:"] @@ -552,6 +553,7 @@ def screenshot(self, include_cursor: bool = False) -> str: wayland_socket=info.wayland_socket, include_cursor=include_cursor, output_dir=info.screenshot_dir, + screenshot_backend=info.screenshot_backend, ) size_kb = path.stat().st_size / 1024 return f"Screenshot saved: {path} ({size_kb:.1f} KB)" diff --git a/src/kwin_mcp/screenshot.py b/src/kwin_mcp/screenshot.py index da0279d..eae292f 100644 --- a/src/kwin_mcp/screenshot.py +++ b/src/kwin_mcp/screenshot.py @@ -2,14 +2,23 @@ from __future__ import annotations +import contextlib +import logging import os import subprocess +import tempfile import time from pathlib import Path import dbus import dbus.bus +logger = logging.getLogger("kwin_mcp.screenshot") + +_NO_SCREENSHOT_BACKEND_MESSAGE = ( + "No screenshot backend available; install spectacle or fix KWin EglBackend" +) + def capture_screenshot_to_file( dbus_address: str = "", @@ -17,6 +26,7 @@ def capture_screenshot_to_file( *, include_cursor: bool = False, output_dir: Path | None = None, + screenshot_backend: str = "unavailable", ) -> Path: """Capture a screenshot and save to a file. @@ -36,12 +46,23 @@ def capture_screenshot_to_file( timestamp = time.strftime("%Y%m%d_%H%M%S") output_path = output_dir / f"screenshot_{timestamp}.png" - _capture_via_spectacle( - dbus_address, - wayland_socket, - output_path=output_path, - include_cursor=include_cursor, - ) + logger.debug("dispatching screenshot backend=%s", screenshot_backend) + match screenshot_backend: + case "screenshot2_dbus": + capture_screenshot_dbus( + dbus_address, + output_path=output_path, + include_cursor=include_cursor, + ) + case "spectacle_cli": + _capture_via_spectacle( + dbus_address, + wayland_socket, + output_path=output_path, + include_cursor=include_cursor, + ) + case _: + raise RuntimeError(_NO_SCREENSHOT_BACKEND_MESSAGE) return output_path @@ -114,6 +135,7 @@ def capture_frame_burst( *, include_cursor: bool = False, wayland_socket: str = "", + screenshot_backend: str = "unavailable", ) -> list[Path]: """Capture multiple screenshots at specified delays after an action. @@ -134,21 +156,25 @@ def capture_frame_burst( output_dir.mkdir(parents=True, exist_ok=True) sorted_delays = sorted(delays_ms) - # Try fast D-Bus capture first - try: - return _capture_frame_burst_dbus( - dbus_address, output_dir, sorted_delays, include_cursor=include_cursor - ) - except dbus.DBusException: - # D-Bus authorization failed (e.g. live session without permission bypass). - # Fall back to spectacle CLI (slower but always authorized). - return _capture_frame_burst_spectacle( - dbus_address, - wayland_socket, - output_dir, - sorted_delays, - include_cursor=include_cursor, - ) + logger.debug("dispatching screenshot backend=%s", screenshot_backend) + match screenshot_backend: + case "screenshot2_dbus": + return _capture_frame_burst_dbus( + dbus_address, + output_dir, + sorted_delays, + include_cursor=include_cursor, + ) + case "spectacle_cli": + return _capture_frame_burst_spectacle( + dbus_address, + wayland_socket, + output_dir, + sorted_delays, + include_cursor=include_cursor, + ) + case _: + raise RuntimeError(_NO_SCREENSHOT_BACKEND_MESSAGE) def _capture_frame_burst_dbus( @@ -287,3 +313,87 @@ def _capture_via_spectacle( if not output_path.exists() or output_path.stat().st_size == 0: msg = "spectacle produced no output" raise RuntimeError(msg) + + +def _probe_screenshot_capability(dbus_address: str, wayland_socket: str) -> str: + """Detect which screenshot backend is usable for this session. + + Tries KWin ScreenShot2 ``CaptureArea(0, 0, 1, 1, ...)`` first; on any + failure falls back to a real ``spectacle`` capture. Per the Task 4 spike + (``docs/design/screenshot2-virtual-feasibility.md``), ScreenShot2 is the + primary fast path in normal virtual KWin sessions on KWin 6.6.x. Forced + compositor variants such as ``KWIN_COMPOSE=O`` / ``KWIN_COMPOSE=Q`` can + return ``org.kde.KWin.ScreenShot2.Error.Cancelled``; in those cases the + probe selects spectacle. + + Returns ``"screenshot2_dbus"``, ``"spectacle_cli"``, or ``"unavailable"``. + The decision is logged via ``kwin_mcp.screenshot`` with the bus address + redacted to drop the GUID suffix. + """ + redacted_dbus = dbus_address.split(",", 1)[0] if dbus_address else "" + + try: + bus = dbus.bus.BusConnection(dbus_address) + screenshot_obj = bus.get_object("org.kde.KWin", "/org/kde/KWin/ScreenShot2") + iface = dbus.Interface(screenshot_obj, "org.kde.KWin.ScreenShot2") + options = {"include-cursor": dbus.Boolean(False)} + + read_fd, write_fd = os.pipe() + try: + try: + iface.CaptureArea(0, 0, 1, 1, options, dbus.types.UnixFd(write_fd)) + finally: + os.close(write_fd) + chunks: list[bytes] = [] + while True: + chunk = os.read(read_fd, 65536) + if not chunk: + break + chunks.append(chunk) + finally: + os.close(read_fd) + + if b"".join(chunks): + logger.info( + "screenshot capability: backend=%s dbus=%s wayland=%s", + "screenshot2_dbus", + redacted_dbus, + wayland_socket, + ) + return "screenshot2_dbus" + except Exception: + pass + + # Phase 2: spectacle CLI fallback. + fd, probe_path_str = tempfile.mkstemp(prefix="screenshot-probe-", suffix=".png") + os.close(fd) + probe_path = Path(probe_path_str) + try: + try: + _capture_via_spectacle( + dbus_address, + wayland_socket, + output_path=probe_path, + include_cursor=False, + ) + if probe_path.exists() and probe_path.stat().st_size > 0: + logger.info( + "screenshot capability: backend=%s dbus=%s wayland=%s", + "spectacle_cli", + redacted_dbus, + wayland_socket, + ) + return "spectacle_cli" + except (subprocess.TimeoutExpired, RuntimeError, OSError): + pass + finally: + with contextlib.suppress(OSError): + probe_path.unlink(missing_ok=True) + + logger.info( + "screenshot capability: backend=%s dbus=%s wayland=%s", + "unavailable", + redacted_dbus, + wayland_socket, + ) + return "unavailable" diff --git a/src/kwin_mcp/session.py b/src/kwin_mcp/session.py index 8d0582b..c0af3ea 100644 --- a/src/kwin_mcp/session.py +++ b/src/kwin_mcp/session.py @@ -8,6 +8,7 @@ from __future__ import annotations import contextlib +import logging import os import shutil import signal @@ -18,6 +19,10 @@ from enum import Enum from pathlib import Path +from kwin_mcp.screenshot import _probe_screenshot_capability + +logger = logging.getLogger("kwin_mcp.session") + class SessionType(Enum): """Type of KWin session.""" @@ -63,6 +68,17 @@ class SessionInfo: wrapper_pid: int | None = None apps: dict[int, AppInfo] = field(default_factory=dict) session_type: SessionType = SessionType.VIRTUAL + screenshot_backend: str = "unavailable" + + +def _probe_and_store_screenshot_backend(info: SessionInfo) -> None: + try: + info.screenshot_backend = _probe_screenshot_capability( + info.dbus_address, + info.wayland_socket, + ) + except Exception as exc: + logger.warning("screenshot probe failed, defaulting to unavailable: %s", exc) class Session: @@ -196,6 +212,7 @@ def start(self, config: SessionConfig | None = None) -> SessionInfo: screenshot_dir=screenshot_dir, home_dir=self._home_dir, ) + _probe_and_store_screenshot_backend(self._info) return self._info def launch_app(self, command: list[str], extra_env: dict[str, str] | None = None) -> AppInfo: @@ -447,6 +464,7 @@ def __init__( screenshot_dir=screenshot_dir, session_type=SessionType.LIVE, ) + _probe_and_store_screenshot_backend(self._info) self._running = True self._app_counter: int = 0 self._keep_screenshots: bool = False diff --git a/tests/integration/test_screenshot_backends.py b/tests/integration/test_screenshot_backends.py new file mode 100644 index 0000000..1fabd5d --- /dev/null +++ b/tests/integration/test_screenshot_backends.py @@ -0,0 +1,139 @@ +"""End-to-end tests for the screenshot backend probe + dispatch + SessionInfo wiring. + +Covers Tasks 16-18 of the kwin-mcp-backend-overhaul plan: + +* Task 16 -- ``_probe_screenshot_capability`` returns one of the three valid + decision strings. +* Task 17 -- ``capture_screenshot_to_file`` and ``capture_frame_burst`` + dispatch on ``SessionInfo.screenshot_backend`` and raise a clear + ``RuntimeError`` when the value is ``"unavailable"``. +* Task 18 -- ``Session.start`` populates ``SessionInfo.screenshot_backend`` + so both single-shot and burst paths share one decision. +""" + +from __future__ import annotations + +from pathlib import Path +from typing import TYPE_CHECKING + +import pytest + +if TYPE_CHECKING: + from kwin_mcp.core import AutomationEngine + +pytestmark = pytest.mark.kwin + +_PNG_SIGNATURE = b"\x89PNG\r\n\x1a\n" +_VALID_BACKENDS = {"screenshot2_dbus", "spectacle_cli", "unavailable"} + + +def _extract_screenshot_path(result: str) -> Path: + """Extract the saved PNG path from ``engine.screenshot``'s return string. + + ``AutomationEngine.screenshot`` returns ``"Screenshot saved: ( KB)"``. + We split on the literal prefix and trailing size annotation rather than + using a regex to keep the parser obvious. + """ + prefix = "Screenshot saved: " + assert result.startswith(prefix), f"unexpected screenshot result: {result!r}" + remainder = result[len(prefix) :] + # Trim the trailing " ( KB)" suffix; the path itself never contains " (". + path_str, _, _ = remainder.rpartition(" (") + assert path_str, f"could not parse path from screenshot result: {result!r}" + return Path(path_str) + + +def _extract_burst_paths(result: str) -> list[Path]: + """Pull every burst frame path out of an action result string. + + ``_with_frame_capture`` formats burst lines as + ``" ms: ( KB)"``. We split each line on ``"ms: "`` + and ``" ("`` so a path containing spaces still survives intact. + """ + paths: list[Path] = [] + for raw_line in result.splitlines(): + line = raw_line.strip() + if "ms: " not in line: + continue + _, _, after = line.partition("ms: ") + path_str, sep, _ = after.rpartition(" (") + if not sep: + continue + paths.append(Path(path_str)) + return paths + + +def _assert_is_png(path: Path) -> None: + assert path.exists(), f"expected PNG at {path} but it does not exist" + size = path.stat().st_size + assert size > 0, f"PNG at {path} is empty (0 bytes)" + with path.open("rb") as fh: + header = fh.read(8) + assert header == _PNG_SIGNATURE, f"file at {path} is not a PNG (size={size}, header={header!r})" + + +def test_probe_returns_valid_decision(engine: AutomationEngine) -> None: + """``Session.start`` must populate ``screenshot_backend`` with a known value.""" + session = engine._session + assert session is not None, "engine fixture should yield a running session" + info = session.info + assert info is not None, "running session must expose SessionInfo" + assert info.screenshot_backend in _VALID_BACKENDS, ( + f"screenshot_backend={info.screenshot_backend!r} not in {_VALID_BACKENDS}" + ) + + +def test_screenshot_to_file_in_virtual_session_produces_png(engine: AutomationEngine) -> None: + """The single-shot ``screenshot`` path must produce a real PNG via the chosen backend.""" + session = engine._session + assert session is not None + info = session.info + assert info is not None + if info.screenshot_backend == "unavailable": + pytest.skip("no screenshot backend available in this environment") + + result = engine.screenshot() + path = _extract_screenshot_path(result) + _assert_is_png(path) + + +def test_capture_frame_burst_uses_same_backend(engine: AutomationEngine) -> None: + """A burst-capable action must reuse the same backend as the single-shot path.""" + session = engine._session + assert session is not None + info = session.info + assert info is not None + if info.screenshot_backend == "unavailable": + pytest.skip("no screenshot backend available in this environment") + + backend_before = info.screenshot_backend + + # ``mouse_move`` is the cheapest action that plumbs ``screenshot_after_ms`` + # through ``_with_frame_capture`` -> ``capture_frame_burst``. + result = engine.mouse_move(x=100, y=100, screenshot_after_ms=[0, 50]) + + backend_after = info.screenshot_backend + assert backend_after == backend_before, ( + f"backend changed mid-call: {backend_before!r} -> {backend_after!r}" + ) + + paths = _extract_burst_paths(result) + assert len(paths) == 2, f"expected 2 burst frames, got {len(paths)} in:\n{result}" + for path in paths: + _assert_is_png(path) + + +def test_unavailable_raises(engine: AutomationEngine) -> None: + """Forcing ``screenshot_backend='unavailable'`` must surface a clear ``RuntimeError``.""" + session = engine._session + assert session is not None + info = session.info + assert info is not None + + original = info.screenshot_backend + info.screenshot_backend = "unavailable" + try: + with pytest.raises(RuntimeError, match="No screenshot backend available"): + engine.screenshot() + finally: + info.screenshot_backend = original