From 2db64498c06376600d774baeaa4c0f29935d3710 Mon Sep 17 00:00:00 2001 From: Xianpeng Shen Date: Sun, 6 Sep 2026 19:26:57 +0000 Subject: [PATCH 1/2] fix: surface errors as tool errors and stop vacuous passes Input, path, and config problems were raised as plain ValueError (or escaped from commit-check as TOMLDecodeError/ValueError), which the mcp 2.x tool manager reports to the client only as the generic "Error executing tool ". Every such failure now raises ToolError with the same message, and config merging / RuleBuilder construction report a malformed or rejected config as "invalid commit-check config: ", so an agent can read what to correct. validate_repository_state validated an empty string instead of HEAD's message, so the message rules always passed. The message helper now accepts None and repository-state passes None, which makes commit-check read the latest commit (git log -1) as the CLI does; the author path already worked that way. A repo_path that is a plain directory silently produced a pass because commit-check's git reads come back empty. Tools that consult git (branch, author, or push_refs omitted; validate_repository_state) now require `git rev-parse --show-toplevel` to succeed in that directory and raise "repo_path is not a git repository: " otherwise. Tools that validate a supplied value still work with a non-git directory that only holds a config file. A blank push_refs (e.g. " ") stripped to "" and passed with no ref and no upstream fallback. It is now rejected with "push_refs cannot be empty when provided", consistent with how the other tools treat blank strings. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_018xYa7m3qup5wyN5MaXFgf6 --- README.md | 12 +- src/commit_check_mcp/server.py | 112 +++++++++++++----- tests/test_server.py | 206 +++++++++++++++++++++++++++++---- 3 files changed, 276 insertions(+), 54 deletions(-) diff --git a/README.md b/README.md index 1a00a2a..9e7ca8f 100644 --- a/README.md +++ b/README.md @@ -50,6 +50,12 @@ a non-empty `fix` as it stands and fall back to `suggest` when it is empty. Populating `fix` needs a commit-check release that carries the field; with an older commit-check the key is present and always empty. +A call that cannot run at all — an empty `message`, a `repo_path` that does not +exist or is not a git repository, a malformed or rejected commit-check config — +is returned as an MCP tool error (`is_error`) whose text names the problem, for +example `repo_path is not a git repository: /path/to/dir` or +`invalid commit-check config: ...`, rather than as a `pass`/`fail` result. + ## Installation ```bash @@ -255,7 +261,7 @@ After the client starts the server, it will expose these tools: The common optional arguments are: -- `repo_path`: repository directory to validate against +- `repo_path`: repository directory to validate against; it must be a git repository when the tool reads git state (branch, author, or push refs omitted, or `validate_repository_state`), and may be a plain directory holding a config file when every value is supplied - `config_path`: explicit TOML config file; relative paths resolve from `repo_path` - `config`: ad-hoc config overrides merged on top of defaults and repo config @@ -290,7 +296,7 @@ Validate the full repository state: } ``` -Validate push safety from git pre-push hook ref metadata: +Validate push safety from git pre-push hook ref metadata (`push_refs` must be non-empty when given; omit it to check the current branch against its upstream): ```json { @@ -323,7 +329,7 @@ Inspect the final merged rules that will be applied: Typical patterns: - Validate an explicit message with a repository's rules -- Validate the current repository state without passing message/branch/author values manually +- Validate the current repository state — the latest commit's message and author, and the current branch — without passing message/branch/author values manually - Validate push safety using pre-push ref metadata, or check the current branch against its upstream - Inspect which rules are actually enabled after config merging diff --git a/src/commit_check_mcp/server.py b/src/commit_check_mcp/server.py index b4391e0..4eeb181 100644 --- a/src/commit_check_mcp/server.py +++ b/src/commit_check_mcp/server.py @@ -6,6 +6,7 @@ from importlib.metadata import version from pathlib import Path import os +import subprocess from typing import Any from commit_check import __version__ as commit_check_version @@ -14,6 +15,7 @@ from commit_check.rule_builder import RuleBuilder, ValidationRule from commit_check.rules_catalog import BRANCH_RULES, COMMIT_RULES, PUSH_RULES from mcp.server.mcpserver import MCPServer +from mcp.server.mcpserver.exceptions import ToolError from . import __version__ @@ -31,7 +33,7 @@ def _normalize_config(config: dict[str, Any] | None) -> dict[str, Any] | None: if config is None: return None if not isinstance(config, dict): - raise ValueError("config must be an object/dictionary when provided") + raise ToolError("config must be an object/dictionary when provided") return config @@ -40,28 +42,50 @@ def _normalize_repo_path(repo_path: str | None) -> Path | None: if repo_path is None: return None if not isinstance(repo_path, str): - raise ValueError("repo_path must be a string when provided") + raise ToolError("repo_path must be a string when provided") normalized = repo_path.strip() if not normalized: - raise ValueError("repo_path cannot be empty when provided") + raise ToolError("repo_path cannot be empty when provided") path = Path(normalized).expanduser().resolve() if not path.exists(): - raise ValueError(f"repo_path does not exist: {path}") + raise ToolError(f"repo_path does not exist: {path}") if not path.is_dir(): - raise ValueError(f"repo_path must be a directory: {path}") + raise ToolError(f"repo_path must be a directory: {path}") return path +def _require_git_repo(repo_path: Path | None) -> None: + """Fail when the directory a git-backed check would run in is not a git work tree. + + commit-check reads the branch, author, HEAD message, and upstream through + ``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() + try: + result = subprocess.run( + ["git", "rev-parse", "--show-toplevel"], + cwd=directory, + capture_output=True, + text=True, + check=False, + ) + except OSError as e: + raise ToolError(f"git is not available to inspect repo_path {directory}: {e}") from e + if result.returncode != 0: + raise ToolError(f"repo_path is not a git repository: {directory}") + + def _normalize_config_path(config_path: str | None, repo_path: Path | None) -> str | None: """Normalize and validate an optional config path.""" if config_path is None: return None if not isinstance(config_path, str): - raise ValueError("config_path must be a string when provided") + raise ToolError("config_path must be a string when provided") normalized = config_path.strip() if not normalized: - raise ValueError("config_path cannot be empty when provided") + raise ToolError("config_path cannot be empty when provided") path = Path(normalized).expanduser() if not path.is_absolute() and repo_path is not None: @@ -69,9 +93,9 @@ def _normalize_config_path(config_path: str | None, repo_path: Path | None) -> s resolved = path.resolve() if not resolved.exists(): - raise ValueError(f"config_path does not exist: {resolved}") + raise ToolError(f"config_path does not exist: {resolved}") if not resolved.is_file(): - raise ValueError(f"config_path must be a file: {resolved}") + raise ToolError(f"config_path must be a file: {resolved}") return str(resolved) @@ -98,15 +122,28 @@ def _merge_config( ) -> dict[str, Any]: """Merge repository config and user config on top of commit-check defaults.""" merged = get_default_config() - with _working_directory(repo_path): - loaded_config = load_toml_config(config_path or "") - if loaded_config: - deep_merge(merged, loaded_config) - if config: - deep_merge(merged, config) + try: + with _working_directory(repo_path): + loaded_config = load_toml_config(config_path or "") + if loaded_config: + deep_merge(merged, loaded_config) + if config: + deep_merge(merged, config) + except ValueError as e: + # tomllib/tomli TOMLDecodeError subclasses ValueError, so this covers a + # malformed config file as well as a value commit-check rejects. + raise ToolError(f"invalid commit-check config: {e}") from e return merged +def _build_rules(config: dict[str, Any]) -> list[ValidationRule]: + """Build commit-check rules, reporting a rejected config as a tool error.""" + try: + return RuleBuilder(config).build_all_rules() + except ValueError as e: + raise ToolError(f"invalid commit-check config: {e}") from e + + def _run_checks( check_names: list[str], context: ValidationContext, @@ -117,7 +154,7 @@ def _run_checks( Uses ValidationEngine.validate_all_detailed() which internally suppresses terminal output and collects structured failure details. """ - rules = RuleBuilder(config).build_all_rules() + rules = _build_rules(config) filtered: list[ValidationRule] = [r for r in rules if r.check in check_names] engine = ValidationEngine(filtered) @@ -134,13 +171,17 @@ def _run_checks( def _validate_message( - message: str, + message: str | None, *, config: dict[str, Any] | None = None, repo_path: Path | None = None, config_path: str | None = None, ) -> dict[str, Any]: - """Validate message using commit-check engine internals.""" + """Validate message using commit-check engine internals. + + A ``None`` message makes commit-check read the latest commit (``git log -1``) + in the working directory, as the CLI does when no message is supplied. + """ cfg = _merge_config(config, repo_path=repo_path, config_path=config_path) with _working_directory(repo_path): return _run_checks( @@ -344,7 +385,7 @@ def validate_commit_message( - config_path (optional): Path to a custom commit-check TOML config file. """ if not isinstance(message, str) or not message.strip(): - raise ValueError("message must be a non-empty string") + raise ToolError("message must be a non-empty string") normalized_repo_path = _normalize_repo_path(repo_path) return _validate_message( message.strip(), @@ -373,8 +414,10 @@ def validate_branch_name( """ normalized_branch = branch.strip() if isinstance(branch, str) else None if isinstance(branch, str) and not normalized_branch: - raise ValueError("branch cannot be empty when provided") + raise ToolError("branch cannot be empty when provided") normalized_repo_path = _normalize_repo_path(repo_path) + if normalized_branch is None: + _require_git_repo(normalized_repo_path) return _validate_branch( normalized_branch, config=_normalize_config(config), @@ -406,10 +449,12 @@ def validate_author_info( normalized_email = author_email.strip() if isinstance(author_email, str) else None if isinstance(author_name, str) and not normalized_name: - raise ValueError("author_name cannot be empty when provided") + raise ToolError("author_name cannot be empty when provided") if isinstance(author_email, str) and not normalized_email: - raise ValueError("author_email cannot be empty when provided") + raise ToolError("author_email cannot be empty when provided") normalized_repo_path = _normalize_repo_path(repo_path) + if normalized_name is None and normalized_email is None: + _require_git_repo(normalized_repo_path) return _validate_author( normalized_name, @@ -432,13 +477,17 @@ def validate_push_safety( Use this before performing a git push to ensure force-push protection rules are satisfied. Only validates the no_force_push rule. Use validate_commit_context for combined checks. Parameters: - - push_refs (optional): The push ref specification to validate. If omitted, checks upstream fallback state. + - push_refs (optional): The push ref specification to validate. If omitted, checks upstream fallback state. Must be non-empty when provided. - config (optional): Inline JSON config overrides. - repo_path (optional): Path to the git repository. - config_path (optional): Path to a custom commit-check TOML config file. """ normalized_push_refs = push_refs.strip() if isinstance(push_refs, str) else None + if isinstance(push_refs, str) and not normalized_push_refs: + raise ToolError("push_refs cannot be empty when provided") normalized_repo_path = _normalize_repo_path(repo_path) + if normalized_push_refs is None: + _require_git_repo(normalized_repo_path) return _validate_push( normalized_push_refs, config=_normalize_config(config), @@ -476,16 +525,16 @@ def validate_commit_context( normalized_email = author_email.strip() if isinstance(author_email, str) else None if isinstance(message, str) and not normalized_message: - raise ValueError("message cannot be empty when provided") + raise ToolError("message cannot be empty when provided") if isinstance(branch, str) and not normalized_branch: - raise ValueError("branch cannot be empty when provided") + raise ToolError("branch cannot be empty when provided") if isinstance(author_name, str) and not normalized_name: - raise ValueError("author_name cannot be empty when provided") + raise ToolError("author_name cannot be empty when provided") if isinstance(author_email, str) and not normalized_email: - raise ValueError("author_email cannot be empty when provided") + raise ToolError("author_email cannot be empty when provided") if not any([normalized_message, normalized_branch, normalized_name, normalized_email]): - raise ValueError( + raise ToolError( "At least one of message, branch, author_name, or author_email must be provided" ) normalized_repo_path = _normalize_repo_path(repo_path) @@ -525,9 +574,10 @@ def validate_repository_state( - include_push (optional, default false): Whether to validate push safety. """ if not any([include_message, include_branch, include_author, include_push]): - raise ValueError("At least one validation target must be enabled") + raise ToolError("At least one validation target must be enabled") normalized_repo_path = _normalize_repo_path(repo_path) + _require_git_repo(normalized_repo_path) normalized_config = _normalize_config(config) normalized_config_path = _normalize_config_path(config_path, normalized_repo_path) @@ -535,7 +585,7 @@ def validate_repository_state( if include_message: checks.extend( _validate_message( - "", + None, config=normalized_config, repo_path=normalized_repo_path, config_path=normalized_config_path, @@ -599,7 +649,7 @@ def describe_validation_rules( repo_path=normalized_repo_path, config_path=normalized_config_path, ) - rules = [rule.to_dict() for rule in RuleBuilder(merged_config).build_all_rules()] + rules = [rule.to_dict() for rule in _build_rules(merged_config)] return { "commit_check_version": commit_check_version, diff --git a/tests/test_server.py b/tests/test_server.py index ddb2f56..ed53fb1 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -3,11 +3,14 @@ from __future__ import annotations from pathlib import Path +import asyncio import os +import subprocess import pytest from commit_check_mcp import server +from mcp.server.mcpserver.exceptions import ToolError # --------------------------------------------------------------------------- @@ -22,7 +25,7 @@ def test_dict(self) -> None: assert server._normalize_config({"key": "val"}) == {"key": "val"} def test_non_dict_raises(self) -> None: - with pytest.raises(ValueError, match="must be an object/dictionary"): + with pytest.raises(ToolError, match="must be an object/dictionary"): server._normalize_config("string") # type: ignore[arg-type] @@ -39,21 +42,21 @@ def test_valid_path(self, tmp_path: Path) -> None: assert result == tmp_path.resolve() def test_non_string_raises(self) -> None: - with pytest.raises(ValueError, match="repo_path must be a string"): + with pytest.raises(ToolError, match="repo_path must be a string"): server._normalize_repo_path(123) # type: ignore[arg-type] def test_empty_string_raises(self) -> None: - with pytest.raises(ValueError, match="repo_path cannot be empty"): + with pytest.raises(ToolError, match="repo_path cannot be empty"): server._normalize_repo_path(" ") def test_non_existent_raises(self) -> None: - with pytest.raises(ValueError, match="repo_path does not exist"): + with pytest.raises(ToolError, match="repo_path does not exist"): server._normalize_repo_path("/non/existent/path/xyz123") def test_file_path_raises(self, tmp_path: Path) -> None: f = tmp_path / "afile.txt" f.write_text("hello") - with pytest.raises(ValueError, match="repo_path must be a directory"): + with pytest.raises(ToolError, match="repo_path must be a directory"): server._normalize_repo_path(str(f)) @@ -80,19 +83,19 @@ def test_relative_resolved_via_repo(self, tmp_path: Path) -> None: assert result == str(cfg.resolve()) def test_non_string_raises(self) -> None: - with pytest.raises(ValueError, match="config_path must be a string"): + with pytest.raises(ToolError, match="config_path must be a string"): server._normalize_config_path(123, None) # type: ignore[arg-type] def test_empty_string_raises(self) -> None: - with pytest.raises(ValueError, match="config_path cannot be empty"): + with pytest.raises(ToolError, match="config_path cannot be empty"): server._normalize_config_path(" ", None) def test_non_existent_raises(self, tmp_path: Path) -> None: - with pytest.raises(ValueError, match="config_path does not exist"): + with pytest.raises(ToolError, match="config_path does not exist"): server._normalize_config_path(str(tmp_path / "missing.toml"), None) def test_non_file_raises(self, tmp_path: Path) -> None: - with pytest.raises(ValueError, match="config_path must be a file"): + with pytest.raises(ToolError, match="config_path must be a file"): server._normalize_config_path(str(tmp_path), None) @@ -516,11 +519,11 @@ def test_returns_expected_keys(self) -> None: class TestValidateCommitMessage: def test_non_string_raises(self) -> None: - with pytest.raises(ValueError, match="non-empty"): + with pytest.raises(ToolError, match="non-empty"): server.validate_commit_message(123) # type: ignore[arg-type] def test_empty_raises(self) -> None: - with pytest.raises(ValueError, match="non-empty"): + 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: @@ -552,7 +555,7 @@ def fake_validate_message(message, *, config, repo_path, config_path): class TestValidateBranchName: def test_empty_string_raises(self) -> None: - with pytest.raises(ValueError, match="branch cannot be empty"): + with pytest.raises(ToolError, match="branch cannot be empty"): server.validate_branch_name(branch=" ") def test_none_passed_through(self, monkeypatch: pytest.MonkeyPatch) -> None: @@ -588,11 +591,11 @@ def fake_validate_branch(branch, *, config, repo_path, config_path): class TestValidateAuthorInfo: def test_empty_name_raises(self) -> None: - with pytest.raises(ValueError, match="author_name cannot be empty"): + with pytest.raises(ToolError, match="author_name cannot be empty"): server.validate_author_info(author_name=" ") def test_empty_email_raises(self) -> None: - with pytest.raises(ValueError, match="author_email cannot be empty"): + with pytest.raises(ToolError, match="author_email cannot be empty"): server.validate_author_info(author_email=" ") def test_valid_values_stripped(self, monkeypatch: pytest.MonkeyPatch) -> None: @@ -665,23 +668,23 @@ def fake_validate_push(push_refs, *, config, repo_path, config_path): class TestValidateCommitContext: def test_empty_message_raises(self) -> None: - with pytest.raises(ValueError, match="message cannot be empty"): + with pytest.raises(ToolError, match="message cannot be empty"): server.validate_commit_context(message=" ") def test_empty_branch_raises(self) -> None: - with pytest.raises(ValueError, match="branch cannot be empty"): + with pytest.raises(ToolError, match="branch cannot be empty"): server.validate_commit_context(branch=" ") def test_empty_author_name_raises(self) -> None: - with pytest.raises(ValueError, match="author_name cannot be empty"): + with pytest.raises(ToolError, match="author_name cannot be empty"): server.validate_commit_context(author_name=" ") def test_empty_author_email_raises(self) -> None: - with pytest.raises(ValueError, match="author_email cannot be empty"): + with pytest.raises(ToolError, match="author_email cannot be empty"): server.validate_commit_context(author_email=" ") def test_no_fields_raises(self) -> None: - with pytest.raises(ValueError, match="At least one"): + with pytest.raises(ToolError, match="At least one"): server.validate_commit_context() def test_all_fields_forwards(self, monkeypatch: pytest.MonkeyPatch) -> None: @@ -735,7 +738,7 @@ def fake_validate_all(message, branch, author_name, author_email, *, config, rep class TestValidateRepositoryState: def test_all_disabled_raises(self) -> None: - with pytest.raises(ValueError, match="At least one validation target"): + with pytest.raises(ToolError, match="At least one validation target"): server.validate_repository_state( include_message=False, include_branch=False, @@ -950,3 +953,166 @@ def to_dict(self) -> dict[str, str]: ["message"], ValidationContext(stdin_text="Fix: add x"), server._merge_config(None) ) assert result["checks"][0]["fix"] == "" + + +# --------------------------------------------------------------------------- +# Errors reach the agent as tool errors (through the MCP tool manager) +# --------------------------------------------------------------------------- + +def _call_tool(name: str, **arguments: object) -> object: + """Invoke a tool the way an MCP client does, through the server's call path.""" + return asyncio.run(server.mcp.call_tool(name, arguments)) + + +def _git(repo: Path, *args: str) -> None: + env = dict( + os.environ, + GIT_AUTHOR_NAME="Alice Example", + GIT_AUTHOR_EMAIL="alice@example.com", + GIT_COMMITTER_NAME="Alice Example", + GIT_COMMITTER_EMAIL="alice@example.com", + ) + subprocess.run( + ["git", "-c", "commit.gpgsign=false", *args], + cwd=repo, + check=True, + env=env, + capture_output=True, + ) + + +def _repo_with_commit(root: Path, subject: str) -> Path: + repo = root / "repo" + repo.mkdir() + _git(repo, "init", "-q", "-b", "main") + (repo / "file.txt").write_text("content\n") + _git(repo, "add", "file.txt") + _git(repo, "commit", "-q", "-m", subject) + return repo + + +class TestToolErrorsReachTheClient: + def test_nonexistent_repo_path(self) -> None: + with pytest.raises(ToolError, match="repo_path does not exist: /no/such/dir"): + _call_tool("validate_commit_message", message="feat: x", repo_path="/no/such/dir") + + def test_empty_message(self) -> None: + with pytest.raises(ToolError, match="message must be a non-empty string"): + _call_tool("validate_commit_message", message=" ") + + def test_malformed_toml_config(self, tmp_path: Path) -> None: + (tmp_path / "cchk.toml").write_text("[commit\nthis is not toml\n") + with pytest.raises(ToolError, match="invalid commit-check config: "): + _call_tool("validate_commit_message", message="feat: x", repo_path=str(tmp_path)) + + def test_rejected_config_value(self, tmp_path: Path) -> None: + (tmp_path / "cchk.toml").write_text('warn = ["no_such_rule"]\n') + with pytest.raises(ToolError, match="invalid commit-check config: "): + _call_tool("describe_validation_rules", repo_path=str(tmp_path)) + + def test_error_text_is_not_the_generic_crash_message(self) -> None: + with pytest.raises(ToolError) as excinfo: + _call_tool("validate_commit_message", message=" ") + assert str(excinfo.value) != "Error executing tool validate_commit_message" + + +# --------------------------------------------------------------------------- +# validate_repository_state reads HEAD's message +# --------------------------------------------------------------------------- + +class TestRepositoryStateValidatesHead: + def test_non_conventional_head_subject_fails(self, tmp_path: Path) -> None: + repo = _repo_with_commit(tmp_path, "this is not conventional") + result = server.validate_repository_state( + repo_path=str(repo), include_branch=False, include_author=False + ) + message = next(c for c in result["checks"] if c["check"] == "message") + assert message["status"] == "fail" + assert message["value"] == "this is not conventional" + assert result["status"] == "fail" + + def test_conventional_head_subject_passes(self, tmp_path: Path) -> None: + repo = _repo_with_commit(tmp_path, "feat: add a file") + result = server.validate_repository_state( + repo_path=str(repo), include_branch=False, include_author=False + ) + message = next(c for c in result["checks"] if c["check"] == "message") + assert message["status"] == "pass" + assert message["value"] == "feat: add a file" + assert result["status"] == "pass" + + def test_message_helper_passes_none_for_head(self, monkeypatch: pytest.MonkeyPatch) -> None: + captured: dict[str, object] = {} + + def fake_validate_message(message, *, config, repo_path, config_path): + captured["message"] = message + return {"status": "pass", "checks": []} + + monkeypatch.setattr(server, "_validate_message", fake_validate_message) + monkeypatch.setattr(server, "_require_git_repo", lambda _path: None) + + server.validate_repository_state( + include_branch=False, include_author=False, include_push=False + ) + assert captured["message"] is None + + +# --------------------------------------------------------------------------- +# A non-git repo_path is rejected by tools that consult git +# --------------------------------------------------------------------------- + +class TestRequireGitRepo: + def test_git_repo_is_accepted(self, tmp_path: Path) -> None: + repo = _repo_with_commit(tmp_path, "feat: add a file") + server._require_git_repo(repo) # should not raise + + def test_plain_directory_is_rejected(self, tmp_path: Path) -> None: + with pytest.raises(ToolError, match="repo_path is not a git repository"): + server._require_git_repo(tmp_path) + + def test_repository_state_rejects_plain_directory(self, tmp_path: Path) -> None: + with pytest.raises(ToolError, match="repo_path is not a git repository"): + server.validate_repository_state(repo_path=str(tmp_path)) + + def test_branch_omitted_rejects_plain_directory(self, tmp_path: Path) -> None: + with pytest.raises(ToolError, match="repo_path is not a git repository"): + server.validate_branch_name(repo_path=str(tmp_path)) + + def test_author_omitted_rejects_plain_directory(self, tmp_path: Path) -> None: + with pytest.raises(ToolError, match="repo_path is not a git repository"): + server.validate_author_info(repo_path=str(tmp_path)) + + def test_push_refs_omitted_rejects_plain_directory(self, tmp_path: Path) -> None: + with pytest.raises(ToolError, match="repo_path is not a git repository"): + server.validate_push_safety(repo_path=str(tmp_path)) + + def test_supplied_values_validate_without_git(self, tmp_path: Path) -> None: + plain = str(tmp_path) + assert server.validate_commit_message("feat: add x", repo_path=plain)["status"] == "pass" + assert server.validate_branch_name("main", repo_path=plain)["status"] in ("pass", "fail") + assert server.validate_author_info( + "Alice Example", "alice@example.com", repo_path=plain + )["status"] in ("pass", "fail") + assert server.validate_push_safety( + "refs/heads/main 0000000000000000000000000000000000000000 " + "refs/heads/main 0000000000000000000000000000000000000000", + repo_path=plain, + )["status"] in ("pass", "fail") + assert server.validate_commit_context(message="feat: add x", repo_path=plain)[ + "status" + ] == "pass" + assert "enabled_rules" in server.describe_validation_rules(repo_path=plain) + + +# --------------------------------------------------------------------------- +# Blank push_refs is an error, not a vacuous pass +# --------------------------------------------------------------------------- + +class TestBlankPushRefs: + def test_blank_push_refs_raises(self) -> None: + with pytest.raises(ToolError, match="push_refs cannot be empty when provided"): + server.validate_push_safety(push_refs=" ") + + def test_blank_push_refs_via_tool_call(self) -> None: + with pytest.raises(ToolError, match="push_refs cannot be empty when provided"): + _call_tool("validate_push_safety", push_refs=" ") From b93c392453462207598d971c2887e51931772b91 Mon Sep 17 00:00:00 2001 From: Xianpeng Shen Date: Sun, 6 Sep 2026 19:32:55 +0000 Subject: [PATCH 2/2] docs: say when a non-git repo_path is an error The error applies only when the tool has to read git state; a plain directory holding a config file is still fine when every value is supplied, as the Tool Usage note already says. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_018xYa7m3qup5wyN5MaXFgf6 --- README.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 9e7ca8f..dc086ef 100644 --- a/README.md +++ b/README.md @@ -51,10 +51,11 @@ Populating `fix` needs a commit-check release that carries the field; with an older commit-check the key is present and always empty. A call that cannot run at all — an empty `message`, a `repo_path` that does not -exist or is not a git repository, a malformed or rejected commit-check config — -is returned as an MCP tool error (`is_error`) whose text names the problem, for -example `repo_path is not a git repository: /path/to/dir` or -`invalid commit-check config: ...`, rather than as a `pass`/`fail` result. +exist, a `repo_path` that is not a git repository when the tool has to read git +state (see the `repo_path` note under [Tool Usage](#tool-usage)), a malformed or rejected +commit-check config — is returned as an MCP tool error (`is_error`) whose text +names the problem, for example `repo_path is not a git repository: /path/to/dir` +or `invalid commit-check config: ...`, rather than as a `pass`/`fail` result. ## Installation