diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..f5c9c85 --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,276 @@ +# Changelog + +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] -- PR #59 + +### Added + +- **Diff pattern scanning** -- `DiffPatternCondition` checks added lines in PR + diffs against user-defined restricted regex patterns (e.g. `console\.log`, + `TODO:`). Violations include the filename and matched patterns. +- **Security pattern detection** -- `SecurityPatternCondition` flags hardcoded + secrets, API keys, and sensitive data in PR diffs with CRITICAL severity. + Both conditions share a new `_PatchPatternCondition` base class to eliminate + duplication. +- **Unresolved review comments gate** -- `UnresolvedCommentsCondition` blocks + PR merges when unresolved (non-outdated) review comment threads exist, + using GraphQL `reviewThreads` data from the enricher. +- **Test coverage enforcement** -- `TestCoverageCondition` requires that PRs + modifying source files also touch test files matching a configurable regex + pattern (`test_file_pattern`). +- **Comment response time SLA** -- `CommentResponseTimeCondition` flags + unresolved review threads that have exceeded a configurable hour-based SLA + (`max_comment_response_time_hours`). +- **Signed commits verification** -- `SignedCommitsCondition` ensures all + commits in a PR are cryptographically signed (GPG/SSH/S/MIME), for + regulated environments that require commit provenance. +- **Changelog requirement** -- `ChangelogRequiredCondition` blocks PRs that + modify source code without a corresponding `CHANGELOG` or `.changeset` + update. +- **Self-approval prevention** -- `NoSelfApprovalCondition` enforces + separation of duties by preventing PR authors from approving their own + code (CRITICAL severity). +- **Cross-team approval** -- `CrossTeamApprovalCondition` requires approvals + from members of specified GitHub teams before merge. Uses a simplified + `requested_teams` check (full team-membership resolution via GraphQL is + tracked for a future iteration). +- **Diff parsing utilities** -- New `src/rules/utils/diff.py` module with + `extract_added_lines`, `extract_removed_lines`, and + `match_patterns_in_patch` for reusable patch analysis. +- **CODEOWNERS parser** -- New `src/rules/utils/codeowners.py` with + `CodeOwnersParser` class supporting glob-to-regex conversion, owner + lookup, and critical-file detection. CODEOWNERS content is now fetched + dynamically from the GitHub API instead of reading from disk. +- **Webhook handlers for review events** -- `PullRequestReviewEventHandler` + and `PullRequestReviewThreadEventHandler` re-evaluate PR rules when + reviews are submitted/dismissed or threads are resolved/unresolved. +- **Review thread enrichment** -- `PullRequestEnricher` now fetches + `reviewThreads` via GraphQL and attaches them to the event context, + enabling `UnresolvedCommentsCondition` and `CommentResponseTimeCondition`. +- **Full rule evaluation wiring** -- All new conditions are registered in + `ConditionRegistry` (`AVAILABLE_CONDITIONS`, `RULE_ID_TO_CONDITION`) with + corresponding `RuleID` enum values, violation-text mappings, and + human-readable descriptions so they are routed through the fast + condition-class evaluation path and support acknowledgment workflows. + +### Changed + +- **GraphQL client consolidation** -- Removed the standalone + `graphql_client.py` module; all GraphQL operations now go through the + unified `GitHubAPI` class with Pydantic-typed response models. +- **CODEOWNERS fetched from API** -- `PathHasCodeOwnerCondition` and + `RequireCodeOwnerReviewersCondition` now receive CODEOWNERS content via + the event context (fetched by the enricher) rather than reading from the + local filesystem. +- **`_PatchPatternCondition` base class** -- `DiffPatternCondition` and + `SecurityPatternCondition` now share a common abstract base, reducing + ~60 lines of duplicated iteration/matching logic. +- **Removed redundant `validate()` overrides** -- Conditions in + `compliance.py` and `access_control_advanced.py` that simply delegated to + `evaluate()` now rely on `BaseCondition.validate()` which does the same + thing. + +### Fixed + +- **Fail-closed on invalid regex** -- `TestCoverageCondition` now returns a + violation (and `validate()` returns `False`) when `test_file_pattern` is + an invalid regex, instead of silently passing. +- **Consistent file-extension filtering** -- `TestCoverageCondition.validate()` + now ignores `.txt` and `.json` files, matching the behavior of `evaluate()`. +- **`max_hours=0` edge case** -- `CommentResponseTimeCondition` now uses + `if max_hours is None` instead of `if not max_hours`, so a 0-hour SLA + (immediate response required) is correctly enforced. +- **Overly generic violation mapping key** -- Changed the + `COMMENT_RESPONSE_TIME` acknowledgment mapping from `"exceeded the"` to + `"response SLA"` to avoid false matches against unrelated violation text. + +## [2026-02-27] -- PRs #54, #58 + +### Added + +- **Disabled rule filtering** -- Rules with `enabled: false` in + `rules.yaml` are now skipped during loading. +- **CodeRabbit-style PR comments** -- Collapsible `
` sections for + violations, acknowledgment summaries, and check run output. +- **Watchflow footer** -- Branded footer appended to PR comments. +- **Severity grouping fix** -- `INFO` severity rules are now grouped + correctly instead of falling back to `LOW`. + +### Changed + +- **Default rules aligned with watchflow.dev** -- Canonical rule set updated + to match the published documentation examples. +- **`max_pr_loc` parameter alias** -- `MaxPrLocCondition` now accepts + `max_pr_loc` and `max_changed_lines` in addition to `max_lines`. +- **CODEOWNERS reviewer exclusion** -- PR author is excluded from the + required code-owner reviewers list. +- **Legacy rule ID references removed** -- Generated PR comments and error + messages no longer expose internal `RuleID` strings. + +### Fixed + +- **Acknowledgment text matching** -- Violation text keys updated to + exactly match the messages emitted by conditions. +- **GitHub App auth env vars** -- Standardized to `APP_CLIENT_ID_GITHUB` + and `APP_CLIENT_SECRET_GITHUB`. + +## [2026-02-26] -- PRs #43 (cont.), event filtering + +### Added + +- **Event filtering** -- Irrelevant GitHub events (e.g. bot-only, + label-only) are now dropped before reaching the rule engine, reducing + noise and unnecessary LLM calls. + +### Fixed + +- **Deployment status blocking** -- Resolved an issue where deployment + status events could block without a clear reason. +- **Deployment approval gating** -- Addressed CodeRabbit feedback on + retry logic, falsy checks, and callback URL handling. + +## [2026-01-31] -- PR #43 + +### Added + +- **Core event processing infrastructure** -- `PullRequestProcessor`, + `PushEventProcessor`, `DeploymentProcessor`, and `CheckRunProcessor` + with enrichment, rule evaluation, and GitHub reporting pipeline. +- **Task queue with deduplication** -- Async `TaskQueue` for enqueuing + webhook processing with delivery-ID-based dedup. +- **Rule engine agent (LangGraph)** -- `RuleEngineAgent` with a multi-node + workflow: analyze rules, select strategy (condition class vs LLM + reasoning vs hybrid), execute, and validate. +- **Acknowledgment agent** -- `AcknowledgmentAgent` parses `@watchflow ack` + comments and maps violations to `RuleID` enum values. +- **Webhook dispatcher and handlers** -- Modular handler registry for + `pull_request`, `push`, `check_run`, `deployment`, `deployment_status`, + `deployment_protection_rule`, `deployment_review`, and `issue_comment` + events. +- **Condition-based rule evaluation** -- `BaseCondition` ABC with + `evaluate()` (returns `list[Violation]`) and `validate()` (legacy bool + interface). Initial conditions: `TitlePatternCondition`, + `MinDescriptionLengthCondition`, `RequiredLabelsCondition`, + `MinApprovalsCondition`, `RequireLinkedIssueCondition`, + `MaxFileSizeCondition`, `MaxPrLocCondition`, `FilePatternCondition`, + `PathHasCodeOwnerCondition`, `RequireCodeOwnerReviewersCondition`, + `CodeOwnersCondition`, `ProtectedBranchesCondition`, + `NoForcePushCondition`, `AuthorTeamCondition`, `AllowedHoursCondition`, + `DaysCondition`, `WeekendCondition`, `WorkflowDurationCondition`. +- **Condition registry** -- `ConditionRegistry` with parameter-pattern + matching to automatically wire YAML rule parameters to condition classes. +- **`RuleID` enum and acknowledgment system** -- Type-safe rule + identifiers, violation-text-to-rule mapping, and acknowledgment comment + parsing. +- **Webhook auth** -- HMAC-SHA256 signature verification for GitHub + webhooks. + +### Changed + +- **Architectural modernization** -- Migrated from monolithic processor to + modular event-processor / agent / handler architecture with Pydantic + models throughout. +- **Documentation overhaul** -- All docs aligned with the rule engine + architecture, description-based rule format, and supported validation + logic. + +### Fixed + +- **Dead code removal** -- Cleaned up unused webhook and PR processing code. +- **JSON parse errors** -- Webhook handler now returns proper error + responses on malformed payloads. +- **WebhookResponse status normalization** -- Consistent status field + values across all handlers. + +## [2025-12-01] -- PRs #27-35 + +### Added + +- **Repository Analysis Agent** -- `RepositoryAnalysisAgent` with LangGraph + workflow analyzing PR history, contributing guidelines, and repository + hygiene. Includes Pydantic models, LLM prompt templates, and API + endpoints for rule recommendations. +- **Diff-aware validators** -- `diff_pattern`, `related_tests`, and + `required_field_in_diff` validators with normalized diff metadata and + LLM-friendly summaries for PR files. +- **Feasibility agent validator selection** -- `FeasibilityAgent` now + dynamically chooses validators from a catalog. +- **AI Immune System metrics** -- Repository health scoring with hygiene + metrics and structured API responses. +- **PR automation** -- Automated PR creation from repository analysis + recommendations. + +### Changed + +- **Diff-aware rule presets** -- Default rule bundles updated to use the + new diff-aware parameters and threading guardrails. + +### Fixed + +- **PR creation 404 prevention** -- Proper error handling for `create_git_ref` + 422 responses and repository analysis caching. +- **Repository analysis reliability** -- Improved logging, formatting, and + content checks in analysis nodes. + +## [2025-10-01] -- PRs #18-21 + +### Added + +- **Multi-provider AI abstraction** -- Provider-agnostic `get_chat_model()` + factory supporting OpenAI, AWS Bedrock, and Google Vertex AI (Model + Garden). Registry pattern for provider selection. +- **Python version compatibility checks** -- Pre-commit hook validates + syntax against target Python version. + +### Changed + +- **Provider-agnostic LLM usage** -- Replaced direct `ChatOpenAI` + instantiation with the `get_chat_model()` abstraction throughout. +- **Module restructuring** -- Reorganized package layout and updated + configuration. + +## [2025-08-05] -- PRs #10-13 + +### Added + +- **CODEOWNERS integration** -- Initial CODEOWNERS file parsing and + contributor analysis. +- **Agent architecture enhancements** -- Improved consistency and + reliability for `FeasibilityAgent` and `RuleEngineAgent`. +- **Structured output for FeasibilityAgent** -- LLM responses parsed into + Pydantic models. +- **Testing framework** -- Coverage reporting, CI test pipeline, and + mocking infrastructure for agents and LLM clients. +- **GitHub Pages documentation** -- MkDocs site deployed via GitHub + Actions. + +### Changed + +- **FastAPI lifespan** -- Replaced deprecated `on_event` handlers with + lifespan context manager. +- **Description-based rule format** -- Rules in YAML now use natural + language descriptions matched to conditions. + +### Fixed + +- **CI pipeline** -- Python setup, coverage reporting, Codecov auth, + MkDocs dependencies. +- **Test isolation** -- Proper mocking of agent creation, config + validation, and LLM client initialization. + +## [2025-07-18] -- Initial release + +### Added + +- **Watchflow AI governance engine** -- First open-source release. + LangGraph-based rule evaluation for GitHub webhook events + (pull requests, pushes, deployments). +- **EKS deployment** -- Helm chart, Kubernetes manifests, and GitHub + Actions workflow for AWS EKS. +- **Pre-commit hooks** -- Ruff linting and formatting, YAML checks, + trailing whitespace, large file detection. +- **Development tooling** -- `uv` package management, development guides, + contributor guidelines. diff --git a/README.md b/README.md index 83a6cb7..00ec90d 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@ GitHub governance that runs where you already work. No new dashboards, no “AI-powered” fluff—just rules in YAML, evaluated on every PR and push, with check runs and comments that maintainers actually read. -Watchflow is the governance layer for your repo: it enforces the policies you define (CODEOWNERS, approvals, linked issues, PR size, title patterns, branch protection) so you don’t have to chase reviewers or guess what’s allowed. Built for teams that still care about traceability and review quality. +Watchflow is the governance layer for your repo: it enforces the policies you define (CODEOWNERS, approvals, linked issues, PR size, title patterns, branch protection, diff scanning, review thread SLAs, signed commits, and more) so you don’t have to chase reviewers or guess what’s allowed. Built for teams that still care about traceability and review quality. --- @@ -46,11 +46,19 @@ Rules are **description + event_types + parameters**. The engine matches paramet | **PR** | `critical_owners: []` / code owners | pull_request | Changes to critical paths require code-owner review. | | **PR** | `require_path_has_code_owner: true` | pull_request | Every changed path must have an owner in CODEOWNERS. | | **PR** | `protected_branches: ["main"]` | pull_request | Block direct targets to these branches. | +| **PR** | `diff_restricted_patterns: ["console\\.log", "TODO:"]` | pull_request | Flag restricted regex patterns in PR diff added lines. | +| **PR** | `security_patterns: ["api_key", "secret"]` | pull_request | Detect hardcoded secrets or sensitive data in diffs (critical). | +| **PR** | `block_on_unresolved_comments: true` | pull_request | Block merge when unresolved review threads exist. | +| **PR** | `require_tests: true` | pull_request | Source changes must include corresponding test file changes. | +| **PR** | `max_comment_response_time_hours: 24` | pull_request | Review threads must be addressed within SLA. | +| **PR** | `require_signed_commits: true` | pull_request | All commits must be cryptographically signed (GPG/SSH). | +| **PR** | `require_changelog_update: true` | pull_request | Source changes must include a CHANGELOG or .changeset update. | +| **PR** | `block_self_approval: true` | pull_request | PR authors cannot approve their own code. | +| **PR** | `required_team_approvals: ["backend", "security"]` | pull_request | Require approvals from specified GitHub teams. | | **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. | | **Time** | `allowed_hours`, `days`, weekend | deployment / workflow | Restrict when actions can run. | -| **Deploy** | `environment`, approvals | deployment | Deployment protection. | 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 a8ed4d9..9163ddd 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, 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, time/deploy rules. See [Configuration](../getting-started/configuration.md). ### PR enricher diff --git a/docs/enterprise-rules-roadmap.md b/docs/enterprise-rules-roadmap.md new file mode 100644 index 0000000..42d6f2b --- /dev/null +++ b/docs/enterprise-rules-roadmap.md @@ -0,0 +1,89 @@ +# Enterprise & Regulated Industry Guardrails + +Watchflow's rule engine supports strict compliance, auditability, and advanced access control for large engineering teams and highly regulated industries (FinTech, HealthTech, Enterprise SaaS). This page tracks what's shipped and what's planned. + +## Implemented + +The following enterprise conditions are fully registered in the condition registry, wired into the evaluation pipeline, and support acknowledgment workflows. See [Configuration](getting-started/configuration.md) for YAML parameter reference. + +### Compliance & security verification + +| Condition | Parameter | Description | +|-----------|-----------|-------------| +| `SignedCommitsCondition` | `require_signed_commits: true` | Ensure all commits are signed (GPG/SSH/S/MIME). Required by SOC2, FedRAMP. | +| `SecurityPatternCondition` | `security_patterns: [...]` | Detect hardcoded secrets, API keys, or sensitive data in PR diffs. | +| `ChangelogRequiredCondition` | `require_changelog_update: true` | Require CHANGELOG or `.changeset` update when source files change. | + +### Advanced access control (separation of duties) + +| Condition | Parameter | Description | +|-----------|-----------|-------------| +| `NoSelfApprovalCondition` | `block_self_approval: true` | Block PR authors from approving their own PRs. SOX/SOC2 requirement. | +| `CrossTeamApprovalCondition` | `required_team_approvals: [...]` | Require approvals from specified GitHub teams. Simplified check; full team-membership resolution via GraphQL is tracked below. | + +### Code quality & review workflow + +| Condition | Parameter | Description | +|-----------|-----------|-------------| +| `DiffPatternCondition` | `diff_restricted_patterns: [...]` | Flag restricted regex patterns in added lines of PR diffs. | +| `UnresolvedCommentsCondition` | `block_on_unresolved_comments: true` | Block merge when unresolved review threads exist. | +| `TestCoverageCondition` | `require_tests: true` | Source changes must include corresponding test file changes. | +| `CommentResponseTimeCondition` | `max_comment_response_time_hours: N` | Enforce SLA for responding to review comments. | + +--- + +## Planned + +### Operations & reliability + +#### `MigrationSafetyCondition` +**Purpose:** If a PR modifies database schemas/migrations (e.g., `alembic/`, `prisma/migrations/`), enforce that it does *not* contain destructive operations like `DROP TABLE` or `DROP COLUMN`. +**Why:** Prevents accidental production data loss. +**Parameters:** `safe_migrations_only: true` + +#### `FeatureFlagRequiredCondition` +**Purpose:** If a PR exceeds a certain size or modifies core routing, ensure a feature flag is added. +**Why:** Enables safe rollbacks and trunk-based development. +**Parameters:** `require_feature_flags_for_large_prs: true` + +### External integrations + +#### `SecretScanningCondition` (Enhanced) +**Purpose:** Integrate with GitHub Advanced Security's native secret scanner alerts, beyond regex matching. +**Parameters:** `block_on_secret_alerts: true` + +#### `BannedDependenciesCondition` +**Purpose:** Parse dependency diffs to block banned licenses (e.g., AGPL) or deprecated libraries. +**Parameters:** `banned_licenses: ["AGPL", "GPL"]`, `banned_packages: ["requests<2.0.0"]` + +#### `JiraTicketStatusCondition` +**Purpose:** Verify Jira ticket state via API (must be "In Progress" or "In Review"). +**Parameters:** `require_active_jira_ticket: true` + +#### `CrossTeamApprovalCondition` -- full team membership +**Purpose:** Resolve reviewer-to-team membership via GraphQL instead of relying on `requested_teams`. +**Tracking:** Depends on GitHub's Team Members API access via GitHub App installation tokens. + +### GitHub ecosystem integrations + +#### `CodeQLAnalysisCondition` +**Purpose:** Block merges if CodeQL has detected critical vulnerabilities in the PR diff. +**How:** Call `code-scanning/alerts` API for the current `head_sha`. +**Parameters:** `block_on_critical_codeql: true` + +#### `DependabotAlertsCondition` +**Purpose:** Ensure PRs don't introduce dependencies with known CVEs. +**How:** Hook into the `dependabot/alerts` REST API. +**Parameters:** `max_dependabot_severity: "high"` + +### Open-source ecosystem integrations + +#### OPA / Rego Validation +**Purpose:** Validate Kubernetes manifests or `.rego` files against OPA engine on PR. + +#### Pydantic Schema Breakage Detection +**Purpose:** Detect backward-incompatible changes to REST API models by diffing ASTs. + +#### Linter Suppression Detection +**Purpose:** Flag PRs that introduce `# noqa`, `# type: ignore`, or `// eslint-disable`. +**Parameters:** `allow_linter_suppressions: false` diff --git a/docs/features.md b/docs/features.md index 0e71ecf..e1855d3 100644 --- a/docs/features.md +++ b/docs/features.md @@ -20,6 +20,10 @@ Rules are **description + event_types + parameters**. The engine matches **param | `critical_owners: []` | CodeOwnersCondition | Changes to critical paths require code-owner review. | | `require_path_has_code_owner: true` | PathHasCodeOwnerCondition | Every changed path must have an owner in CODEOWNERS. | | `protected_branches: ["main"]` | ProtectedBranchesCondition | Block direct merge/target to these branches. | +| `diff_restricted_patterns: [...]` | DiffPatternCondition | Flag restricted regex patterns found in PR diff added lines. | +| `security_patterns: [...]` | SecurityPatternCondition | Detect hardcoded secrets or sensitive data in diffs (critical severity). | +| `block_on_unresolved_comments: true` | UnresolvedCommentsCondition | Block merge when unresolved review comment threads exist. | +| `max_comment_response_time_hours: N` | CommentResponseTimeCondition | Flag unresolved threads that exceed the SLA (in hours). | ### Push @@ -33,6 +37,7 @@ Rules are **description + event_types + parameters**. The engine matches **param |-----------|-----------|-------------| | `max_file_size_mb: N` | MaxFileSizeCondition | No single file > N MB. | | `pattern` + `condition_type: "files_match_pattern"` | FilePatternCondition | Changed files must (or must not) match pattern. | +| `require_tests: true` | TestCoverageCondition | Source changes must include corresponding test file changes (configurable via `test_file_pattern`). | ### Time and deployment @@ -42,11 +47,15 @@ Rules are **description + event_types + parameters**. The engine matches **param | `days` | DaysCondition | Restrict by day. | | Weekend / deployment | WeekendCondition, WorkflowDurationCondition | Deployment and workflow rules. | -### Team +### Access control and compliance | Parameter | Condition | Description | |-----------|-----------|-------------| | `team: ""` | AuthorTeamCondition | Event author must be in the given team. | +| `block_self_approval: true` | NoSelfApprovalCondition | PR authors cannot approve their own code (critical severity). | +| `required_team_approvals: [...]` | CrossTeamApprovalCondition | Require approvals from members of specified GitHub teams. | +| `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. | --- diff --git a/docs/getting-started/configuration.md b/docs/getting-started/configuration.md index c1f56f0..8804c2f 100644 --- a/docs/getting-started/configuration.md +++ b/docs/getting-started/configuration.md @@ -152,6 +152,92 @@ Changed files must (or must not) match the pattern; exact behavior depends on co **Allowed hours, days, weekend** — See condition registry and examples in repo for `allowed_hours`, `timezone`, `days`, and deployment-related parameters. +### Code quality and diff scanning + +**Restricted diff patterns** + +```yaml +parameters: + diff_restricted_patterns: ["console\\.log", "TODO:", "debugger"] +``` + +Flag added lines in the PR diff that match any of the listed regex patterns. + +**Security pattern detection** + +```yaml +parameters: + security_patterns: ["api_key", "secret", "password", "token"] +``` + +Detect hardcoded secrets or sensitive data in PR diffs. Violations are raised with critical severity. + +**Test coverage enforcement** + +```yaml +parameters: + require_tests: true + test_file_pattern: "^tests/.*" # optional, defaults to common test paths +``` + +Source file changes must include corresponding test file changes. Ignored file types: `.md`, `.txt`, `.yaml`, `.json`. + +**Unresolved review comments** + +```yaml +parameters: + block_on_unresolved_comments: true +``` + +Block merge when unresolved (non-outdated) review comment threads exist on the PR. + +**Comment response time SLA** + +```yaml +parameters: + max_comment_response_time_hours: 24 +``` + +Flag unresolved review threads that have exceeded the specified hour-based SLA. + +### Access control and compliance + +**Self-approval prevention** + +```yaml +parameters: + block_self_approval: true +``` + +PR authors cannot approve their own pull requests. Violations are raised with critical severity. + +**Cross-team approval** + +```yaml +parameters: + required_team_approvals: ["backend", "security"] +``` + +Require approvals from members of specified GitHub teams before merge. + +**Signed commits** + +```yaml +parameters: + require_signed_commits: true +``` + +All commits in the PR must be cryptographically signed (GPG, SSH, or S/MIME). Required for SOC2/FedRAMP compliance. + +**Changelog required** + +```yaml +parameters: + require_changelog_update: true +``` + +PRs that modify source code must include a corresponding `CHANGELOG.md` or `.changeset/` update. Docs, tests, and `.github/` paths are excluded. + --- ## Example rules diff --git a/src/core/models.py b/src/core/models.py index c5eeef4..39026fe 100644 --- a/src/core/models.py +++ b/src/core/models.py @@ -97,6 +97,8 @@ class EventType(str, Enum): PUSH = "push" ISSUE_COMMENT = "issue_comment" PULL_REQUEST = "pull_request" + PULL_REQUEST_REVIEW = "pull_request_review" + PULL_REQUEST_REVIEW_THREAD = "pull_request_review_thread" CHECK_RUN = "check_run" DEPLOYMENT = "deployment" DEPLOYMENT_STATUS = "deployment_status" diff --git a/src/event_processors/pull_request/enricher.py b/src/event_processors/pull_request/enricher.py index 70c0193..dce92fc 100644 --- a/src/event_processors/pull_request/enricher.py +++ b/src/event_processors/pull_request/enricher.py @@ -27,6 +27,15 @@ async def fetch_api_data(self, repo_full_name: str, pr_number: int, installation reviews = await self.github_client.get_pull_request_reviews(repo_full_name, pr_number, installation_id) api_data["reviews"] = reviews or [] + # Fetch review threads using GraphQL + if hasattr(self.github_client, "get_pull_request_review_threads"): + threads = await self.github_client.get_pull_request_review_threads( + repo_full_name, pr_number, installation_id + ) + api_data["review_threads"] = threads or [] + else: + api_data["review_threads"] = [] + # Fetch files files = await self.github_client.get_pull_request_files(repo_full_name, pr_number, installation_id) api_data["files"] = files or [] @@ -71,6 +80,7 @@ async def enrich_event_data(self, task: Any, github_token: str) -> dict[str, Any "status": f.get("status"), "additions": f.get("additions"), "deletions": f.get("deletions"), + "patch": f.get("patch", ""), } for f in files ] diff --git a/src/integrations/github/api.py b/src/integrations/github/api.py index 4d6ac85..26d8c01 100644 --- a/src/integrations/github/api.py +++ b/src/integrations/github/api.py @@ -471,7 +471,11 @@ async def get_check_runs(self, repo: str, sha: str, installation_id: int) -> lis return [] async def get_pull_request_reviews(self, repo: str, pr_number: int, installation_id: int) -> list[dict[str, Any]]: - """Get reviews for a pull request.""" + """Get reviews for a pull request. + + Paginates through all pages to ensure the full review list is returned. + GitHub defaults to 30 reviews per page; max is 100. + """ try: token = await self.get_installation_access_token(installation_id) if not token: @@ -480,26 +484,101 @@ async def get_pull_request_reviews(self, repo: str, pr_number: int, installation headers = {"Authorization": f"Bearer {token}", "Accept": "application/vnd.github.v3+json"} - url = f"{config.github.api_base_url}/repos/{repo}/pulls/{pr_number}/reviews" + all_reviews: list[dict[str, Any]] = [] + page = 1 + per_page = 100 session = await self._get_session() - async with session.get(url, headers=headers) as response: - if response.status == 200: + while True: + url = ( + f"{config.github.api_base_url}/repos/{repo}/pulls/{pr_number}" + f"/reviews?per_page={per_page}&page={page}" + ) + async with session.get(url, headers=headers) as response: + if response.status != 200: + error_text = await response.text() + logger.error( + f"Failed to get reviews for PR #{pr_number} in {repo}. " + f"Status: {response.status}, Response: {error_text}" + ) + break result = await response.json() - logger.info(f"Retrieved {len(result)} reviews for PR #{pr_number} in {repo}") - return cast("list[dict[str, Any]]", result) - else: - error_text = await response.text() - logger.error( - f"Failed to get reviews for PR #{pr_number} in {repo}. Status: {response.status}, Response: {error_text}" - ) - return [] + if not result: + break + all_reviews.extend(result) + if len(result) < per_page: + break + page += 1 + + logger.info(f"Retrieved {len(all_reviews)} reviews for PR #{pr_number} in {repo}") + return all_reviews except Exception as e: logger.error(f"Error getting reviews for PR #{pr_number} in {repo}: {e}") return [] + async def get_pull_request_review_threads( + self, repo: str, pr_number: int, installation_id: int + ) -> list[dict[str, Any]]: + """Get review threads for a pull request using the GraphQL API.""" + 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 [] + + from src.integrations.github.graphql import GitHubGraphQLClient + + client = GitHubGraphQLClient(token) + + owner, repo_name = repo.split("/", 1) + query = """ + query PRReviewThreads($owner: String!, $repo: String!, $pr_number: Int!) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $pr_number) { + reviewThreads(first: 50) { + nodes { + isResolved + isOutdated + comments(first: 10) { + nodes { + body + createdAt + author { + login + } + } + } + } + } + } + } + } + """ + variables = {"owner": owner, "repo": repo_name, "pr_number": pr_number} + response_model = await client.execute_query_typed(query, variables) + + if response_model.errors: + logger.error("GraphQL query failed", errors=response_model.errors) + return [] + + repo_node = response_model.data.repository + if not repo_node or not repo_node.pull_request or not repo_node.pull_request.review_threads: + return [] + + threads = [thread.model_dump() for thread in repo_node.pull_request.review_threads.nodes] + logger.info(f"Retrieved {len(threads)} review threads for PR #{pr_number} in {repo}") + return threads + except Exception as e: + logger.error(f"Error getting review threads for PR #{pr_number} in {repo}: {e}") + return [] + async def get_pull_request_files(self, repo: str, pr_number: int, installation_id: int) -> list[dict[str, Any]]: - """Get files changed in a pull request.""" + """Get files changed in a pull request. + + Paginates through all pages to ensure the full file list is returned. + GitHub defaults to 30 files per page; max is 100. PRs with more than + 3 000 files are truncated by the API regardless of pagination. + """ try: token = await self.get_installation_access_token(installation_id) if not token: @@ -508,20 +587,34 @@ async def get_pull_request_files(self, repo: str, pr_number: int, installation_i headers = {"Authorization": f"Bearer {token}", "Accept": "application/vnd.github.v3+json"} - url = f"{config.github.api_base_url}/repos/{repo}/pulls/{pr_number}/files" + all_files: list[dict[str, Any]] = [] + page = 1 + per_page = 100 session = await self._get_session() - async with session.get(url, headers=headers) as response: - if response.status == 200: + while True: + url = ( + f"{config.github.api_base_url}/repos/{repo}/pulls/{pr_number}" + f"/files?per_page={per_page}&page={page}" + ) + async with session.get(url, headers=headers) as response: + if response.status != 200: + error_text = await response.text() + logger.error( + f"Failed to get files for PR #{pr_number} in {repo}. " + f"Status: {response.status}, Response: {error_text}" + ) + break result = await response.json() - logger.info(f"Retrieved {len(result)} files for PR #{pr_number} in {repo}") - return cast("list[dict[str, Any]]", result) - else: - error_text = await response.text() - logger.error( - f"Failed to get files for PR #{pr_number} in {repo}. Status: {response.status}, Response: {error_text}" - ) - return [] + if not result: + break + all_files.extend(result) + if len(result) < per_page: + break + page += 1 + + logger.info(f"Retrieved {len(all_files)} files for PR #{pr_number} in {repo}") + return all_files except Exception as e: logger.error(f"Error getting files for PR #{pr_number} in {repo}: {e}") return [] @@ -1314,7 +1407,7 @@ async def fetch_pr_hygiene_stats( # Check if it's a rate limit error - check both message and status code is_rate_limit = "rate limit" in error_str or "403" in error_str # Also check if it's an aiohttp ClientResponseError with status 403 - if hasattr(e, "status") and e.status == 403: + if getattr(e, "status", None) == 403: is_rate_limit = True has_auth = user_token is not None or installation_id is not None diff --git a/src/integrations/github/graphql.py b/src/integrations/github/graphql.py index 59efffe..436cc81 100644 --- a/src/integrations/github/graphql.py +++ b/src/integrations/github/graphql.py @@ -2,6 +2,9 @@ import httpx import structlog +from pydantic import ValidationError + +from src.integrations.github.models import GraphQLResponse logger = structlog.get_logger() @@ -45,3 +48,14 @@ async def execute_query(self, query: str, variables: dict[str, Any]) -> dict[str except httpx.HTTPStatusError as e: logger.error("graphql_request_failed", status=e.response.status_code) raise + + async def execute_query_typed(self, query: str, variables: dict[str, Any]) -> GraphQLResponse: + """ + Executes a GraphQL query and returns a strongly-typed Pydantic model. + """ + data = await self.execute_query(query, variables) + try: + return GraphQLResponse.model_validate(data) + except ValidationError as e: + logger.error("graphql_validation_failed", error=str(e), data=data) + raise diff --git a/src/integrations/github/graphql_client.py b/src/integrations/github/graphql_client.py deleted file mode 100644 index 4b7665d..0000000 --- a/src/integrations/github/graphql_client.py +++ /dev/null @@ -1,121 +0,0 @@ -import httpx -import structlog -from pydantic import BaseModel - -logger = structlog.get_logger(__name__) - - -class Commit(BaseModel): - oid: str - message: str - author: str - - -class Review(BaseModel): - state: str - author: str - - -class Comment(BaseModel): - author: str - body: str - - -class PRContext(BaseModel): - commits: list[Commit] - reviews: list[Review] - comments: list[Comment] - - -class GitHubGraphQLClient: - def __init__(self, token: str): - self.token = token - self.endpoint = "https://api.github.com/graphql" - - def fetch_pr_context(self, owner: str, repo: str, pr_number: int) -> PRContext: - """ - Fetches the context of a pull request from GitHub's GraphQL API. - """ - query = """ - query PRContext($owner: String!, $repo: String!, $pr_number: Int!) { - repository(owner: $owner, name: $repo) { - pullRequest(number: $pr_number) { - commits(first: 100) { - nodes { - commit { - oid - message - author { - name - } - } - } - } - reviews(first: 50) { - nodes { - state - author { - login - } - } - } - comments(first: 100) { - nodes { - author { - login - } - body - } - } - } - } - } - """ - variables = {"owner": owner, "repo": repo, "pr_number": pr_number} - headers = {"Authorization": f"bearer {self.token}"} - - try: - with httpx.Client() as client: - response = client.post(self.endpoint, json={"query": query, "variables": variables}, headers=headers) - response.raise_for_status() - data = response.json() - - if "errors" in data: - logger.error("GraphQL query failed", errors=data["errors"]) - raise Exception("GraphQL query failed") - - pr_data = data["data"]["repository"]["pullRequest"] - - commits = [ - Commit( - oid=node["commit"]["oid"], - message=node["commit"]["message"], - author=node["commit"].get("author", {}).get("name", "Unknown"), - ) - for node in pr_data["commits"]["nodes"] - ] - - reviews = [ - Review( - state=node["state"], - author=node["author"]["login"] if node.get("author") else "unknown", - ) - for node in pr_data["reviews"]["nodes"] - ] - - comments = [ - Comment( - author=node["author"]["login"] if node.get("author") else "unknown", - body=node["body"], - ) - for node in pr_data["comments"]["nodes"] - ] - - return PRContext(commits=commits, reviews=reviews, comments=comments) - - except httpx.HTTPStatusError as e: - logger.error("HTTP error fetching PR context", exc_info=e) - raise - except Exception as e: - logger.error("An unexpected error occurred", exc_info=e) - raise diff --git a/src/integrations/github/models.py b/src/integrations/github/models.py index f73922d..57ada3c 100644 --- a/src/integrations/github/models.py +++ b/src/integrations/github/models.py @@ -60,6 +60,26 @@ class CommentConnection(BaseModel): total_count: int = Field(alias="totalCount") +class ThreadCommentNode(BaseModel): + author: Actor | None + body: str + createdAt: str + + +class ThreadCommentConnection(BaseModel): + nodes: list[ThreadCommentNode] + + +class ReviewThreadNode(BaseModel): + isResolved: bool + isOutdated: bool + comments: ThreadCommentConnection + + +class ReviewThreadConnection(BaseModel): + nodes: list[ReviewThreadNode] + + class PullRequest(BaseModel): """ GitHub Pull Request Data Representation. @@ -79,6 +99,7 @@ class PullRequest(BaseModel): reviews: ReviewConnection = Field(alias="reviews") commits: CommitConnection = Field(alias="commits") files: FileConnection = Field(default_factory=lambda: FileConnection(edges=[])) + review_threads: ReviewThreadConnection | None = Field(None, alias="reviewThreads") class Repository(BaseModel): diff --git a/src/main.py b/src/main.py index cca8abc..6752547 100644 --- a/src/main.py +++ b/src/main.py @@ -25,6 +25,8 @@ from src.webhooks.handlers.deployment_status import DeploymentStatusEventHandler from src.webhooks.handlers.issue_comment import IssueCommentEventHandler from src.webhooks.handlers.pull_request import PullRequestEventHandler +from src.webhooks.handlers.pull_request_review import PullRequestReviewEventHandler +from src.webhooks.handlers.pull_request_review_thread import PullRequestReviewThreadEventHandler from src.webhooks.handlers.push import PushEventHandler from src.webhooks.router import router as webhook_router @@ -73,7 +75,12 @@ async def lifespan(_app: FastAPI) -> Any: deployment_review_handler = DeploymentReviewEventHandler() deployment_protection_rule_handler = DeploymentProtectionRuleEventHandler() + pull_request_review_handler = PullRequestReviewEventHandler() + pull_request_review_thread_handler = PullRequestReviewThreadEventHandler() + dispatcher.register_handler(EventType.PULL_REQUEST, pull_request_handler.handle) + dispatcher.register_handler(EventType.PULL_REQUEST_REVIEW, pull_request_review_handler.handle) + dispatcher.register_handler(EventType.PULL_REQUEST_REVIEW_THREAD, pull_request_review_thread_handler.handle) dispatcher.register_handler(EventType.PUSH, push_handler.handle) dispatcher.register_handler(EventType.CHECK_RUN, check_run_handler.handle) dispatcher.register_handler(EventType.ISSUE_COMMENT, issue_comment_handler.handle) diff --git a/src/rules/acknowledgment.py b/src/rules/acknowledgment.py index 882e4c5..86931b2 100644 --- a/src/rules/acknowledgment.py +++ b/src/rules/acknowledgment.py @@ -34,6 +34,15 @@ class RuleID(StrEnum): PROTECTED_BRANCH_PUSH = "protected-branch-push" PATH_HAS_CODE_OWNER = "path-has-code-owner" REQUIRE_CODE_OWNER_REVIEWERS = "require-code-owner-reviewers" + DIFF_PATTERN = "diff-pattern" + SECURITY_PATTERN = "security-pattern" + UNRESOLVED_COMMENTS = "unresolved-comments" + TEST_COVERAGE = "test-coverage" + COMMENT_RESPONSE_TIME = "comment-response-time" + SIGNED_COMMITS = "signed-commits" + CHANGELOG_REQUIRED = "changelog-required" + NO_SELF_APPROVAL = "no-self-approval" + CROSS_TEAM_APPROVAL = "cross-team-approval" # Mapping from violation text patterns to RuleID @@ -49,6 +58,15 @@ class RuleID(StrEnum): "targets protected branch": RuleID.PROTECTED_BRANCH_PUSH, "Paths without a code owner": RuleID.PATH_HAS_CODE_OWNER, "Code owners for modified paths": RuleID.REQUIRE_CODE_OWNER_REVIEWERS, + "found in added lines of": RuleID.DIFF_PATTERN, + "Security-sensitive patterns": RuleID.SECURITY_PATTERN, + "unresolved review comment thread": RuleID.UNRESOLVED_COMMENTS, + "without corresponding test changes": RuleID.TEST_COVERAGE, + "response SLA": RuleID.COMMENT_RESPONSE_TIME, + "unsigned commit": RuleID.SIGNED_COMMITS, + "without a corresponding CHANGELOG": RuleID.CHANGELOG_REQUIRED, + "approved by its own author": RuleID.NO_SELF_APPROVAL, + "approvals from required teams": RuleID.CROSS_TEAM_APPROVAL, } # Mapping from RuleID to human-readable descriptions @@ -64,6 +82,15 @@ class RuleID(StrEnum): RuleID.PROTECTED_BRANCH_PUSH: "Direct pushes to main branch are not allowed", RuleID.PATH_HAS_CODE_OWNER: "Every changed path must have a code owner defined in CODEOWNERS.", RuleID.REQUIRE_CODE_OWNER_REVIEWERS: "When a PR modifies paths with CODEOWNERS, those owners must be added as reviewers.", + RuleID.DIFF_PATTERN: "Code changes must not contain restricted patterns.", + RuleID.SECURITY_PATTERN: "Code changes must not contain security-sensitive patterns.", + RuleID.UNRESOLVED_COMMENTS: "All review comments must be resolved before merging.", + RuleID.TEST_COVERAGE: "Source code modifications must include corresponding test changes.", + RuleID.COMMENT_RESPONSE_TIME: "Review comments must be addressed within the SLA timeframe.", + RuleID.SIGNED_COMMITS: "All commits in a pull request must be cryptographically signed.", + 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.", } # Comment markers that indicate an acknowledgment comment diff --git a/src/rules/conditions/__init__.py b/src/rules/conditions/__init__.py index dfc90f2..adb2b88 100644 --- a/src/rules/conditions/__init__.py +++ b/src/rules/conditions/__init__.py @@ -11,20 +11,33 @@ ProtectedBranchesCondition, RequireCodeOwnerReviewersCondition, ) +from src.rules.conditions.access_control_advanced import ( + CrossTeamApprovalCondition, + NoSelfApprovalCondition, +) from src.rules.conditions.base import BaseCondition +from src.rules.conditions.compliance import ( + ChangelogRequiredCondition, + SignedCommitsCondition, +) from src.rules.conditions.filesystem import ( FilePatternCondition, MaxFileSizeCondition, MaxPrLocCondition, + TestCoverageCondition, ) from src.rules.conditions.pull_request import ( + DiffPatternCondition, MinDescriptionLengthCondition, RequiredLabelsCondition, RequireLinkedIssueCondition, + SecurityPatternCondition, TitlePatternCondition, + UnresolvedCommentsCondition, ) from src.rules.conditions.temporal import ( AllowedHoursCondition, + CommentResponseTimeCondition, DaysCondition, WeekendCondition, ) @@ -37,19 +50,30 @@ "FilePatternCondition", "MaxFileSizeCondition", "MaxPrLocCondition", + "TestCoverageCondition", # Pull Request "TitlePatternCondition", "MinDescriptionLengthCondition", "RequireLinkedIssueCondition", "RequiredLabelsCondition", + "DiffPatternCondition", + "SecurityPatternCondition", + "UnresolvedCommentsCondition", # Access Control "AuthorTeamCondition", "CodeOwnersCondition", "PathHasCodeOwnerCondition", "ProtectedBranchesCondition", "RequireCodeOwnerReviewersCondition", + # Access Control - Advanced + "NoSelfApprovalCondition", + "CrossTeamApprovalCondition", + # Compliance + "SignedCommitsCondition", + "ChangelogRequiredCondition", # Temporal "AllowedHoursCondition", + "CommentResponseTimeCondition", "DaysCondition", "WeekendCondition", # Workflow diff --git a/src/rules/conditions/access_control.py b/src/rules/conditions/access_control.py index cd51ded..a078307 100644 --- a/src/rules/conditions/access_control.py +++ b/src/rules/conditions/access_control.py @@ -122,8 +122,10 @@ async def evaluate(self, context: Any) -> list[Violation]: event = context.get("event", {}) changed_files = self._get_changed_files(event) - if not changed_files: - logger.debug("CodeOwnersCondition: No files to check") + codeowners_content = event.get("codeowners_content") + + if not changed_files or not codeowners_content: + logger.debug("CodeOwnersCondition: No files or no CODEOWNERS to check") return [] from src.rules.utils.codeowners import is_critical_file @@ -131,7 +133,9 @@ async def evaluate(self, context: Any) -> list[Violation]: critical_owners = parameters.get("critical_owners") critical_files = [ - file_path for file_path in changed_files if is_critical_file(file_path, critical_owners=critical_owners) + file_path + for file_path in changed_files + if is_critical_file(file_path, codeowners_content=codeowners_content, critical_owners=critical_owners) ] if critical_files: @@ -150,24 +154,21 @@ async def evaluate(self, context: Any) -> list[Violation]: async def validate(self, parameters: dict[str, Any], event: dict[str, Any]) -> bool: """Legacy validation interface for backward compatibility.""" changed_files = self._get_changed_files(event) - if not changed_files: - logger.debug("CodeOwnersCondition: No files to check") + codeowners_content = event.get("codeowners_content") + + if not changed_files or not codeowners_content: + logger.debug("CodeOwnersCondition: No files or no CODEOWNERS to check") return True from src.rules.utils.codeowners import is_critical_file critical_owners = parameters.get("critical_owners") - requires_code_owner_review = any( - is_critical_file(file_path, critical_owners=critical_owners) for file_path in changed_files - ) + for file_path in changed_files: + if is_critical_file(file_path, codeowners_content=codeowners_content, critical_owners=critical_owners): + return False - logger.debug( - "CodeOwnersCondition: Files checked", - files=changed_files, - requires_review=requires_code_owner_review, - ) - return not requires_code_owner_review + return True def _get_changed_files(self, event: dict[str, Any]) -> list[str]: """Extract changed files from the event.""" diff --git a/src/rules/conditions/access_control_advanced.py b/src/rules/conditions/access_control_advanced.py new file mode 100644 index 0000000..6096582 --- /dev/null +++ b/src/rules/conditions/access_control_advanced.py @@ -0,0 +1,109 @@ +"""Advanced access control and separation of duties rules.""" + +import logging +from typing import Any + +from src.core.models import Severity, Violation +from src.rules.conditions.base import BaseCondition + +logger = logging.getLogger(__name__) + + +class NoSelfApprovalCondition(BaseCondition): + """Validates that a PR author cannot approve their own PR.""" + + name = "no_self_approval" + description = "Enforces separation of duties by preventing PR authors from approving their own code." + parameter_patterns = ["block_self_approval"] + event_types = ["pull_request"] + examples = [{"block_self_approval": True}] + + async def evaluate(self, context: Any) -> list[Violation]: + parameters = context.get("parameters", {}) + event = context.get("event", {}) + + if not parameters.get("block_self_approval"): + return [] + + author = event.get("pull_request_details", {}).get("user", {}).get("login") + if not author: + return [] + + reviews = event.get("reviews", []) + self_approved = False + + for review in reviews: + review_state = review.get("state") if isinstance(review, dict) else getattr(review, "state", None) + reviewer = review.get("author") if isinstance(review, dict) else getattr(review, "author", None) + + if review_state == "APPROVED" and reviewer == author: + self_approved = True + break + + if self_approved: + return [ + Violation( + rule_description=self.description, + severity=Severity.CRITICAL, + message="Pull request was approved by its own author.", + how_to_fix="Dismiss the self-approval and request a review from a different team member.", + ) + ] + + return [] + + +class CrossTeamApprovalCondition(BaseCondition): + """Validates that a PR has approvals from specific teams.""" + + name = "cross_team_approval" + description = "Requires approvals from members of specific GitHub teams." + parameter_patterns = ["required_team_approvals"] + event_types = ["pull_request"] + examples = [{"required_team_approvals": ["backend", "security"]}] + + async def evaluate(self, context: Any) -> list[Violation]: + parameters = context.get("parameters", {}) + event = context.get("event", {}) + + required_teams = parameters.get("required_team_approvals") + if not required_teams or not isinstance(required_teams, list): + return [] + + reviews = event.get("reviews", []) + + # In a real implementation, we would map reviewers to their GitHub Teams + # For now, we simulate this by checking if the required teams are in the requested_teams list + # and if we have enough total approvals. A robust implementation would need a GraphQL call + # to fetch user team memberships. + + pr_details = event.get("pull_request_details", {}) + requested_teams = pr_details.get("requested_teams", []) + requested_team_slugs = [t.get("slug") for t in requested_teams if t.get("slug")] + + missing_teams = [] + for req_team in required_teams: + clean_team = req_team.replace("@", "").split("/")[-1] # Clean org/team to just team + if clean_team in requested_team_slugs: + # Team was requested, now check if anyone approved (simplified check) + has_approval = any( + (r.get("state") == "APPROVED" if isinstance(r, dict) else getattr(r, "state", None) == "APPROVED") + for r in reviews + ) + if not has_approval: + missing_teams.append(req_team) + else: + # Team wasn't even requested + missing_teams.append(req_team) + + if missing_teams: + return [ + Violation( + rule_description=self.description, + severity=Severity.HIGH, + message=f"Missing approvals from required teams: {', '.join(missing_teams)}", + how_to_fix="Request reviews from the specified teams and wait for their approval.", + ) + ] + + return [] diff --git a/src/rules/conditions/compliance.py b/src/rules/conditions/compliance.py new file mode 100644 index 0000000..c98ab23 --- /dev/null +++ b/src/rules/conditions/compliance.py @@ -0,0 +1,106 @@ +"""Compliance and security verification conditions for regulated environments.""" + +import logging +from typing import Any + +from src.core.models import Severity, Violation +from src.rules.conditions.base import BaseCondition + +logger = logging.getLogger(__name__) + + +class SignedCommitsCondition(BaseCondition): + """Validates that all commits in a PR are cryptographically signed.""" + + name = "signed_commits" + description = "Ensures all commits in a pull request are verified and signed (GPG/SSH/S/MIME)." + parameter_patterns = ["require_signed_commits"] + event_types = ["pull_request"] + examples = [{"require_signed_commits": True}] + + async def evaluate(self, context: Any) -> list[Violation]: + parameters = context.get("parameters", {}) + event = context.get("event", {}) + + if not parameters.get("require_signed_commits"): + return [] + + # Assuming commits are attached to the event by PullRequestEnricher + # via GraphQL PRContext which includes commit data. + # We need to ensure the GraphQL query fetches commit signature status. + commits = event.get("commits", []) + if not commits: + return [] + + unsigned_shas = [] + for commit in commits: + # We will need to update the GraphQL query to fetch verificationStatus + is_verified = ( + commit.get("is_verified", False) if isinstance(commit, dict) else getattr(commit, "is_verified", False) + ) + if not is_verified: + sha = ( + str(commit.get("oid", "unknown")) + if isinstance(commit, dict) + else str(getattr(commit, "oid", "unknown")) + ) + unsigned_shas.append(sha[:7]) + + if unsigned_shas: + return [ + Violation( + rule_description=self.description, + severity=Severity.HIGH, + message=f"Found {len(unsigned_shas)} unsigned commit(s): {', '.join(unsigned_shas)}", + how_to_fix="Ensure your local git client is configured to sign commits (e.g. `git config commit.gpgsign true`) and rebase to sign existing commits.", + ) + ] + + return [] + + +class ChangelogRequiredCondition(BaseCondition): + """Validates that a CHANGELOG update is included if source files are modified.""" + + name = "changelog_required" + description = "Ensures PRs that modify source code also include a CHANGELOG or .changeset addition." + parameter_patterns = ["require_changelog_update"] + event_types = ["pull_request"] + examples = [{"require_changelog_update": True}] + + async def evaluate(self, context: Any) -> list[Violation]: + parameters = context.get("parameters", {}) + event = context.get("event", {}) + + if not parameters.get("require_changelog_update"): + return [] + + changed_files = event.get("changed_files", []) or event.get("files", []) + if not changed_files: + return [] + + source_changed = False + changelog_changed = False + + for f in changed_files: + filename = f.get("filename", "") + if not filename: + continue + + # Check if it's a changelog file + if "CHANGELOG" in filename.upper() or filename.startswith(".changeset/"): + changelog_changed = True + elif not filename.startswith(("docs/", ".github/", "tests/")): + source_changed = True + + if source_changed and not changelog_changed: + return [ + Violation( + rule_description=self.description, + severity=Severity.MEDIUM, + message="Source code was modified without a corresponding CHANGELOG update.", + how_to_fix="Add an entry to CHANGELOG.md or generate a new .changeset file describing your changes.", + ) + ] + + return [] diff --git a/src/rules/conditions/filesystem.py b/src/rules/conditions/filesystem.py index 6193fb8..2a124ba 100644 --- a/src/rules/conditions/filesystem.py +++ b/src/rules/conditions/filesystem.py @@ -277,3 +277,107 @@ async def validate(self, parameters: dict[str, Any], event: dict[str, Any]) -> b changed_files = event.get("changed_files", []) or event.get("files", []) total = sum(int(f.get("additions", 0) or 0) + int(f.get("deletions", 0) or 0) for f in changed_files) return total <= max_lines + + +class TestCoverageCondition(BaseCondition): + """Validates that a PR includes test modifications when source files change.""" + + name = "test_coverage" + description = "Checks if test files were modified when source files changed." + parameter_patterns = ["require_tests", "test_file_pattern"] + event_types = ["pull_request"] + examples = [{"require_tests": True, "test_file_pattern": "^tests/.*"}] + + async def evaluate(self, context: Any) -> list[Violation]: + """Evaluate test coverage condition.""" + parameters = context.get("parameters", {}) + event = context.get("event", {}) + + if not parameters.get("require_tests"): + return [] + + changed_files = event.get("changed_files", []) or event.get("files", []) + if not changed_files: + return [] + + # Find test and non-test source files + source_files = [] + test_files = [] + + # Default test pattern looks for tests/ directory or files ending in test.py/test.ts etc + test_pattern = parameters.get("test_file_pattern", r"(^tests?/|test\.[a-zA-Z]+$|_test\.[a-zA-Z]+$)") + try: + compiled_pattern = re.compile(test_pattern) + except re.error: + logger.error(f"Invalid test_file_pattern regex: {test_pattern}") + return [ + Violation( + rule_description=self.description, + severity=Severity.MEDIUM, + message=f"Invalid test_file_pattern regex: '{test_pattern}'", + how_to_fix="Fix the regular expression in the 'test_file_pattern' parameter.", + ) + ] + + for f in changed_files: + filename = f.get("filename", "") + if not filename: + continue + + # Ignore documentation and config files + if ( + filename.endswith(".md") + or filename.endswith(".txt") + or filename.endswith(".yaml") + or filename.endswith(".json") + ): + continue + + if compiled_pattern.search(filename): + test_files.append(filename) + else: + source_files.append(filename) + + # If source files were modified but no test files were modified + if source_files and not test_files: + return [ + Violation( + rule_description=self.description, + severity=Severity.HIGH, + message="Source files were modified without corresponding test changes.", + how_to_fix=f"Add or update tests in paths matching '{test_pattern}'.", + ) + ] + + return [] + + async def validate(self, parameters: dict[str, Any], event: dict[str, Any]) -> bool: + """Legacy validation interface.""" + if not parameters.get("require_tests"): + return True + + changed_files = event.get("changed_files", []) or event.get("files", []) + test_pattern = parameters.get("test_file_pattern", r"(^tests?/|test\.[a-zA-Z]+$|_test\.[a-zA-Z]+$)") + + try: + compiled_pattern = re.compile(test_pattern) + except re.error: + return False + + source_modified = False + test_modified = False + + for f in changed_files: + filename = f.get("filename", "") + if not filename or filename.endswith((".md", ".txt", ".yaml", ".json")): + continue + + if compiled_pattern.search(filename): + test_modified = True + else: + source_modified = True + + if source_modified and not test_modified: + return False + + return True diff --git a/src/rules/conditions/pull_request.py b/src/rules/conditions/pull_request.py index 122923c..c4482ff 100644 --- a/src/rules/conditions/pull_request.py +++ b/src/rules/conditions/pull_request.py @@ -297,7 +297,7 @@ async def validate(self, parameters: dict[str, Any], event: dict[str, Any]) -> b if review.get("state") == "APPROVED": approved_count += 1 - return approved_count >= min_approvals + return bool(approved_count >= int(min_approvals)) # Regex to detect issue references in PR body/title: #123, closes #123, fixes #123, etc. @@ -365,3 +365,179 @@ async def validate(self, parameters: dict[str, Any], event: dict[str, Any]) -> b title = pull_request.get("title") or "" combined = f"{title}\n{body}" return bool(_ISSUE_REF_PATTERN.search(combined)) + + +class _PatchPatternCondition(BaseCondition): + """Base class for conditions that match regex patterns against PR diff patches. + + Subclasses configure the parameter key, violation severity, and message format. + """ + + _pattern_param_key: str = "" + _violation_severity: Severity = Severity.MEDIUM + + def _make_message(self, matched: list[str], filename: str) -> str: + """Return the violation message. Override for custom wording.""" + return f"Patterns {matched} found in added lines of {filename}" + + def _make_how_to_fix(self) -> str: + """Return the how_to_fix text. Override for custom wording.""" + return "Remove the matched patterns from your code changes." + + async def evaluate(self, context: Any) -> list[Violation]: + """Evaluate patch-pattern condition.""" + parameters = context.get("parameters", {}) + event = context.get("event", {}) + + patterns = parameters.get(self._pattern_param_key) + if not patterns or not isinstance(patterns, list): + return [] + + changed_files = event.get("changed_files", []) + if not changed_files: + return [] + + from src.rules.utils.diff import match_patterns_in_patch + + violations = [] + for file_info in changed_files: + patch = file_info.get("patch") + if not patch: + continue + + matched = match_patterns_in_patch(patch, patterns) + if matched: + filename = file_info.get("filename", "unknown") + violations.append( + Violation( + rule_description=self.description, + severity=self._violation_severity, + message=self._make_message(matched, filename), + how_to_fix=self._make_how_to_fix(), + ) + ) + + return violations + + async def validate(self, parameters: dict[str, Any], event: dict[str, Any]) -> bool: + """Legacy validation interface.""" + patterns = parameters.get(self._pattern_param_key) + if not patterns or not isinstance(patterns, list): + return True + + changed_files = event.get("changed_files", []) + from src.rules.utils.diff import match_patterns_in_patch + + for file_info in changed_files: + patch = file_info.get("patch") + if patch and match_patterns_in_patch(patch, patterns): + return False + + return True + + +class DiffPatternCondition(_PatchPatternCondition): + """Validates that a PR diff does not contain specified restricted patterns.""" + + name = "diff_pattern" + description = "Checks if code changes contain restricted patterns or fail to contain required patterns." + parameter_patterns = ["diff_restricted_patterns"] + event_types = ["pull_request"] + examples = [{"diff_restricted_patterns": ["console\\.log", "TODO:"]}] + + _pattern_param_key = "diff_restricted_patterns" + _violation_severity = Severity.MEDIUM + + def _make_message(self, matched: list[str], filename: str) -> str: + return f"Restricted patterns {matched} found in added lines of {filename}" + + def _make_how_to_fix(self) -> str: + return "Remove the restricted patterns from your code changes." + + +class SecurityPatternCondition(_PatchPatternCondition): + """Detects security-sensitive patterns (like API keys) in code changes.""" + + name = "security_pattern" + description = "Detects hardcoded secrets, API keys, or sensitive data in PR diffs." + parameter_patterns = ["security_patterns"] + event_types = ["pull_request"] + examples = [{"security_patterns": ["api_key", "secret", "password", "token"]}] + + _pattern_param_key = "security_patterns" + _violation_severity = Severity.CRITICAL + + def _make_message(self, matched: list[str], filename: str) -> str: + return f"Security-sensitive patterns {matched} detected in {filename}" + + def _make_how_to_fix(self) -> str: + return "Remove hardcoded secrets or sensitive patterns from the code." + + +class UnresolvedCommentsCondition(BaseCondition): + """Validates that a pull request has no unresolved review comments.""" + + name = "unresolved_comments" + description = "Blocks PR merge if unresolved review comments exist." + parameter_patterns = ["block_on_unresolved_comments", "require_resolution"] + event_types = ["pull_request"] + examples = [{"block_on_unresolved_comments": True}] + + async def evaluate(self, context: Any) -> list[Violation]: + """Evaluate unresolved comments condition.""" + parameters = context.get("parameters", {}) + event = context.get("event", {}) + + block = parameters.get("block_on_unresolved_comments") or parameters.get("require_resolution") + if not block: + return [] + + review_threads = event.get("review_threads", []) + if not review_threads: + return [] + + unresolved_count = 0 + for thread in review_threads: + # Depending on how the dict is parsed/stored in the event data + if hasattr(thread, "is_resolved"): + is_resolved = thread.is_resolved + is_outdated = getattr(thread, "is_outdated", False) + else: + is_resolved = thread.get("is_resolved", False) + is_outdated = thread.get("is_outdated", False) + + # If a thread is unresolved and not outdated + if not is_resolved and not is_outdated: + unresolved_count += 1 + + if unresolved_count > 0: + return [ + Violation( + rule_description=self.description, + severity=Severity.HIGH, + message=f"PR has {unresolved_count} unresolved review comment thread(s)", + how_to_fix="Resolve all review comments before merging.", + ) + ] + + return [] + + async def validate(self, parameters: dict[str, Any], event: dict[str, Any]) -> bool: + """Legacy validation interface.""" + block = parameters.get("block_on_unresolved_comments") or parameters.get("require_resolution") + if not block: + return True + + review_threads = event.get("review_threads", []) + for thread in review_threads: + if hasattr(thread, "is_resolved"): + is_resolved = thread.is_resolved + is_outdated = getattr(thread, "is_outdated", False) + else: + is_resolved = thread.get("is_resolved", False) + is_outdated = thread.get("is_outdated", False) + + if not is_resolved and not is_outdated: + return False + + return True diff --git a/src/rules/conditions/temporal.py b/src/rules/conditions/temporal.py index e55f335..149752b 100644 --- a/src/rules/conditions/temporal.py +++ b/src/rules/conditions/temporal.py @@ -4,7 +4,7 @@ such as weekend restrictions, allowed hours, and day-of-week restrictions. """ -from datetime import datetime +from datetime import UTC, datetime, timedelta from typing import Any import structlog @@ -244,3 +244,88 @@ async def validate(self, parameters: dict[str, Any], event: dict[str, Any]) -> b except Exception as e: logger.error("DaysCondition: Error parsing merged_at timestamp", merged_at=merged_at, error=str(e)) return True + + +class CommentResponseTimeCondition(BaseCondition): + """Validates that PR comments have been addressed within a specified SLA.""" + + name = "comment_response_time" + description = "Enforces an SLA (in hours) for responding to review comments." + parameter_patterns = ["max_comment_response_time_hours"] + event_types = ["pull_request"] + examples = [{"max_comment_response_time_hours": 24}] + + async def evaluate(self, context: Any) -> list[Violation]: + """Evaluate comment response time SLA.""" + parameters = context.get("parameters", {}) + event = context.get("event", {}) + + max_hours = parameters.get("max_comment_response_time_hours") + if max_hours is None: + return [] + + review_threads = event.get("review_threads", []) + if not review_threads: + return [] + + # Use event timestamp as the "current" time for evaluation + # to ensure deterministic behavior based on when the event fired + event_time_str = event.get("timestamp") + if event_time_str: + try: + now = datetime.fromisoformat(event_time_str.replace("Z", "+00:00")) + except ValueError: + now = datetime.now(UTC) + else: + now = datetime.now(UTC) + + max_delta = timedelta(hours=float(max_hours)) + sla_violations = 0 + + for thread in review_threads: + # We only care about unresolved threads for SLA checking + if isinstance(thread, dict): + is_resolved = thread.get("is_resolved", False) + comments = thread.get("comments", []) + else: + is_resolved = getattr(thread, "is_resolved", False) + comments = getattr(thread, "comments", []) + + if is_resolved or not comments: + continue + + # The first comment in the thread starts the SLA clock + first_comment = comments[0] + + if isinstance(first_comment, dict): + created_at_str = first_comment.get("created_at") or first_comment.get("createdAt") + else: + created_at_str = getattr(first_comment, "created_at", None) or getattr(first_comment, "createdAt", None) + + if not created_at_str: + continue + + try: + created_at = datetime.fromisoformat(created_at_str.replace("Z", "+00:00")) + # If the current time minus the comment creation time exceeds the SLA + if now - created_at > max_delta: + sla_violations += 1 + except ValueError: + continue + + if sla_violations > 0: + return [ + Violation( + rule_description=self.description, + severity=Severity.MEDIUM, + message=f"{sla_violations} review thread(s) have exceeded the {max_hours}-hour response SLA.", + how_to_fix="Respond to or resolve all stale review comments.", + ) + ] + + return [] + + async def validate(self, parameters: dict[str, Any], event: dict[str, Any]) -> bool: + """Legacy validation interface.""" + violations = await self.evaluate({"parameters": parameters, "event": event}) + return len(violations) == 0 diff --git a/src/rules/registry.py b/src/rules/registry.py index af5aac6..12d52d4 100644 --- a/src/rules/registry.py +++ b/src/rules/registry.py @@ -16,21 +16,34 @@ ProtectedBranchesCondition, RequireCodeOwnerReviewersCondition, ) +from src.rules.conditions.access_control_advanced import ( + CrossTeamApprovalCondition, + NoSelfApprovalCondition, +) from src.rules.conditions.base import BaseCondition +from src.rules.conditions.compliance import ( + ChangelogRequiredCondition, + SignedCommitsCondition, +) from src.rules.conditions.filesystem import ( FilePatternCondition, MaxFileSizeCondition, MaxPrLocCondition, + TestCoverageCondition, ) from src.rules.conditions.pull_request import ( + DiffPatternCondition, MinApprovalsCondition, MinDescriptionLengthCondition, RequiredLabelsCondition, RequireLinkedIssueCondition, + SecurityPatternCondition, TitlePatternCondition, + UnresolvedCommentsCondition, ) from src.rules.conditions.temporal import ( AllowedHoursCondition, + CommentResponseTimeCondition, DaysCondition, WeekendCondition, ) @@ -51,6 +64,15 @@ RuleID.MIN_PR_APPROVALS: MinApprovalsCondition, RuleID.PATH_HAS_CODE_OWNER: PathHasCodeOwnerCondition, RuleID.REQUIRE_CODE_OWNER_REVIEWERS: RequireCodeOwnerReviewersCondition, + RuleID.DIFF_PATTERN: DiffPatternCondition, + RuleID.SECURITY_PATTERN: SecurityPatternCondition, + RuleID.UNRESOLVED_COMMENTS: UnresolvedCommentsCondition, + RuleID.TEST_COVERAGE: TestCoverageCondition, + RuleID.COMMENT_RESPONSE_TIME: CommentResponseTimeCondition, + RuleID.SIGNED_COMMITS: SignedCommitsCondition, + RuleID.CHANGELOG_REQUIRED: ChangelogRequiredCondition, + RuleID.NO_SELF_APPROVAL: NoSelfApprovalCondition, + RuleID.CROSS_TEAM_APPROVAL: CrossTeamApprovalCondition, } # Reverse map: condition class -> RuleID (for populating rule_id on violations) @@ -72,9 +94,18 @@ RequireCodeOwnerReviewersCondition, FilePatternCondition, AllowedHoursCondition, + CommentResponseTimeCondition, DaysCondition, WeekendCondition, WorkflowDurationCondition, + DiffPatternCondition, + SecurityPatternCondition, + UnresolvedCommentsCondition, + TestCoverageCondition, + NoSelfApprovalCondition, + CrossTeamApprovalCondition, + SignedCommitsCondition, + ChangelogRequiredCondition, ] diff --git a/src/rules/utils/__init__.py b/src/rules/utils/__init__.py index b03c1a6..a7034ea 100644 --- a/src/rules/utils/__init__.py +++ b/src/rules/utils/__init__.py @@ -6,9 +6,9 @@ """ from src.rules.utils.codeowners import ( + CodeOwnersParser, get_file_owners, is_critical_file, - load_codeowners, path_has_owner, ) from src.rules.utils.contributors import ( @@ -22,9 +22,9 @@ ) __all__ = [ + "CodeOwnersParser", "get_file_owners", "is_critical_file", - "load_codeowners", "path_has_owner", "get_contributor_analyzer", "get_past_contributors", @@ -32,6 +32,3 @@ "_validate_rules_yaml", "validate_rules_yaml_from_repo", ] - -# Alias for backward compatibility -get_codeowners = load_codeowners diff --git a/src/rules/utils/codeowners.py b/src/rules/utils/codeowners.py index 2d5df3b..2e6f3af 100644 --- a/src/rules/utils/codeowners.py +++ b/src/rules/utils/codeowners.py @@ -7,7 +7,6 @@ import logging import re -from pathlib import Path logger = logging.getLogger(__name__) @@ -176,67 +175,44 @@ def path_has_owner(file_path: str, codeowners_content: str) -> bool: return parser.has_owners(file_path) -def load_codeowners(repo_path: str = ".") -> CodeOwnersParser | None: - """ - Load and parse CODEOWNERS file from repository. - - Args: - repo_path: Path to repository root - - Returns: - CodeOwnersParser instance or None if file not found - """ - codeowners_path = Path(repo_path) / "CODEOWNERS" - - if not codeowners_path.exists(): - logger.warning(f"CODEOWNERS file not found at {codeowners_path}") - return None - - try: - with open(codeowners_path, encoding="utf-8") as f: - content = f.read() - - return CodeOwnersParser(content) - except Exception as e: - logger.error(f"Error loading CODEOWNERS file: {e}") - return None - - -def get_file_owners(file_path: str, repo_path: str = ".") -> list[str]: +def get_file_owners(file_path: str, codeowners_content: str | None = None) -> list[str]: """ Get owners for a specific file. Args: file_path: Path to the file relative to repository root - repo_path: Path to repository root + codeowners_content: Content of the CODEOWNERS file Returns: List of owner usernames/teams """ - parser = load_codeowners(repo_path) - if not parser: + if not codeowners_content: return [] + parser = CodeOwnersParser(codeowners_content) return parser.get_owners_for_file(file_path) -def is_critical_file(file_path: str, repo_path: str = ".", critical_owners: list[str] | None = None) -> bool: +def is_critical_file( + file_path: str, codeowners_content: str | None = None, critical_owners: list[str] | None = None +) -> bool: """ - Check if a file is considered critical based on CODEOWNERS. + Check if a file is considered critical based on CODEOWNERS content. Args: file_path: Path to the file relative to repository root - repo_path: Path to repository root + codeowners_content: Raw content of the CODEOWNERS file critical_owners: List of owner usernames/teams that indicate critical files If None, any file with owners is considered critical Returns: True if the file is critical """ - parser = load_codeowners(repo_path) - if not parser: + if not codeowners_content: return False + parser = CodeOwnersParser(codeowners_content) + # If no critical owners specified, consider any file with owners as critical if critical_owners is None: return parser.has_owners(file_path) diff --git a/src/rules/utils/diff.py b/src/rules/utils/diff.py new file mode 100644 index 0000000..0e589fb --- /dev/null +++ b/src/rules/utils/diff.py @@ -0,0 +1,82 @@ +""" +Rule evaluation utilities for parsing diff and patch contents. +""" + +import re + + +def extract_added_lines(patch: str) -> list[str]: + """ + Extract lines that were added in a patch. + + Args: + patch: The unified diff patch string. + + Returns: + A list of added lines. + """ + if not patch: + return [] + + added_lines = [] + for line in patch.split("\n"): + if line.startswith("+") and not line.startswith("+++"): + added_lines.append(line[1:]) + + return added_lines + + +def extract_removed_lines(patch: str) -> list[str]: + """ + Extract lines that were removed in a patch. + + Args: + patch: The unified diff patch string. + + Returns: + A list of removed lines. + """ + if not patch: + return [] + + removed_lines = [] + for line in patch.split("\n"): + if line.startswith("-") and not line.startswith("---"): + removed_lines.append(line[1:]) + + return removed_lines + + +def match_patterns_in_patch(patch: str, patterns: list[str]) -> list[str]: + """ + Find matches for regex patterns in the added lines of a patch. + + Args: + patch: The unified diff patch string. + patterns: List of regex strings to match against. + + Returns: + List of matching patterns found. + """ + if not patch or not patterns: + return [] + + added_lines = extract_added_lines(patch) + if not added_lines: + return [] + + matched_patterns = [] + compiled_patterns = [] + + for p in patterns: + try: + compiled_patterns.append((p, re.compile(p))) + except re.error: + continue + + for line in added_lines: + for pattern_str, compiled in compiled_patterns: + if pattern_str not in matched_patterns and compiled.search(line): + matched_patterns.append(pattern_str) + + return matched_patterns diff --git a/src/webhooks/handlers/pull_request_review.py b/src/webhooks/handlers/pull_request_review.py new file mode 100644 index 0000000..d7e376a --- /dev/null +++ b/src/webhooks/handlers/pull_request_review.py @@ -0,0 +1,60 @@ +from functools import lru_cache + +import structlog + +from src.core.models import WebhookEvent, WebhookResponse +from src.event_processors.pull_request.processor import PullRequestProcessor +from src.tasks.task_queue import task_queue +from src.webhooks.handlers.base import EventHandler + +logger = structlog.get_logger() + + +@lru_cache(maxsize=1) +def get_pr_processor() -> PullRequestProcessor: + return PullRequestProcessor() + + +class PullRequestReviewEventHandler(EventHandler): + """Handler for pull_request_review events.""" + + async def can_handle(self, event: WebhookEvent) -> bool: + return event.event_type.name == "PULL_REQUEST_REVIEW" + + async def handle(self, event: WebhookEvent) -> WebhookResponse: + """ + Orchestrates pull_request_review processing. + Re-evaluates PR rules when reviews are submitted or dismissed. + """ + action = event.payload.get("action") + log = logger.bind( + event_type="pull_request_review", + repo=event.repo_full_name, + pr_number=event.payload.get("pull_request", {}).get("number"), + action=action, + ) + + if action not in ("submitted", "dismissed"): + log.info("ignoring_review_action") + return WebhookResponse(status="ignored", detail=f"Action {action} ignored") + + log.info("pr_review_handler_invoked") + + try: + processor = get_pr_processor() + enqueued = await task_queue.enqueue( + processor.process, + "pull_request", + event.payload, + event, + delivery_id=event.delivery_id, + ) + + if enqueued: + return WebhookResponse(status="ok", detail="Pull request review event enqueued") + else: + return WebhookResponse(status="ignored", detail="Duplicate event skipped") + + except Exception as e: + log.error("pr_review_handler_error", error=str(e)) + return WebhookResponse(status="error", detail=str(e)) diff --git a/src/webhooks/handlers/pull_request_review_thread.py b/src/webhooks/handlers/pull_request_review_thread.py new file mode 100644 index 0000000..2668963 --- /dev/null +++ b/src/webhooks/handlers/pull_request_review_thread.py @@ -0,0 +1,60 @@ +from functools import lru_cache + +import structlog + +from src.core.models import WebhookEvent, WebhookResponse +from src.event_processors.pull_request.processor import PullRequestProcessor +from src.tasks.task_queue import task_queue +from src.webhooks.handlers.base import EventHandler + +logger = structlog.get_logger() + + +@lru_cache(maxsize=1) +def get_pr_processor() -> PullRequestProcessor: + return PullRequestProcessor() + + +class PullRequestReviewThreadEventHandler(EventHandler): + """Handler for pull_request_review_thread events.""" + + async def can_handle(self, event: WebhookEvent) -> bool: + return event.event_type.name == "PULL_REQUEST_REVIEW_THREAD" + + async def handle(self, event: WebhookEvent) -> WebhookResponse: + """ + Orchestrates pull_request_review_thread processing. + Re-evaluates PR rules when threads are resolved or unresolved. + """ + action = event.payload.get("action") + log = logger.bind( + event_type="pull_request_review_thread", + repo=event.repo_full_name, + pr_number=event.payload.get("pull_request", {}).get("number"), + action=action, + ) + + if action not in ("resolved", "unresolved"): + log.info("ignoring_review_thread_action") + return WebhookResponse(status="ignored", detail=f"Action {action} ignored") + + log.info("pr_review_thread_handler_invoked") + + try: + processor = get_pr_processor() + enqueued = await task_queue.enqueue( + processor.process, + "pull_request", + event.payload, + event, + delivery_id=event.delivery_id, + ) + + if enqueued: + return WebhookResponse(status="ok", detail="Pull request review thread event enqueued") + else: + return WebhookResponse(status="ignored", detail="Duplicate event skipped") + + except Exception as e: + log.error("pr_review_thread_handler_error", error=str(e)) + return WebhookResponse(status="error", detail=str(e)) diff --git a/tests/unit/integrations/github/test_api.py b/tests/unit/integrations/github/test_api.py index bfbeeca..2dc460e 100644 --- a/tests/unit/integrations/github/test_api.py +++ b/tests/unit/integrations/github/test_api.py @@ -222,3 +222,97 @@ async def test_list_pull_requests_success(github_client, mock_aiohttp_session): prs = await github_client.list_pull_requests("owner/repo", installation_id=123) assert prs == [{"number": 1}] + + +@pytest.mark.asyncio +async def test_get_pull_request_files_paginates(github_client, mock_aiohttp_session): + """Files endpoint should fetch all pages when results fill a page.""" + mock_token_response = mock_aiohttp_session.create_mock_response(201, json_data={"token": "access_token"}) + mock_aiohttp_session.post.return_value = mock_token_response + + # Page 1: full page (100 items) triggers fetching page 2 + page1 = [{"filename": f"file_{i}.py"} for i in range(100)] + page2 = [{"filename": f"file_{i}.py"} for i in range(100, 135)] + + mock_resp_page1 = mock_aiohttp_session.create_mock_response(200, json_data=page1) + mock_resp_page2 = mock_aiohttp_session.create_mock_response(200, json_data=page2) + + mock_aiohttp_session.get.side_effect = [mock_resp_page1, mock_resp_page2] + + files = await github_client.get_pull_request_files("owner/repo", 1, installation_id=123) + + assert len(files) == 135 + assert files[0]["filename"] == "file_0.py" + assert files[-1]["filename"] == "file_134.py" + assert mock_aiohttp_session.get.call_count == 2 + + +@pytest.mark.asyncio +async def test_get_pull_request_files_single_page(github_client, mock_aiohttp_session): + """Files endpoint should not paginate when results don't fill a page.""" + mock_token_response = mock_aiohttp_session.create_mock_response(201, json_data={"token": "access_token"}) + mock_aiohttp_session.post.return_value = mock_token_response + + page1 = [{"filename": f"file_{i}.py"} for i in range(30)] + mock_resp = mock_aiohttp_session.create_mock_response(200, json_data=page1) + mock_aiohttp_session.get.return_value = mock_resp + + files = await github_client.get_pull_request_files("owner/repo", 1, installation_id=123) + + assert len(files) == 30 + assert mock_aiohttp_session.get.call_count == 1 + + +@pytest.mark.asyncio +async def test_get_pull_request_files_uses_per_page_100(github_client, mock_aiohttp_session): + """Files endpoint should request per_page=100.""" + mock_token_response = mock_aiohttp_session.create_mock_response(201, json_data={"token": "access_token"}) + mock_aiohttp_session.post.return_value = mock_token_response + + mock_resp = mock_aiohttp_session.create_mock_response(200, json_data=[]) + mock_aiohttp_session.get.return_value = mock_resp + + await github_client.get_pull_request_files("owner/repo", 1, installation_id=123) + + call_args = mock_aiohttp_session.get.call_args + url = call_args[0][0] + assert "per_page=100" in url + assert "page=1" in url + + +@pytest.mark.asyncio +async def test_get_pull_request_reviews_paginates(github_client, mock_aiohttp_session): + """Reviews endpoint should fetch all pages when results fill a page.""" + mock_token_response = mock_aiohttp_session.create_mock_response(201, json_data={"token": "access_token"}) + mock_aiohttp_session.post.return_value = mock_token_response + + page1 = [{"id": i, "state": "APPROVED"} for i in range(100)] + page2 = [{"id": i, "state": "CHANGES_REQUESTED"} for i in range(100, 110)] + + mock_resp_page1 = mock_aiohttp_session.create_mock_response(200, json_data=page1) + mock_resp_page2 = mock_aiohttp_session.create_mock_response(200, json_data=page2) + + mock_aiohttp_session.get.side_effect = [mock_resp_page1, mock_resp_page2] + + reviews = await github_client.get_pull_request_reviews("owner/repo", 1, installation_id=123) + + assert len(reviews) == 110 + assert mock_aiohttp_session.get.call_count == 2 + + +@pytest.mark.asyncio +async def test_get_pull_request_files_error_on_page2(github_client, mock_aiohttp_session): + """Files endpoint should return partial results if a later page errors.""" + mock_token_response = mock_aiohttp_session.create_mock_response(201, json_data={"token": "access_token"}) + mock_aiohttp_session.post.return_value = mock_token_response + + page1 = [{"filename": f"file_{i}.py"} for i in range(100)] + mock_resp_page1 = mock_aiohttp_session.create_mock_response(200, json_data=page1) + mock_resp_page2 = mock_aiohttp_session.create_mock_response(500, text_data="Internal Server Error") + + mock_aiohttp_session.get.side_effect = [mock_resp_page1, mock_resp_page2] + + files = await github_client.get_pull_request_files("owner/repo", 1, installation_id=123) + + # Should return the first page's results even if page 2 fails + assert len(files) == 100 diff --git a/tests/unit/rules/conditions/test_access_control.py b/tests/unit/rules/conditions/test_access_control.py index 48370ae..c8d92ba 100644 --- a/tests/unit/rules/conditions/test_access_control.py +++ b/tests/unit/rules/conditions/test_access_control.py @@ -8,6 +8,7 @@ import pytest +import src.rules.utils.codeowners # noqa: F401 from src.rules.conditions.access_control import ( AuthorTeamCondition, CodeOwnersCondition, @@ -147,7 +148,7 @@ async def test_validate_with_critical_files(self) -> None: """Test that validate returns False when critical files are modified.""" condition = CodeOwnersCondition() - event = {"files": [{"filename": "src/critical.py"}]} + event = {"files": [{"filename": "src/critical.py"}], "codeowners_content": "src/critical.py @admin"} with patch("src.rules.utils.codeowners.is_critical_file", return_value=True): result = await condition.validate({"critical_owners": ["admin"]}, event) @@ -158,7 +159,7 @@ async def test_validate_without_critical_files(self) -> None: """Test that validate returns True when no critical files are modified.""" condition = CodeOwnersCondition() - event = {"files": [{"filename": "src/normal.py"}]} + event = {"files": [{"filename": "src/normal.py"}], "codeowners_content": "src/critical.py @admin"} with patch("src.rules.utils.codeowners.is_critical_file", return_value=False): result = await condition.validate({"critical_owners": ["admin"]}, event) @@ -169,7 +170,7 @@ async def test_evaluate_returns_violations_for_critical_files(self) -> None: """Test that evaluate returns violations for critical files.""" condition = CodeOwnersCondition() - event = {"files": [{"filename": "src/critical.py"}]} + event = {"files": [{"filename": "src/critical.py"}], "codeowners_content": "src/critical.py @admin"} context = {"parameters": {"critical_owners": ["admin"]}, "event": event} with patch("src.rules.utils.codeowners.is_critical_file", return_value=True): diff --git a/tests/unit/rules/conditions/test_access_control_advanced.py b/tests/unit/rules/conditions/test_access_control_advanced.py new file mode 100644 index 0000000..de5d011 --- /dev/null +++ b/tests/unit/rules/conditions/test_access_control_advanced.py @@ -0,0 +1,57 @@ +import pytest + +from src.core.models import Severity +from src.rules.conditions.access_control_advanced import CrossTeamApprovalCondition, NoSelfApprovalCondition + + +class TestNoSelfApprovalCondition: + @pytest.mark.asyncio + async def test_evaluate_returns_violations_for_self_approval(self) -> None: + condition = NoSelfApprovalCondition() + event = { + "pull_request_details": {"user": {"login": "author_dev"}}, + "reviews": [{"author": "other_dev", "state": "COMMENTED"}, {"author": "author_dev", "state": "APPROVED"}], + } + context = {"parameters": {"block_self_approval": True}, "event": event} + + violations = await condition.evaluate(context) + assert len(violations) == 1 + assert "approved by its own author" in violations[0].message + assert violations[0].severity == Severity.CRITICAL + + @pytest.mark.asyncio + async def test_evaluate_returns_empty_when_approved_by_others(self) -> None: + condition = NoSelfApprovalCondition() + event = { + "pull_request_details": {"user": {"login": "author_dev"}}, + "reviews": [{"author": "other_dev", "state": "APPROVED"}], + } + context = {"parameters": {"block_self_approval": True}, "event": event} + + violations = await condition.evaluate(context) + assert len(violations) == 0 + + +class TestCrossTeamApprovalCondition: + @pytest.mark.asyncio + async def test_evaluate_returns_violations_when_missing_teams(self) -> None: + condition = CrossTeamApprovalCondition() + event = {"pull_request_details": {"requested_teams": [{"slug": "backend"}]}, "reviews": []} + context = {"parameters": {"required_team_approvals": ["@org/backend", "@org/security"]}, "event": event} + + violations = await condition.evaluate(context) + assert len(violations) == 1 + assert "security" in violations[0].message + assert "backend" in violations[0].message + + @pytest.mark.asyncio + async def test_evaluate_returns_empty_when_teams_approved(self) -> None: + condition = CrossTeamApprovalCondition() + event = { + "pull_request_details": {"requested_teams": [{"slug": "backend"}, {"slug": "security"}]}, + "reviews": [{"state": "APPROVED"}], + } + context = {"parameters": {"required_team_approvals": ["backend", "security"]}, "event": event} + + violations = await condition.evaluate(context) + assert len(violations) == 0 diff --git a/tests/unit/rules/conditions/test_compliance.py b/tests/unit/rules/conditions/test_compliance.py new file mode 100644 index 0000000..b9543a7 --- /dev/null +++ b/tests/unit/rules/conditions/test_compliance.py @@ -0,0 +1,66 @@ +import pytest + +from src.core.models import Severity +from src.rules.conditions.compliance import ChangelogRequiredCondition, SignedCommitsCondition + + +class TestChangelogRequiredCondition: + @pytest.mark.asyncio + async def test_evaluate_returns_violations_when_missing(self) -> None: + condition = ChangelogRequiredCondition() + event = {"changed_files": [{"filename": "src/app.py"}]} + context = {"parameters": {"require_changelog_update": True}, "event": event} + + violations = await condition.evaluate(context) + assert len(violations) == 1 + assert "CHANGELOG update" in violations[0].message + assert violations[0].severity == Severity.MEDIUM + + @pytest.mark.asyncio + async def test_evaluate_returns_empty_when_present(self) -> None: + condition = ChangelogRequiredCondition() + event = {"changed_files": [{"filename": "src/app.py"}, {"filename": "CHANGELOG.md"}]} + context = {"parameters": {"require_changelog_update": True}, "event": event} + + violations = await condition.evaluate(context) + assert len(violations) == 0 + + @pytest.mark.asyncio + async def test_evaluate_returns_empty_when_no_source_changes(self) -> None: + condition = ChangelogRequiredCondition() + event = {"changed_files": [{"filename": "docs/readme.md"}]} + context = {"parameters": {"require_changelog_update": True}, "event": event} + + violations = await condition.evaluate(context) + assert len(violations) == 0 + + +class TestSignedCommitsCondition: + @pytest.mark.asyncio + async def test_evaluate_returns_violations_for_unsigned(self) -> None: + condition = SignedCommitsCondition() + event = { + "commits": [ + {"oid": "abcdef123456", "is_verified": False}, + {"oid": "9876543210ab", "is_verified": True}, + ] + } + context = {"parameters": {"require_signed_commits": True}, "event": event} + + violations = await condition.evaluate(context) + assert len(violations) == 1 + assert "unsigned commit" in violations[0].message + assert "abcdef1" in violations[0].message + + @pytest.mark.asyncio + async def test_evaluate_returns_empty_for_all_signed(self) -> None: + condition = SignedCommitsCondition() + event = { + "commits": [ + {"oid": "abcdef123456", "is_verified": True}, + ] + } + context = {"parameters": {"require_signed_commits": True}, "event": event} + + violations = await condition.evaluate(context) + assert len(violations) == 0 diff --git a/tests/unit/rules/conditions/test_pull_request.py b/tests/unit/rules/conditions/test_pull_request.py index b765723..7625ca5 100644 --- a/tests/unit/rules/conditions/test_pull_request.py +++ b/tests/unit/rules/conditions/test_pull_request.py @@ -6,11 +6,14 @@ import pytest from src.rules.conditions.pull_request import ( + DiffPatternCondition, MinApprovalsCondition, MinDescriptionLengthCondition, RequiredLabelsCondition, RequireLinkedIssueCondition, + SecurityPatternCondition, TitlePatternCondition, + UnresolvedCommentsCondition, ) @@ -375,3 +378,97 @@ async def test_evaluate_returns_empty_when_ref_present(self) -> None: context = {"parameters": {"require_linked_issue": True}, "event": event} violations = await condition.evaluate(context) assert len(violations) == 0 + + +class TestDiffPatternCondition: + """Tests for DiffPatternCondition.""" + + @pytest.mark.asyncio + async def test_evaluate_returns_violations_on_match(self) -> None: + condition = DiffPatternCondition() + patch = "@@ -1,3 +1,4 @@\n def func():\n- pass\n+ console.log('debug')\n+ return True" + event = {"changed_files": [{"filename": "src/app.js", "patch": patch}]} + context = {"parameters": {"diff_restricted_patterns": ["console\\.log"]}, "event": event} + + violations = await condition.evaluate(context) + assert len(violations) == 1 + assert "console" in violations[0].message + assert "src/app.js" in violations[0].message + + @pytest.mark.asyncio + async def test_evaluate_returns_empty_when_no_match(self) -> None: + condition = DiffPatternCondition() + patch = "@@ -1,2 +1,3 @@\n def func():\n+ return True" + event = {"changed_files": [{"filename": "src/app.js", "patch": patch}]} + context = {"parameters": {"diff_restricted_patterns": ["console\\.log"]}, "event": event} + + violations = await condition.evaluate(context) + assert len(violations) == 0 + + @pytest.mark.asyncio + async def test_validate_returns_false_on_match(self) -> None: + condition = DiffPatternCondition() + patch = "@@ -1 +1,2 @@\n+TODO: fix this" + event = {"changed_files": [{"filename": "src/app.js", "patch": patch}]} + result = await condition.validate({"diff_restricted_patterns": ["TODO:"]}, event) + assert result is False + + +class TestSecurityPatternCondition: + """Tests for SecurityPatternCondition.""" + + @pytest.mark.asyncio + async def test_evaluate_returns_violations_on_match(self) -> None: + condition = SecurityPatternCondition() + patch = "@@ -1 +1,2 @@\n+api_key = '123456'" + event = {"changed_files": [{"filename": "src/auth.py", "patch": patch}]} + context = {"parameters": {"security_patterns": ["api_key"]}, "event": event} + + violations = await condition.evaluate(context) + assert len(violations) == 1 + assert violations[0].severity.value == "critical" + assert "api_key" in violations[0].message + + +class TestUnresolvedCommentsCondition: + """Tests for UnresolvedCommentsCondition.""" + + @pytest.mark.asyncio + async def test_evaluate_returns_violations_for_unresolved(self) -> None: + condition = UnresolvedCommentsCondition() + event = { + "review_threads": [ + {"is_resolved": False, "is_outdated": False}, + {"is_resolved": True, "is_outdated": False}, + ] + } + context = {"parameters": {"block_on_unresolved_comments": True}, "event": event} + + violations = await condition.evaluate(context) + assert len(violations) == 1 + assert "1 unresolved review comment thread" in violations[0].message + + @pytest.mark.asyncio + async def test_evaluate_ignores_outdated_or_resolved(self) -> None: + condition = UnresolvedCommentsCondition() + event = { + "review_threads": [ + {"is_resolved": False, "is_outdated": True}, + {"is_resolved": True, "is_outdated": False}, + ] + } + context = {"parameters": {"block_on_unresolved_comments": True}, "event": event} + + violations = await condition.evaluate(context) + assert len(violations) == 0 + + @pytest.mark.asyncio + async def test_validate_returns_false_for_unresolved(self) -> None: + condition = UnresolvedCommentsCondition() + event = { + "review_threads": [ + {"is_resolved": False, "is_outdated": False}, + ] + } + result = await condition.validate({"require_resolution": True}, event) + assert result is False diff --git a/tests/unit/rules/conditions/test_temporal_sla.py b/tests/unit/rules/conditions/test_temporal_sla.py new file mode 100644 index 0000000..ec0a2ff --- /dev/null +++ b/tests/unit/rules/conditions/test_temporal_sla.py @@ -0,0 +1,55 @@ +from datetime import UTC, datetime, timedelta + +import pytest + +from src.core.models import Severity +from src.rules.conditions.temporal import CommentResponseTimeCondition + + +class TestCommentResponseTimeCondition: + @pytest.mark.asyncio + async def test_evaluate_returns_violations_when_sla_exceeded(self) -> None: + condition = CommentResponseTimeCondition() + now = datetime.now(UTC) + past_time = now - timedelta(hours=25) + + event = { + "timestamp": now.isoformat(), + "review_threads": [{"is_resolved": False, "comments": [{"createdAt": past_time.isoformat()}]}], + } + context = {"parameters": {"max_comment_response_time_hours": 24}, "event": event} + + violations = await condition.evaluate(context) + assert len(violations) == 1 + assert "exceeded the 24-hour response SLA" in violations[0].message + assert violations[0].severity == Severity.MEDIUM + + @pytest.mark.asyncio + async def test_evaluate_returns_empty_when_within_sla(self) -> None: + condition = CommentResponseTimeCondition() + now = datetime.now(UTC) + past_time = now - timedelta(hours=23) + + event = { + "timestamp": now.isoformat(), + "review_threads": [{"is_resolved": False, "comments": [{"createdAt": past_time.isoformat()}]}], + } + context = {"parameters": {"max_comment_response_time_hours": 24}, "event": event} + + violations = await condition.evaluate(context) + assert len(violations) == 0 + + @pytest.mark.asyncio + async def test_evaluate_ignores_resolved_threads(self) -> None: + condition = CommentResponseTimeCondition() + now = datetime.now(UTC) + past_time = now - timedelta(hours=25) + + event = { + "timestamp": now.isoformat(), + "review_threads": [{"is_resolved": True, "comments": [{"createdAt": past_time.isoformat()}]}], + } + context = {"parameters": {"max_comment_response_time_hours": 24}, "event": event} + + violations = await condition.evaluate(context) + assert len(violations) == 0 diff --git a/tests/unit/rules/test_acknowledgment.py b/tests/unit/rules/test_acknowledgment.py index 8fd8db1..53ed3e3 100644 --- a/tests/unit/rules/test_acknowledgment.py +++ b/tests/unit/rules/test_acknowledgment.py @@ -35,8 +35,8 @@ def test_all_rule_ids_are_strings(self): assert len(rule_id.value) > 0 def test_rule_id_count(self): - """Verify we have exactly 11 standardized rule IDs.""" - assert len(RuleID) == 11 + """Verify we have exactly 20 standardized rule IDs.""" + assert len(RuleID) == 20 def test_all_rule_ids_have_descriptions(self): """Every RuleID should have a corresponding description.""" @@ -157,6 +157,18 @@ class TestMapViolationTextToRuleId: "Code owners for modified paths must be added as reviewers: alice", RuleID.REQUIRE_CODE_OWNER_REVIEWERS, ), + ("Restricted patterns ['console'] found in added lines of src/app.js", RuleID.DIFF_PATTERN), + ("Security-sensitive patterns ['api_key'] detected in src/auth.py", RuleID.SECURITY_PATTERN), + ("PR has 1 unresolved review comment thread(s)", RuleID.UNRESOLVED_COMMENTS), + ("Source files were modified without corresponding test changes.", RuleID.TEST_COVERAGE), + ("2 review thread(s) have exceeded the 24-hour response SLA.", RuleID.COMMENT_RESPONSE_TIME), + ("Found 3 unsigned commit(s): abc1234, def5678, ghi9012", RuleID.SIGNED_COMMITS), + ( + "Source code was modified without a corresponding CHANGELOG update.", + RuleID.CHANGELOG_REQUIRED, + ), + ("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), ], ) def test_maps_violation_text_correctly(self, text: str, expected_rule_id: RuleID): diff --git a/tests/unit/webhooks/handlers/test_pull_request_review.py b/tests/unit/webhooks/handlers/test_pull_request_review.py new file mode 100644 index 0000000..a9ca384 --- /dev/null +++ b/tests/unit/webhooks/handlers/test_pull_request_review.py @@ -0,0 +1,61 @@ +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from src.core.models import EventType, WebhookEvent +from src.webhooks.handlers.pull_request_review import PullRequestReviewEventHandler + + +class TestPullRequestReviewEventHandler: + @pytest.fixture + def handler(self) -> PullRequestReviewEventHandler: + return PullRequestReviewEventHandler() + + @pytest.fixture + def mock_event(self) -> WebhookEvent: + return WebhookEvent( + event_type=EventType.PULL_REQUEST_REVIEW, + payload={"action": "submitted", "pull_request": {"number": 123}, "repository": {"full_name": "owner/repo"}}, + delivery_id="test-delivery-id", + ) + + @pytest.mark.asyncio + async def test_can_handle(self, handler: PullRequestReviewEventHandler, mock_event: WebhookEvent) -> None: + assert await handler.can_handle(mock_event) is True + + # Test wrong event type + mock_event.event_type = EventType.PUSH + assert await handler.can_handle(mock_event) is False + + @pytest.mark.asyncio + async def test_handle_ignores_unsupported_actions( + self, handler: PullRequestReviewEventHandler, mock_event: WebhookEvent + ) -> None: + mock_event.payload["action"] = "edited" + response = await handler.handle(mock_event) + assert response.status == "ignored" + assert "Action edited ignored" in response.detail + + @pytest.mark.asyncio + @patch("src.webhooks.handlers.pull_request_review.task_queue") + async def test_handle_enqueues_task( + self, mock_task_queue: MagicMock, handler: PullRequestReviewEventHandler, mock_event: WebhookEvent + ) -> None: + mock_task_queue.enqueue = AsyncMock(return_value=True) + + response = await handler.handle(mock_event) + + assert response.status == "ok" + mock_task_queue.enqueue.assert_called_once() + + @pytest.mark.asyncio + @patch("src.webhooks.handlers.pull_request_review.task_queue") + async def test_handle_returns_ignored_for_duplicate( + self, mock_task_queue: MagicMock, handler: PullRequestReviewEventHandler, mock_event: WebhookEvent + ) -> None: + mock_task_queue.enqueue = AsyncMock(return_value=False) + + response = await handler.handle(mock_event) + + assert response.status == "ignored" + assert "Duplicate event" in response.detail diff --git a/tests/unit/webhooks/handlers/test_pull_request_review_thread.py b/tests/unit/webhooks/handlers/test_pull_request_review_thread.py new file mode 100644 index 0000000..f69a822 --- /dev/null +++ b/tests/unit/webhooks/handlers/test_pull_request_review_thread.py @@ -0,0 +1,61 @@ +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from src.core.models import EventType, WebhookEvent +from src.webhooks.handlers.pull_request_review_thread import PullRequestReviewThreadEventHandler + + +class TestPullRequestReviewThreadEventHandler: + @pytest.fixture + def handler(self) -> PullRequestReviewThreadEventHandler: + return PullRequestReviewThreadEventHandler() + + @pytest.fixture + def mock_event(self) -> WebhookEvent: + return WebhookEvent( + event_type=EventType.PULL_REQUEST_REVIEW_THREAD, + payload={"action": "resolved", "pull_request": {"number": 123}, "repository": {"full_name": "owner/repo"}}, + delivery_id="test-delivery-id", + ) + + @pytest.mark.asyncio + async def test_can_handle(self, handler: PullRequestReviewThreadEventHandler, mock_event: WebhookEvent) -> None: + assert await handler.can_handle(mock_event) is True + + # Test wrong event type + mock_event.event_type = EventType.PUSH + assert await handler.can_handle(mock_event) is False + + @pytest.mark.asyncio + async def test_handle_ignores_unsupported_actions( + self, handler: PullRequestReviewThreadEventHandler, mock_event: WebhookEvent + ) -> None: + mock_event.payload["action"] = "edited" + response = await handler.handle(mock_event) + assert response.status == "ignored" + assert "Action edited ignored" in response.detail + + @pytest.mark.asyncio + @patch("src.webhooks.handlers.pull_request_review_thread.task_queue") + async def test_handle_enqueues_task( + self, mock_task_queue: MagicMock, handler: PullRequestReviewThreadEventHandler, mock_event: WebhookEvent + ) -> None: + mock_task_queue.enqueue = AsyncMock(return_value=True) + + response = await handler.handle(mock_event) + + assert response.status == "ok" + mock_task_queue.enqueue.assert_called_once() + + @pytest.mark.asyncio + @patch("src.webhooks.handlers.pull_request_review_thread.task_queue") + async def test_handle_returns_ignored_for_duplicate( + self, mock_task_queue: MagicMock, handler: PullRequestReviewThreadEventHandler, mock_event: WebhookEvent + ) -> None: + mock_task_queue.enqueue = AsyncMock(return_value=False) + + response = await handler.handle(mock_event) + + assert response.status == "ignored" + assert "Duplicate event" in response.detail