From 7fc6e3a0d6877e9f5c5e145c465d1dbb37b44eed Mon Sep 17 00:00:00 2001 From: Xianpeng Shen Date: Sun, 6 Sep 2026 23:16:24 +0000 Subject: [PATCH 1/2] feat: describe every tool parameter and annotate tools for MCP clients An MCP client hands the model the tool's JSON input schema, not its docstring. Until now every property carried only pydantic's auto title (push_refs: string | null), so the pre-push line format, the "omit to check upstream" behaviour and the "non-empty when provided" rule were discovered by trial and error. Every parameter now has a description through Annotated[..., Field(description=...)], which the SDK preserves in the schema; the shared ones (config, repo_path, config_path, branch, author_*) are defined once as module-level aliases. Every tool also gets a display title and ToolAnnotations with readOnlyHint=True, destructiveHint=False and idempotentHint=True, so hosts that gate calls on those hints can auto-approve them. openWorldHint is True only for validate_push_safety and validate_repository_state, because the force-push rule may run `git fetch ` to resolve a SHA it does not know locally. MCPServer now receives version=__version__, so serverInfo.version is no longer an empty string. The tool descriptions are rewritten around one shared paragraph that defines the result: {status, warnings, checks[]}, that only 'fail' is a rejection and 'skip' means nothing was validated, what 'warn' and 'skip' mean per check, and that rule_id and docs_url exist. Each tool keeps its own "when to use" sentence. validate_push_safety no longer claims that push.allow_force_push can re-enable force pushes: _validate_push forces that flag off after merging config, so the tool always rejects them. The server instructions now describe the loop an agent should follow: validate first, read status, apply a non-empty fix verbatim or follow suggest, validate again, and consult describe_validation_rules before guessing at a format. Tests list the tools through the in-process server API and assert the titles, annotations, open-world split, per-property descriptions, the version and the instructions. README documents rule_id/docs_url, the schema metadata, and that config_path replaces the repository's own config file rather than merging with it. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_018xYa7m3qup5wyN5MaXFgf6 --- README.md | 28 ++- src/commit_check_mcp/server.py | 395 ++++++++++++++++++++++++--------- tests/test_server.py | 69 ++++++ 3 files changed, 377 insertions(+), 115 deletions(-) diff --git a/README.md b/README.md index 2b21570..5e37943 100644 --- a/README.md +++ b/README.md @@ -19,7 +19,7 @@ This MCP server exposes commit-check validations as MCP tools: - `server_health` — returns server/sdk versions - `validate_commit_message` — validates a commit message - `validate_branch_name` — validates a branch name or the current repo branch -- `validate_push_safety` — validates that a push is not a force push +- `validate_push_safety` — validates that a push is not a force push (force pushes are always rejected by this tool) - `validate_author_info` — validates author name/email or the repo's git author config - `validate_commit_context` — runs combined checks in one call - `validate_repository_state` — validates latest commit, current branch, author state, and optional push safety for a repo @@ -33,17 +33,22 @@ All validation tools return the same structured commit-check result shape: "warnings": 0, "checks": [ { + "rule_id": "CC001", "check": "message", "status": "pass|fail|warn|skip", "value": "...", "error": "...", "suggest": "...", - "fix": "..." + "fix": "...", + "docs_url": "https://commit-check.com/rules/#cc001" } ] } ``` +`rule_id` is the stable id of the rule that produced the check and `docs_url` +links to its documentation. + Only `fail` is a rejection. A check reports `skip` when it did not run — the author matched `ignore_authors`, or there was nothing to check — and the top-level `status` is `skip` only when **every** check skipped, so a run that @@ -266,10 +271,22 @@ After the client starts the server, it will expose these tools: - `validate_repository_state(repo_path?, config?, config_path?, include_message?, include_branch?, include_author?, include_push?)` - `describe_validation_rules(config?, repo_path?, config_path?)` +Every parameter carries a description in the tool's JSON input schema, so an +MCP client (and the model behind it) can see what each one expects without +reading this file: for example `push_refs` documents the git pre-push line +format ` `. Each tool also has +a display `title` and is annotated `readOnlyHint: true` (with `openWorldHint: +true` only on `validate_push_safety` and `validate_repository_state`, which may +run `git fetch` to resolve push SHAs), so clients that gate tool calls on those +hints can auto-approve them. The server's `instructions` describe the intended +loop: validate first, read `status` (only `fail` rejects, `skip` is not +approval), apply a non-empty `fix` verbatim or follow `suggest`, then validate +again. + The common optional arguments are: - `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_path`: explicit TOML config file, used instead of the repository's own `cchk.toml`/`commit-check.toml`; relative paths resolve from `repo_path` - `config`: ad-hoc config overrides merged on top of defaults and repo config ## Common Examples @@ -355,9 +372,8 @@ Example payload for a repository-wide validation: Config precedence is: 1. `commit-check` built-in defaults -2. repository config loaded from `repo_path` -3. `config_path` when explicitly provided -4. inline `config` overrides passed to the tool +2. repository config loaded from `repo_path`, or the file named by `config_path` when it is provided (it replaces the repository's own config file) +3. inline `config` overrides passed to the tool ## Published On diff --git a/src/commit_check_mcp/server.py b/src/commit_check_mcp/server.py index 60e5c4b..c27333a 100644 --- a/src/commit_check_mcp/server.py +++ b/src/commit_check_mcp/server.py @@ -2,12 +2,14 @@ from __future__ import annotations +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 Any +from typing import Annotated, Any, TypeVar from commit_check import __version__ as commit_check_version from commit_check.config_merger import deep_merge, get_default_config, load_toml_config @@ -22,17 +24,161 @@ 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 mcp.types import ToolAnnotations +from pydantic import Field from . import __version__ -mcp = MCPServer( - "commit-check-mcp", - instructions=( - "Use these tools to validate commit messages, branch names, author metadata, " - "and push safety with commit-check." - ), +INSTRUCTIONS = ( + "commit-check-mcp validates commit messages, branch names, author metadata and push safety " + "against commit-check rules. Every tool is read-only.\n" + "Workflow: (1) validate FIRST, before you commit or push, with the matching validate_* tool " + "(pass repo_path so the repository's own cchk.toml/commit-check.toml is used). " + "(2) Read the top-level status: only 'fail' is a rejection; 'skip' means nothing was validated " + "(do not treat it as approval); 'warn' checks are reported but do not fail the run. " + "(3) On 'fail', for each check with status 'fail': if its fix is non-empty, apply fix " + "verbatim; otherwise rewrite the value following suggest (rule_id and docs_url point at the " + "rule's docs). " + "(4) Validate AGAIN with the corrected value and repeat until status is 'pass'. " + "Use describe_validation_rules to see which rules are enabled before guessing at a format." +) + +mcp = MCPServer("commit-check-mcp", instructions=INSTRUCTIONS, version=__version__) + +# The result every validate_* tool returns, described once and spliced into each +# tool description where its docstring says ``{result_shape}``. +RESULT_SHAPE = ( + "Returns {status, warnings, checks[]}. status is 'pass', 'fail' or 'skip': only 'fail' is a " + "rejection; 'skip' means every check skipped, so nothing was validated and the result is not " + "approval. warnings is the number of checks with status 'warn'. Each check has: rule_id " + "(stable rule id, e.g. 'CC001'); check (rule name, e.g. 'message'); status 'pass' | 'fail' | " + "'warn' | 'skip' ('warn' = the config lists the check under warn, so the finding is reported " + "without failing the run; 'skip' = the rule did not run, e.g. the author is in ignore_authors " + "or there was nothing to check); value (what was checked); error (why it failed); suggest " + "(advice for a person); fix (the corrected value when the correction is unambiguous, else ''); " + "docs_url (documentation for the rule). On 'fail', apply a non-empty fix verbatim; when fix is " + "'', rewrite following suggest; then validate again." ) +_F = TypeVar("_F", bound=Callable[..., Any]) + + +def _tool(title: str, *, open_world: bool = False) -> Callable[[_F], _F]: + """Register a commit-check tool with the hints every one of them shares. + + All tools are read-only and idempotent. ``open_world`` marks the two that + can reach the network: the force-push check runs ``git fetch`` when a SHA + is not known locally. The function docstring becomes the tool description, + with ``{result_shape}`` replaced by :data:`RESULT_SHAPE`. + """ + annotations = ToolAnnotations( + readOnlyHint=True, + destructiveHint=False, + idempotentHint=True, + openWorldHint=open_world, + ) + + def register(fn: _F) -> _F: + description = inspect.cleandoc(fn.__doc__ or "").replace("{result_shape}", RESULT_SHAPE) + return mcp.tool(title=title, description=description, annotations=annotations)(fn) + + return register + + +# Parameter types shared by the tools; the Field description reaches the tool's +# JSON schema, which is what an MCP client shows the model. +ConfigParam = Annotated[ + dict[str, Any] | None, + Field( + description=( + "Inline commit-check config overrides as a JSON object, merged on top of the built-in " + 'defaults and any config file, e.g. {"warn": ["message"]} or ' + '{"commit": {"require_body": true}}.' + ) + ), +] +RepoPathParam = Annotated[ + str | None, + Field( + description=( + "Path to the git repository to validate against. Its cchk.toml or commit-check.toml " + "(also looked up under .github/) is loaded, and a relative config_path is resolved " + "from it. Omit to use the server's working directory." + ) + ), +] +ConfigPathParam = Annotated[ + str | None, + Field( + description=( + "Path to a commit-check TOML config file, used instead of the repository's own " + "cchk.toml/commit-check.toml; a relative path is resolved from repo_path." + ) + ), +] +MessageParam = Annotated[ + str, + Field( + description=( + "Full commit message text to validate: subject line, optional blank line, body " + "(and trailers such as Signed-off-by). Must be non-empty." + ) + ), +] +OptionalMessageParam = Annotated[ + str | None, + Field( + description=( + "Commit message text to validate: subject line, optional blank line, body. Omit to " + "skip the message checks. Must be non-empty when provided." + ) + ), +] +BranchParam = Annotated[ + str | None, + Field( + description=( + "Branch name to validate, e.g. 'feature/login'. Omit to validate the branch currently " + "checked out in repo_path, which must then be a git repository. Must be non-empty when " + "provided." + ) + ), +] +AuthorNameParam = Annotated[ + str | None, + Field( + description=( + "Author name to validate, e.g. 'Alice Example'. Omit to read it from repo_path " + "(git config user.name, falling back to the latest commit's author). Must be " + "non-empty when provided." + ) + ), +] +AuthorEmailParam = Annotated[ + str | None, + Field( + description=( + "Author email to validate, e.g. 'alice@example.com'. Omit to read it from repo_path " + "(git config user.email, falling back to the latest commit's author). Must be " + "non-empty when provided." + ) + ), +] +PushRefsParam = Annotated[ + str | None, + Field( + description=( + "Refs about to be pushed, in git pre-push hook stdin format, one ref per line: " + "' ', e.g. " + "'refs/heads/main 1a2b3c... refs/heads/main 9f8e7d...'. A remote_sha of 40 zeros " + "means a new branch (never a force push). SHAs must be resolvable in repo_path (the " + "check runs git merge-base and may fetch the remote ref); unresolvable SHAs cannot be " + "judged and pass. Omit to check the current branch of repo_path against its upstream " + "instead. Must be non-empty when provided." + ) + ), +] + def _normalize_config(config: dict[str, Any] | None) -> dict[str, Any] | None: """Ensure tool config input is JSON-object-like.""" @@ -364,9 +510,13 @@ def _validate_all( return _summarize(checks) -@mcp.tool() +@_tool("Server health") def server_health() -> dict[str, str]: - """Return server and dependency versions. Read-only, no side effects. Returns dict with server name, server version, commit-check version, and MCP SDK version. Useful as a first call to verify the server is running and check version compatibility.""" + """Return server and dependency versions. Read-only, no side effects. + + Returns {server, server_version, commit_check_version, mcp_sdk_version}. Useful as a first call + to verify the server is running and to check version compatibility. + """ return { "server": "commit-check-mcp", "server_version": __version__, @@ -375,22 +525,22 @@ def server_health() -> dict[str, str]: } -@mcp.tool() +@_tool("Validate commit message") def validate_commit_message( - message: str, - config: dict[str, Any] | None = None, - repo_path: str | None = None, - config_path: str | None = None, + message: MessageParam, + config: ConfigParam = None, + repo_path: RepoPathParam = None, + config_path: ConfigPathParam = None, ) -> dict[str, Any]: - """Validate a commit message against commit-check rules. Read-only validation. Returns a structured result with overall status ('pass'/'fail') and a list of per-check results. Each check includes the check name, status, value, error message (on failure), suggestion (on failure), and fix: the corrected value when the correction is unambiguous (a type's case, a missing colon, a WIP marker, a missing sign-off), otherwise an empty string. Apply a non-empty fix as it stands; when fix is empty, rewrite from the suggestion. + """Validate a commit message against commit-check rules (Conventional Commits type and format, + subject length and case, body, sign-off, WIP/fixup markers, AI attribution: whatever the + effective config enables). Read-only; touches no git state. - Use this tool when you have a specific commit message string to validate. For batch validation of message, branch, and author together, use validate_commit_context instead. + {result_shape} - Parameters: - - message (required): The commit message text to validate. - - config (optional): Inline JSON config overrides on top of any loaded config file. - - repo_path (optional): Path to the git repository for repo-relative config loading. - - config_path (optional): Path to a custom commit-check TOML config file. + Use this when you have one commit message string to check before committing. To check message, + branch and author in one call use validate_commit_context; to check the latest commit already + in a repository use validate_repository_state. """ if not isinstance(message, str) or not message.strip(): raise ToolError("message must be a non-empty string") @@ -403,22 +553,20 @@ def validate_commit_message( ) -@mcp.tool() +@_tool("Validate branch name") def validate_branch_name( - branch: str | None = None, - config: dict[str, Any] | None = None, - repo_path: str | None = None, - config_path: str | None = None, + branch: BranchParam = None, + config: ConfigParam = None, + repo_path: RepoPathParam = None, + config_path: ConfigPathParam = None, ) -> dict[str, Any]: - """Validate branch naming conventions with commit-check. Read-only validation. Returns a structured result with overall status ('pass'/'fail') and per-check results (check name, status, value, error, suggest, and fix: the corrected value when unambiguous, else empty). + """Validate a branch name against the configured naming convention (e.g. feature/*, bugfix/*) + and, when configured, that the branch is based on the required merge base. Read-only. - Use this when you need to verify a branch name follows configured convention rules (e.g., feature/*, bugfix/*). For combined message+branch+author validation, use validate_commit_context. + {result_shape} - Parameters: - - branch (optional): The branch name to validate. If omitted, detected from the current repo. - - 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. + Use this before creating or pushing a branch. Omit branch to check the branch currently checked + out in repo_path. For combined message+branch+author validation use validate_commit_context. """ normalized_branch = branch.strip() if isinstance(branch, str) else None if isinstance(branch, str) and not normalized_branch: @@ -434,24 +582,22 @@ def validate_branch_name( ) -@mcp.tool() +@_tool("Validate author info") def validate_author_info( - author_name: str | None = None, - author_email: str | None = None, - config: dict[str, Any] | None = None, - repo_path: str | None = None, - config_path: str | None = None, + author_name: AuthorNameParam = None, + author_email: AuthorEmailParam = None, + config: ConfigParam = None, + repo_path: RepoPathParam = None, + config_path: ConfigPathParam = None, ) -> dict[str, Any]: - """Validate commit author name and/or email with commit-check. Read-only validation. Returns a structured result with overall status and per-check results (check name, status, value, error, suggest, and fix: the corrected value when unambiguous, else empty). + """Validate a commit author's name and/or email against the configured rules (e.g. allowed + email domains, name patterns). Read-only. - Use this when you need to verify author metadata against configured rules (e.g., allowed email domains, name patterns). When both name and email are provided, both are validated. If neither is provided, both are checked against repo context. For combined validation, use validate_commit_context. + {result_shape} - Parameters: - - author_name (optional): The author name to validate. - - author_email (optional): The author email to validate. - - 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. + Use this to check author metadata before committing. Only the values you pass are checked; if + neither is given, both are read from repo_path's git config (falling back to the latest commit) + and repo_path must be a git repository. For combined validation use validate_commit_context. """ normalized_name = author_name.strip() if isinstance(author_name, str) else None normalized_email = author_email.strip() if isinstance(author_email, str) else None @@ -473,22 +619,24 @@ def validate_author_info( ) -@mcp.tool() +@_tool("Validate push safety", open_world=True) def validate_push_safety( - push_refs: str | None = None, - config: dict[str, Any] | None = None, - repo_path: str | None = None, - config_path: str | None = None, + push_refs: PushRefsParam = None, + config: ConfigParam = None, + repo_path: RepoPathParam = None, + config_path: ConfigPathParam = None, ) -> dict[str, Any]: - """Validate that a push is not a force push. Read-only validation. Returns a structured result with overall status and per-check results (check name, status, value, error, suggest, and fix: the corrected value when unambiguous, else empty). By default, force push is rejected; configure via 'push.allow_force_push' in config. + """Check that a pending push is not a force push (rule CC301, no_force_push): fails when a + remote_sha is not an ancestor of its local_sha, i.e. the push would rewrite remote history. + Read-only for your working tree and commits; it may run `git fetch ` to resolve + SHAs. Force pushes are always rejected by this tool; push.allow_force_push in config cannot + re-enable them here. - 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. + {result_shape} - Parameters: - - 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. + Call this before `git push`. Only the no_force_push rule runs. When it fails there is no + automatic fix (fix is ''): follow suggest, i.e. push without --force/--force-with-lease or + rebase onto the remote first, then validate again. """ normalized_push_refs = push_refs.strip() if isinstance(push_refs, str) else None if isinstance(push_refs, str) and not normalized_push_refs: @@ -504,28 +652,25 @@ def validate_push_safety( ) -@mcp.tool() +@_tool("Validate commit context") def validate_commit_context( - message: str | None = None, - branch: str | None = None, - author_name: str | None = None, - author_email: str | None = None, - config: dict[str, Any] | None = None, - repo_path: str | None = None, - config_path: str | None = None, + message: OptionalMessageParam = None, + branch: BranchParam = None, + author_name: AuthorNameParam = None, + author_email: AuthorEmailParam = None, + config: ConfigParam = None, + repo_path: RepoPathParam = None, + config_path: ConfigPathParam = None, ) -> dict[str, Any]: - """Run combined commit-check validations for message, branch, and/or author in one call. Read-only validation. Returns a structured result with overall status and a unified list of per-check results (check name, status, value, error, suggest, and fix: the corrected value when unambiguous, else empty). - - Use this when you need to validate multiple commit aspects simultaneously in a single call. At least one of message, branch, author_name, or author_email must be provided. For individual aspects, use the specific validate_commit_message, validate_branch_name, or validate_author_info tools. - - Parameters: - - message (optional): Commit message text to validate. - - branch (optional): Branch name to validate. - - author_name (optional): Author name to validate. - - author_email (optional): Author email to validate. - - config (optional): Inline JSON config overrides on top of any loaded config file. - - repo_path (optional): Path to the git repository for repo-relative config loading. - - config_path (optional): Path to a custom commit-check TOML config file. + """Run the commit message, branch name and author checks together in one call, for whichever of + message, branch, author_name and author_email you pass. Read-only. + + {result_shape} + + Use this to validate several aspects of a commit you are about to make with a single call. At + least one of message, branch, author_name or author_email is required; omitted aspects are not + checked. For one aspect use validate_commit_message, validate_branch_name or + validate_author_info; for the commit already at HEAD use validate_repository_state. """ normalized_message = message.strip() if isinstance(message, str) else None normalized_branch = branch.strip() if isinstance(branch, str) else None @@ -558,28 +703,58 @@ def validate_commit_context( ) -@mcp.tool() +@_tool("Validate repository state", open_world=True) def validate_repository_state( - repo_path: str | None = None, - config: dict[str, Any] | None = None, - config_path: str | None = None, - include_message: bool = True, - include_branch: bool = True, - include_author: bool = True, - include_push: bool = False, + repo_path: Annotated[ + str | None, + Field( + description=( + "Path to the git repository to inspect; its cchk.toml or commit-check.toml is " + "loaded and a relative config_path is resolved from it. Omit to use the server's " + "working directory, which must then be a git repository." + ) + ), + ] = None, + config: ConfigParam = None, + config_path: ConfigPathParam = None, + include_message: Annotated[ + bool, + Field(description="Validate the message of the latest commit (HEAD). Default true."), + ] = True, + include_branch: Annotated[ + bool, + Field(description="Validate the name of the currently checked-out branch. Default true."), + ] = True, + include_author: Annotated[ + bool, + Field( + description=( + "Validate the author name and email of the latest commit (falling back to git " + "config user.name/user.email). Default true." + ) + ), + ] = True, + include_push: Annotated[ + bool, + Field( + description=( + "Also check that pushing the current branch to its upstream would not be a " + "force push; may run git fetch, and passes when the branch has no upstream. " + "Default false." + ) + ), + ] = False, ) -> dict[str, Any]: - """Validate the current repository state including latest commit message, active branch, author metadata, and optional push safety. Read-only validation. Reads git data (message, branch, author) from the local repository. Returns a structured result with overall status and per-check results. - - Use this to validate the entire state of a local git repository in one call — ideal for pre-commit or CI hooks. Controls which checks run via boolean include_* flags. For validating arbitrary (non-repo) values, use validate_commit_context or individual validation tools instead. - - Parameters: - - repo_path (optional): Path to the git repository. If omitted, uses current working directory. - - config (optional): Inline JSON config overrides on top of any loaded config file. - - config_path (optional): Path to a custom commit-check TOML config file. - - include_message (optional, default true): Whether to validate the latest commit message. - - include_branch (optional, default true): Whether to validate the current branch name. - - include_author (optional, default true): Whether to validate the latest commit author. - - include_push (optional, default false): Whether to validate push safety. + """Validate what is already in a local git repository: the latest commit's message and author, + the checked-out branch name and, optionally, whether pushing that branch to its upstream would + be a force push. Reads git data only; it may run `git fetch` for the push check. + + {result_shape} + + Use this to check a repository's current state in one call, e.g. after committing and before + pushing, or in a hook. The include_* flags select the checks; at least one must be true. To + validate values that are not yet committed use validate_commit_context or the single-aspect + tools. """ if not any([include_message, include_branch, include_author, include_push]): raise ToolError("At least one validation target must be enabled") @@ -631,20 +806,22 @@ def validate_repository_state( return _summarize(checks) -@mcp.tool() +@_tool("Describe validation rules") def describe_validation_rules( - config: dict[str, Any] | None = None, - repo_path: str | None = None, - config_path: str | None = None, + config: ConfigParam = None, + repo_path: RepoPathParam = None, + config_path: ConfigPathParam = None, ) -> dict[str, Any]: - """Return enabled commit-check rules after merging defaults, repo config, and inline overrides. Read-only, no side effects. Returns a dict with commit_check_version, the full merged config, supported check types, and enabled rules (each with check name, config, and pattern details). + """Return the commit-check rules that are in effect after merging the built-in defaults, the + repository's config file (or config_path) and the inline config overrides. Read-only, no side + effects. - Use this to inspect which validation rules are currently active before running any validation. Helps debug rule configuration and check which checks will be applied. + Returns {commit_check_version, config (the merged config), supported_checks (every check name + commit-check knows), enabled_rules[]} where each enabled rule carries its check name, config + and pattern details. - Parameters: - - config (optional): Inline JSON config overrides on top of any loaded config file. - - repo_path (optional): Path to the git repository for repo-relative config loading. - - config_path (optional): Path to a custom commit-check TOML config file. + Use this before writing a commit message or branch name to learn the expected format instead + of guessing, and to debug why a validation failed or was skipped. """ normalized_repo_path = _normalize_repo_path(repo_path) normalized_config = _normalize_config(config) diff --git a/tests/test_server.py b/tests/test_server.py index fc8f9cb..574cb82 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -1194,3 +1194,72 @@ def test_blank_push_refs_raises(self) -> None: 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=" ") + + +# --------------------------------------------------------------------------- +# What an MCP client is told about the tools: titles, annotations, schema +# descriptions, server version and instructions +# --------------------------------------------------------------------------- + +OPEN_WORLD_TOOLS = {"validate_push_safety", "validate_repository_state"} + + +def _list_tools() -> list: + """List tools the way an MCP client does, through the server's list path.""" + return asyncio.run(server.mcp.list_tools()) + + +class TestToolMetadata: + def test_every_tool_has_a_title_and_read_only_annotations(self) -> None: + tools = _list_tools() + assert len(tools) == 8 + for tool in tools: + assert tool.title, tool.name + assert tool.annotations is not None, tool.name + assert tool.annotations.read_only_hint is True, tool.name + assert tool.annotations.destructive_hint is False, tool.name + assert tool.annotations.idempotent_hint is True, tool.name + + def test_open_world_hint_marks_only_the_tools_that_may_fetch(self) -> None: + for tool in _list_tools(): + expected = tool.name in OPEN_WORLD_TOOLS + assert tool.annotations.open_world_hint is expected, tool.name + + def test_every_parameter_has_a_description(self) -> None: + seen = 0 + for tool in _list_tools(): + for name, prop in tool.input_schema.get("properties", {}).items(): + seen += 1 + assert prop.get("description"), f"{tool.name}.{name} has no description" + assert seen > 0 + + def test_push_refs_description_explains_the_pre_push_line_format(self) -> None: + tool = next(t for t in _list_tools() if t.name == "validate_push_safety") + description = tool.input_schema["properties"]["push_refs"]["description"] + assert " " in description + assert "40 zeros" in description + assert "upstream" in description + assert "non-empty when provided" in description + + def test_validation_tools_describe_the_result_shape(self) -> None: + validators = [t for t in _list_tools() if t.name.startswith("validate_")] + assert len(validators) == 6 + for tool in validators: + for term in ("rule_id", "docs_url", "'warn'", "'skip'", "fix", "suggest"): + assert term in tool.description, f"{tool.name} does not mention {term}" + + def test_push_safety_says_force_pushes_are_always_rejected(self) -> None: + tool = next(t for t in _list_tools() if t.name == "validate_push_safety") + assert "always rejected" in tool.description + assert "configure via" not in tool.description + + def test_server_reports_its_version(self) -> None: + assert server.mcp.version == server.__version__ + assert server.mcp.version != "" + + def test_instructions_describe_the_validate_fix_workflow(self) -> None: + instructions = server.mcp.instructions + assert instructions is not None + assert "validate" in instructions + assert "fix" in instructions + assert "describe_validation_rules" in instructions From 96fb7f0f7e1ad299d5bef47ec89b8787e10d5ac8 Mon Sep 17 00:00:00 2001 From: Xianpeng Shen Date: Sun, 6 Sep 2026 23:28:03 +0000 Subject: [PATCH 2/2] fix: reject unresolvable push SHAs and stop calling fetching tools read-only Two review findings on the previous commit. The force-push rule answers with `git merge-base --is-ancestor`. When a SHA in push_refs is not a commit the repository knows, git exits 128, commit-check tries to fetch the remote ref, and if the SHA is still unknown the rule falls through to PASS. validate_push_safety therefore reported `status: pass` for made-up SHAs, and an agent would read that as clearance to push. After the rule has run (so a SHA its fetch brought in counts as resolved), _validate_push now checks every SHA in explicit push_refs with `git cat-file -e ^{commit}` inside repo_path, skipping the 40-zero new-branch placeholder, and raises a ToolError naming the SHA when one is not a commit. An engine fail still wins: a rejection is already the right answer. The upstream-fallback path (push_refs omitted, and validate_repository_state's include_push) reads HEAD and the upstream ref, which always resolve, and is unchanged. validate_push_safety and validate_repository_state may run `git fetch`, which writes FETCH_HEAD and remote-tracking refs, so advertising them with readOnlyHint=True was not honest. _tool now takes `fetches` and sets readOnlyHint=False, openWorldHint=True for those two; the other six keep readOnlyHint=True, openWorldHint=False. destructiveHint stays False and idempotentHint True everywhere: repeating the same fetch has no further effect. The tool descriptions, push_refs description, server instructions and README say the same thing instead of "read-only". Tests: fake SHAs are a tool error, a real fast-forward pair passes, a real history rewrite fails, a zero remote SHA passes, an engine fail is not turned into an error by a later unresolvable line, repository_state include_push is unaffected, and the annotation split is asserted. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_018xYa7m3qup5wyN5MaXFgf6 --- README.md | 25 ++++--- src/commit_check_mcp/server.py | 97 +++++++++++++++++++------ tests/test_server.py | 127 ++++++++++++++++++++++++++++++--- 3 files changed, 208 insertions(+), 41 deletions(-) diff --git a/README.md b/README.md index 5e37943..f053535 100644 --- a/README.md +++ b/README.md @@ -64,9 +64,14 @@ a non-empty `fix` as it stands and fall back to `suggest` when it is empty. A call that cannot run at all — an empty `message`, a `repo_path` that does not 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. +commit-check config, a `push_refs` SHA that is not a commit in `repo_path` even +after the force-push check tried to fetch it — 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`, +`invalid commit-check config: ...` or +`push_refs: is not a commit in the repository; fetch it first, the force-push check cannot be judged`, +rather than as a `pass`/`fail` result. In particular a push whose SHAs cannot +be judged is never reported as a pass. ## Installation @@ -275,17 +280,21 @@ Every parameter carries a description in the tool's JSON input schema, so an MCP client (and the model behind it) can see what each one expects without reading this file: for example `push_refs` documents the git pre-push line format ` `. Each tool also has -a display `title` and is annotated `readOnlyHint: true` (with `openWorldHint: -true` only on `validate_push_safety` and `validate_repository_state`, which may -run `git fetch` to resolve push SHAs), so clients that gate tool calls on those -hints can auto-approve them. The server's `instructions` describe the intended +a display `title` and MCP tool annotations: `destructiveHint: false` and +`idempotentHint: true` everywhere, `readOnlyHint: true` on the six tools that +only read, and `readOnlyHint: false` with `openWorldHint: true` on +`validate_push_safety` and `validate_repository_state`, because the force-push +check may run `git fetch` to resolve a SHA, which updates `FETCH_HEAD` and +remote-tracking refs (the working tree and commits are never touched). Clients +that gate tool calls on those hints can auto-approve the read-only six. The +server's `instructions` describe the intended loop: validate first, read `status` (only `fail` rejects, `skip` is not approval), apply a non-empty `fix` verbatim or follow `suggest`, then validate again. The common optional arguments are: -- `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 +- `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, `validate_repository_state`, or `push_refs` given, whose SHAs must resolve there), and may be a plain directory holding a config file when every other value is supplied - `config_path`: explicit TOML config file, used instead of the repository's own `cchk.toml`/`commit-check.toml`; relative paths resolve from `repo_path` - `config`: ad-hoc config overrides merged on top of defaults and repo config diff --git a/src/commit_check_mcp/server.py b/src/commit_check_mcp/server.py index c27333a..84ce307 100644 --- a/src/commit_check_mcp/server.py +++ b/src/commit_check_mcp/server.py @@ -31,7 +31,8 @@ INSTRUCTIONS = ( "commit-check-mcp validates commit messages, branch names, author metadata and push safety " - "against commit-check rules. Every tool is read-only.\n" + "against commit-check rules. Every tool leaves your working tree and commits untouched; the " + "two push checks may run git fetch to resolve SHAs.\n" "Workflow: (1) validate FIRST, before you commit or push, with the matching validate_* tool " "(pass repo_path so the repository's own cchk.toml/commit-check.toml is used). " "(2) Read the top-level status: only 'fail' is a rejection; 'skip' means nothing was validated " @@ -63,19 +64,22 @@ _F = TypeVar("_F", bound=Callable[..., Any]) -def _tool(title: str, *, open_world: bool = False) -> Callable[[_F], _F]: +def _tool(title: str, *, fetches: bool = False) -> Callable[[_F], _F]: """Register a commit-check tool with the hints every one of them shares. - All tools are read-only and idempotent. ``open_world`` marks the two that - can reach the network: the force-push check runs ``git fetch`` when a SHA - is not known locally. The function docstring becomes the tool description, - with ``{result_shape}`` replaced by :data:`RESULT_SHAPE`. + No tool changes the working tree or the commits, and repeating a call has + no further effect, so every tool is idempotent and none is destructive. + ``fetches`` marks the two that run the force-push check: to resolve a SHA it + does not know locally it may run ``git fetch``, which reaches the network + and updates FETCH_HEAD and remote-tracking refs, so those two are neither + read-only nor closed-world. The function docstring becomes the tool + description, with ``{result_shape}`` replaced by :data:`RESULT_SHAPE`. """ annotations = ToolAnnotations( - readOnlyHint=True, + readOnlyHint=not fetches, destructiveHint=False, idempotentHint=True, - openWorldHint=open_world, + openWorldHint=fetches, ) def register(fn: _F) -> _F: @@ -171,10 +175,11 @@ def register(fn: _F) -> _F: "Refs about to be pushed, in git pre-push hook stdin format, one ref per line: " "' ', e.g. " "'refs/heads/main 1a2b3c... refs/heads/main 9f8e7d...'. A remote_sha of 40 zeros " - "means a new branch (never a force push). SHAs must be resolvable in repo_path (the " - "check runs git merge-base and may fetch the remote ref); unresolvable SHAs cannot be " - "judged and pass. Omit to check the current branch of repo_path against its upstream " - "instead. Must be non-empty when provided." + "means a new branch (never a force push). Every other SHA must resolve to a commit in " + "repo_path (the check runs git merge-base and may fetch the remote ref first); a SHA " + "that still cannot be resolved is a tool error, not a pass, so fetch it first. Omit to " + "check the current branch of repo_path against its upstream instead. Must be non-empty " + "when provided." ) ), ] @@ -383,6 +388,43 @@ def _validate_branch( ) +ZERO_SHA = "0" * 40 + + +def _require_push_shas_resolvable(push_refs: str) -> None: + """Fail when a pushed SHA is not a commit in the current working directory. + + The force-push rule asks ``git merge-base`` whether the remote SHA is an + ancestor of the local one. When either SHA is unknown, git exits 128 and, + after commit-check's own attempt to fetch the remote ref, the rule falls + through to PASS. Nothing was judged in that case, so the pass must not + reach the caller. This runs after the rule, so a SHA the rule's fetch + brought in counts as resolved. The 40-zero placeholder for a new branch is + not a commit and is skipped. + """ + for line in push_refs.splitlines(): + parts = line.split() + if len(parts) < 4: + continue + for sha in (parts[1], parts[3]): + if sha == ZERO_SHA: + continue + try: + result = subprocess.run( + ["git", "cat-file", "-e", f"{sha}^{{commit}}"], + capture_output=True, + text=True, + check=False, + ) + except OSError as e: + raise ToolError(f"git is not available to inspect push_refs: {e}") from e + if result.returncode != 0: + raise ToolError( + f"push_refs: {sha} is not a commit in the repository; fetch it first, " + "the force-push check cannot be judged" + ) + + def _validate_push( push_refs: str | None = None, *, @@ -390,11 +432,17 @@ def _validate_push( repo_path: Path | None = None, config_path: str | None = None, ) -> dict[str, Any]: - """Validate push ref updates against commit-check force-push protection.""" + """Validate push ref updates against commit-check force-push protection. + + With explicit ``push_refs``, a pass is only returned once every SHA in + them has been confirmed to be a commit in the repository; the + upstream-fallback path (``push_refs`` is ``None``) reads HEAD and the + upstream ref, which always resolve. + """ cfg = _merge_config(config, repo_path=repo_path, config_path=config_path) cfg.setdefault("push", {})["allow_force_push"] = False with _working_directory(repo_path): - return _run_checks( + result = _run_checks( ["no_force_push"], ValidationContext( stdin_text=push_refs, @@ -403,6 +451,9 @@ def _validate_push( ), cfg, ) + if push_refs and result["status"] != "fail": + _require_push_shas_resolvable(push_refs) + return result def _validate_author( @@ -619,7 +670,7 @@ def validate_author_info( ) -@_tool("Validate push safety", open_world=True) +@_tool("Validate push safety", fetches=True) def validate_push_safety( push_refs: PushRefsParam = None, config: ConfigParam = None, @@ -628,9 +679,10 @@ def validate_push_safety( ) -> dict[str, Any]: """Check that a pending push is not a force push (rule CC301, no_force_push): fails when a remote_sha is not an ancestor of its local_sha, i.e. the push would rewrite remote history. - Read-only for your working tree and commits; it may run `git fetch ` to resolve - SHAs. Force pushes are always rejected by this tool; push.allow_force_push in config cannot - re-enable them here. + Leaves your working tree and commits untouched, but may run `git fetch ` to + resolve SHAs, which updates FETCH_HEAD and remote-tracking refs. A SHA that cannot be resolved + even then is a tool error, never a pass. Force pushes are always rejected by this tool; + push.allow_force_push in config cannot re-enable them here. {result_shape} @@ -703,7 +755,7 @@ def validate_commit_context( ) -@_tool("Validate repository state", open_world=True) +@_tool("Validate repository state", fetches=True) def validate_repository_state( repo_path: Annotated[ str | None, @@ -739,15 +791,16 @@ def validate_repository_state( Field( description=( "Also check that pushing the current branch to its upstream would not be a " - "force push; may run git fetch, and passes when the branch has no upstream. " - "Default false." + "force push; may run git fetch (updating FETCH_HEAD), and passes when the branch " + "has no upstream. Default false." ) ), ] = False, ) -> dict[str, Any]: """Validate what is already in a local git repository: the latest commit's message and author, the checked-out branch name and, optionally, whether pushing that branch to its upstream would - be a force push. Reads git data only; it may run `git fetch` for the push check. + be a force push. Leaves the working tree and commits untouched; the push check may run + `git fetch`, which updates FETCH_HEAD and remote-tracking refs. {result_shape} diff --git a/tests/test_server.py b/tests/test_server.py index 574cb82..83e2b82 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -208,11 +208,11 @@ def test_called_with_arguments(self) -> None: # --------------------------------------------------------------------------- class TestValidatePush: - def test_no_force_push_rule_is_enforced(self) -> None: - result = server._validate_push("refs/heads/main abc refs/heads/main def") - # The rule is enabled, but depending on context it may pass or fail - assert "status" in result - assert isinstance(result["checks"], list) + def test_unresolvable_shas_are_an_error_not_a_pass(self) -> None: + # git merge-base exits 128 on SHAs it does not know and the rule falls + # through to PASS; the server must not hand that pass to the caller. + with pytest.raises(ToolError, match="push_refs: abc is not a commit in the repository"): + server._validate_push("refs/heads/main abc refs/heads/main def") def test_with_push_refs_none_and_patch(self, monkeypatch: pytest.MonkeyPatch) -> None: captured: list[dict] = [] @@ -1201,7 +1201,7 @@ def test_blank_push_refs_via_tool_call(self) -> None: # descriptions, server version and instructions # --------------------------------------------------------------------------- -OPEN_WORLD_TOOLS = {"validate_push_safety", "validate_repository_state"} +FETCHING_TOOLS = {"validate_push_safety", "validate_repository_state"} def _list_tools() -> list: @@ -1210,20 +1210,26 @@ def _list_tools() -> list: class TestToolMetadata: - def test_every_tool_has_a_title_and_read_only_annotations(self) -> None: + def test_every_tool_has_a_title_and_non_destructive_annotations(self) -> None: tools = _list_tools() assert len(tools) == 8 for tool in tools: assert tool.title, tool.name assert tool.annotations is not None, tool.name - assert tool.annotations.read_only_hint is True, tool.name assert tool.annotations.destructive_hint is False, tool.name assert tool.annotations.idempotent_hint is True, tool.name - def test_open_world_hint_marks_only_the_tools_that_may_fetch(self) -> None: + def test_only_the_tools_that_may_fetch_are_not_read_only(self) -> None: + # The force-push check may run git fetch, which writes FETCH_HEAD and + # remote-tracking refs, so those two tools cannot claim to be read-only. + seen = set() for tool in _list_tools(): - expected = tool.name in OPEN_WORLD_TOOLS - assert tool.annotations.open_world_hint is expected, tool.name + fetches = tool.name in FETCHING_TOOLS + assert tool.annotations.read_only_hint is (not fetches), tool.name + assert tool.annotations.open_world_hint is fetches, tool.name + if fetches: + seen.add(tool.name) + assert seen == FETCHING_TOOLS def test_every_parameter_has_a_description(self) -> None: seen = 0 @@ -1240,6 +1246,7 @@ def test_push_refs_description_explains_the_pre_push_line_format(self) -> None: assert "40 zeros" in description assert "upstream" in description assert "non-empty when provided" in description + assert "tool error, not a pass" in description def test_validation_tools_describe_the_result_shape(self) -> None: validators = [t for t in _list_tools() if t.name.startswith("validate_")] @@ -1263,3 +1270,101 @@ def test_instructions_describe_the_validate_fix_workflow(self) -> None: assert "validate" in instructions assert "fix" in instructions assert "describe_validation_rules" in instructions + assert "read-only" not in instructions + + +# --------------------------------------------------------------------------- +# validate_push_safety: a push whose SHAs cannot be judged is not a pass +# --------------------------------------------------------------------------- + +FAKE_SHA_A = "0123456789abcdef0123456789abcdef01234567" +FAKE_SHA_B = "fedcba9876543210fedcba9876543210fedcba98" +ZERO_SHA = "0" * 40 + + +def _rev(repo: Path, ref: str) -> str: + return subprocess.run( + ["git", "rev-parse", ref], cwd=repo, check=True, capture_output=True, text=True + ).stdout.strip() + + +def _repo_with_two_commits(root: Path) -> Path: + repo = _repo_with_commit(root, "feat: first") + (repo / "file.txt").write_text("more\n") + _git(repo, "commit", "-q", "-am", "feat: second") + return repo + + +class TestPushRefsMustResolve: + def test_fake_shas_are_a_tool_error(self, tmp_path: Path) -> None: + repo = _repo_with_two_commits(tmp_path) + with pytest.raises(ToolError, match=f"push_refs: {FAKE_SHA_A} is not a commit"): + _call_tool( + "validate_push_safety", + push_refs=f"refs/heads/main {FAKE_SHA_A} refs/heads/main {FAKE_SHA_B}", + repo_path=str(repo), + ) + + def test_fake_remote_sha_alone_is_a_tool_error(self, tmp_path: Path) -> None: + repo = _repo_with_two_commits(tmp_path) + head = _rev(repo, "HEAD") + with pytest.raises(ToolError, match=f"push_refs: {FAKE_SHA_B} is not a commit"): + server.validate_push_safety( + push_refs=f"refs/heads/main {head} refs/heads/main {FAKE_SHA_B}", + repo_path=str(repo), + ) + + def test_fast_forward_pair_passes(self, tmp_path: Path) -> None: + repo = _repo_with_two_commits(tmp_path) + head, parent = _rev(repo, "HEAD"), _rev(repo, "HEAD~1") + result = server.validate_push_safety( + push_refs=f"refs/heads/main {head} refs/heads/main {parent}", + repo_path=str(repo), + ) + assert result["status"] == "pass" + assert [c["check"] for c in result["checks"]] == ["no_force_push"] + + def test_rewriting_remote_history_fails(self, tmp_path: Path) -> None: + repo = _repo_with_two_commits(tmp_path) + head, parent = _rev(repo, "HEAD"), _rev(repo, "HEAD~1") + result = server.validate_push_safety( + push_refs=f"refs/heads/main {parent} refs/heads/main {head}", + repo_path=str(repo), + ) + assert result["status"] == "fail" + assert result["checks"][0]["check"] == "no_force_push" + + def test_new_branch_zero_remote_sha_passes(self, tmp_path: Path) -> None: + repo = _repo_with_two_commits(tmp_path) + head = _rev(repo, "HEAD") + result = server.validate_push_safety( + push_refs=f"refs/heads/topic {head} refs/heads/topic {ZERO_SHA}", + repo_path=str(repo), + ) + assert result["status"] == "pass" + + def test_engine_failure_wins_over_resolvability(self, tmp_path: Path) -> None: + # The first line is a real force push; the engine reports fail and the + # unresolvable SHA on the second line does not turn that into an error. + repo = _repo_with_two_commits(tmp_path) + head, parent = _rev(repo, "HEAD"), _rev(repo, "HEAD~1") + result = server.validate_push_safety( + push_refs=( + f"refs/heads/main {parent} refs/heads/main {head}\n" + f"refs/heads/other {FAKE_SHA_A} refs/heads/other {FAKE_SHA_B}" + ), + repo_path=str(repo), + ) + assert result["status"] == "fail" + + def test_repository_state_include_push_is_unaffected(self, tmp_path: Path) -> None: + repo = _repo_with_two_commits(tmp_path) + result = server.validate_repository_state( + repo_path=str(repo), + include_message=False, + include_branch=False, + include_author=False, + include_push=True, + ) + assert result["status"] == "pass" + assert [c["check"] for c in result["checks"]] == ["no_force_push"]