From 51e2aa9e17c113820ac8f587e4200a5886a70fda Mon Sep 17 00:00:00 2001 From: Xianpeng Shen Date: Sun, 6 Sep 2026 22:13:53 +0000 Subject: [PATCH] fix: report a fully skipped run as skip, add warnings, require commit-check 2.17 Every validation tool reduced its per-check statuses with its own copy of `"fail" if any failed else "pass"`, so a run in which every check skipped (the author is on `ignore_authors`, for example) came back as `pass`. An agent reading that took a bypassed policy for an enforced one. The four copies are replaced by one `_summarize` helper built on commit-check's own `overall_status` and `count_warnings`, so the MCP result now matches the CLI's `--format json`: `status` is `skip` when nothing was validated, a `warn` check leaves `status` at `pass`, and a top-level `warnings` field counts them. Both helpers exist since commit-check 2.17.0, which becomes the floor; `uv.lock` is regenerated accordingly. The `fix` key has been part of every release the floor allows, so the `setdefault("fix", "")` shim for older engines and its test are removed, along with the README caveat about them. The README documents the `skip` and `warn` statuses and the `warnings` field. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_018xYa7m3qup5wyN5MaXFgf6 --- README.md | 14 ++-- pyproject.toml | 2 +- src/commit_check_mcp/server.py | 49 +++++++------ tests/test_server.py | 124 +++++++++++++++++++++++++++------ uv.lock | 8 +-- 5 files changed, 143 insertions(+), 54 deletions(-) diff --git a/README.md b/README.md index dc086ef..2b21570 100644 --- a/README.md +++ b/README.md @@ -29,11 +29,12 @@ All validation tools return the same structured commit-check result shape: ```json { - "status": "pass|fail", + "status": "pass|fail|skip", + "warnings": 0, "checks": [ { "check": "message", - "status": "pass|fail", + "status": "pass|fail|warn|skip", "value": "...", "error": "...", "suggest": "...", @@ -43,12 +44,17 @@ All validation tools return the same structured commit-check result shape: } ``` +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 +validated nothing is never reported as a pass. A check reports `warn` when the +config lists it under `warn`: the finding is complete, but it does not fail +the run, the top-level `status` stays `pass`, and `warnings` counts them. + `suggest` is the advice a person reads. `fix` is the corrected value itself, present only when the correction is unambiguous — `Fix: add x` comes back with `"fix": "fix: add x"` — and an empty string otherwise, so an agent can apply 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, a `repo_path` that is not a git repository when the tool has to read git diff --git a/pyproject.toml b/pyproject.toml index 8943973..8dc4e06 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -25,7 +25,7 @@ classifiers = [ "Topic :: Software Development", ] dependencies = [ - "commit-check>=2.11.0,<3", + "commit-check>=2.17.0,<3", "mcp>=2,<3" ] dynamic = ["version"] diff --git a/src/commit_check_mcp/server.py b/src/commit_check_mcp/server.py index 4eeb181..60e5c4b 100644 --- a/src/commit_check_mcp/server.py +++ b/src/commit_check_mcp/server.py @@ -11,7 +11,13 @@ from commit_check import __version__ as commit_check_version from commit_check.config_merger import deep_merge, get_default_config, load_toml_config -from commit_check.engine import ValidationContext, ValidationEngine, CheckOutcome +from commit_check.engine import ( + CheckOutcome, + ValidationContext, + ValidationEngine, + count_warnings, + overall_status, +) 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 @@ -159,15 +165,24 @@ def _run_checks( engine = ValidationEngine(filtered) outcomes: list[CheckOutcome] = engine.validate_all_detailed(context) - checks = [o.to_dict() for o in outcomes] - # commit-check names the corrected value in "fix" when a failure has an - # unambiguous one. Older engines have no such field; give the key a - # stable presence so an agent can always test it instead of probing for it. - for check in checks: - check.setdefault("fix", "") + return _summarize([o.to_dict() for o in outcomes]) + - overall = "fail" if any(c["status"] == "fail" for c in checks) else "pass" - return {"status": overall, "checks": checks} +def _summarize(checks: list[dict[str, Any]]) -> dict[str, Any]: + """Wrap per-check results in the shape every validation tool returns. + + The overall ``status`` comes from commit-check's own reducer, so a run in + which every check skipped is reported as ``"skip"`` rather than ``"pass"``: + nothing was validated, and an agent must not read that as approval. A + ``"warn"`` is a finding the config asked to report without enforcing; it + leaves ``status`` at ``"pass"`` and is counted in ``warnings``. + """ + statuses = [c["status"] for c in checks] + return { + "status": overall_status(statuses), + "warnings": count_warnings(statuses), + "checks": checks, + } def _validate_message( @@ -267,11 +282,7 @@ def _validate_author( ValidationContext(stdin_text=email, config=cfg), cfg, ) - checks = name_result["checks"] + email_result["checks"] - return { - "status": "fail" if any(c["status"] == "fail" for c in checks) else "pass", - "checks": checks, - } + return _summarize(name_result["checks"] + email_result["checks"]) check_names: list[str] = [] stdin = None @@ -350,10 +361,7 @@ def _validate_all( )["checks"] ) - return { - "status": "fail" if any(c["status"] == "fail" for c in checks) else "pass", - "checks": checks, - } + return _summarize(checks) @mcp.tool() @@ -620,10 +628,7 @@ def validate_repository_state( )["checks"] ) - return { - "status": "fail" if any(c["status"] == "fail" for c in checks) else "pass", - "checks": checks, - } + return _summarize(checks) @mcp.tool() diff --git a/tests/test_server.py b/tests/test_server.py index ed53fb1..fc8f9cb 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -890,6 +890,107 @@ def fake_run(*, transport: str) -> None: assert called +# --------------------------------------------------------------------------- +# _summarize: the overall status and the warnings count +# --------------------------------------------------------------------------- + + +def _outcome(status: str, check: str = "message") -> object: + class Outcome: + def to_dict(self) -> dict[str, str]: + return { + "check": check, + "status": status, + "value": "x", + "error": "", + "suggest": "", + "fix": "", + } + + return Outcome() + + +class TestSummarize: + def test_a_fully_skipped_run_is_reported_as_skip_not_pass( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Every check skipped means nothing was validated. The old hand-rolled + reducer defaulted anything that was not a failure to "pass", which + told an agent a bypassed policy had been enforced.""" + from commit_check.engine import ValidationContext + + monkeypatch.setattr( + server.ValidationEngine, + "validate_all_detailed", + lambda self, context: [_outcome("skip"), _outcome("skip", "author_name")], + ) + result = server._run_checks( + ["message", "author_name"], ValidationContext(stdin_text="x"), server._merge_config(None) + ) + assert result["status"] == "skip" + assert result["warnings"] == 0 + + def test_one_real_verdict_outweighs_the_skips(self, monkeypatch: pytest.MonkeyPatch) -> None: + from commit_check.engine import ValidationContext + + monkeypatch.setattr( + server.ValidationEngine, + "validate_all_detailed", + lambda self, context: [_outcome("skip"), _outcome("pass", "branch")], + ) + result = server._run_checks( + ["message", "branch"], ValidationContext(stdin_text="x"), server._merge_config(None) + ) + assert result["status"] == "pass" + + def test_a_warned_rule_is_counted_and_does_not_fail_the_run(self) -> None: + """Real engine: the config asks for the message rule as a warning.""" + from commit_check.engine import ValidationContext + + cfg = server._merge_config({"warn": ["message"]}) + result = server._run_checks( + ["message"], ValidationContext(stdin_text="not conventional", config=cfg), cfg + ) + assert result["status"] == "pass" + assert result["warnings"] == 1 + assert result["checks"][0]["status"] == "warn" + assert result["checks"][0]["error"] + + def test_a_failure_still_fails(self) -> None: + from commit_check.engine import ValidationContext + + cfg = server._merge_config(None) + result = server._run_checks( + ["message"], ValidationContext(stdin_text="not conventional", config=cfg), cfg + ) + assert result["status"] == "fail" + assert result["warnings"] == 0 + + def test_the_combined_tools_reduce_with_the_same_rule( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """validate_commit_context, validate_author_info with both inputs and + validate_repository_state used to carry their own copy of the reducer.""" + + def skipped(check_names, context, config): + return server._summarize( + [ + {"check": cn, "status": "skip", "value": "", "error": "", "suggest": "", "fix": ""} + for cn in check_names + ] + ) + + monkeypatch.setattr(server, "_run_checks", skipped) + + context = server._validate_all(message="x", branch="main") + assert context["status"] == "skip" + assert context["warnings"] == 0 + + author = server._validate_author(name="A", email="a@b.c") + assert author["status"] == "skip" + assert "warnings" in author + + # --------------------------------------------------------------------------- # _run_checks: the "fix" key # --------------------------------------------------------------------------- @@ -931,29 +1032,6 @@ def to_dict(self) -> dict[str, str]: assert result["checks"][0]["fix"] == "fix: add x" assert result["checks"][0]["suggest"] == 'Use "fix: add x"' - def test_an_engine_without_the_field_yields_an_empty_fix( - self, monkeypatch: pytest.MonkeyPatch - ) -> None: - from commit_check.engine import ValidationContext - - class OldOutcome: - def to_dict(self) -> dict[str, str]: - return { - "check": "message", - "status": "fail", - "value": "Fix: add x", - "error": "Not conventional", - "suggest": "Use a conventional type", - } - - monkeypatch.setattr( - server.ValidationEngine, "validate_all_detailed", lambda self, context: [OldOutcome()] - ) - result = server._run_checks( - ["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) diff --git a/uv.lock b/uv.lock index 7e29547..8315c18 100644 --- a/uv.lock +++ b/uv.lock @@ -143,14 +143,14 @@ wheels = [ [[package]] name = "commit-check" -version = "2.12.2" +version = "2.17.0" source = { registry = "https://pypi.org/simple" } dependencies = [ { name = "pyyaml" }, { name = "tomli", marker = "python_full_version < '3.11'" }, ] wheels = [ - { url = "https://files.pythonhosted.org/packages/16/8e/e9f05fc05532cf256b8723b9305c81c2ee6cf46d377974296d6682a6a68a/commit_check-2.12.2-py3-none-any.whl", hash = "sha256:cec9f7a5f59005880e983d8b93ce91aa1403e034c4931a363a3c3932a297c27f", size = 42328, upload-time = "2026-07-31T22:31:04.262Z" }, + { url = "https://files.pythonhosted.org/packages/e0/a7/532db04d1a8ea626b529d29d1d8beac768a6b3ff936f0e5f66e0941e8636/commit_check-2.17.0-py3-none-any.whl", hash = "sha256:dcebc3b9fefa53fca3c0a490565ef9f623bfa372ed9968a533413edeab751e98", size = 68964, upload-time = "2026-09-06T09:54:42.556Z" }, ] [[package]] @@ -169,7 +169,7 @@ dev = [ [package.metadata] requires-dist = [ - { name = "commit-check", specifier = ">=2.11.0,<3" }, + { name = "commit-check", specifier = ">=2.17.0,<3" }, { name = "mcp", specifier = ">=2,<3" }, { name = "pytest", marker = "extra == 'dev'", specifier = ">=9.0.0,<10" }, { name = "pytest-cov", marker = "extra == 'dev'", specifier = ">=6.0.0,<8" }, @@ -359,7 +359,7 @@ name = "exceptiongroup" version = "1.3.1" source = { registry = "https://pypi.org/simple" } dependencies = [ - { name = "typing-extensions" }, + { name = "typing-extensions", marker = "python_full_version < '3.13'" }, ] sdist = { url = "https://files.pythonhosted.org/packages/50/79/66800aadf48771f6b62f7eb014e352e5d06856655206165d775e675a02c9/exceptiongroup-1.3.1.tar.gz", hash = "sha256:8b412432c6055b0b7d14c310000ae93352ed6754f70fa8f7c34141f91c4e3219", size = 30371, upload-time = "2025-11-21T23:01:54.787Z" } wheels = [