diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 022ce8b..6679c28 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -22,7 +22,7 @@ jobs: strategy: fail-fast: false matrix: - python-version: ["3.10", "3.11", "3.12", "3.13"] + python-version: ["3.10", "3.11", "3.12", "3.13", "3.14"] steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 @@ -45,6 +45,27 @@ jobs: slug: commit-check/commit-check-mcp files: ./coverage.xml + lint: + runs-on: ubuntu-24.04 + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + - uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 + with: + python-version: "3.12" + + - name: Install dependencies + run: | + python -m pip install --upgrade pip + python -m pip install -e .[dev] + + - name: Lint (ruff) + run: python -m ruff check src tests + + - name: Type check (mypy) + run: python -m mypy src + build: runs-on: ubuntu-24.04 steps: diff --git a/README.md b/README.md index f053535..4391337 100644 --- a/README.md +++ b/README.md @@ -100,109 +100,10 @@ uvx commit-check-mcp > **Tip**: If `uv` is not installed, get it via `curl -LsSf https://astral.sh/uv/install.sh | sh`. ---- - -### Claude Desktop - -```json -{ - "mcpServers": { - "commit-check": { - "command": "uvx", - "args": ["commit-check-mcp"] - } - } -} -``` - -### Claude Code CLI - -```json -{ - "mcpServers": { - "commit-check": { - "command": "uvx", - "args": ["commit-check-mcp"] - } - } -} -``` - -Add to your `~/.claude/settings.json` or project-level `.claude/settings.local.json`. - -### Cursor - -In Cursor, go to **Settings → Cursor Settings → MCP → Add new MCP server** and paste: - -| Field | Value | -|---|---| -| **Name** | `commit-check` | -| **Type** | `command` | -| **Command** | `uvx commit-check-mcp` | - -Or add to your project's `.cursor/mcp.json`: - -```json -{ - "mcpServers": { - "commit-check": { - "command": "uvx", - "args": ["commit-check-mcp"] - } - } -} -``` - -### Windsurf - -Add to your `~/.codeium/windsurf/mcp_config.json`: - -```json -{ - "mcpServers": { - "commit-check": { - "command": "uvx", - "args": ["commit-check-mcp"] - } - } -} -``` - -### Cline (VS Code) - -Add a new MCP server in the Cline extension settings: - -```json -{ - "mcpServers": { - "commit-check": { - "command": "uvx", - "args": ["commit-check-mcp"] - } - } -} -``` - -### Continue.dev (VS Code / JetBrains) - -Add to your `~/.continue/config.json`: - -```json -{ - "experimental": { - "mcpServers": { - "commit-check": { - "command": "uvx", - "args": ["commit-check-mcp"] - } - } - } -} -``` - -### Roo Code +### Configure your client -Add to your Roo Code MCP settings: +Every client below launches the same command; only the config file and, for a +few clients, the wrapper key differ. This is the object to register: ```json { @@ -215,40 +116,29 @@ Add to your Roo Code MCP settings: } ``` -### Zed - -Add to your `~/.config/zed/settings.json`: - -```json -{ - "mcp_servers": { - "commit-check": { - "command": "uvx", - "args": ["commit-check-mcp"] - } - } -} -``` - -### Generic / Any MCP Client - -If your client does not support `uvx`, use `pip` and the direct path: - -```bash -pip install commit-check-mcp -which commit-check-mcp -``` - -Then use the absolute path in your config: - -```json -{ - "mcpServers": { - "commit-check": { - "command": "/path/to/commit-check-mcp" - } - } -} +| Client | Where it goes | Notes | +|---|---|---| +| Claude Code | `claude mcp add commit-check -- uvx commit-check-mcp` | Add `--scope project` to write a shareable `.mcp.json` at the repo root (`--scope user` makes it available in all your projects). You can also commit a `.mcp.json` containing the block above; `"type": "stdio"` may be added inside the server object. MCP servers are **not** configured in `~/.claude/settings.json`. | +| Claude Desktop | macOS `~/Library/Application Support/Claude/claude_desktop_config.json`; Windows `%APPDATA%\Claude\claude_desktop_config.json` | Block above as-is; restart Claude Desktop. | +| Cursor | project `.cursor/mcp.json` or global `~/.cursor/mcp.json` | Block above as-is (or **Settings → Cursor Settings → MCP → Add new MCP server** with command `uvx commit-check-mcp`). | +| VS Code (Copilot agent mode) | `.vscode/mcp.json` | **Different key**: `{"servers": {"commit-check": {"type": "stdio", "command": "uvx", "args": ["commit-check-mcp"]}}}` | +| Cline | MCP Servers panel → Configure → `cline_mcp_settings.json` (check your client's docs) | Block above as-is. | +| Roo Code | project `.roo/mcp.json` or global `mcp_settings.json` (**Edit Global MCP**) | Block above as-is; optional `"alwaysAllow": [...]`. | +| Windsurf | `~/.codeium/windsurf/mcp_config.json` (check your client's docs) | Block above as-is. | +| Continue | `config.yaml` (or a file in `.continue/mcpServers/`) | **YAML list** under `mcpServers:`, see below. Continue also picks up the JSON block above when dropped into `.continue/mcpServers/`. | +| Zed | `~/.config/zed/settings.json` | **Different key**: `{"context_servers": {"commit-check": {"command": "uvx", "args": ["commit-check-mcp"]}}}` | +| Anything else | your client's MCP config | If the client cannot run `uvx`: `pip install commit-check-mcp`, then set `"command"` to the absolute path of the installed binary and drop `args`. Find it with `which commit-check-mcp` (macOS/Linux), `where commit-check-mcp` (Windows cmd) or `Get-Command commit-check-mcp \| Select-Object -ExpandProperty Source` (PowerShell). | + +Continue's `config.yaml` entry in full (`name`, `version` and `schema` are required by Continue; drop them if you are adding only the `mcpServers` fragment to an existing file, or save this as a standalone file in `.continue/mcpServers/`): + +```yaml +name: commit-check +version: 0.0.1 +schema: v1 +mcpServers: + - name: commit-check + command: uvx + args: ["commit-check-mcp"] ``` ## Run Manually diff --git a/pyproject.toml b/pyproject.toml index 8dc4e06..2bfbefa 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -37,7 +37,9 @@ tracker = "https://github.com/commit-check/commit-check-mcp/issues" [project.optional-dependencies] dev = [ "pytest>=9.0.0,<10", - "pytest-cov>=6.0.0,<8" + "pytest-cov>=6.0.0,<8", + "ruff>=0.16,<1", + "mypy>=1.14,<3", ] [project.scripts] @@ -56,3 +58,16 @@ fallback_version = "0.0.0" [tool.pytest.ini_options] testpaths = ["tests"] + +[tool.ruff] +line-length = 100 +target-version = "py310" + +[tool.ruff.lint] +select = ["E", "F", "W", "I", "UP", "B"] + +[tool.mypy] +python_version = "3.10" +warn_unused_ignores = true +packages = ["commit_check_mcp"] +mypy_path = "src" diff --git a/src/commit_check_mcp/server.py b/src/commit_check_mcp/server.py index 84ce307..a4248d0 100644 --- a/src/commit_check_mcp/server.py +++ b/src/commit_check_mcp/server.py @@ -2,13 +2,14 @@ from __future__ import annotations +import inspect +import os +import subprocess +import threading from collections.abc import Callable from contextlib import contextmanager from importlib.metadata import version from pathlib import Path -import inspect -import os -import subprocess from typing import Annotated, Any, TypeVar from commit_check import __version__ as commit_check_version @@ -76,10 +77,10 @@ def _tool(title: str, *, fetches: bool = False) -> Callable[[_F], _F]: description, with ``{result_shape}`` replaced by :data:`RESULT_SHAPE`. """ annotations = ToolAnnotations( - readOnlyHint=not fetches, - destructiveHint=False, - idempotentHint=True, - openWorldHint=fetches, + read_only_hint=not fetches, + destructive_hint=False, + idempotent_hint=True, + open_world_hint=fetches, ) def register(fn: _F) -> _F: @@ -204,7 +205,10 @@ def _normalize_repo_path(repo_path: str | None) -> Path | None: if not normalized: raise ToolError("repo_path cannot be empty when provided") - path = Path(normalized).expanduser().resolve() + path = Path(normalized).expanduser() + if not path.is_absolute(): + path = _at_rest_cwd() / path + path = path.resolve() if not path.exists(): raise ToolError(f"repo_path does not exist: {path}") if not path.is_dir(): @@ -219,7 +223,7 @@ def _require_git_repo(repo_path: Path | None) -> None: ``git``; outside a repository those reads come back empty and every rule passes vacuously, so tools that will consult git call this first. """ - directory = repo_path if repo_path is not None else Path.cwd() + directory = repo_path if repo_path is not None else _at_rest_cwd() try: result = subprocess.run( ["git", "rev-parse", "--show-toplevel"], @@ -245,8 +249,8 @@ def _normalize_config_path(config_path: str | None, repo_path: Path | None) -> s raise ToolError("config_path cannot be empty when provided") path = Path(normalized).expanduser() - if not path.is_absolute() and repo_path is not None: - path = repo_path / path + if not path.is_absolute(): + path = (repo_path if repo_path is not None else _at_rest_cwd()) / path resolved = path.resolve() if not resolved.exists(): @@ -256,19 +260,41 @@ def _normalize_config_path(config_path: str | None, repo_path: Path | None) -> s return str(resolved) +# os.chdir is process-global and the SDK runs sync tools on worker threads +# concurrently, so every chdir window is serialised on one lock. Long-term fix: +# pass cwd to git and to the config loader instead of changing directory. +_CWD_LOCK = threading.Lock() + + +def _at_rest_cwd() -> Path: + """The process cwd with no chdir window in flight. + + A relative ``repo_path`` or ``config_path``, and the git check for a call + without ``repo_path``, must resolve against the directory the server was + started in, not against whatever another worker thread has temporarily + switched to. Taking the lock guarantees no window is open. Call it before + entering :func:`_working_directory`, never inside (the lock is not + re-entrant). + """ + with _CWD_LOCK: + return Path.cwd() + + @contextmanager def _working_directory(repo_path: Path | None): """Temporarily switch working directory for repo-relative config and git checks.""" - if repo_path is None: - yield - return - - original_cwd = Path.cwd() - os.chdir(repo_path) - try: - yield - finally: - os.chdir(original_cwd) + # The lock is held even when repo_path is None: a tool that reads the + # process cwd must not observe another thread's temporary chdir. + with _CWD_LOCK: + if repo_path is None: + yield + return + original_cwd = Path.cwd() + os.chdir(repo_path) + try: + yield + finally: + os.chdir(original_cwd) def _merge_config( diff --git a/tests/test_cwd_at_rest.py b/tests/test_cwd_at_rest.py new file mode 100644 index 0000000..f751a37 --- /dev/null +++ b/tests/test_cwd_at_rest.py @@ -0,0 +1,167 @@ +"""cwd-dependent reads that happen before a tool enters ``_working_directory``. + +Another thread is parked inside ``_working_directory(other)``, so the process +cwd is ``other`` and the lock is held. A call that resolves a relative path or +checks the git repository for ``repo_path=None`` must still see the directory +the server was started in, otherwise it validates against a repository it was +never asked about. +""" + +from __future__ import annotations + +import os +import subprocess +import threading +from pathlib import Path + +import pytest +from mcp.server.mcpserver.exceptions import ToolError + +from commit_check_mcp import server + + +def _git_repo(root: Path, name: str) -> Path: + repo = root / name + repo.mkdir() + env = { + **os.environ, + "GIT_AUTHOR_NAME": "T", + "GIT_AUTHOR_EMAIL": "t@example.com", + "GIT_COMMITTER_NAME": "T", + "GIT_COMMITTER_EMAIL": "t@example.com", + } + + def git(*args: str) -> None: + subprocess.run(["git", *args], cwd=repo, check=True, capture_output=True, env=env) + + git("init", "-q", "-b", "main") + (repo / "f").write_text("x") + git("add", "f") + git("commit", "-q", "-m", "feat: init") + return repo + + +class _Parked: + """Hold ``_working_directory(repo)`` open on another thread until released.""" + + def __init__(self, repo: Path) -> None: + self.repo = repo + self.inside = threading.Event() + self.release = threading.Event() + self.thread = threading.Thread(target=self._run, daemon=True) + + def _run(self) -> None: + with server._working_directory(self.repo): + self.inside.set() + self.release.wait(30) + + def __enter__(self) -> _Parked: + self.thread.start() + assert self.inside.wait(5) + return self + + def __exit__(self, *exc: object) -> None: + self.release.set() + self.thread.join(5) + + +def test_relative_config_path_without_repo_path_uses_the_at_rest_cwd( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + home = tmp_path / "home" + home.mkdir() + (home / "cchk.toml").write_text("[commit]\nsubject_max_length = 10\n") + other = tmp_path / "other" + other.mkdir() + (other / "cchk.toml").write_text("[commit]\nsubject_max_length = 99\n") + monkeypatch.chdir(home) + parked = _Parked(other) + parked.__enter__() + try: + resolver = threading.Thread( + target=lambda: results.append(server._normalize_config_path("cchk.toml", None)), + daemon=True, + ) + results: list[str | None] = [] + resolver.start() + resolver.join(0.5) + assert resolver.is_alive(), "should be waiting for the parked chdir window to close" + finally: + parked.__exit__() + resolver.join(5) + assert results == [str(home / "cchk.toml")] + + +def test_relative_repo_path_uses_the_at_rest_cwd( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + home = tmp_path / "home" + (home / "sub").mkdir(parents=True) + other = tmp_path / "other" + (other / "sub").mkdir(parents=True) + monkeypatch.chdir(home) + results: list[Path | None] = [] + with _Parked(other): + t = threading.Thread( + target=lambda: results.append(server._normalize_repo_path("sub")), daemon=True + ) + t.start() + t.join(0.5) + assert t.is_alive(), "should be waiting for the parked chdir window to close" + t.join(5) + assert results == [home / "sub"] + + +def test_require_git_repo_without_repo_path_checks_the_at_rest_cwd( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + plain = tmp_path / "plain" + plain.mkdir() + other = _git_repo(tmp_path, "other") + monkeypatch.chdir(plain) + results: list[BaseException | None] = [] + + def check() -> None: + try: + server._require_git_repo(None) + except ToolError as e: + results.append(e) + else: + results.append(None) + + with _Parked(other): + t = threading.Thread(target=check, daemon=True) + t.start() + t.join(0.5) + assert t.is_alive(), "should be waiting for the parked chdir window to close" + t.join(10) + assert isinstance(results[0], ToolError) + + +def test_tool_with_relative_config_path_loads_the_right_file( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """End to end: config_path='cchk.toml' with no repo_path, while another + call is inside its chdir window, loads the file next to the server.""" + home = tmp_path / "home" + home.mkdir() + (home / "cchk.toml").write_text("[commit]\nsubject_max_length = 100\n") + other = tmp_path / "other" + other.mkdir() + (other / "cchk.toml").write_text("[commit]\nsubject_max_length = 5\n") + monkeypatch.chdir(home) + results: list[dict] = [] + + def call() -> None: + results.append( + server.validate_commit_message("feat: a long enough subject", config_path="cchk.toml") + ) + + with _Parked(other): + t = threading.Thread(target=call, daemon=True) + t.start() + t.join(0.5) + assert t.is_alive(), "should be waiting for the parked chdir window to close" + t.join(10) + sub = next(c for c in results[0]["checks"] if c["check"] == "subject_max_length") + assert sub["status"] == "pass", sub diff --git a/tests/test_server.py b/tests/test_server.py index 83e2b82..0cf5422 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -2,16 +2,15 @@ from __future__ import annotations -from pathlib import Path import asyncio import os import subprocess +from pathlib import Path import pytest - -from commit_check_mcp import server from mcp.server.mcpserver.exceptions import ToolError +from commit_check_mcp import server # --------------------------------------------------------------------------- # _normalize_config @@ -125,7 +124,13 @@ def test_valid_message_passes(self, tmp_path: Path) -> None: repo = tmp_path / "repo" repo.mkdir() (repo / "cchk.toml").write_text( - "[commit]\nallow_commit_types = []\nallow_merge_commits = true\nallow_revert_commits = true\nallow_empty_commits = true\nallow_fixup_commits = true\nallow_wip_commits = true" + "[commit]\n" + "allow_commit_types = []\n" + "allow_merge_commits = true\n" + "allow_revert_commits = true\n" + "allow_empty_commits = true\n" + "allow_fixup_commits = true\n" + "allow_wip_commits = true" ) result = server._validate_message( "feat: add new feature", @@ -345,7 +350,13 @@ def fake_run_checks(check_names, context, config): return { "status": "fail", "checks": [ - {"check": cn, "status": "fail", "value": "", "error": "bad", "suggest": "fix it"} + { + "check": cn, + "status": "fail", + "value": "", + "error": "bad", + "suggest": "fix it", + } for cn in check_names ], } @@ -526,7 +537,9 @@ def test_empty_raises(self) -> None: with pytest.raises(ToolError, match="non-empty"): server.validate_commit_message(" ") - def test_valid_message_with_repo_path(self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + def test_valid_message_with_repo_path( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: repo = tmp_path / "repo" repo.mkdir() @@ -690,7 +703,9 @@ def test_no_fields_raises(self) -> None: def test_all_fields_forwards(self, monkeypatch: pytest.MonkeyPatch) -> None: captured: dict[str, object] = {} - def fake_validate_all(message, branch, author_name, author_email, *, config, repo_path, config_path): + def fake_validate_all( + message, branch, author_name, author_email, *, config, repo_path, config_path + ): captured["message"] = message captured["branch"] = branch captured["author_name"] = author_name @@ -716,7 +731,9 @@ def fake_validate_all(message, branch, author_name, author_email, *, config, rep def test_message_only(self, monkeypatch: pytest.MonkeyPatch) -> None: captured: dict[str, object] = {} - def fake_validate_all(message, branch, author_name, author_email, *, config, repo_path, config_path): + def fake_validate_all( + message, branch, author_name, author_email, *, config, repo_path, config_path + ): captured["message"] = message captured["branch"] = branch captured["author_name"] = author_name @@ -925,7 +942,9 @@ def test_a_fully_skipped_run_is_reported_as_skip_not_pass( lambda self, context: [_outcome("skip"), _outcome("skip", "author_name")], ) result = server._run_checks( - ["message", "author_name"], ValidationContext(stdin_text="x"), server._merge_config(None) + ["message", "author_name"], + ValidationContext(stdin_text="x"), + server._merge_config(None), ) assert result["status"] == "skip" assert result["warnings"] == 0 @@ -975,7 +994,14 @@ def test_the_combined_tools_reduce_with_the_same_rule( def skipped(check_names, context, config): return server._summarize( [ - {"check": cn, "status": "skip", "value": "", "error": "", "suggest": "", "fix": ""} + { + "check": cn, + "status": "skip", + "value": "", + "error": "", + "suggest": "", + "fix": "", + } for cn in check_names ] ) @@ -1368,3 +1394,46 @@ def test_repository_state_include_push_is_unaffected(self, tmp_path: Path) -> No ) assert result["status"] == "pass" assert [c["check"] for c in result["checks"]] == ["no_force_push"] + + +# --------------------------------------------------------------------------- +# Concurrent tool calls with different repo_path values must not see each +# other's working directory (os.chdir is process-global) +# --------------------------------------------------------------------------- + +def _repo_on_branch(root: Path, name: str, branch: str) -> Path: + repo = root / name + repo.mkdir() + _git(repo, "init", "-q", "-b", branch) + (repo / "file.txt").write_text("content\n") + _git(repo, "add", "file.txt") + _git(repo, "commit", "-q", "-m", "feat: init") + return repo + + +class TestConcurrentWorkingDirectory: + def test_parallel_calls_read_their_own_repo(self, tmp_path: Path) -> None: + # The SDK runs sync tools on worker threads, so two in-flight calls + # each chdir the one process. Without the lock about half of the + # results below report the other repository's branch as `pass`. + repos = { + _repo_on_branch(tmp_path, "a", "feature/alpha"): "feature/alpha", + _repo_on_branch(tmp_path, "b", "bugfix/beta"): "bugfix/beta", + } + start_cwd = Path.cwd() + + async def one_round() -> list[object]: + return await asyncio.gather( + *( + server.mcp.call_tool("validate_branch_name", {"repo_path": str(repo)}) + for repo in repos + ) + ) + + for _ in range(30): + results = asyncio.run(one_round()) + assert Path.cwd() == start_cwd + for repo, result in zip(repos, results, strict=True): + checks = result.structured_content["checks"] + branch = next(c["value"] for c in checks if c["check"] == "branch") + assert branch == repos[repo], f"{repo} reported branch {branch!r}"