From db7ff4a5e1658eea30401cfdd93bb5135c5e6d57 Mon Sep 17 00:00:00 2001 From: Dimitris Kargatzis Date: Sun, 1 Mar 2026 15:38:36 +0200 Subject: [PATCH 1/6] feat: add LLM-backed DescriptionDiffAlignmentCondition First LLM-assisted condition in Watchflow. Uses the configured AI provider to verify that the PR description semantically matches the actual code diff. Flags mismatches like 'description says fix login but diff only touches billing code.' - New src/rules/conditions/llm_assisted.py with structured output (AlignmentVerdict Pydantic model) and graceful degradation on LLM failure (logs warning, returns no violation) - Registered in ConditionRegistry, RuleID enum, acknowledgment mappings, and conditions/__init__.py - 10 unit tests covering: aligned, misaligned, empty description, LLM failure, provider error, file list truncation - Enabled in .watchflow/rules.yaml for dogfooding - Updated README, features.md, configuration.md, overview.md, and CHANGELOG.md --- .watchflow/rules.yaml | 7 + CHANGELOG.md | 10 + README.md | 1 + docs/concepts/overview.md | 2 +- docs/features.md | 6 + docs/getting-started/configuration.md | 11 + src/rules/acknowledgment.py | 3 + src/rules/conditions/__init__.py | 3 + src/rules/conditions/llm_assisted.py | 146 +++++++++++++ src/rules/registry.py | 3 + .../rules/conditions/test_llm_assisted.py | 199 ++++++++++++++++++ tests/unit/rules/test_acknowledgment.py | 6 +- 12 files changed, 395 insertions(+), 2 deletions(-) create mode 100644 src/rules/conditions/llm_assisted.py create mode 100644 tests/unit/rules/conditions/test_llm_assisted.py diff --git a/.watchflow/rules.yaml b/.watchflow/rules.yaml index b4b9d88..21d4844 100644 --- a/.watchflow/rules.yaml +++ b/.watchflow/rules.yaml @@ -41,3 +41,10 @@ rules: event_types: ["pull_request"] parameters: block_on_unresolved_comments: true + + - description: "PR description must accurately reflect the actual code changes in the diff." + enabled: true + severity: "medium" + event_types: ["pull_request"] + parameters: + require_description_diff_alignment: true diff --git a/CHANGELOG.md b/CHANGELOG.md index 122b4e9..aee2129 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,16 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). +## [Unreleased] + +### Added + +- **Description-diff alignment** -- `DescriptionDiffAlignmentCondition` uses + the configured AI provider (OpenAI / Bedrock / Vertex AI) to verify that + the PR description semantically matches the actual code changes. First + LLM-backed condition in Watchflow; adds ~1-3s latency. Gracefully skips + (no violation) if the LLM is unavailable. + ## [2026-03-01] -- PR #59 ### Added diff --git a/README.md b/README.md index d4365e7..76b5f88 100644 --- a/README.md +++ b/README.md @@ -65,6 +65,7 @@ Rules are **description + event_types + parameters**. The engine matches paramet | **Push** | `no_force_push: true` | push | Reject force pushes. | | **Files** | `max_file_size_mb: 1` | pull_request | No single file > N MB. | | **Files** | `pattern` + `condition_type: "files_match_pattern"` | pull_request | Changed files must (or must not) match glob/regex. | +| **PR** | `require_description_diff_alignment: true` | pull_request | Description must match code changes (LLM-assisted). | | **Time** | `allowed_hours`, `days`, weekend | deployment / workflow | Restrict when actions can run. | Rules are read from the **default branch** (e.g. `main`). Each webhook delivery is deduplicated by `X-GitHub-Delivery` so handler and processor both run; comments and check runs stay in sync. diff --git a/docs/concepts/overview.md b/docs/concepts/overview.md index 9163ddd..b20ad21 100644 --- a/docs/concepts/overview.md +++ b/docs/concepts/overview.md @@ -41,7 +41,7 @@ graph TD ### Condition registry - Maps parameter names to condition classes (e.g. `require_linked_issue` → `RequireLinkedIssueCondition`, `max_lines` → `MaxPrLocCondition`, `require_code_owner_reviewers` → `RequireCodeOwnerReviewersCondition`). -- Supported conditions: linked issue, title pattern, description length, labels, approvals, PR size (lines), CODEOWNERS (path has owner, require owners as reviewers), protected branches, no force push, file size, file pattern, diff pattern scanning, security pattern detection, unresolved comments, test coverage, comment response SLA, signed commits, changelog required, self-approval prevention, cross-team approval, time/deploy rules. See [Configuration](../getting-started/configuration.md). +- Supported conditions: linked issue, title pattern, description length, labels, approvals, PR size (lines), CODEOWNERS (path has owner, require owners as reviewers), protected branches, no force push, file size, file pattern, diff pattern scanning, security pattern detection, unresolved comments, test coverage, comment response SLA, signed commits, changelog required, self-approval prevention, cross-team approval, description-diff alignment (LLM-assisted), time/deploy rules. See [Configuration](../getting-started/configuration.md). ### PR enricher diff --git a/docs/features.md b/docs/features.md index e1855d3..2de2259 100644 --- a/docs/features.md +++ b/docs/features.md @@ -57,6 +57,12 @@ Rules are **description + event_types + parameters**. The engine matches **param | `require_signed_commits: true` | SignedCommitsCondition | All commits must be cryptographically signed (GPG/SSH/S/MIME). | | `require_changelog_update: true` | ChangelogRequiredCondition | Source changes must include a CHANGELOG or `.changeset` update. | +### LLM-assisted + +| Parameter | Condition | Description | +|-----------|-----------|-------------| +| `require_description_diff_alignment: true` | DescriptionDiffAlignmentCondition | PR description must semantically match the code diff (uses LLM; ~1-3s latency). | + --- ## Repository analysis → one-click rules PR diff --git a/docs/getting-started/configuration.md b/docs/getting-started/configuration.md index 8804c2f..99398ed 100644 --- a/docs/getting-started/configuration.md +++ b/docs/getting-started/configuration.md @@ -238,6 +238,17 @@ parameters: PRs that modify source code must include a corresponding `CHANGELOG.md` or `.changeset/` update. Docs, tests, and `.github/` paths are excluded. +### LLM-assisted conditions + +**Description-diff alignment** + +```yaml +parameters: + require_description_diff_alignment: true +``` + +Uses the configured AI provider to check whether the PR description semantically reflects the actual code changes. Flags mismatches like "description says fix login but diff only touches billing code." Adds ~1-3s latency per evaluation. If the LLM is unavailable (provider not configured, rate limit), the condition gracefully skips without blocking the PR. + --- ## Example rules diff --git a/src/rules/acknowledgment.py b/src/rules/acknowledgment.py index 86931b2..ccbbbbf 100644 --- a/src/rules/acknowledgment.py +++ b/src/rules/acknowledgment.py @@ -43,6 +43,7 @@ class RuleID(StrEnum): CHANGELOG_REQUIRED = "changelog-required" NO_SELF_APPROVAL = "no-self-approval" CROSS_TEAM_APPROVAL = "cross-team-approval" + DESCRIPTION_DIFF_ALIGNMENT = "description-diff-alignment" # Mapping from violation text patterns to RuleID @@ -67,6 +68,7 @@ class RuleID(StrEnum): "without a corresponding CHANGELOG": RuleID.CHANGELOG_REQUIRED, "approved by its own author": RuleID.NO_SELF_APPROVAL, "approvals from required teams": RuleID.CROSS_TEAM_APPROVAL, + "does not align with code changes": RuleID.DESCRIPTION_DIFF_ALIGNMENT, } # Mapping from RuleID to human-readable descriptions @@ -91,6 +93,7 @@ class RuleID(StrEnum): RuleID.CHANGELOG_REQUIRED: "Source code changes must include a CHANGELOG or .changeset update.", RuleID.NO_SELF_APPROVAL: "PR authors cannot approve their own pull requests.", RuleID.CROSS_TEAM_APPROVAL: "Pull requests require approvals from specified GitHub teams.", + RuleID.DESCRIPTION_DIFF_ALIGNMENT: "PR description must accurately reflect the actual code changes.", } # Comment markers that indicate an acknowledgment comment diff --git a/src/rules/conditions/__init__.py b/src/rules/conditions/__init__.py index adb2b88..1e5e14c 100644 --- a/src/rules/conditions/__init__.py +++ b/src/rules/conditions/__init__.py @@ -26,6 +26,7 @@ MaxPrLocCondition, TestCoverageCondition, ) +from src.rules.conditions.llm_assisted import DescriptionDiffAlignmentCondition from src.rules.conditions.pull_request import ( DiffPatternCondition, MinDescriptionLengthCondition, @@ -71,6 +72,8 @@ # Compliance "SignedCommitsCondition", "ChangelogRequiredCondition", + # LLM-assisted + "DescriptionDiffAlignmentCondition", # Temporal "AllowedHoursCondition", "CommentResponseTimeCondition", diff --git a/src/rules/conditions/llm_assisted.py b/src/rules/conditions/llm_assisted.py new file mode 100644 index 0000000..da59711 --- /dev/null +++ b/src/rules/conditions/llm_assisted.py @@ -0,0 +1,146 @@ +"""LLM-assisted conditions for semantic rule evaluation. + +This module contains conditions that use an LLM to perform evaluations +that cannot be expressed as deterministic checks. These conditions are +opt-in and clearly documented as having LLM latency in the evaluation path. +""" + +import logging +from typing import Any + +from pydantic import BaseModel, Field + +from src.core.models import Severity, Violation +from src.rules.conditions.base import BaseCondition + +logger = logging.getLogger(__name__) + + +class AlignmentVerdict(BaseModel): + """Structured LLM response for description-diff alignment evaluation.""" + + is_aligned: bool = Field(description="Whether the PR description accurately reflects the code changes") + reason: str = Field(description="Brief explanation of the alignment or mismatch") + how_to_fix: str | None = Field( + description="Actionable suggestion for improving the description (only if misaligned)", default=None + ) + + +_SYSTEM_PROMPT = """\ +You are a senior code reviewer evaluating whether a pull request description \ +accurately reflects the actual code changes shown in the diff. + +Guidelines: +- A description is "aligned" if it describes the INTENT and SCOPE of the \ +changes, even if it does not list every file. +- Minor omissions are acceptable (e.g., not mentioning a test file that \ +accompanies a feature). Focus on whether the description would mislead a reviewer. +- Flag clear mismatches: description says "fix login bug" but diff only touches \ +billing code; description claims refactoring but diff adds a new feature; \ +description is entirely generic ("update code") with no mention of what changed. +- If the description is empty or trivially short (e.g. "fix", "update"), treat \ +it as misaligned. +- Respond with structured output only. Do NOT include markdown or extra text.""" + +_HUMAN_PROMPT_TEMPLATE = """\ +## PR title +{title} + +## PR description +{description} + +## Diff summary (top changed files) +{diff_summary} + +## Changed file list +{file_list} + +Evaluate whether the PR description aligns with the actual code changes.""" + + +class DescriptionDiffAlignmentCondition(BaseCondition): + """Validates that the PR description semantically matches the code diff. + + This is the first LLM-backed condition in Watchflow. It uses the configured + AI provider (OpenAI / Bedrock / Vertex AI) to compare the PR description + against the diff summary and flag mismatches. Because it calls an LLM, it + adds latency (~1-3s) compared to deterministic conditions. + + The condition gracefully degrades: if the LLM call fails (provider not + configured, rate limit, network error), it logs a warning and returns no + violation rather than blocking the PR. + """ + + name = "description_diff_alignment" + description = "Validates that the PR description accurately reflects the actual code changes." + parameter_patterns = ["require_description_diff_alignment"] + event_types = ["pull_request"] + examples = [{"require_description_diff_alignment": True}] + + async def evaluate(self, context: Any) -> list[Violation]: + """Evaluate description-diff alignment using an LLM.""" + parameters = context.get("parameters", {}) + event = context.get("event", {}) + + if not parameters.get("require_description_diff_alignment"): + return [] + + pr_details = event.get("pull_request_details", {}) + title = pr_details.get("title", "") + description_body = pr_details.get("body") or "" + diff_summary = event.get("diff_summary", "") + changed_files = event.get("changed_files", []) + + # Nothing to compare against + if not changed_files: + return [] + + file_list = "\n".join( + f"- {f.get('filename', '?')} ({f.get('status', '?')}, " + f"+{f.get('additions', 0)}/-{f.get('deletions', 0)})" + for f in changed_files[:20] + ) + + human_prompt = _HUMAN_PROMPT_TEMPLATE.format( + title=title or "(no title)", + description=description_body or "(empty)", + diff_summary=diff_summary or "(no diff summary available)", + file_list=file_list, + ) + + try: + from langchain_core.messages import HumanMessage, SystemMessage + + from src.integrations.providers import get_chat_model + + llm = get_chat_model( + temperature=0.0, + max_tokens=512, + ) + structured_llm = llm.with_structured_output(AlignmentVerdict, method="function_calling") + + messages = [ + SystemMessage(content=_SYSTEM_PROMPT), + HumanMessage(content=human_prompt), + ] + + verdict: AlignmentVerdict = await structured_llm.ainvoke(messages) + + if not verdict.is_aligned: + return [ + Violation( + rule_description=self.description, + severity=Severity.MEDIUM, + message=f"PR description does not align with code changes: {verdict.reason}", + how_to_fix=verdict.how_to_fix + or "Update the PR description to accurately summarize the intent and scope of the code changes.", + ) + ] + + except Exception: + logger.warning( + "LLM call failed for description-diff alignment check; skipping.", + exc_info=True, + ) + + return [] diff --git a/src/rules/registry.py b/src/rules/registry.py index 12d52d4..4a165d0 100644 --- a/src/rules/registry.py +++ b/src/rules/registry.py @@ -31,6 +31,7 @@ MaxPrLocCondition, TestCoverageCondition, ) +from src.rules.conditions.llm_assisted import DescriptionDiffAlignmentCondition from src.rules.conditions.pull_request import ( DiffPatternCondition, MinApprovalsCondition, @@ -73,6 +74,7 @@ RuleID.CHANGELOG_REQUIRED: ChangelogRequiredCondition, RuleID.NO_SELF_APPROVAL: NoSelfApprovalCondition, RuleID.CROSS_TEAM_APPROVAL: CrossTeamApprovalCondition, + RuleID.DESCRIPTION_DIFF_ALIGNMENT: DescriptionDiffAlignmentCondition, } # Reverse map: condition class -> RuleID (for populating rule_id on violations) @@ -106,6 +108,7 @@ CrossTeamApprovalCondition, SignedCommitsCondition, ChangelogRequiredCondition, + DescriptionDiffAlignmentCondition, ] diff --git a/tests/unit/rules/conditions/test_llm_assisted.py b/tests/unit/rules/conditions/test_llm_assisted.py new file mode 100644 index 0000000..506c1a2 --- /dev/null +++ b/tests/unit/rules/conditions/test_llm_assisted.py @@ -0,0 +1,199 @@ +"""Tests for LLM-assisted conditions.""" + +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from src.rules.conditions.llm_assisted import ( + AlignmentVerdict, + DescriptionDiffAlignmentCondition, +) + + +@pytest.fixture +def condition(): + return DescriptionDiffAlignmentCondition() + + +def _make_context( + description="Fix login bug by correcting session validation", + title="fix: resolve login session timeout", + diff_summary="- src/auth/session.py (modified, +10/-3)\n +validate_session()", + changed_files=None, + require=True, +): + if changed_files is None: + changed_files = [ + {"filename": "src/auth/session.py", "status": "modified", "additions": 10, "deletions": 3, "patch": ""}, + ] + return { + "parameters": {"require_description_diff_alignment": require}, + "event": { + "pull_request_details": {"title": title, "body": description}, + "diff_summary": diff_summary, + "changed_files": changed_files, + }, + } + + +class TestDescriptionDiffAlignmentCondition: + """Tests for DescriptionDiffAlignmentCondition.""" + + def test_class_attributes(self, condition): + assert condition.name == "description_diff_alignment" + assert "require_description_diff_alignment" in condition.parameter_patterns + assert "pull_request" in condition.event_types + + @pytest.mark.asyncio + async def test_skips_when_disabled(self, condition): + context = _make_context(require=False) + violations = await condition.evaluate(context) + assert violations == [] + + @pytest.mark.asyncio + async def test_skips_when_no_changed_files(self, condition): + context = _make_context(changed_files=[]) + violations = await condition.evaluate(context) + assert violations == [] + + @pytest.mark.asyncio + @patch("src.integrations.providers.get_chat_model") + async def test_no_violation_when_aligned(self, mock_get_chat_model, condition): + """LLM says description is aligned -> no violation.""" + verdict = AlignmentVerdict(is_aligned=True, reason="Description matches diff.", how_to_fix=None) + + mock_llm = MagicMock() + mock_structured = MagicMock() + mock_structured.ainvoke = AsyncMock(return_value=verdict) + mock_llm.with_structured_output.return_value = mock_structured + mock_get_chat_model.return_value = mock_llm + + context = _make_context() + violations = await condition.evaluate(context) + + assert violations == [] + mock_get_chat_model.assert_called_once_with(temperature=0.0, max_tokens=512) + mock_structured.ainvoke.assert_awaited_once() + + @pytest.mark.asyncio + @patch("src.integrations.providers.get_chat_model") + async def test_violation_when_misaligned(self, mock_get_chat_model, condition): + """LLM says description is misaligned -> violation with reason.""" + verdict = AlignmentVerdict( + is_aligned=False, + reason="Description says 'fix login' but diff only touches billing code.", + how_to_fix="Update the description to mention billing changes.", + ) + + mock_llm = MagicMock() + mock_structured = MagicMock() + mock_structured.ainvoke = AsyncMock(return_value=verdict) + mock_llm.with_structured_output.return_value = mock_structured + mock_get_chat_model.return_value = mock_llm + + context = _make_context( + description="Fix login bug", + changed_files=[ + {"filename": "src/billing/invoice.py", "status": "modified", "additions": 50, "deletions": 10}, + ], + ) + violations = await condition.evaluate(context) + + assert len(violations) == 1 + assert "does not align with code changes" in violations[0].message + assert "billing" in violations[0].message + assert violations[0].how_to_fix == "Update the description to mention billing changes." + assert violations[0].severity.value == "medium" + + @pytest.mark.asyncio + @patch("src.integrations.providers.get_chat_model") + async def test_violation_uses_default_how_to_fix(self, mock_get_chat_model, condition): + """When LLM returns no how_to_fix, a sensible default is used.""" + verdict = AlignmentVerdict(is_aligned=False, reason="Generic description.", how_to_fix=None) + + mock_llm = MagicMock() + mock_structured = MagicMock() + mock_structured.ainvoke = AsyncMock(return_value=verdict) + mock_llm.with_structured_output.return_value = mock_structured + mock_get_chat_model.return_value = mock_llm + + context = _make_context(description="update code") + violations = await condition.evaluate(context) + + assert len(violations) == 1 + assert "accurately summarize" in violations[0].how_to_fix + + @pytest.mark.asyncio + @patch("src.integrations.providers.get_chat_model") + async def test_graceful_degradation_on_llm_failure(self, mock_get_chat_model, condition): + """When LLM call fails, condition returns no violation (fail-open).""" + mock_get_chat_model.side_effect = Exception("Provider not configured") + + context = _make_context() + violations = await condition.evaluate(context) + + assert violations == [] + + @pytest.mark.asyncio + @patch("src.integrations.providers.get_chat_model") + async def test_graceful_degradation_on_invoke_failure(self, mock_get_chat_model, condition): + """When structured invoke fails, condition returns no violation.""" + mock_llm = MagicMock() + mock_structured = MagicMock() + mock_structured.ainvoke = AsyncMock(side_effect=RuntimeError("Rate limited")) + mock_llm.with_structured_output.return_value = mock_structured + mock_get_chat_model.return_value = mock_llm + + context = _make_context() + violations = await condition.evaluate(context) + + assert violations == [] + + @pytest.mark.asyncio + @patch("src.integrations.providers.get_chat_model") + async def test_empty_description_sent_to_llm(self, mock_get_chat_model, condition): + """Empty description is forwarded as '(empty)' so the LLM can flag it.""" + verdict = AlignmentVerdict( + is_aligned=False, + reason="PR description is empty.", + how_to_fix="Add a description.", + ) + + mock_llm = MagicMock() + mock_structured = MagicMock() + mock_structured.ainvoke = AsyncMock(return_value=verdict) + mock_llm.with_structured_output.return_value = mock_structured + mock_get_chat_model.return_value = mock_llm + + context = _make_context(description="") + violations = await condition.evaluate(context) + + assert len(violations) == 1 + # Verify "(empty)" was in the prompt + call_messages = mock_structured.ainvoke.call_args[0][0] + human_msg = call_messages[1].content + assert "(empty)" in human_msg + + @pytest.mark.asyncio + @patch("src.integrations.providers.get_chat_model") + async def test_file_list_truncated_to_20(self, mock_get_chat_model, condition): + """File list sent to LLM is capped at 20 entries.""" + verdict = AlignmentVerdict(is_aligned=True, reason="OK", how_to_fix=None) + + mock_llm = MagicMock() + mock_structured = MagicMock() + mock_structured.ainvoke = AsyncMock(return_value=verdict) + mock_llm.with_structured_output.return_value = mock_structured + mock_get_chat_model.return_value = mock_llm + + files = [ + {"filename": f"src/file_{i}.py", "status": "modified", "additions": 1, "deletions": 0} + for i in range(50) + ] + context = _make_context(changed_files=files) + await condition.evaluate(context) + + call_messages = mock_structured.ainvoke.call_args[0][0] + human_msg = call_messages[1].content + assert "file_19.py" in human_msg + assert "file_20.py" not in human_msg diff --git a/tests/unit/rules/test_acknowledgment.py b/tests/unit/rules/test_acknowledgment.py index 53ed3e3..0ee77c2 100644 --- a/tests/unit/rules/test_acknowledgment.py +++ b/tests/unit/rules/test_acknowledgment.py @@ -36,7 +36,7 @@ def test_all_rule_ids_are_strings(self): def test_rule_id_count(self): """Verify we have exactly 20 standardized rule IDs.""" - assert len(RuleID) == 20 + assert len(RuleID) == 21 def test_all_rule_ids_have_descriptions(self): """Every RuleID should have a corresponding description.""" @@ -169,6 +169,10 @@ class TestMapViolationTextToRuleId: ), ("Pull request was approved by its own author.", RuleID.NO_SELF_APPROVAL), ("Missing approvals from required teams: @org/security, @org/qa", RuleID.CROSS_TEAM_APPROVAL), + ( + "PR description does not align with code changes: desc says X but diff does Y", + RuleID.DESCRIPTION_DIFF_ALIGNMENT, + ), ], ) def test_maps_violation_text_correctly(self, text: str, expected_rule_id: RuleID): From db223d30661a5222c62291dbf49ec89992f050f0 Mon Sep 17 00:00:00 2001 From: Dimitris Kargatzis Date: Sun, 1 Mar 2026 19:30:23 +0200 Subject: [PATCH 2/6] fix: implement CodeRabbit review feedback for DescriptionDiffAlignmentCondition - Extend AlignmentVerdict to standard agent output schema with decision, confidence, reasoning, recommendations, and strategy_used fields - Add _truncate_text() helper to sanitize inputs (max 2000 chars for description/diff, 200 for title) preventing prompt injection - Check provider.supports_structured_output before invoking to handle providers without function calling capability - Implement exponential backoff retry loop (3 attempts: 2s, 4s, 8s waits) with structured logging including latency_ms and attempt count - Add human-in-the-loop gating: confidence < 0.5 flags for manual review - Add comprehensive test coverage for truncation, retry logic, low confidence fallback, malformed output, and unsupported provider - Update docs/concepts/overview.md to clarify LLM use is opt-in and LLM-assisted conditions gracefully degrade on failure - Fix stale docstring in test_acknowledgment.py (21 rule IDs, not 20) --- docs/concepts/overview.md | 6 +- src/rules/conditions/llm_assisted.py | 165 +++++++++++++----- .../rules/conditions/test_llm_assisted.py | 161 +++++++++++++++-- tests/unit/rules/test_acknowledgment.py | 2 +- 4 files changed, 275 insertions(+), 59 deletions(-) diff --git a/docs/concepts/overview.md b/docs/concepts/overview.md index b20ad21..2e01c8f 100644 --- a/docs/concepts/overview.md +++ b/docs/concepts/overview.md @@ -5,7 +5,7 @@ Watchflow is a **rule engine** for GitHub: you define rules in YAML; we evaluate ## Design principles - **Repo-native** — Rules live in `.watchflow/rules.yaml` on the default branch; same mental model as branch protection and CODEOWNERS. -- **Condition-based enforcement** — Rule evaluation is deterministic: parameters map to conditions (e.g. `require_linked_issue`, `max_lines`, `require_code_owner_reviewers`). No LLM in the hot path for “did this PR violate the rule?” +- **Condition-based enforcement** — Rule evaluation is deterministic by default: parameters map to conditions (e.g. `require_linked_issue`, `max_lines`, `require_code_owner_reviewers`). Optional LLM-assisted conditions (e.g. `require_description_diff_alignment`) are clearly documented and opt-in. - **Webhook-first** — Each delivery is identified by `X-GitHub-Delivery`; handler and processor get distinct task IDs so both run and comments/check runs stay in sync. - **Optional intelligence** — Repo analysis and feasibility checks use LLMs to *suggest* rules; enforcement stays rule-driven. @@ -53,12 +53,12 @@ graph TD ## Where AI is used (and where it isn’t) -- **Rule evaluation** — No. Violations are determined by conditions only. +- **Rule evaluation** — No LLM by default. Violations are determined by deterministic conditions. Optional LLM-assisted conditions (e.g. `DescriptionDiffAlignmentCondition`) are clearly marked and gracefully degrade on failure. - **Acknowledgment parsing** — Optional LLM to interpret reason; can be extended. - **Repo analysis** — Yes. `POST /api/v1/rules/recommend` uses an agent to suggest rules from repo structure and PR history; you copy/paste or create a PR. - **Feasibility** — Yes. “Can I enforce this rule?” uses an agent to map natural language to supported conditions and suggest YAML. -So: **enforcement is deterministic and condition-based**; **suggestions and feasibility are agent-assisted**. That keeps the hot path simple and auditable. +So: **enforcement is deterministic and condition-based by default**; **LLM-assisted conditions are opt-in and fail-open**; **suggestions and feasibility are agent-assisted**. That keeps the hot path simple and auditable. ## Use cases diff --git a/src/rules/conditions/llm_assisted.py b/src/rules/conditions/llm_assisted.py index da59711..7c6f346 100644 --- a/src/rules/conditions/llm_assisted.py +++ b/src/rules/conditions/llm_assisted.py @@ -6,6 +6,7 @@ """ import logging +import time from typing import Any from pydantic import BaseModel, Field @@ -16,14 +17,35 @@ logger = logging.getLogger(__name__) +def _truncate_text(text: str, max_length: int = 2000) -> str: + """Truncate text to prevent excessively large prompts and potential injection. + + Args: + text: The text to truncate + max_length: Maximum length in characters (default: 2000) + + Returns: + Truncated text with ellipsis if needed + """ + if len(text) <= max_length: + return text + return text[:max_length] + "... [truncated]" + + class AlignmentVerdict(BaseModel): - """Structured LLM response for description-diff alignment evaluation.""" + """Structured LLM response for description-diff alignment evaluation. + + This follows the standard agent output schema for consistency across + LLM-assisted conditions. + """ - is_aligned: bool = Field(description="Whether the PR description accurately reflects the code changes") - reason: str = Field(description="Brief explanation of the alignment or mismatch") - how_to_fix: str | None = Field( - description="Actionable suggestion for improving the description (only if misaligned)", default=None + decision: str = Field(description="Whether the description is 'aligned' or 'misaligned'") + confidence: float = Field(description="Confidence score between 0.0 and 1.0", ge=0.0, le=1.0) + reasoning: str = Field(description="Brief explanation of the alignment or mismatch") + recommendations: list[str] | None = Field( + description="Actionable suggestions for improving the description (only if misaligned)", default=None ) + strategy_used: str = Field(description="The strategy used to evaluate the description") _SYSTEM_PROMPT = """\ @@ -78,7 +100,7 @@ class DescriptionDiffAlignmentCondition(BaseCondition): examples = [{"require_description_diff_alignment": True}] async def evaluate(self, context: Any) -> list[Violation]: - """Evaluate description-diff alignment using an LLM.""" + """Evaluate description-diff alignment using an LLM with retries and graceful degradation.""" parameters = context.get("parameters", {}) event = context.get("event", {}) @@ -95,6 +117,11 @@ async def evaluate(self, context: Any) -> list[Violation]: if not changed_files: return [] + # Truncate inputs to prevent prompt injection and token overflow + title_sanitized = _truncate_text(title, 200) + description_sanitized = _truncate_text(description_body, 2000) + diff_sanitized = _truncate_text(diff_summary, 2000) + file_list = "\n".join( f"- {f.get('filename', '?')} ({f.get('status', '?')}, " f"+{f.get('additions', 0)}/-{f.get('deletions', 0)})" @@ -102,45 +129,101 @@ async def evaluate(self, context: Any) -> list[Violation]: ) human_prompt = _HUMAN_PROMPT_TEMPLATE.format( - title=title or "(no title)", - description=description_body or "(empty)", - diff_summary=diff_summary or "(no diff summary available)", + title=title_sanitized or "(no title)", + description=description_sanitized or "(empty)", + diff_summary=diff_sanitized or "(no diff summary available)", file_list=file_list, ) - try: - from langchain_core.messages import HumanMessage, SystemMessage - - from src.integrations.providers import get_chat_model - - llm = get_chat_model( - temperature=0.0, - max_tokens=512, - ) - structured_llm = llm.with_structured_output(AlignmentVerdict, method="function_calling") - - messages = [ - SystemMessage(content=_SYSTEM_PROMPT), - HumanMessage(content=human_prompt), - ] - - verdict: AlignmentVerdict = await structured_llm.ainvoke(messages) - - if not verdict.is_aligned: - return [ - Violation( - rule_description=self.description, - severity=Severity.MEDIUM, - message=f"PR description does not align with code changes: {verdict.reason}", - how_to_fix=verdict.how_to_fix - or "Update the PR description to accurately summarize the intent and scope of the code changes.", - ) + # Retry loop with exponential backoff + max_attempts = 3 + for attempt in range(1, max_attempts + 1): + try: + from langchain_core.messages import HumanMessage, SystemMessage + + from src.integrations.providers import get_chat_model + + llm = get_chat_model( + temperature=0.0, + max_tokens=512, + ) + + # Check if provider supports structured output + supports_structured = hasattr(llm, "with_structured_output") and callable( + getattr(llm, "with_structured_output") + ) + + if not supports_structured: + logger.warning("Provider does not support structured output; skipping alignment check.") + return [] + + structured_llm = llm.with_structured_output(AlignmentVerdict, method="function_calling") + + messages = [ + SystemMessage(content=_SYSTEM_PROMPT), + HumanMessage(content=human_prompt), ] - except Exception: - logger.warning( - "LLM call failed for description-diff alignment check; skipping.", - exc_info=True, - ) + start_time = time.time() + verdict: AlignmentVerdict = await structured_llm.ainvoke(messages) + latency_ms = int((time.time() - start_time) * 1000) + + logger.info( + "LLM alignment check completed", + extra={ + "attempt": attempt, + "latency_ms": latency_ms, + "decision": getattr(verdict, "decision", "unknown"), + "confidence": getattr(verdict, "confidence", 0.0), + }, + ) + + # Validate response type + if not isinstance(verdict, AlignmentVerdict): + logger.warning("LLM returned unexpected type; skipping.") + return [] + + # Human-in-the-loop fallback for low confidence + if verdict.confidence < 0.5: + logger.info(f"Low confidence ({verdict.confidence:.2f}); flagging for human review.") + return [ + Violation( + rule_description=self.description, + severity=Severity.MEDIUM, + message=f"LLM confidence is low ({verdict.confidence:.1%}), requiring human review. Reasoning: {verdict.reasoning}", + how_to_fix="Manually review the PR description to ensure it aligns with the code changes.", + ) + ] + + # Check alignment decision + if verdict.decision == "misaligned": + recommendation = verdict.recommendations[0] if verdict.recommendations else None + return [ + Violation( + rule_description=self.description, + severity=Severity.MEDIUM, + message=f"PR description does not align with code changes: {verdict.reasoning}", + how_to_fix=recommendation + or "Update the PR description to accurately summarize the intent and scope of the code changes.", + ) + ] + + # Success: aligned + return [] + + except Exception as e: + wait_time = 2**attempt # Exponential backoff: 2s, 4s, 8s + logger.warning( + f"LLM call failed (attempt {attempt}/{max_attempts})", + extra={"error": str(e), "retry_in_seconds": wait_time if attempt < max_attempts else None}, + exc_info=True, + ) + + if attempt < max_attempts: + time.sleep(wait_time) + else: + # All attempts failed - gracefully degrade + logger.error("All LLM retry attempts exhausted; skipping alignment check.") + return [] return [] diff --git a/tests/unit/rules/conditions/test_llm_assisted.py b/tests/unit/rules/conditions/test_llm_assisted.py index 506c1a2..70aa4ac 100644 --- a/tests/unit/rules/conditions/test_llm_assisted.py +++ b/tests/unit/rules/conditions/test_llm_assisted.py @@ -7,6 +7,7 @@ from src.rules.conditions.llm_assisted import ( AlignmentVerdict, DescriptionDiffAlignmentCondition, + _truncate_text, ) @@ -36,6 +37,27 @@ def _make_context( } +class TestTruncateText: + """Tests for _truncate_text helper function.""" + + def test_no_truncation_when_under_limit(self): + text = "Short text" + assert _truncate_text(text, 100) == "Short text" + + def test_truncation_when_over_limit(self): + text = "a" * 2500 + result = _truncate_text(text, 2000) + assert len(result) <= 2000 + len("... [truncated]") + assert result.endswith("... [truncated]") + assert result.startswith("a" * 100) + + def test_default_max_length(self): + text = "b" * 3000 + result = _truncate_text(text) + assert result.endswith("... [truncated]") + assert len(result) <= 2000 + len("... [truncated]") + + class TestDescriptionDiffAlignmentCondition: """Tests for DescriptionDiffAlignmentCondition.""" @@ -60,7 +82,13 @@ async def test_skips_when_no_changed_files(self, condition): @patch("src.integrations.providers.get_chat_model") async def test_no_violation_when_aligned(self, mock_get_chat_model, condition): """LLM says description is aligned -> no violation.""" - verdict = AlignmentVerdict(is_aligned=True, reason="Description matches diff.", how_to_fix=None) + verdict = AlignmentVerdict( + decision="aligned", + confidence=0.9, + reasoning="Description matches diff.", + recommendations=None, + strategy_used="semantic_comparison", + ) mock_llm = MagicMock() mock_structured = MagicMock() @@ -80,9 +108,11 @@ async def test_no_violation_when_aligned(self, mock_get_chat_model, condition): async def test_violation_when_misaligned(self, mock_get_chat_model, condition): """LLM says description is misaligned -> violation with reason.""" verdict = AlignmentVerdict( - is_aligned=False, - reason="Description says 'fix login' but diff only touches billing code.", - how_to_fix="Update the description to mention billing changes.", + decision="misaligned", + confidence=0.9, + reasoning="Description says 'fix login' but diff only touches billing code.", + recommendations=["Update the description to mention billing changes."], + strategy_used="semantic_comparison", ) mock_llm = MagicMock() @@ -108,8 +138,14 @@ async def test_violation_when_misaligned(self, mock_get_chat_model, condition): @pytest.mark.asyncio @patch("src.integrations.providers.get_chat_model") async def test_violation_uses_default_how_to_fix(self, mock_get_chat_model, condition): - """When LLM returns no how_to_fix, a sensible default is used.""" - verdict = AlignmentVerdict(is_aligned=False, reason="Generic description.", how_to_fix=None) + """When LLM returns no recommendations, a sensible default is used.""" + verdict = AlignmentVerdict( + decision="misaligned", + confidence=0.8, + reasoning="Generic description.", + recommendations=None, + strategy_used="semantic_comparison", + ) mock_llm = MagicMock() mock_structured = MagicMock() @@ -123,6 +159,31 @@ async def test_violation_uses_default_how_to_fix(self, mock_get_chat_model, cond assert len(violations) == 1 assert "accurately summarize" in violations[0].how_to_fix + @pytest.mark.asyncio + @patch("src.integrations.providers.get_chat_model") + async def test_human_in_the_loop_for_low_confidence(self, mock_get_chat_model, condition): + """When confidence < 0.5, requires human review.""" + verdict = AlignmentVerdict( + decision="aligned", + confidence=0.4, + reasoning="Uncertain about alignment due to vague description.", + recommendations=None, + strategy_used="semantic_comparison", + ) + + mock_llm = MagicMock() + mock_structured = MagicMock() + mock_structured.ainvoke = AsyncMock(return_value=verdict) + mock_llm.with_structured_output.return_value = mock_structured + mock_get_chat_model.return_value = mock_llm + + context = _make_context() + violations = await condition.evaluate(context) + + assert len(violations) == 1 + assert "human review" in violations[0].message.lower() + assert "40.0%" in violations[0].message or "0.4" in violations[0].message + @pytest.mark.asyncio @patch("src.integrations.providers.get_chat_model") async def test_graceful_degradation_on_llm_failure(self, mock_get_chat_model, condition): @@ -135,9 +196,10 @@ async def test_graceful_degradation_on_llm_failure(self, mock_get_chat_model, co assert violations == [] @pytest.mark.asyncio + @patch("time.sleep", return_value=None) # Mock sleep to speed up test @patch("src.integrations.providers.get_chat_model") - async def test_graceful_degradation_on_invoke_failure(self, mock_get_chat_model, condition): - """When structured invoke fails, condition returns no violation.""" + async def test_retry_logic_with_exponential_backoff(self, mock_get_chat_model, mock_sleep, condition): + """When structured invoke fails, retries with exponential backoff.""" mock_llm = MagicMock() mock_structured = MagicMock() mock_structured.ainvoke = AsyncMock(side_effect=RuntimeError("Rate limited")) @@ -148,15 +210,50 @@ async def test_graceful_degradation_on_invoke_failure(self, mock_get_chat_model, violations = await condition.evaluate(context) assert violations == [] + # Should have retried 3 times total + assert mock_structured.ainvoke.await_count == 3 + # Should have slept twice (2s, 4s) + assert mock_sleep.call_count == 2 + + @pytest.mark.asyncio + @patch("src.integrations.providers.get_chat_model") + async def test_graceful_degradation_on_malformed_output(self, mock_get_chat_model, condition): + """When structured.ainvoke returns unexpected type, no violation.""" + mock_llm = MagicMock() + mock_structured = MagicMock() + mock_structured.ainvoke = AsyncMock(return_value={"decision": "aligned"}) + mock_llm.with_structured_output.return_value = mock_structured + mock_get_chat_model.return_value = mock_llm + + context = _make_context() + violations = await condition.evaluate(context) + + assert violations == [] + + @pytest.mark.asyncio + @patch("src.integrations.providers.get_chat_model") + async def test_skips_when_no_structured_output_support(self, mock_get_chat_model, condition): + """When provider doesn't support structured output, gracefully skip.""" + mock_llm = MagicMock() + # Remove with_structured_output method to simulate unsupported provider + delattr(mock_llm, "with_structured_output") + mock_get_chat_model.return_value = mock_llm + + context = _make_context() + violations = await condition.evaluate(context) + + assert violations == [] @pytest.mark.asyncio @patch("src.integrations.providers.get_chat_model") async def test_empty_description_sent_to_llm(self, mock_get_chat_model, condition): """Empty description is forwarded as '(empty)' so the LLM can flag it.""" verdict = AlignmentVerdict( - is_aligned=False, - reason="PR description is empty.", - how_to_fix="Add a description.", + decision="misaligned", + confidence=0.9, + reasoning="PR description is empty.", + recommendations=["Add a description."], + strategy_used="semantic_comparison", ) mock_llm = MagicMock() @@ -178,7 +275,13 @@ async def test_empty_description_sent_to_llm(self, mock_get_chat_model, conditio @patch("src.integrations.providers.get_chat_model") async def test_file_list_truncated_to_20(self, mock_get_chat_model, condition): """File list sent to LLM is capped at 20 entries.""" - verdict = AlignmentVerdict(is_aligned=True, reason="OK", how_to_fix=None) + verdict = AlignmentVerdict( + decision="aligned", + confidence=0.9, + reasoning="OK", + recommendations=None, + strategy_used="semantic_comparison", + ) mock_llm = MagicMock() mock_structured = MagicMock() @@ -187,8 +290,7 @@ async def test_file_list_truncated_to_20(self, mock_get_chat_model, condition): mock_get_chat_model.return_value = mock_llm files = [ - {"filename": f"src/file_{i}.py", "status": "modified", "additions": 1, "deletions": 0} - for i in range(50) + {"filename": f"src/file_{i}.py", "status": "modified", "additions": 1, "deletions": 0} for i in range(50) ] context = _make_context(changed_files=files) await condition.evaluate(context) @@ -197,3 +299,34 @@ async def test_file_list_truncated_to_20(self, mock_get_chat_model, condition): human_msg = call_messages[1].content assert "file_19.py" in human_msg assert "file_20.py" not in human_msg + + @pytest.mark.asyncio + @patch("src.integrations.providers.get_chat_model") + async def test_text_truncation_applied(self, mock_get_chat_model, condition): + """Very long description/title/diff are truncated before sending to LLM.""" + verdict = AlignmentVerdict( + decision="aligned", + confidence=0.9, + reasoning="OK", + recommendations=None, + strategy_used="semantic_comparison", + ) + + mock_llm = MagicMock() + mock_structured = MagicMock() + mock_structured.ainvoke = AsyncMock(return_value=verdict) + mock_llm.with_structured_output.return_value = mock_structured + mock_get_chat_model.return_value = mock_llm + + # Create excessively long inputs + long_description = "x" * 3000 + long_title = "y" * 500 + long_diff = "z" * 3000 + + context = _make_context(description=long_description, title=long_title, diff_summary=long_diff) + await condition.evaluate(context) + + call_messages = mock_structured.ainvoke.call_args[0][0] + human_msg = call_messages[1].content + # Should contain truncation markers + assert "[truncated]" in human_msg diff --git a/tests/unit/rules/test_acknowledgment.py b/tests/unit/rules/test_acknowledgment.py index 0ee77c2..8fd132c 100644 --- a/tests/unit/rules/test_acknowledgment.py +++ b/tests/unit/rules/test_acknowledgment.py @@ -35,7 +35,7 @@ def test_all_rule_ids_are_strings(self): assert len(rule_id.value) > 0 def test_rule_id_count(self): - """Verify we have exactly 20 standardized rule IDs.""" + """Verify we have the correct number of standardized rule IDs.""" assert len(RuleID) == 21 def test_all_rule_ids_have_descriptions(self): From c2689d57291fca849e397260ff733c07b7a5e07c Mon Sep 17 00:00:00 2001 From: Dimitris Kargatzis Date: Sun, 1 Mar 2026 19:31:23 +0200 Subject: [PATCH 3/6] style: apply pre-commit formatting fixes - Simplify supports_structured check (remove redundant getattr) --- src/rules/conditions/llm_assisted.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/rules/conditions/llm_assisted.py b/src/rules/conditions/llm_assisted.py index 7c6f346..9dbbc00 100644 --- a/src/rules/conditions/llm_assisted.py +++ b/src/rules/conditions/llm_assisted.py @@ -149,9 +149,7 @@ async def evaluate(self, context: Any) -> list[Violation]: ) # Check if provider supports structured output - supports_structured = hasattr(llm, "with_structured_output") and callable( - getattr(llm, "with_structured_output") - ) + supports_structured = hasattr(llm, "with_structured_output") and callable(llm.with_structured_output) if not supports_structured: logger.warning("Provider does not support structured output; skipping alignment check.") From fc417d60d0b47f103c8b54f86c60cc0efc4d4987 Mon Sep 17 00:00:00 2001 From: Dimitris Kargatzis Date: Sun, 1 Mar 2026 20:03:56 +0200 Subject: [PATCH 4/6] fix: prevent duplicate Watchflow violation comments Add comment-level deduplication to prevent posting identical violations multiple times when GitHub sends duplicate webhooks or the LRU cache evicts entries. Changes: - Add hidden HTML marker to violation comments with content hash () - Compute stable hash of violations based on rule_description, message, and severity (sorted for consistency) - Check existing PR comments before posting and skip if identical comment already exists - Fail open on duplicate check errors to avoid blocking legitimate posts - Add structured logging for duplicate detection Tests: - Hash computation stability (order-independent) - Hash uniqueness for different violations - Duplicate detection via hidden marker - Fail-open behavior on API errors - Skip posting when duplicate exists - Post proceeds when no duplicate found - Formatter includes/excludes marker based on content_hash parameter Fixes the issue seen in PR #62 where 3 identical back-to-back comments were posted due to webhook redeliveries with different X-GitHub-Delivery headers. No persistent storage required - deduplication happens at request time by fetching existing PR comments. --- .../pull_request/processor.py | 82 ++++++++++- src/presentation/github_formatter.py | 9 +- .../test_pull_request_processor.py | 130 ++++++++++++++++++ .../presentation/test_github_formatter.py | 25 ++++ 4 files changed, 242 insertions(+), 4 deletions(-) diff --git a/src/event_processors/pull_request/processor.py b/src/event_processors/pull_request/processor.py index 501180f..30a1e83 100644 --- a/src/event_processors/pull_request/processor.py +++ b/src/event_processors/pull_request/processor.py @@ -1,4 +1,6 @@ +import hashlib import logging +import re import time from typing import Any @@ -208,19 +210,95 @@ async def process(self, task: Task) -> ProcessingResult: ) async def _post_violations_to_github(self, task: Task, violations: list[Violation]) -> None: - """Post violations as comments on the pull request.""" + """Post violations as comments on the pull request. + + Implements comment-level deduplication by checking existing PR comments + and skipping if an identical Watchflow violations comment already exists. + """ try: pr_number = task.payload.get("pull_request", {}).get("number") if not pr_number or not task.installation_id: return - comment_body = github_formatter.format_violations_comment(violations) + # Compute content hash for deduplication + violations_signature = self._compute_violations_hash(violations) + + # Check if identical comment already exists + if await self._has_duplicate_comment( + task.repo_full_name, pr_number, violations_signature, task.installation_id + ): + logger.info( + "Skipping duplicate violations comment", + extra={ + "pr_number": pr_number, + "repo": task.repo_full_name, + "violations_hash": violations_signature, + }, + ) + return + + # Post new comment with hash marker + comment_body = github_formatter.format_violations_comment(violations, content_hash=violations_signature) await self.github_client.create_pull_request_comment( task.repo_full_name, pr_number, comment_body, task.installation_id ) + logger.info( + "Posted violations comment", + extra={ + "pr_number": pr_number, + "repo": task.repo_full_name, + "violations_count": len(violations), + "violations_hash": violations_signature, + }, + ) except Exception as e: logger.error(f"Error posting violations to GitHub: {e}") + def _compute_violations_hash(self, violations: list[Violation]) -> str: + """Compute a stable hash of violations for deduplication. + + Uses rule_description + message + severity to create a fingerprint. + This allows detecting identical violation sets regardless of delivery_id. + """ + # Sort violations to ensure consistent ordering + sorted_violations = sorted( + violations, + key=lambda v: (v.rule_description or "", v.message, v.severity.value if v.severity else ""), + ) + + # Build signature from key fields + signature_parts = [] + for v in sorted_violations: + signature_parts.append(f"{v.rule_description}|{v.message}|{v.severity.value if v.severity else ''}") + + signature_string = "::".join(signature_parts) + return hashlib.sha256(signature_string.encode()).hexdigest()[:12] # Use first 12 chars for readability + + async def _has_duplicate_comment( + self, repo: str, pr_number: int, violations_hash: str, installation_id: int + ) -> bool: + """Check if a comment with the same violations hash already exists. + + Looks for the hidden HTML marker in existing comments to detect duplicates. + """ + try: + existing_comments = await self.github_client.get_issue_comments(repo, pr_number, installation_id) + + # Pattern to extract hash from hidden marker: + hash_pattern = re.compile(r"") + + for comment in existing_comments: + body = comment.get("body", "") + match = hash_pattern.search(body) + if match and match.group(1) == violations_hash: + return True + + return False + except Exception as e: + logger.warning(f"Error checking for duplicate comments: {e}. Proceeding with post.") + # Fail open: if we can't check, allow posting to avoid blocking + return False + async def prepare_webhook_data(self, task: Task) -> dict[str, Any]: """Extract data available in webhook payload.""" return self.enricher.prepare_webhook_data(task) diff --git a/src/presentation/github_formatter.py b/src/presentation/github_formatter.py index f2a6c00..c159ddc 100644 --- a/src/presentation/github_formatter.py +++ b/src/presentation/github_formatter.py @@ -190,11 +190,12 @@ def format_rules_not_configured_comment( ) -def format_violations_comment(violations: list[Violation]) -> str: +def format_violations_comment(violations: list[Violation], content_hash: str | None = None) -> str: """Format violations as a GitHub comment. Args: violations: List of rule violations to include in the comment. + content_hash: Optional hash to include as a hidden marker for deduplication. Returns: A Markdown formatted string suitable for a Pull Request timeline comment. @@ -203,7 +204,11 @@ def format_violations_comment(violations: list[Violation]) -> str: if not violations: return "" - comment = f"### 🛡️ Watchflow Governance Checks\n**Status:** ❌ {len(violations)} Violations Found\n\n" + # Add hidden HTML marker for deduplication (not visible in rendered markdown) + marker = f"\n" if content_hash else "" + + comment = marker + comment += f"### 🛡️ Watchflow Governance Checks\n**Status:** ❌ {len(violations)} Violations Found\n\n" comment += _build_collapsible_violations_text(violations) comment += "---\n" comment += ( diff --git a/tests/unit/event_processors/test_pull_request_processor.py b/tests/unit/event_processors/test_pull_request_processor.py index c299832..b2a8ed8 100644 --- a/tests/unit/event_processors/test_pull_request_processor.py +++ b/tests/unit/event_processors/test_pull_request_processor.py @@ -99,3 +99,133 @@ async def test_process_with_violations(processor, mock_agent): assert result.violations[0].rule_description == "Rule 1" # Ensure check run manager called for violation processor.check_run_manager.create_check_run.assert_awaited_once() + + +@pytest.mark.asyncio +async def test_compute_violations_hash_stable_ordering(processor): + """Test that violations hash is stable regardless of input order.""" + from src.core.models import Severity + + violation1 = Violation(rule_description="Rule A", severity=Severity.HIGH, message="Message 1") + violation2 = Violation(rule_description="Rule B", severity=Severity.MEDIUM, message="Message 2") + + # Hash should be the same regardless of input order + hash1 = processor._compute_violations_hash([violation1, violation2]) + hash2 = processor._compute_violations_hash([violation2, violation1]) + + assert hash1 == hash2 + assert len(hash1) == 12 # Should be 12 chars + + +@pytest.mark.asyncio +async def test_compute_violations_hash_different_for_different_violations(processor): + """Test that different violations produce different hashes.""" + from src.core.models import Severity + + violation1 = Violation(rule_description="Rule A", severity=Severity.HIGH, message="Message 1") + violation2 = Violation(rule_description="Rule B", severity=Severity.MEDIUM, message="Message 2") + + hash1 = processor._compute_violations_hash([violation1]) + hash2 = processor._compute_violations_hash([violation2]) + + assert hash1 != hash2 + + +@pytest.mark.asyncio +async def test_has_duplicate_comment_finds_existing(processor): + """Test that existing comment with matching hash is detected.""" + processor.github_client.get_issue_comments = AsyncMock( + return_value=[ + {"body": "Some other comment"}, + {"body": "\n### Violations\nContent here"}, + {"body": "Another comment"}, + ] + ) + + has_duplicate = await processor._has_duplicate_comment("owner/repo", 123, "abc123def456", 1) + + assert has_duplicate is True + + +@pytest.mark.asyncio +async def test_has_duplicate_comment_no_match(processor): + """Test that comments without matching hash are not detected as duplicates.""" + processor.github_client.get_issue_comments = AsyncMock( + return_value=[ + {"body": "Some other comment"}, + {"body": "\n### Violations\nContent here"}, + ] + ) + + has_duplicate = await processor._has_duplicate_comment("owner/repo", 123, "abc123def456", 1) + + assert has_duplicate is False + + +@pytest.mark.asyncio +async def test_has_duplicate_comment_no_existing_comments(processor): + """Test that no duplicate is found when there are no comments.""" + processor.github_client.get_issue_comments = AsyncMock(return_value=[]) + + has_duplicate = await processor._has_duplicate_comment("owner/repo", 123, "abc123def456", 1) + + assert has_duplicate is False + + +@pytest.mark.asyncio +async def test_has_duplicate_comment_fails_open_on_error(processor): + """Test that duplicate check fails open (returns False) if API call fails.""" + processor.github_client.get_issue_comments = AsyncMock(side_effect=Exception("API error")) + + has_duplicate = await processor._has_duplicate_comment("owner/repo", 123, "abc123def456", 1) + + assert has_duplicate is False # Fail open to allow posting + + +@pytest.mark.asyncio +async def test_post_violations_skips_duplicate(processor): + """Test that posting is skipped when identical comment already exists.""" + from src.core.models import Severity + + task = MagicMock(spec=Task) + task.repo_full_name = "owner/repo" + task.installation_id = 1 + task.payload = {"pull_request": {"number": 123}} + + violations = [Violation(rule_description="Rule A", severity=Severity.HIGH, message="Message 1")] + + # Mock that a duplicate exists + processor.github_client.get_issue_comments = AsyncMock( + return_value=[{"body": "\nContent"}] + ) + + # Compute the hash (we'll mock it to match) + with MagicMock() as mock_hash: + processor._compute_violations_hash = MagicMock(return_value="abc123def456") + + await processor._post_violations_to_github(task, violations) + + # Should NOT have called create_pull_request_comment + processor.github_client.create_pull_request_comment.assert_not_called() + + +@pytest.mark.asyncio +async def test_post_violations_posts_when_no_duplicate(processor): + """Test that posting proceeds when no duplicate comment exists.""" + from src.core.models import Severity + + task = MagicMock(spec=Task) + task.repo_full_name = "owner/repo" + task.installation_id = 1 + task.payload = {"pull_request": {"number": 123}} + + violations = [Violation(rule_description="Rule A", severity=Severity.HIGH, message="Message 1")] + + # Mock that no duplicate exists + processor.github_client.get_issue_comments = AsyncMock(return_value=[]) + processor.github_client.create_pull_request_comment = AsyncMock() + + await processor._post_violations_to_github(task, violations) + + # Should have called create_pull_request_comment + processor.github_client.create_pull_request_comment.assert_called_once() diff --git a/tests/unit/presentation/test_github_formatter.py b/tests/unit/presentation/test_github_formatter.py index a4c6393..1878bb7 100644 --- a/tests/unit/presentation/test_github_formatter.py +++ b/tests/unit/presentation/test_github_formatter.py @@ -102,5 +102,30 @@ def test_format_violations_for_check_run(): assert "• **Lint** - Trailing space" in result + def test_format_violations_for_check_run_empty(): assert format_violations_for_check_run([]) == "None" + + +def test_format_violations_comment_includes_hash_marker(): + """Test that comment includes hidden HTML marker when content_hash is provided.""" + violations = [ + Violation(rule_description="Rule 1", severity=Severity.HIGH, message="Error 1"), + ] + + comment = format_violations_comment(violations, content_hash="abc123def456") + + assert "" in comment + assert "### 🛡️ Watchflow Governance Checks" in comment + + +def test_format_violations_comment_no_hash_marker_when_not_provided(): + """Test that comment does not include marker when content_hash is None.""" + violations = [ + Violation(rule_description="Rule 1", severity=Severity.HIGH, message="Error 1"), + ] + + comment = format_violations_comment(violations, content_hash=None) + + assert "\nContent"}] ) - # Compute the hash (we'll mock it to match) - with MagicMock() as mock_hash: - processor._compute_violations_hash = MagicMock(return_value="abc123def456") + # Mock the hash to match the existing comment + processor._compute_violations_hash = MagicMock(return_value="abc123def456") - await processor._post_violations_to_github(task, violations) + await processor._post_violations_to_github(task, violations) - # Should NOT have called create_pull_request_comment - processor.github_client.create_pull_request_comment.assert_not_called() + # Should NOT have called create_pull_request_comment + processor.github_client.create_pull_request_comment.assert_not_called() @pytest.mark.asyncio From 2eb38c5f0f2de56b3902b62638c97df84446ae1c Mon Sep 17 00:00:00 2001 From: Dimitris Kargatzis Date: Sun, 1 Mar 2026 20:50:50 +0200 Subject: [PATCH 6/6] style: fix trailing whitespace (pre-commit auto-fix) --- src/event_processors/pull_request/processor.py | 18 +++++++++--------- src/presentation/github_formatter.py | 2 +- .../unit/presentation/test_github_formatter.py | 1 - 3 files changed, 10 insertions(+), 11 deletions(-) diff --git a/src/event_processors/pull_request/processor.py b/src/event_processors/pull_request/processor.py index 30a1e83..640d70c 100644 --- a/src/event_processors/pull_request/processor.py +++ b/src/event_processors/pull_request/processor.py @@ -211,7 +211,7 @@ async def process(self, task: Task) -> ProcessingResult: async def _post_violations_to_github(self, task: Task, violations: list[Violation]) -> None: """Post violations as comments on the pull request. - + Implements comment-level deduplication by checking existing PR comments and skipping if an identical Watchflow violations comment already exists. """ @@ -222,7 +222,7 @@ async def _post_violations_to_github(self, task: Task, violations: list[Violatio # Compute content hash for deduplication violations_signature = self._compute_violations_hash(violations) - + # Check if identical comment already exists if await self._has_duplicate_comment( task.repo_full_name, pr_number, violations_signature, task.installation_id @@ -256,7 +256,7 @@ async def _post_violations_to_github(self, task: Task, violations: list[Violatio def _compute_violations_hash(self, violations: list[Violation]) -> str: """Compute a stable hash of violations for deduplication. - + Uses rule_description + message + severity to create a fingerprint. This allows detecting identical violation sets regardless of delivery_id. """ @@ -265,12 +265,12 @@ def _compute_violations_hash(self, violations: list[Violation]) -> str: violations, key=lambda v: (v.rule_description or "", v.message, v.severity.value if v.severity else ""), ) - + # Build signature from key fields signature_parts = [] for v in sorted_violations: signature_parts.append(f"{v.rule_description}|{v.message}|{v.severity.value if v.severity else ''}") - + signature_string = "::".join(signature_parts) return hashlib.sha256(signature_string.encode()).hexdigest()[:12] # Use first 12 chars for readability @@ -278,21 +278,21 @@ async def _has_duplicate_comment( self, repo: str, pr_number: int, violations_hash: str, installation_id: int ) -> bool: """Check if a comment with the same violations hash already exists. - + Looks for the hidden HTML marker in existing comments to detect duplicates. """ try: existing_comments = await self.github_client.get_issue_comments(repo, pr_number, installation_id) - + # Pattern to extract hash from hidden marker: hash_pattern = re.compile(r"") - + for comment in existing_comments: body = comment.get("body", "") match = hash_pattern.search(body) if match and match.group(1) == violations_hash: return True - + return False except Exception as e: logger.warning(f"Error checking for duplicate comments: {e}. Proceeding with post.") diff --git a/src/presentation/github_formatter.py b/src/presentation/github_formatter.py index c159ddc..6e78f59 100644 --- a/src/presentation/github_formatter.py +++ b/src/presentation/github_formatter.py @@ -206,7 +206,7 @@ def format_violations_comment(violations: list[Violation], content_hash: str | N # Add hidden HTML marker for deduplication (not visible in rendered markdown) marker = f"\n" if content_hash else "" - + comment = marker comment += f"### 🛡️ Watchflow Governance Checks\n**Status:** ❌ {len(violations)} Violations Found\n\n" comment += _build_collapsible_violations_text(violations) diff --git a/tests/unit/presentation/test_github_formatter.py b/tests/unit/presentation/test_github_formatter.py index 1878bb7..efa51c4 100644 --- a/tests/unit/presentation/test_github_formatter.py +++ b/tests/unit/presentation/test_github_formatter.py @@ -102,7 +102,6 @@ def test_format_violations_for_check_run(): assert "• **Lint** - Trailing space" in result - def test_format_violations_for_check_run_empty(): assert format_violations_for_check_run([]) == "None"