diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 4c933c6..455d2b2 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -7,7 +7,7 @@ Thanks for considering contributing. Watchflow is a **rule engine** for GitHub ## Direction and scope - **Rule engine** — Conditions map parameter keys to built-in logic (e.g. `require_linked_issue`, `max_lines`, `require_code_owner_reviewers`). New conditions live in `src/rules/conditions/` and are registered in `src/rules/registry.py` and `src/rules/acknowledgment.py`. -- **Webhooks** — Delivery ID–based dedup so handler and processor both run; welcome comment when no rules file exists. +- **Webhooks** — Delivery ID–based dedup so handler and processor both run; one setup-awareness comment on an initially opened PR when no rules file exists. - **API** — Repo analysis and proceed-with-PR support `installation_id` so install-flow users don’t need a PAT. - **Docs** — All MD files should speak to engineers: direct, no fluff, immune-system framing (Watchflow as necessary governance, not “another AI tool”). diff --git a/README.md b/README.md index 76b5f88..91afa22 100644 --- a/README.md +++ b/README.md @@ -134,7 +134,7 @@ Detailed steps: [Quick Start](docs/getting-started/quick-start.md). Configuratio - **`POST /api/v1/rules/recommend`** — Analyze a repo (structure, PR history) and return suggested rules. Accepts `repo_url`; optional `installation_id` (from install link) or user token for private repos and higher rate limits. - **`POST /api/v1/rules/recommend/proceed-with-pr`** — Create a PR that adds `.watchflow/rules.yaml` from recommended rules. Auth: Bearer token or `installation_id` in body. -When no `.watchflow/rules.yaml` exists and a PR is opened, Watchflow posts a **welcome comment** with a link to watchflow.dev (including `installation_id` and `repo`) so maintainers can run analysis and create a rules PR without entering a PAT. +When a newly opened PR has no `.watchflow/rules.yaml`, Watchflow posts one **setup-awareness comment** with a link to watchflow.dev (including `installation_id` and `repo`) so maintainers can run analysis and create a rules PR without entering a PAT. Later commits, reviews, and re-runs update the neutral check without repeating the comment. --- diff --git a/docs/concepts/overview.md b/docs/concepts/overview.md index 2e01c8f..6b689f9 100644 --- a/docs/concepts/overview.md +++ b/docs/concepts/overview.md @@ -25,10 +25,10 @@ graph TD 1. **Webhook** — GitHub sends `pull_request` or `push`; router reads `X-GitHub-Delivery`, builds `WebhookEvent` with `delivery_id`. 2. **Handler** — Enqueues a processor task with `event_type + delivery_id + func` so dedup doesn’t skip the processor. -3. **Processor** — Loads `.watchflow/rules.yaml` from default branch (via GitHub API). If missing, creates a neutral check run and posts a **welcome comment** with a link to watchflow.dev (`installation_id` + `repo`). +3. **Processor** — Loads `.watchflow/rules.yaml` from default branch (via GitHub API). If missing, creates a neutral check run; on the initial PR-open event only, it also posts one setup-awareness comment with a link to watchflow.dev (`installation_id` + `repo`). 4. **Enrichment** — Fetches PR files, reviews, CODEOWNERS content so conditions can run without a local clone. 5. **Rule engine** — Passes **Rule objects** (with attached condition instances) to the engine. Engine runs each rule’s conditions; no conversion to dicts that would drop conditions. -6. **Output** — Violations → check run + PR comment; developers can reply `@watchflow acknowledge "reason"` where the rule allows it. +6. **Output** — Violations → current check run + an initial PR snapshot. Regressions add a concise follow-up alert; improvements change the check only. Developers can reply `@watchflow acknowledge "reason"` where the rule allows it. ## Core components diff --git a/docs/features.md b/docs/features.md index 2de2259..5829451 100644 --- a/docs/features.md +++ b/docs/features.md @@ -77,7 +77,7 @@ Suggested rules use the **same parameter names** as above so they work out of th ## Welcome comment when no rules file -When `.watchflow/rules.yaml` is missing and a PR is opened, Watchflow: +When `.watchflow/rules.yaml` is missing when a PR is initially opened, Watchflow: 1. Creates a **neutral check run** (“Rules not configured”). 2. Posts a **welcome comment** with: @@ -87,6 +87,8 @@ When `.watchflow/rules.yaml` is missing and a PR is opened, Watchflow: So maintainers get one clear next step instead of a silent skip. +Later commits, reviews, review-thread changes, and re-runs refresh the neutral check run but do not repeat the setup-awareness comment. + --- ## Webhook and task dedup @@ -111,7 +113,7 @@ So maintainers get one clear next step instead of a silent skip. - **GitHub App** — Install per org/repo; we use installation tokens for API access and webhooks. - **Webhooks** — `pull_request`, `push`; we also support `issue_comment` for acknowledgments, and deployment/workflow events for time-based and deploy rules. - **Check runs** — Violations show up as failed/neutral check runs with a summary and link to the rules file. -- **PR comments** — Violation summary and remediation hints; acknowledgment replies parsed in-thread. +- **PR comments** — An initial violation snapshot with remediation hints, plus concise follow-up alerts only for new or higher-severity violations. The Watchflow Rules check always shows the complete current result; acknowledgment replies are parsed in-thread. --- diff --git a/docs/getting-started/quick-start.md b/docs/getting-started/quick-start.md index b17fe15..94390d0 100644 --- a/docs/getting-started/quick-start.md +++ b/docs/getting-started/quick-start.md @@ -7,7 +7,7 @@ Get Watchflow running in a few minutes: install the app, add `.watchflow/rules.y ## What you get - **Rule evaluation** on every PR and push against your YAML rules. -- **Check runs** and **PR comments** when rules are violated (or when no rules file exists, a welcome comment with a link to set one up). +- **Check runs** and **PR comments** when rules are violated (or one setup-awareness comment when a newly opened PR has no rules file). - **Acknowledgment** in-thread: `@watchflow acknowledge "reason"` where the rule allows it. - **One config file** — `.watchflow/rules.yaml` on the default branch; rules are loaded from there via the GitHub API. @@ -26,15 +26,15 @@ Get Watchflow running in a few minutes: install the app, add `.watchflow/rules.y 2. Click **Install** and choose the org/repos you want to protect. 3. Grant the requested permissions (webhooks, repo content for rules and PR data). -Watchflow will start receiving webhooks. If there’s no `.watchflow/rules.yaml` yet, the first PR will get a **welcome comment** with a link to [watchflow.dev](https://watchflow.dev) (including `installation_id` and `repo`) so you can run repo analysis and create a rules PR **without entering a PAT**. +Watchflow will start receiving webhooks. If a newly opened PR has no `.watchflow/rules.yaml`, it gets one **setup-awareness comment** with a link to [watchflow.dev](https://watchflow.dev) (including `installation_id` and `repo`) so you can run repo analysis and create a rules PR **without entering a PAT**. Later commits and reviews do not repeat it. --- ## Step 2: Add rules -**Option A — From the welcome comment (no PAT)** +**Option A — From the setup-awareness comment (no PAT)** -1. Open a PR (or any PR) and find the Watchflow welcome comment. +1. Open a new PR and find the Watchflow setup-awareness comment. 2. Click the link to **watchflow.dev/analyze?installation_id=…&repo=owner/repo**. 3. Run repo analysis; review suggested rules and click **Create PR** to add `.watchflow/rules.yaml` to a branch. @@ -74,7 +74,7 @@ Parameter names must match the [supported conditions](configuration.md); see [Co 1. **Open a PR** (or push to a protected branch if you use `no_force_push`). 2. Check **GitHub Checks** for the Watchflow check run (pass / fail / neutral). -3. If a rule is violated, you should see a **PR comment** with the violation and remediation hint. +3. If a rule is violated, you should see a **PR comment** with the violation and remediation hint. Use **Watchflow Rules** in Checks for the current result; later new or higher-severity violations add a short follow-up alert. 4. Where the rule allows it, reply with: `@watchflow acknowledge "Documentation-only change, no code impact"` (or `@watchflow ack "…"`). diff --git a/docs/index.md b/docs/index.md index cebf03c..9626411 100644 --- a/docs/index.md +++ b/docs/index.md @@ -44,7 +44,7 @@ We built it for teams that still care about traceability, CODEOWNERS, and review - **Condition-based rules** — `require_linked_issue`, `max_lines`, `require_code_owner_reviewers`, `no_force_push`, title patterns, approvals, labels, and more. - **CODEOWNERS-aware** — Require owners for modified paths to be requested as reviewers; or require every changed path to have an owner. - **Webhook-native** — Uses GitHub delivery IDs so handler and processor both run; comments and check runs stay in sync. -- **Install-flow friendly** — When no rules file exists, we post a welcome comment with a link to watchflow.dev (installation_id + repo) so you can run analysis and create a rules PR without a PAT. +- **Install-flow friendly** — When a newly opened PR has no rules file, we post one setup-awareness comment with a link to watchflow.dev (installation_id + repo) so you can run analysis and create a rules PR without a PAT. ## Quick example diff --git a/src/event_processors/pull_request/enricher.py b/src/event_processors/pull_request/enricher.py index 36c16d9..1e82aeb 100644 --- a/src/event_processors/pull_request/enricher.py +++ b/src/event_processors/pull_request/enricher.py @@ -113,25 +113,26 @@ async def fetch_acknowledgments(self, repo: str, pr_number: int, installation_id """Fetch and parse previous acknowledgments from PR comments.""" try: comments = await self.github_client.get_issue_comments(repo, pr_number, installation_id) - if not comments: - return {} - - acknowledgments = {} - for comment in comments: - comment_body = comment.get("body", "") - commenter = comment.get("user", {}).get("login", "") - - if is_acknowledgment_comment(comment_body): - acknowledged_violations = parse_acknowledgment_comment(comment_body, commenter) - for ack in acknowledged_violations: - if ack.rule_id: - acknowledgments[ack.rule_id] = ack - - return acknowledgments + return self.parse_acknowledgments(comments or []) except Exception as e: logger.error(f"Error fetching acknowledgments: {e}") return {} + def parse_acknowledgments(self, comments: list[dict[str, Any]]) -> dict[str, Acknowledgment]: + """Parse acknowledgments from a previously fetched PR-comment snapshot.""" + acknowledgments = {} + for comment in comments: + comment_body = comment.get("body", "") + commenter = (comment.get("user") or {}).get("login", "") + + if is_acknowledgment_comment(comment_body): + acknowledged_violations = parse_acknowledgment_comment(comment_body, commenter) + for ack in acknowledged_violations: + if ack.rule_id: + acknowledgments[ack.rule_id] = ack + + return acknowledgments + def prepare_webhook_data(self, task: Any) -> dict[str, Any]: """Extract data available in webhook payload.""" if not task or not hasattr(task, "payload") or not task.payload: diff --git a/src/event_processors/pull_request/processor.py b/src/event_processors/pull_request/processor.py index 0b15618..4ba720c 100644 --- a/src/event_processors/pull_request/processor.py +++ b/src/event_processors/pull_request/processor.py @@ -1,13 +1,16 @@ +import asyncio import hashlib -import logging -import re +import json import time +from collections import Counter, defaultdict from typing import Any +import structlog import yaml from src.agents import get_agent from src.api.recommendations import get_suggested_rules_from_repo +from src.core.config import config from src.core.models import Violation from src.event_processors.base import BaseEventProcessor, ProcessingResult from src.event_processors.pull_request.enricher import PullRequestEnricher @@ -17,7 +20,7 @@ from src.rules.loaders.github_loader import GitHubRuleLoader, RulesFileNotFoundError from src.tasks.task_queue import Task -logger = logging.getLogger(__name__) +logger = structlog.get_logger() class PullRequestProcessor(BaseEventProcessor): @@ -28,6 +31,10 @@ def __init__(self) -> None: self.engine_agent = get_agent("engine") self.enricher = PullRequestEnricher(self.github_client) self.check_run_manager = CheckRunManager(self.github_client) + self._setup_awareness_locks: dict[tuple[str, int], tuple[asyncio.Lock, int]] = {} + self._violation_comment_locks: dict[tuple[str, int], tuple[asyncio.Lock, int]] = {} + self._violation_states: dict[tuple[str, int], list[dict[str, str]]] = {} + self._violation_reports: dict[tuple[str, int], bool] = {} def get_event_type(self) -> str: return "pull_request" @@ -156,18 +163,7 @@ async def process(self, task: Task) -> ProcessingResult: conclusion="neutral", error="Rules not configured. Please create `.watchflow/rules.yaml` in your repository.", ) - # Post welcome comment with instructions and link to watchflow.dev (installation_id as URL param) - if pr_number and installation_id: - try: - welcome_comment = github_formatter.format_rules_not_configured_comment( - repo_full_name=repo_full_name, - installation_id=installation_id, - ) - await self.github_client.create_pull_request_comment( - repo_full_name, pr_number, welcome_comment, installation_id - ) - except Exception as comment_err: - logger.warning(f"Could not post rules-not-configured comment: {comment_err}") + await self._post_rules_not_configured_comment(task, pr_number, installation_id) return ProcessingResult( success=True, violations=[], @@ -216,10 +212,13 @@ async def process(self, task: Task) -> ProcessingResult: # 3. Check for existing acknowledgments previous_acknowledgments = {} + comments_snapshot: list[dict[str, Any]] | None = None if pr_number: - previous_acknowledgments = await self.enricher.fetch_acknowledgments( + comments_snapshot = await self.github_client.get_issue_comments( repo_full_name, pr_number, installation_id ) + if comments_snapshot: + previous_acknowledgments = self.enricher.parse_acknowledgments(comments_snapshot) if previous_acknowledgments: logger.info(f"📋 Found {len(previous_acknowledgments)} previous acknowledgments") @@ -253,6 +252,9 @@ async def process(self, task: Task) -> ProcessingResult: violations = require_acknowledgment_violations # 6. Report results to GitHub + previous_violation_state, has_previous_violation_report = await self._get_previous_violation_state( + task, sha, installation_id + ) if sha: if previous_acknowledgments and original_violations: await self.check_run_manager.create_acknowledgment_check_run( @@ -271,9 +273,10 @@ async def process(self, task: Task) -> ProcessingResult: violations=violations, ) - if violations: - logger.info(f"🚨 Found {len(violations)} violations, posting to PR...") - await self._post_violations_to_github(task, violations) + if pr_number: + await self._post_violations_to_github( + task, violations, previous_violation_state, has_previous_violation_report + ) api_calls += 1 processing_time = int((time.time() - start_time) * 1000) @@ -307,55 +310,292 @@ async def process(self, task: Task) -> ProcessingResult: error=str(e), ) - async def _post_violations_to_github(self, task: Task, violations: list[Violation]) -> None: - """Post violations as comments on the pull request. + async def _post_violations_to_github( + self, + task: Task, + violations: list[Violation], + previous_state: list[dict[str, str]] | None = None, + has_previous_report: bool = False, + ) -> None: + """Keep comments immutable: create a snapshot first, then alert only on regressions.""" + pr_number = task.payload.get("pull_request", {}).get("number") + installation_id = task.installation_id + if not pr_number or not installation_id: + return + + lock_key = (task.repo_full_name, pr_number) + lock_entry = self._violation_comment_locks.get(lock_key) + if lock_entry is None: + lock, waiting_tasks = asyncio.Lock(), 0 + else: + lock, waiting_tasks = lock_entry + self._violation_comment_locks[lock_key] = (lock, waiting_tasks + 1) - 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 - - # Compute content hash for deduplication - violations_signature = self._compute_violations_hash(violations) + async with lock: + effective_previous_state = self._violation_states.get(lock_key, previous_state) + effective_has_previous_report = self._violation_reports.get(lock_key, has_previous_report) + violations_hash = self._compute_violations_hash(violations) + current_state = github_formatter.build_violations_state(violations) + if violations and not effective_has_previous_report: + snapshot = github_formatter.format_violations_comment( + violations, content_hash=violations_hash, current_report=True + ) + created = await self.github_client.create_pull_request_comment( + task.repo_full_name, pr_number, snapshot, installation_id + ) + if created: + effective_has_previous_report = True + logger.info("violations_snapshot_posted", repo=task.repo_full_name, pr_number=pr_number) + else: + logger.warning("violations_snapshot_post_failed", repo=task.repo_full_name, pr_number=pr_number) + elif effective_previous_state is not None: + added, worsened = self._find_violation_regressions(effective_previous_state, violations) + if added or worsened: + alert = github_formatter.format_violation_regression_alert( + self._describe_violation_trigger(task), added, worsened, violations + ) + alert_created = await self.github_client.create_pull_request_comment( + task.repo_full_name, pr_number, alert, installation_id + ) + if alert_created: + logger.info( + "violations_regression_alert_posted", + repo=task.repo_full_name, + pr_number=pr_number, + added_count=len(added), + worsened_count=len(worsened), + ) + else: + logger.warning( + "violations_regression_alert_failed", repo=task.repo_full_name, pr_number=pr_number + ) + else: + logger.info( + "violations_comment_skipped_non_regression", repo=task.repo_full_name, pr_number=pr_number + ) + self._violation_states[lock_key] = current_state + self._violation_reports[lock_key] = effective_has_previous_report + except Exception as error: + logger.warning( + "violations_comment_sync_failed", repo=task.repo_full_name, pr_number=pr_number, error=str(error) + ) + finally: + current_lock, waiting_tasks = self._violation_comment_locks.get(lock_key, (lock, 1)) + if current_lock is lock: + if waiting_tasks == 1: + self._violation_comment_locks.pop(lock_key, None) + else: + self._violation_comment_locks[lock_key] = (lock, waiting_tasks - 1) - # 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, - }, + async def _get_previous_violation_state( + self, task: Task, sha: str | None, installation_id: int + ) -> tuple[list[dict[str, str]] | None, bool]: + """Load the last state from Checks, falling back to Watchflow's immutable comment snapshots.""" + pr_number = task.payload.get("pull_request", {}).get("number") + if not pr_number: + return None, False + lock_key = (task.repo_full_name, pr_number) + if lock_key in self._violation_states: + state = self._violation_states[lock_key] + return state, self._violation_reports.get(lock_key, bool(state)) + + reference_sha = sha + before_sha = task.payload.get("before") + if task.payload.get("action") == "synchronize" and isinstance(before_sha, str) and before_sha.strip("0"): + reference_sha = before_sha + check_state: list[dict[str, str]] | None = None + if reference_sha: + try: + check_runs = await self.github_client.get_check_runs( + task.repo_full_name, reference_sha, installation_id + ) + for check_run in sorted(check_runs or [], key=lambda item: int(item.get("id", 0)), reverse=True): + if str(check_run.get("name", "")).lower() not in {"watchflow rules", "watchflow-rules"}: + continue + output = check_run.get("output") or {} + state = github_formatter.parse_violations_state_marker(str(output.get("text") or "")) + if state is not None: + # A non-empty Check is enough to establish that violations were + # already reported. For a passing Check, inspect comments below: + # it may precede the first violations report on this PR. + if state: + return state, True + check_state = state + break + except Exception as error: + logger.warning( + "violations_state_check_lookup_failed", + repo=task.repo_full_name, + pr_number=pr_number, + error=str(error), ) - 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 + try: + comments = await self.github_client.get_issue_comments(task.repo_full_name, pr_number, installation_id) + if comments is None: + return None, True + for comment in reversed(comments): + if not self._is_watchflow_comment(comment): + continue + body = str(comment.get("body") or "") + state = github_formatter.parse_violations_state_marker(body) + if state is not None: + return check_state if check_state is not None else state, True + if github_formatter.VIOLATIONS_HASH_MARKER_PATTERN.search(body): + return check_state, True + except Exception as error: + logger.warning( + "violations_state_comment_lookup_failed", + repo=task.repo_full_name, + pr_number=pr_number, + error=str(error), ) + return None, True + return check_state, False + + @staticmethod + def _severity_rank(severity: str) -> int: + return {"info": 0, "low": 1, "medium": 2, "high": 3, "critical": 4}.get(severity, 0) + + @staticmethod + def _violation_identity(violation: Violation) -> str: + return violation.rule_id or violation.rule_description + + def _find_violation_regressions( + self, previous_state: list[dict[str, str]], violations: list[Violation] + ) -> tuple[list[Violation], list[tuple[Violation, str]]]: + """Return violations added or raised in severity since the canonical report.""" + previous_by_identity: dict[str, list[str]] = defaultdict(list) + for item in previous_state: + previous_by_identity[item["id"]].append(item["severity"]) + current_by_identity: dict[str, list[Violation]] = defaultdict(list) + for violation in violations: + current_by_identity[self._violation_identity(violation)].append(violation) + + added: list[Violation] = [] + worsened: list[tuple[Violation, str]] = [] + for identity in set(previous_by_identity) | set(current_by_identity): + previous = previous_by_identity[identity] + current = current_by_identity[identity] + previous_counts = Counter(previous) + unmatched_current: list[Violation] = [] + for violation in current: + severity = violation.severity.value if hasattr(violation.severity, "value") else str(violation.severity) + if previous_counts[severity]: + previous_counts[severity] -= 1 + else: + unmatched_current.append(violation) + unmatched_previous = [severity for severity, count in previous_counts.items() for _ in range(count)] + # Pair the most severe findings first. This avoids treating an + # improvement of a high finding plus resolution of a low finding + # as a false increase from low to medium when a rule emits more + # than one finding with the same identity. + unmatched_current.sort( + key=lambda violation: self._severity_rank( + violation.severity.value if hasattr(violation.severity, "value") else str(violation.severity) + ), + reverse=True, + ) + unmatched_previous.sort(key=self._severity_rank, reverse=True) + for previous_severity, violation in zip(unmatched_previous, unmatched_current, strict=False): + current_severity = ( + violation.severity.value if hasattr(violation.severity, "value") else str(violation.severity) + ) + if self._severity_rank(current_severity) > self._severity_rank(previous_severity): + worsened.append((violation, previous_severity)) + added.extend(unmatched_current[len(unmatched_previous) :]) + return added, worsened + + @staticmethod + def _describe_violation_trigger(task: Task) -> str: + event_type = getattr(task, "event_type", "") + event_type = getattr(event_type, "value", event_type) + event_type = event_type.lower() if isinstance(event_type, str) else "" + action = task.payload.get("action", "") + if event_type == "pull_request_review": + reviewer = task.payload.get("review", {}).get("user", {}).get("login") + return f"a review was {action}{f' by @{reviewer}' if reviewer else ''}" + if event_type == "pull_request_review_thread": + return f"a review thread was {action}" + return { + "synchronize": "new commits were pushed", + "edited": "the pull request details were edited", + "opened": "the pull request was opened", + "reopened": "the pull request was reopened", + "ready_for_review": "the pull request was marked ready for review", + }.get(action, "the pull request was re-evaluated") + + async def _post_rules_not_configured_comment(self, task: Task, pr_number: int | None, installation_id: int) -> None: + """Post one setup-awareness comment for an initially opened PR. + + Missing rules are surfaced in the neutral check run on every relevant + event. The timeline comment is onboarding-only, so it must never be + repeated after the initial ``pull_request.opened`` delivery. + """ + if not pr_number: + return + + if task.payload.get("action") != "opened": logger.info( - "Posted violations comment", - extra={ - "pr_number": pr_number, - "repo": task.repo_full_name, - "violations_count": len(violations), - "violations_hash": violations_signature, - }, + "setup_awareness_skipped_non_opened", + pr_number=pr_number, + repo=task.repo_full_name, + action=task.payload.get("action"), ) - except Exception as e: - logger.error(f"Error posting violations to GitHub: {e}") + return + + lock_key = (task.repo_full_name, pr_number) + lock_entry = self._setup_awareness_locks.get(lock_key) + if lock_entry is None: + lock, waiting_tasks = asyncio.Lock(), 0 + else: + lock, waiting_tasks = lock_entry + self._setup_awareness_locks[lock_key] = (lock, waiting_tasks + 1) + try: + async with lock: + existing_comments = await self.github_client.get_issue_comments( + task.repo_full_name, pr_number, installation_id + ) + if existing_comments is None: + logger.warning( + "setup_awareness_skipped_lookup_error", pr_number=pr_number, repo=task.repo_full_name + ) + return + if self._has_setup_awareness_comment(existing_comments): + logger.info("setup_awareness_skipped_existing", pr_number=pr_number, repo=task.repo_full_name) + return + + welcome_comment = github_formatter.format_rules_not_configured_comment( + repo_full_name=task.repo_full_name, + installation_id=installation_id, + ) + result = await self.github_client.create_pull_request_comment( + task.repo_full_name, pr_number, welcome_comment, installation_id + ) + if result: + logger.info("setup_awareness_posted", pr_number=pr_number, repo=task.repo_full_name) + else: + logger.warning("setup_awareness_post_failed", pr_number=pr_number, repo=task.repo_full_name) + except Exception as comment_err: + logger.warning( + "setup_awareness_post_failed", + pr_number=pr_number, + repo=task.repo_full_name, + error=str(comment_err), + ) + finally: + current_lock, waiting_tasks = self._setup_awareness_locks.get(lock_key, (lock, 1)) + if current_lock is lock: + if waiting_tasks == 1: + self._setup_awareness_locks.pop(lock_key, None) + else: + self._setup_awareness_locks[lock_key] = (lock, waiting_tasks - 1) 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. + Uses the rendered violation fields to create a fingerprint. This allows detecting identical violation sets regardless of delivery_id. """ # Sort violations to ensure consistent ordering @@ -367,35 +607,31 @@ def _compute_violations_hash(self, violations: list[Violation]) -> str: # 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 ''}") + details = json.dumps(v.details, sort_keys=True, default=str, separators=(",", ":")) + signature_parts.append( + f"{v.rule_id or ''}|{v.rule_description}|{v.message}|{v.severity.value if v.severity else ''}|" + f"{v.how_to_fix or ''}|{details}" + ) 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 + @staticmethod + def _is_watchflow_comment(comment: dict[str, Any]) -> bool: + expected_author = f"{config.github.app_name}[bot]" + return (comment.get("user") or {}).get("login", "").lower() == expected_author.lower() + + def _has_setup_awareness_comment(self, comments: list[dict[str, Any]]) -> bool: + """Recognize marked comments and the legacy setup message emitted before markers existed.""" + legacy_heading = "watchflow rules not configured" + return any( + self._is_watchflow_comment(comment) + and ( + github_formatter.RULES_NOT_CONFIGURED_COMMENT_MARKER in str(comment.get("body") or "") + or legacy_heading in str(comment.get("body") or "").lower() + ) + for comment in comments + ) async def prepare_webhook_data(self, task: Task) -> dict[str, Any]: """Extract data available in webhook payload.""" diff --git a/src/integrations/github/api.py b/src/integrations/github/api.py index 8ccc29f..b1ad0fd 100644 --- a/src/integrations/github/api.py +++ b/src/integrations/github/api.py @@ -964,33 +964,47 @@ async def review_deployment_protection_rule( logger.error(f"Error reviewing deployment protection rule: {e}") return None - async def get_issue_comments(self, repo: str, issue_number: int, installation_id: int) -> list[dict[str, Any]]: - """Get comments for an issue.""" + async def get_issue_comments( + self, repo: str, issue_number: int, installation_id: int + ) -> list[dict[str, Any]] | None: + """Get every comment for an issue, or ``None`` if the request fails.""" try: token = await self.get_installation_access_token(installation_id) if not token: logger.error(f"Failed to get installation token for {installation_id}") - return [] + return None headers = {"Authorization": f"Bearer {token}", "Accept": "application/vnd.github.v3+json"} url = f"{config.github.api_base_url}/repos/{repo}/issues/{issue_number}/comments" session = await self._get_session() - async with session.get(url, headers=headers) as response: - if response.status == 200: + comments: list[dict[str, Any]] = [] + page = 1 + while True: + async with session.get(url, headers=headers, params={"per_page": 100, "page": page}) as response: + if response.status != 200: + error_text = await response.text() + logger.error( + f"Failed to get comments for issue #{issue_number} in {repo}. " + f"Status: {response.status}, Response: {error_text}" + ) + return None + result = await response.json() - logger.info(f"Retrieved {len(result)} comments for issue #{issue_number} in {repo}") - return cast("list[dict[str, Any]]", result) - else: - error_text = await response.text() - logger.error( - f"Failed to get comments for issue #{issue_number} in {repo}. Status: {response.status}, Response: {error_text}" - ) - return [] + if not isinstance(result, list): + logger.error(f"Unexpected comments response for issue #{issue_number} in {repo}") + return None + + page_comments = cast("list[dict[str, Any]]", result) + comments.extend(page_comments) + if len(page_comments) < 100: + logger.info(f"Retrieved {len(comments)} comments for issue #{issue_number} in {repo}") + return comments + page += 1 except Exception as e: logger.error(f"Error getting comments for issue #{issue_number} in {repo}: {e}") - return [] + return None async def update_deployment_status( self, callback_url: str, state: str, description: str, environment_url: str | None = None diff --git a/src/integrations/github/check_runs.py b/src/integrations/github/check_runs.py index 84669f7..c5cd6b3 100644 --- a/src/integrations/github/check_runs.py +++ b/src/integrations/github/check_runs.py @@ -101,6 +101,7 @@ async def create_acknowledgment_check_run( output_data = github_formatter.format_acknowledgment_check_run( acknowledgable_violations, violations, acknowledgments ) + output_data["text"] += f"\n\n{github_formatter.format_violations_state_marker(violations)}" await self.github_client.create_check_run( repo=repo, diff --git a/src/presentation/github_formatter.py b/src/presentation/github_formatter.py index 2367f19..1938788 100644 --- a/src/presentation/github_formatter.py +++ b/src/presentation/github_formatter.py @@ -1,9 +1,20 @@ +import base64 +import json import logging +import re +from binascii import Error as BinasciiError from typing import Any from src.agents.base import AgentResult from src.core.models import Acknowledgment, Severity, Violation +RULES_NOT_CONFIGURED_COMMENT_MARKER = "" +VIOLATIONS_CURRENT_REPORT_MARKER_PREFIX = "" +) +VIOLATIONS_HASH_MARKER_PATTERN = re.compile(r"") + logger = logging.getLogger(__name__) SEVERITY_EMOJI = { @@ -125,7 +136,10 @@ def format_check_run_output( return { "title": "All rules passed", "summary": "✅ No rule violations detected", - "text": "All configured rules in `.watchflow/rules.yaml` have passed successfully.", + "text": ( + "All configured rules in `.watchflow/rules.yaml` have passed successfully.\n\n" + f"{format_violations_state_marker([])}" + ), } # Group violations by severity @@ -156,6 +170,7 @@ def format_check_run_output( text += _build_collapsible_violations_text(violations) text += "---\n" text += "💡 *To configure rules, edit the `.watchflow/rules.yaml` file in this repository.*" + text += f"\n\n{format_violations_state_marker(violations)}" return {"title": f"{len(violations)} rule violations found", "summary": summary, "text": text} @@ -172,6 +187,7 @@ def format_rules_not_configured_comment( landing_url = f"https://watchflow.dev/analyze?repo={repo_full_name}" return ( + f"{RULES_NOT_CONFIGURED_COMMENT_MARKER}\n" "## ⚙️ Watchflow rules not configured\n\n" "No rules file found in your repository. Watchflow can help enforce governance rules for your team.\n\n" "**Quick setup:**\n" @@ -230,7 +246,54 @@ def format_suggested_rules_ambiguous_comment( return "\n".join(lines) -def format_violations_comment(violations: list[Violation], content_hash: str | None = None) -> str: +def build_violations_state(violations: list[Violation]) -> list[dict[str, str]]: + """Build stable, minimal state used to compare violation severity across evaluations.""" + state = [] + for violation in violations: + identity = violation.rule_id or violation.rule_description + state.append( + { + "id": identity, + "severity": violation.severity.value + if hasattr(violation.severity, "value") + else str(violation.severity), + } + ) + return sorted(state, key=lambda item: (item["id"], item["severity"])) + + +def format_violations_state_marker(violations: list[Violation]) -> str: + """Encode a versioned violation state in an invisible, HTML-comment-safe marker.""" + payload = json.dumps( + {"version": 1, "violations": build_violations_state(violations)}, separators=(",", ":"), sort_keys=True + ) + encoded = base64.b64encode(payload.encode("utf-8")).decode("ascii") + return f"{VIOLATIONS_CURRENT_REPORT_MARKER_PREFIX}{encoded} -->" + + +def parse_violations_state_marker(comment_body: str) -> list[dict[str, str]] | None: + """Return a canonical report's saved state, or ``None`` when it is absent or malformed.""" + match = VIOLATIONS_CURRENT_REPORT_MARKER_PATTERN.search(comment_body) + if not match: + return None + try: + payload = json.loads(base64.b64decode(match.group(1)).decode("utf-8")) + violations = payload.get("violations") + if payload.get("version") != 1 or not isinstance(violations, list): + return None + if not all( + isinstance(item, dict) and isinstance(item.get("id"), str) and isinstance(item.get("severity"), str) + for item in violations + ): + return None + return [{"id": item["id"], "severity": item["severity"]} for item in violations] + except (BinasciiError, UnicodeDecodeError, ValueError, json.JSONDecodeError): + return None + + +def format_violations_comment( + violations: list[Violation], content_hash: str | None = None, current_report: bool = False +) -> str: """Format violations as a GitHub comment. Args: @@ -244,8 +307,10 @@ def format_violations_comment(violations: list[Violation], content_hash: str | N if not violations: return "" - # Add hidden HTML marker for deduplication (not visible in rendered markdown) + # Add hidden HTML markers for deduplication and canonical-report state. marker = f"\n" if content_hash else "" + if current_report: + marker += f"{format_violations_state_marker(violations)}\n" comment = marker comment += f"### 🛡️ Watchflow Governance Checks\n**Status:** ❌ {len(violations)} Violations Found\n\n" @@ -254,6 +319,7 @@ def format_violations_comment(violations: list[Violation], content_hash: str | N comment += ( "💡 *Reply with `@watchflow ack [reason]` to override these rules, or `@watchflow help` for commands.*\n\n" ) + comment += "*This is a snapshot. Check **Watchflow Rules** for the current result.*\n\n" comment += ( "Thanks for using [Watchflow](https://watchflow.dev)! It's completely free for OSS and private repositories. " ) @@ -262,6 +328,31 @@ def format_violations_comment(violations: list[Violation], content_hash: str | N return comment +def format_violation_regression_alert( + trigger: str, + added: list[Violation], + worsened: list[tuple[Violation, str]], + current_violations: list[Violation], +) -> str: + """Format a concise timeline alert for newly introduced or worsened violations.""" + lines = ["### 🛡️ Watchflow Governance Update", "", f"After {trigger}, Watchflow found a regression:", ""] + for violation in added: + severity = violation.severity.value if hasattr(violation.severity, "value") else str(violation.severity) + lines.append(f"- New **{severity}** violation: {violation.rule_description}") + for violation, previous_severity in worsened: + severity = violation.severity.value if hasattr(violation.severity, "value") else str(violation.severity) + lines.append(f"- **{violation.rule_description}** increased from **{previous_severity}** to **{severity}**") + lines.extend( + [ + "", + "This alert highlights only new or worsened findings. See **Watchflow Rules** in Checks for the complete current result.", + "", + format_violations_state_marker(current_violations), + ] + ) + return "\n".join(lines) + + def format_acknowledgment_summary( acknowledgable_violations: list[Violation], acknowledgments: dict[str, Acknowledgment] ) -> str: diff --git a/tests/unit/event_processors/test_pull_request_processor.py b/tests/unit/event_processors/test_pull_request_processor.py index 6e5049b..b4b4c33 100644 --- a/tests/unit/event_processors/test_pull_request_processor.py +++ b/tests/unit/event_processors/test_pull_request_processor.py @@ -1,11 +1,19 @@ +import asyncio from unittest.mock import AsyncMock, MagicMock import pytest +from src.core.config import config from src.core.models import Violation from src.event_processors.pull_request.enricher import PullRequestEnricher from src.event_processors.pull_request.processor import PullRequestProcessor from src.integrations.github.check_runs import CheckRunManager +from src.presentation.github_formatter import ( + RULES_NOT_CONFIGURED_COMMENT_MARKER, + build_violations_state, + format_check_run_output, +) +from src.rules.loaders.github_loader import RulesFileNotFoundError from src.tasks.task_queue import Task @@ -18,21 +26,44 @@ def mock_agent(): @pytest.fixture def processor(monkeypatch, mock_agent): monkeypatch.setattr("src.event_processors.pull_request.processor.get_agent", lambda x: mock_agent) + monkeypatch.setattr(config.github, "app_name", "watchflow") proc = PullRequestProcessor() # Create a mock for the GitHub client that returns a token mock_github_client = AsyncMock() mock_github_client.get_installation_access_token.return_value = "fake_token" + mock_github_client.get_issue_comments.return_value = [] + mock_github_client.get_check_runs.return_value = [] + mock_github_client.create_pull_request_comment.return_value = {"id": 1} # Patch the instance's github_client proc.github_client = mock_github_client proc.enricher = MagicMock(spec=PullRequestEnricher) + proc.enricher.parse_acknowledgments.return_value = {} proc.check_run_manager = AsyncMock(spec=CheckRunManager) return proc +def missing_rules_task(action: str) -> MagicMock: + task = MagicMock(spec=Task) + task.repo_full_name = "owner/repo" + task.installation_id = 1 + task.payload = { + "action": action, + "repository": {"default_branch": "main"}, + "pull_request": { + "number": 123, + "state": "open", + "head": {"sha": "sha123"}, + # Avoid the unrelated agentic scan in these onboarding tests. + "base": {"ref": "release"}, + }, + } + return task + + @pytest.mark.asyncio async def test_process_success(processor, mock_agent): task = MagicMock(spec=Task) @@ -41,7 +72,7 @@ async def test_process_success(processor, mock_agent): task.payload = {"pull_request": {"number": 1, "head": {"sha": "sha123"}}} processor.enricher.enrich_event_data.return_value = {"enriched": "data"} - processor.enricher.fetch_acknowledgments.return_value = {} + processor.github_client.get_issue_comments = AsyncMock(return_value=[]) processor.rule_provider.get_rules = AsyncMock(return_value=[]) mock_agent.execute.return_value = MagicMock(data={"evaluation_result": MagicMock(violations=[])}) @@ -101,6 +132,31 @@ async def test_process_with_violations(processor, mock_agent): processor.check_run_manager.create_check_run.assert_awaited_once() +@pytest.mark.asyncio +async def test_process_fetches_a_fresh_comment_snapshot_before_syncing_violation_report(processor, mock_agent): + task = MagicMock(spec=Task) + task.repo_full_name = "owner/repo" + task.installation_id = 1 + task.payload = { + "repository": {"default_branch": "main"}, + "pull_request": { + "number": 1, + "head": {"sha": "sha123"}, + "base": {"ref": "release"}, + }, + } + processor.enricher.enrich_event_data.return_value = {"enriched": "data"} + processor.github_client.get_issue_comments = AsyncMock(return_value=[]) + processor.rule_provider.get_rules = AsyncMock(return_value=[]) + violation = Violation(rule_description="Rule 1", severity="high", message="Violation message") + mock_agent.execute.return_value = MagicMock(data={"evaluation_result": MagicMock(violations=[violation])}) + + await processor.process(task) + + assert processor.github_client.get_issue_comments.await_count == 2 + processor.github_client.get_issue_comments.assert_awaited_with("owner/repo", 1, 1) + + @pytest.mark.asyncio async def test_compute_violations_hash_stable_ordering(processor): """Test that violations hash is stable regardless of input order.""" @@ -132,99 +188,367 @@ async def test_compute_violations_hash_different_for_different_violations(proces @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"}, - ] +async def test_initial_violations_create_a_marked_current_report(processor): + """The first violation result creates the canonical report without a delta alert.""" + from src.core.models import Severity + + task = violation_task() + violations = [Violation(rule_description="Rule A", severity=Severity.HIGH, message="Message 1")] + processor.github_client.get_issue_comments = AsyncMock(return_value=[]) + + await processor._post_violations_to_github(task, violations) + + processor.github_client.create_pull_request_comment.assert_awaited_once() + report = processor.github_client.create_pull_request_comment.call_args.args[2] + assert "watchflow:report=violations-current" in report + + +@pytest.mark.asyncio +async def test_first_violation_after_a_passing_check_creates_a_full_snapshot(processor): + task = violation_task() + violation = Violation(rule_id="description", rule_description="Description", severity="high", message="Missing") + + await processor._post_violations_to_github(task, []) + await processor._post_violations_to_github(task, [violation]) + + processor.github_client.create_pull_request_comment.assert_awaited_once() + comment = processor.github_client.create_pull_request_comment.call_args.args[2] + assert "### 🛡️ Watchflow Governance Checks" in comment + assert "Governance Update" not in comment + + +def violation_task(event_type: str = "pull_request", action: str = "synchronize") -> MagicMock: + task = MagicMock(spec=Task) + task.repo_full_name = "owner/repo" + task.installation_id = 1 + task.event_type = event_type + task.payload = { + "action": action, + "pull_request": {"number": 123}, + "review": {"user": {"login": "coderabbitai"}}, + } + return task + + +@pytest.mark.asyncio +async def test_identical_violations_leave_current_report_untouched(processor): + violations = [ + Violation(rule_id="description", rule_description="Description", severity="medium", message="Missing") + ] + + await processor._post_violations_to_github(violation_task(), violations, build_violations_state(violations), True) + + processor.github_client.create_pull_request_comment.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_improved_violations_refresh_checks_without_a_timeline_comment(processor): + previous = [ + Violation(rule_id="description", rule_description="Description", severity="medium", message="Missing"), + Violation(rule_id="changelog", rule_description="Changelog", severity="medium", message="Missing"), + Violation(rule_id="body", rule_description="Body", severity="medium", message="Empty"), + ] + current = previous[:2] + await processor._post_violations_to_github( + violation_task("pull_request_review", "submitted"), current, build_violations_state(previous), True ) - has_duplicate = await processor._has_duplicate_comment("owner/repo", 123, "abc123def456", 1) + processor.github_client.create_pull_request_comment.assert_not_awaited() + - assert has_duplicate is True +@pytest.mark.asyncio +async def test_new_violation_posts_a_regression_alert_without_editing_the_snapshot(processor): + previous = [Violation(rule_id="description", rule_description="Description", severity="medium", message="Missing")] + added = Violation(rule_id="changelog", rule_description="Changelog", severity="high", message="Missing") + await processor._post_violations_to_github( + violation_task("pull_request_review", "submitted"), [*previous, added], build_violations_state(previous), True + ) + + processor.github_client.create_pull_request_comment.assert_awaited_once() + alert = processor.github_client.create_pull_request_comment.call_args.args[2] + assert "review was submitted by @coderabbitai" in alert + assert "New **high** violation: Changelog" in alert + assert "highlights only new or worsened findings" in alert @pytest.mark.asyncio -async def test_has_duplicate_comment_no_match(processor): - """Test that comments without matching hash are not detected as duplicates.""" +async def test_severity_increase_posts_regression_alert(processor): + previous = [Violation(rule_id="description", rule_description="Description", severity="low", message="Missing")] + worsened = Violation(rule_id="description", rule_description="Description", severity="high", message="Missing") + await processor._post_violations_to_github(violation_task(), [worsened], build_violations_state(previous), True) + + alert = processor.github_client.create_pull_request_comment.call_args.args[2] + assert "increased from **low** to **high**" in alert + + +def test_mixed_severity_changes_for_the_same_rule_do_not_create_a_false_regression(processor): + previous = [ + {"id": "rule-a", "severity": "high"}, + {"id": "rule-a", "severity": "low"}, + ] + current = [Violation(rule_id="rule-a", rule_description="Rule A", severity="medium", message="Missing")] + + added, worsened = processor._find_violation_regressions(previous, current) + + assert added == [] + assert worsened == [] + + +@pytest.mark.asyncio +async def test_resolved_violations_do_not_create_a_timeline_comment(processor): + previous = [Violation(rule_id="description", rule_description="Description", severity="medium", message="Missing")] + + await processor._post_violations_to_github(violation_task(), [], build_violations_state(previous), True) + + processor.github_client.create_pull_request_comment.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_violation_reintroduced_after_resolution_posts_a_regression_alert(processor): + previous = [Violation(rule_id="description", rule_description="Description", severity="medium", message="Missing")] + + await processor._post_violations_to_github(violation_task(), [], build_violations_state(previous), True) + await processor._post_violations_to_github(violation_task(), previous, build_violations_state(previous), True) + + processor.github_client.create_pull_request_comment.assert_awaited_once() + alert = processor.github_client.create_pull_request_comment.call_args.args[2] + assert "Governance Update" in alert + assert "New **medium** violation: Description" in alert + + +@pytest.mark.asyncio +async def test_previous_state_is_loaded_from_the_prior_check_on_a_new_commit(processor): + previous = [Violation(rule_id="description", rule_description="Description", severity="medium", message="Missing")] + task = violation_task("pull_request", "synchronize") + task.payload["before"] = "previous-sha" + processor.github_client.get_check_runs.return_value = [ + { + "id": 7, + "name": "Watchflow Rules", + "output": format_check_run_output(previous), + } + ] + + state, has_previous_report = await processor._get_previous_violation_state(task, "current-sha", 1) + + assert state == build_violations_state(previous) + assert has_previous_report is True + processor.github_client.get_check_runs.assert_awaited_once_with("owner/repo", "previous-sha", 1) + processor.github_client.get_issue_comments.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_passing_check_without_a_report_keeps_the_first_violation_eligible_for_a_snapshot(processor): + task = violation_task("pull_request", "synchronize") + task.payload["before"] = "previous-sha" + processor.github_client.get_check_runs.return_value = [ + { + "id": 7, + "name": "Watchflow Rules", + "output": format_check_run_output([]), + } + ] + + state, has_previous_report = await processor._get_previous_violation_state(task, "current-sha", 1) + + assert state == [] + assert has_previous_report is False + processor.github_client.get_issue_comments.assert_awaited_once_with("owner/repo", 123, 1) + + +@pytest.mark.asyncio +async def test_legacy_report_is_adopted_silently_and_non_watchflow_markers_are_ignored(processor): + violations = [ + Violation(rule_id="description", rule_description="Description", severity="medium", message="Missing") + ] + legacy = { + "id": 2, + "body": "\nold report", + "user": {"login": "watchflow[bot]"}, + } + imitation = { + "id": 3, + "body": "", + "user": {"login": "someone-else"}, + } + processor.github_client.get_issue_comments.return_value = [imitation, legacy] + + previous_state, has_previous_report = await processor._get_previous_violation_state(violation_task(), "sha123", 1) + await processor._post_violations_to_github(violation_task(), violations, previous_state, has_previous_report) + + processor.github_client.create_pull_request_comment.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_violation_comment_lookup_failure_does_not_create_comments(processor): + processor.github_client.get_issue_comments.return_value = None + violations = [Violation(rule_description="Description", severity="medium", message="Missing")] + + previous_state, has_previous_report = await processor._get_previous_violation_state(violation_task(), "sha123", 1) + await processor._post_violations_to_github(violation_task(), violations, previous_state, has_previous_report) + + processor.github_client.create_pull_request_comment.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_unchanged_remediation_does_not_create_a_follow_up_comment(processor): + previous = [ + Violation( + rule_id="description", + rule_description="Description", + severity="medium", + message="Missing", + how_to_fix="Add a description.", + ) + ] + current = [ + Violation( + rule_id="description", + rule_description="Description", + severity="medium", + message="Missing", + how_to_fix="Explain the implementation and tests.", + ) + ] + await processor._post_violations_to_github(violation_task(), current, build_violations_state(previous), True) + + processor.github_client.create_pull_request_comment.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_concurrent_violation_sync_posts_one_regression_alert(processor): + previous = [Violation(rule_id="description", rule_description="Description", severity="medium", message="Missing")] + current = [ + *previous, + Violation(rule_id="changelog", rule_description="Changelog", severity="high", message="Missing"), + ] + await asyncio.gather( + processor._post_violations_to_github(violation_task(), current, build_violations_state(previous), True), + processor._post_violations_to_github(violation_task(), current, build_violations_state(previous), True), + ) + + processor.github_client.create_pull_request_comment.assert_awaited_once() + + +@pytest.mark.asyncio +async def test_opened_pr_without_rules_posts_one_setup_awareness_comment(processor): + task = missing_rules_task("opened") + processor.enricher.enrich_event_data.return_value = {"enriched": "data"} + processor.rule_provider.get_rules = AsyncMock(side_effect=RulesFileNotFoundError("rules missing")) + processor.github_client.get_issue_comments = AsyncMock(return_value=[]) + processor.github_client.create_pull_request_comment = AsyncMock(return_value={"id": 1}) + + result = await processor.process(task) + + assert result.success is True + processor.check_run_manager.create_check_run.assert_awaited_once() + processor.github_client.create_pull_request_comment.assert_awaited_once() + comment = processor.github_client.create_pull_request_comment.call_args.args[2] + assert RULES_NOT_CONFIGURED_COMMENT_MARKER in comment + + +@pytest.mark.asyncio +async def test_opened_pr_with_existing_setup_awareness_comment_does_not_post_again(processor): + task = missing_rules_task("opened") + processor.enricher.enrich_event_data.return_value = {"enriched": "data"} + processor.rule_provider.get_rules = AsyncMock(side_effect=RulesFileNotFoundError("rules missing")) processor.github_client.get_issue_comments = AsyncMock( return_value=[ - {"body": "Some other comment"}, - {"body": "\n### Violations\nContent here"}, + { + "body": RULES_NOT_CONFIGURED_COMMENT_MARKER, + "user": {"login": "watchflow[bot]"}, + } ] ) - has_duplicate = await processor._has_duplicate_comment("owner/repo", 123, "abc123def456", 1) + await processor.process(task) - assert has_duplicate is False + processor.check_run_manager.create_check_run.assert_awaited_once() + processor.github_client.create_pull_request_comment.assert_not_awaited() @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=[]) +async def test_opened_pr_with_legacy_setup_awareness_comment_does_not_post_again(processor): + task = missing_rules_task("opened") + processor.enricher.enrich_event_data.return_value = {"enriched": "data"} + processor.rule_provider.get_rules = AsyncMock(side_effect=RulesFileNotFoundError("rules missing")) + processor.github_client.get_issue_comments = AsyncMock( + return_value=[ + { + "body": "## ⚙️ Watchflow rules not configured\n\nNo rules file found.", + "user": {"login": "watchflow[bot]"}, + } + ] + ) - has_duplicate = await processor._has_duplicate_comment("owner/repo", 123, "abc123def456", 1) + await processor.process(task) - assert has_duplicate is False + processor.github_client.create_pull_request_comment.assert_not_awaited() @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")) +@pytest.mark.parametrize( + "action", ["synchronize", "reopened", "ready_for_review", "submitted", "dismissed", "resolved", "unresolved"] +) +async def test_non_opened_events_without_rules_do_not_post_setup_awareness_comment(processor, action): + task = missing_rules_task(action) + processor.enricher.enrich_event_data.return_value = {"enriched": "data"} + processor.rule_provider.get_rules = AsyncMock(side_effect=RulesFileNotFoundError("rules missing")) - has_duplicate = await processor._has_duplicate_comment("owner/repo", 123, "abc123def456", 1) + await processor.process(task) - assert has_duplicate is False # Fail open to allow posting + processor.check_run_manager.create_check_run.assert_awaited_once() + processor.github_client.get_issue_comments.assert_not_awaited() + processor.github_client.create_pull_request_comment.assert_not_awaited() @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 +async def test_setup_awareness_lookup_failure_does_not_post_comment(processor): + task = missing_rules_task("opened") + processor.enricher.enrich_event_data.return_value = {"enriched": "data"} + processor.rule_provider.get_rules = AsyncMock(side_effect=RulesFileNotFoundError("rules missing")) + processor.github_client.get_issue_comments = AsyncMock(return_value=None) - task = MagicMock(spec=Task) - task.repo_full_name = "owner/repo" - task.installation_id = 1 - task.payload = {"pull_request": {"number": 123}} + await processor.process(task) - violations = [Violation(rule_description="Rule A", severity=Severity.HIGH, message="Message 1")] + processor.check_run_manager.create_check_run.assert_awaited_once() + processor.github_client.create_pull_request_comment.assert_not_awaited() - # Mock that a duplicate exists + +@pytest.mark.asyncio +async def test_marker_from_non_watchflow_user_does_not_suppress_setup_awareness(processor): + task = missing_rules_task("opened") + processor.enricher.enrich_event_data.return_value = {"enriched": "data"} + processor.rule_provider.get_rules = AsyncMock(side_effect=RulesFileNotFoundError("rules missing")) processor.github_client.get_issue_comments = AsyncMock( - return_value=[{"body": "\nContent"}] + return_value=[{"body": RULES_NOT_CONFIGURED_COMMENT_MARKER, "user": {"login": "someone-else"}}] ) + processor.github_client.create_pull_request_comment = AsyncMock(return_value={"id": 1}) - # 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.process(task) - # Should NOT have called create_pull_request_comment - processor.github_client.create_pull_request_comment.assert_not_called() + processor.github_client.create_pull_request_comment.assert_awaited_once() @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 +async def test_concurrent_opened_events_post_one_setup_awareness_comment(processor): + task = missing_rules_task("opened") + existing_comments: list[dict] = [] - task = MagicMock(spec=Task) - task.repo_full_name = "owner/repo" - task.installation_id = 1 - task.payload = {"pull_request": {"number": 123}} + async def get_comments(*_args): + return list(existing_comments) - violations = [Violation(rule_description="Rule A", severity=Severity.HIGH, message="Message 1")] + async def create_comment(_repo, _pr_number, comment, _installation_id): + await asyncio.sleep(0) + existing_comments.append({"body": comment, "user": {"login": "watchflow[bot]"}}) + return {"id": 1} - # Mock that no duplicate exists - processor.github_client.get_issue_comments = AsyncMock(return_value=[]) - processor.github_client.create_pull_request_comment = AsyncMock() + processor.github_client.get_issue_comments = AsyncMock(side_effect=get_comments) + processor.github_client.create_pull_request_comment = AsyncMock(side_effect=create_comment) - await processor._post_violations_to_github(task, violations) + await asyncio.gather( + processor._post_rules_not_configured_comment(task, 123, 1), + processor._post_rules_not_configured_comment(task, 123, 1), + ) - # Should have called create_pull_request_comment - processor.github_client.create_pull_request_comment.assert_called_once() + processor.github_client.create_pull_request_comment.assert_awaited_once() diff --git a/tests/unit/integrations/github/test_api.py b/tests/unit/integrations/github/test_api.py index 587592f..35e0b72 100644 --- a/tests/unit/integrations/github/test_api.py +++ b/tests/unit/integrations/github/test_api.py @@ -113,6 +113,31 @@ async def test_get_installation_access_token_failure(github_client, mock_aiohttp assert token is None +@pytest.mark.asyncio +async def test_get_issue_comments_paginates_all_results(github_client, mock_aiohttp_session): + github_client._token_cache[123] = "access_token" + first_page = mock_aiohttp_session.create_mock_response(200, json_data=[{"id": i} for i in range(100)]) + second_page = mock_aiohttp_session.create_mock_response(200, json_data=[{"id": 100}]) + mock_aiohttp_session.get.side_effect = [first_page, second_page] + + comments = await github_client.get_issue_comments("owner/repo", 42, 123) + + assert comments == [{"id": i} for i in range(101)] + assert mock_aiohttp_session.get.call_count == 2 + assert mock_aiohttp_session.get.call_args_list[0].kwargs["params"] == {"per_page": 100, "page": 1} + assert mock_aiohttp_session.get.call_args_list[1].kwargs["params"] == {"per_page": 100, "page": 2} + + +@pytest.mark.asyncio +async def test_get_issue_comments_returns_none_on_api_failure(github_client, mock_aiohttp_session): + github_client._token_cache[123] = "access_token" + mock_aiohttp_session.get.return_value = mock_aiohttp_session.create_mock_response(500, text_data="Server error") + + comments = await github_client.get_issue_comments("owner/repo", 42, 123) + + assert comments is None + + @pytest.mark.asyncio async def test_get_repository_success(github_client, mock_aiohttp_session): # Initial token mock diff --git a/tests/unit/presentation/test_github_formatter.py b/tests/unit/presentation/test_github_formatter.py index efa51c4..9aafe44 100644 --- a/tests/unit/presentation/test_github_formatter.py +++ b/tests/unit/presentation/test_github_formatter.py @@ -1,9 +1,14 @@ from src.core.models import Acknowledgment, Severity, Violation from src.presentation.github_formatter import ( + RULES_NOT_CONFIGURED_COMMENT_MARKER, + VIOLATIONS_CURRENT_REPORT_MARKER_PREFIX, format_acknowledgment_summary, format_check_run_output, + format_rules_not_configured_comment, + format_violation_regression_alert, format_violations_comment, format_violations_for_check_run, + parse_violations_state_marker, ) @@ -86,6 +91,12 @@ def test_format_check_run_output_rules_not_configured(): assert f"installation_id={inst_id}" in output["text"] +def test_format_rules_not_configured_comment_includes_deduplication_marker(): + comment = format_rules_not_configured_comment(repo_full_name="owner/repo", installation_id=123) + + assert comment.startswith(f"{RULES_NOT_CONFIGURED_COMMENT_MARKER}\n") + + def test_format_acknowledgment_summary(): violations = [Violation(rule_description="PR Title", severity=Severity.MEDIUM, message="Bad title")] acks = {"pr-title": Acknowledgment(rule_id="pr-title", reason="One-off", commenter="tom")} @@ -118,6 +129,27 @@ def test_format_violations_comment_includes_hash_marker(): assert "### 🛡️ Watchflow Governance Checks" in comment +def test_current_violations_report_includes_parseable_state_marker(): + violations = [Violation(rule_id="rule-a", rule_description="Rule A", severity=Severity.HIGH, message="Error")] + + comment = format_violations_comment(violations, content_hash="abc123def456", current_report=True) + + assert VIOLATIONS_CURRENT_REPORT_MARKER_PREFIX in comment + assert parse_violations_state_marker(comment) == [{"id": "rule-a", "severity": "high"}] + + +def test_check_output_and_regression_alert_include_current_state(): + violation = Violation(rule_description="Rule A", severity=Severity.HIGH, message="Error") + + check_output = format_check_run_output([violation]) + alert = format_violation_regression_alert("a review was submitted by @coderabbitai", [violation], [], [violation]) + + assert parse_violations_state_marker(check_output["text"]) == [{"id": "Rule A", "severity": "high"}] + assert "New **high** violation: Rule A" in alert + assert "Watchflow Rules** in Checks" in alert + assert parse_violations_state_marker(alert) == [{"id": "Rule A", "severity": "high"}] + + def test_format_violations_comment_no_hash_marker_when_not_provided(): """Test that comment does not include marker when content_hash is None.""" violations = [