diff --git a/foc-board-rules/pr-hygiene.md b/foc-board-rules/pr-hygiene.md index 206e178..b83da90 100644 --- a/foc-board-rules/pr-hygiene.md +++ b/foc-board-rules/pr-hygiene.md @@ -35,17 +35,32 @@ Rules for keeping pull request items on the FOC board well-formed. ## R-PR-005: Draft PRs should be In Progress +**Enforced mechanically for the Triage case, hourly:** see [R-PR-010](#r-pr-010-triage-prs-should-be-routed-to-the-correct-status) below — a sweep no longer needs to move a draft PR out of Triage by hand. Non-Triage cases (Awaiting review, Approved, Issue awaiting PR merge) are still sweep-applied. + **When:** A PR is a draft and its status is "📌 Triage", "🔎 Awaiting review", "✔️ Approved by reviewer", or "⌚️ Issue awaiting PR merge". **Action:** Set Status to `⌨️ In Progress`. **Why:** A draft PR is not ready for review or approval. If it's in Triage, it should move to In Progress since someone is actively working on it. If it's in Awaiting Review or later, the author likely converted it back to draft after feedback — the board should reflect that it's back in active development. Draft PRs in Todo or In Progress are fine as-is. ## R-PR-006: Non-draft, non-bot PRs in Triage or In Progress — determine correct status +**Enforced mechanically for the Triage case, hourly:** see [R-PR-010](#r-pr-010-triage-prs-should-be-routed-to-the-correct-status) below. The In Progress re-evaluation case is still sweep-applied. + **When:** A PR is not a draft, not authored by a bot, not a release PR, and its status is "📌 Triage" or "⌨️ In Progress". **Action:** Compute the derived inputs and apply the [PR status determination table](pr-status-table.md); it is the canonical routing logic (this rule's former prose cases 1-4, the comments-count-as-reviews paragraph, and the carve-outs from pdp-explorer#118, dealbot#638, and filecoin-services#522 all live there now, alongside the R-SL-001/007 routing they interact with). **How to check:** Use `gh pr view -R --json reviews,commits,comments --jq '{reviews: [.reviews[] | {author: .author.login, state: .state, submittedAt: .submittedAt}], comments: [.comments[] | {author: .author.login, authorAssociation: .authorAssociation, body: .body, createdAt: .createdAt}], lastCommit: .commits[-1].committedDate}'` to get the timestamps and comment data the table's `last_feedback` input needs — Phase 1 (`gh pr list`) never includes `comments`, so this Phase 2 call is required for every R-PR-006 candidate, not just ones where Phase 1 shows formal review engagement. A PR with only substantive comments and no formal review looks identical to a PR with zero engagement in Phase 1 data, and skipping Phase 2 there would miss it. **Why:** Non-draft, non-bot PRs should always leave Triage, and In Progress PRs may need re-evaluation. But the destination depends on review state; not every PR goes to Awaiting Review. A PR with unaddressed feedback belongs in In Progress, a PR with a merge-authority approval belongs in Approved, and a PR with no feedback or addressed feedback belongs in Awaiting Review. This is the counterpart to R-PR-005: when a draft PR becomes non-draft, it advances, but the destination depends on what reviewers have already said. +## R-PR-010: Triage PRs should be routed to the correct status + +**Enforced mechanically, hourly:** this rule is a pure function of observable state, so it runs automatically via [`foc-mechanical-rules`](../foc-mechanical-rules/) (see [R-PR-010's implementation](../foc-mechanical-rules/foc_mechanical_rules/rules/pr_status.py)), scheduled by [`.github/workflows/foc-board-mechanical-rules.yml`](../.github/workflows/foc-board-mechanical-rules.yml). A sweep no longer needs to apply the Triage case of R-PR-005/R-PR-006 by hand — the prose there stays canonical for what those rules do and why; this module is canonical for exactly how the Triage case is evaluated. + +**When:** A PR on the board has status "📌 Triage". +**Action:** Compute the [PR status determination table](pr-status-table.md)'s target status (row 1: draft → In Progress; row 2: bot/release → Todo; row 3: authoritative approval and no blocking changes-requested → Approved by reviewer; rows 4-6: based on formal reviews vs. last commit → In Progress or Awaiting review) and set Status to it. +**Guard — respect an explicit return to Triage:** Before computing a target, check the PR's Status field change history (GitHub's `ProjectV2ItemStatusChangedEvent` timeline — the one project field with real GitHub-provided history; see [R-FC-012](field-completeness.md#r-fc-012-recently-active-items-without-a-cycle-get-the-current-cycle)'s note on why Cycle needs its own log but Status doesn't). If the most recent status change moved the item *into* Triage from some other status (Todo, In Progress, Awaiting review, etc.) rather than the initial add-to-project default, a human deliberately moved it back — flag instead of auto-routing it back out. +**Simplification vs. the full table — comments:** unlike the full table, this rule never treats informal PR *comments* as feedback (`last_feedback` only comes from formal reviews: APPROVED / CHANGES_REQUESTED / COMMENTED) — judging whether a comment is substantive feedback or coordination chatter requires reading it, which isn't a pure function of structured state. If a Triage PR has qualifying human comments after its last commit, this rule flags it instead of auto-routing so a human applies R-PR-006/R-SL-010 judgment. +**Simplification vs. R-SL-001 — approval language:** `authoritative_approval` doesn't parse approval text for conditional language ("approving assuming you address X"); any qualifying APPROVED review counts. R-SL-001's language-based superseding nuance is a judgment call, not a fact lookup, so it's out of scope here. +**Why:** Triage PRs should never sit there once someone starts real work on them (draft) or once they're ready for eyes (non-draft) — see R-PR-005/R-PR-006. But automation shouldn't fight a human who deliberately re-triaged something; the status-history guard makes that distinction on real GitHub data instead of guessing from board state alone. + ## R-PR-007: Awaiting Review PRs must have human reviewer engagement **When:** A PR has status "🔎 Awaiting review" (including PRs just routed there by R-PR-006) but has no human reviewer — neither a pending request nor a submitted review from a human. diff --git a/foc-mechanical-rules/README.md b/foc-mechanical-rules/README.md index d35a81a..3618c91 100644 --- a/foc-mechanical-rules/README.md +++ b/foc-mechanical-rules/README.md @@ -33,6 +33,7 @@ Neither `mutation_log.py` nor any rule module knows or cares how the log survive | [R-PR-001](../foc-board-rules/pr-hygiene.md#r-pr-001-unassigned-prs-should-be-assigned-to-their-author) | assignee | Unassigned open PRs are assigned to their author (skipping bots, with a merged-release-PR carve-out, and skipping PRs where a human explicitly removed the assignee) | | [R-FC-012](../foc-board-rules/field-completeness.md#r-fc-012-recently-active-items-without-a-cycle-get-the-current-cycle) | cycle | Issues/PRs updated in the last 3 days with no Cycle get the current cycle, unless this tool previously set that item's Cycle to the current one and a human has since cleared it | | [R-FC-013](../foc-board-rules/field-completeness.md#r-fc-013-open-items-in-a-past-cycle-should-move-to-the-current-cycle) | cycle | Open issues/PRs whose Cycle is a past iteration move to the current cycle, unless this tool previously moved that item off the same past cycle and a human has since moved it back | +| [R-PR-010](../foc-board-rules/pr-hygiene.md#r-pr-010-triage-prs-should-be-routed-to-the-correct-status) | status | PRs in Triage are routed to In Progress / Todo / Approved by reviewer / Awaiting review per the [PR status determination table](../foc-board-rules/pr-status-table.md), unless GitHub's real Status-field history shows a human explicitly moved the PR back to Triage | ## API call pattern per rule @@ -48,6 +49,7 @@ Board size (~180 open items and growing) makes it easy for a rule to accidentall | R-PR-001 (assignee) | 1 paginated board query | — | 1 REST `GET` (PR metadata) per candidate; **+1 REST `GET`** (issue events, paginated) unless skipped as a bot author | 1 REST `POST` per applied item (not batched — see gap below) | | R-FC-012 (cycle) | 1 paginated board query | 1 GraphQL query (iterations) | none | Batched: all applied items in the run share 1 GraphQL mutation per 25 items (`_CycleFieldRule.mutate_pending`) | | R-FC-013 (cycle) | 1 paginated board query (Cycle field included, so no separate read is needed to know an item's current cycle) | 1 GraphQL query (iterations, shared helper with R-FC-012) | none | Batched, same mechanism as R-FC-012 (via the shared `_CycleFieldRule.mutate_pending`) | +| R-PR-010 (status) | 1 paginated board query (Triage items only) | — | 1 GraphQL query per candidate (`get_pr_review_context`: draft/author/commits/reviews/reviewRequests/comments/status history in one round trip); **+1 REST `GET`** (collaborator permission) per unique reviewer login with a qualifying review, cached per (owner, repo, login) for the run | Batched: all applied items grouped by target status, 1 GraphQL mutation per 25 items per group (same `mutate_pending` pattern as `_CycleFieldRule`) | **Known gap, not yet worth fixing:** R-PR-001's 1-2 REST reads per candidate PR are real per-item calls with no batched equivalent used today, unlike the two cycle rules. At current volume (dozens of candidates per hourly run, most REST GETs) it's well within GitHub's rate limits and not worth the complexity, but if candidate volume grows a lot, `github_projects_client`'s `nodes(ids: [ID!]!)` batching pattern (used by `set_field_value_bulk`'s old-value fetch) generalizes: a GraphQL `nodes()` query keyed by PR node IDs could fetch author + merge state for many PRs in one call, cutting the metadata `GET` to near-zero; issue-events (used only to detect a human `unassigned` event) would need a similar `timelineItems` batch to fully close the gap. diff --git a/foc-mechanical-rules/foc_mechanical_rules/github_api.py b/foc-mechanical-rules/foc_mechanical_rules/github_api.py index e39883d..9c87af5 100644 --- a/foc-mechanical-rules/foc_mechanical_rules/github_api.py +++ b/foc-mechanical-rules/foc_mechanical_rules/github_api.py @@ -7,9 +7,11 @@ from __future__ import annotations +import re from typing import Any, Dict, List, Optional import requests +from github_projects_client import graphql_query # Orgs where we have write access to manage assignees, milestones, reviewers, # etc. Items from repos outside these orgs are "external items" (see @@ -19,6 +21,36 @@ FILOZ_ORG = "FilOzone" PROJECT_NUMBER = 14 +# Matches R-PR-004's release-PR detection regex. Shared by any rule that +# needs to tell a release PR apart from a regular one (e.g. R-PR-001, +# R-PR-010). +RELEASE_PR_TITLE_RE = re.compile(r"^chore\((master|main)\):?\s*release|^chore: release") + +BOT_LOGINS = {"dependabot", "filozzy"} + + +def is_bot_author(login: str) -> bool: + """True if a REST-style login (e.g. a PR author) belongs to a bot. + + Covers dependabot/FilOzzy by name, any `app/*` author, and the `[bot]` + suffix GitHub's REST API appends to bot logins. GraphQL results instead + expose an `author { __typename }` field ("Bot" vs "User") that's more + reliable when available -- see `is_bot_actor`. + """ + lower = login.lower() + return lower in BOT_LOGINS or lower.startswith("app/") or lower.endswith("[bot]") + + +def is_bot_actor(login: str, typename: str) -> bool: + """True if a GraphQL actor (review/comment author, timeline actor, ...) is a bot. + + Prefer this over `is_bot_author` for GraphQL results: `__typename == + "Bot"` is authoritative (e.g. catches `copilot-pull-request-reviewer`, + which has no `[bot]` suffix on GraphQL), and the login-pattern check + still catches anything `__typename` alone might miss. + """ + return typename == "Bot" or is_bot_author(login) + def build_session(token: str) -> requests.Session: """Build a requests.Session authenticated with the given token.""" @@ -79,3 +111,74 @@ def add_assignee( timeout=30, ) resp.raise_for_status() + + +def get_collaborator_permission( + session: requests.Session, *, owner: str, repo: str, username: str +) -> Optional[str]: + """Return a collaborator's permission level on a repo ("admin"/"write"/"maintain"/"triage"/"read"). + + Used to verify a reviewer actually has merge authority before treating + their review as authoritative (see R-SL-001's verification step). Returns + None if the lookup fails (e.g. the token lacks access to an external + repo, or the user isn't a collaborator) -- callers should treat that as + "not verified as write access", not as "read access confirmed". + """ + resp = session.get( + f"https://api.github.com/repos/{owner}/{repo}/collaborators/{username}/permission", + timeout=30, + ) + if not resp.ok: + return None + return resp.json().get("permission") + + +PR_REVIEW_CONTEXT_QUERY = """ +query($owner: String!, $repo: String!, $number: Int!) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $number) { + isDraft + author { login } + commits(last: 1) { nodes { commit { committedDate } } } + reviews(first: 100) { + nodes { author { login __typename } state submittedAt } + } + reviewRequests(first: 20) { + nodes { requestedReviewer { ... on User { login } } } + } + comments(last: 30) { + nodes { author { login __typename } createdAt } + } + timelineItems(last: 1, itemTypes: [PROJECT_V2_ITEM_STATUS_CHANGED_EVENT]) { + nodes { + ... on ProjectV2ItemStatusChangedEvent { + createdAt + previousStatus + status + } + } + } + } + } +} +""" + + +def get_pr_review_context( + session: requests.Session, *, owner: str, repo: str, number: str +) -> Dict[str, Any]: + """Fetch everything R-PR-010 needs about a PR in one GraphQL round trip. + + Draft state, author, last commit timestamp, reviews, pending review + requests, recent comments, and its Status field's change history + (`ProjectV2ItemStatusChangedEvent` -- see foc-mechanical-rules/README.md's + "Mutation log" section for why Status, uniquely among project fields, has + real GitHub-provided history instead of needing this tool's own log). + """ + data = graphql_query( + session, + PR_REVIEW_CONTEXT_QUERY, + {"owner": owner, "repo": repo, "number": int(number)}, + ) + pr = ((data.get("repository") or {}).get("pullRequest")) or {} + return pr diff --git a/foc-mechanical-rules/foc_mechanical_rules/registry.py b/foc-mechanical-rules/foc_mechanical_rules/registry.py index 11eac03..ba5b4b4 100644 --- a/foc-mechanical-rules/foc_mechanical_rules/registry.py +++ b/foc-mechanical-rules/foc_mechanical_rules/registry.py @@ -10,7 +10,8 @@ from .rule import Rule from .rules.assignee import AssigneeRule from .rules.cycle import CycleRule, PastCycleRule +from .rules.pr_status import PRStatusRule def default_rules() -> List[Rule]: - return [AssigneeRule(), CycleRule(), PastCycleRule()] + return [AssigneeRule(), CycleRule(), PastCycleRule(), PRStatusRule()] diff --git a/foc-mechanical-rules/foc_mechanical_rules/rules/assignee.py b/foc-mechanical-rules/foc_mechanical_rules/rules/assignee.py index 727939c..a07e68f 100644 --- a/foc-mechanical-rules/foc_mechanical_rules/rules/assignee.py +++ b/foc-mechanical-rules/foc_mechanical_rules/rules/assignee.py @@ -13,7 +13,6 @@ from __future__ import annotations -import re from typing import Any, Dict, List, Optional import requests @@ -23,24 +22,16 @@ BLESSED_ORGS, FILOZ_ORG, PROJECT_NUMBER, + RELEASE_PR_TITLE_RE, add_assignee, get_issue_events, get_pull_request, + is_bot_author, parse_repo_ref, ) from ..mutation_log import MutationLog from ..rule import ActionResult, Rule -# Matches R-PR-004's release-PR detection regex. -RELEASE_PR_TITLE_RE = re.compile(r"^chore\((master|main)\):?\s*release|^chore: release") - -BOT_LOGINS = {"dependabot", "filozzy"} - - -def _is_bot_author(login: str) -> bool: - lower = login.lower() - return lower in BOT_LOGINS or lower.startswith("app/") or lower.endswith("[bot]") - class AssigneeRule(Rule): id = "R-PR-001" @@ -112,7 +103,7 @@ def apply_one( merged_by = (pr.get("merged_by") or {}).get("login") is_release_pr = bool(RELEASE_PR_TITLE_RE.match(title)) - if _is_bot_author(author): + if is_bot_author(author): if merged and is_release_pr: if not merged_by: return ActionResult( diff --git a/foc-mechanical-rules/foc_mechanical_rules/rules/pr_status.py b/foc-mechanical-rules/foc_mechanical_rules/rules/pr_status.py new file mode 100644 index 0000000..8f76c4c --- /dev/null +++ b/foc-mechanical-rules/foc_mechanical_rules/rules/pr_status.py @@ -0,0 +1,379 @@ +"""R-PR-010: route Triage PRs to their correct Status. + +Canonical English rules: +- foc-board-rules/pr-hygiene.md#r-pr-010-triage-prs-should-be-routed-to-the-correct-status +- foc-board-rules/pr-status-table.md (the decision table this rule implements) + +This module is that rule's canonical implementation -- see rules/assignee.py's +docstring for why the markdown and this module link back to each other. + +This mechanizes the Triage slice of R-PR-005 (draft -> In Progress) and +R-PR-006 (the pr-status-table.md decision table), plus a guard the prose +rules don't yet state explicitly: if a human has ever moved this PR *out* +of Triage and then explicitly moved it back, that's a deliberate re-triage +decision and this rule leaves it alone rather than routing it back out. +Unlike Cycle (see rules/cycle.py), Status is the one project field GitHub +exposes real change history for -- `ProjectV2ItemStatusChangedEvent` on the +PR's timeline -- so this guard reads that directly instead of needing this +tool's own mutation log. + +Deliberate simplifications vs. the full pr-status-table.md logic (both are +about avoiding judgment calls this mechanical-rules system isn't meant to +make -- see rule.py's module docstring): +- Informal PR *comments* are never used to compute `last_feedback` -- + judging whether a comment is substantive feedback or coordination chatter + requires reading the comment, which isn't a pure function of structured + data. Only formal reviews (APPROVED / CHANGES_REQUESTED / COMMENTED) count. + If a Triage PR has qualifying comments after its last commit, this rule + flags it instead of auto-routing, so a human applies R-PR-006/R-SL-010 + judgment by hand. +- `authoritative_approval` doesn't parse approval text for conditional + language ("approving assuming you address X") -- it treats any qualifying + APPROVED review as authoritative. R-SL-001's language-based superseding + nuance is judgment, not a fact lookup. + +API call pattern: see README.md's "API call pattern per rule" table. If you +change what this rule reads or writes per item, update that table too. +""" + +from __future__ import annotations + +from datetime import datetime +from typing import Any, Dict, List, Optional + +import requests +from github_projects_client import GitHubAPIError, list_items, set_field_value_bulk + +from ..github_api import ( + FILOZ_ORG, + PROJECT_NUMBER, + RELEASE_PR_TITLE_RE, + get_collaborator_permission, + get_pr_review_context, + is_bot_actor, + is_bot_author, + parse_repo_ref, +) +from ..mutation_log import MutationLog +from ..rule import ActionResult, Rule + +STATUS_TRIAGE = "📌 Triage" +STATUS_TODO = "🐱 Todo" +STATUS_IN_PROGRESS = "⌨️ In Progress" +STATUS_AWAITING_REVIEW = "🔎 Awaiting review" +STATUS_APPROVED = "✔️ Approved by reviewer" + +# Permission levels that count as "merge authority" per R-SL-001. +_WRITE_LEVELS = {"admin", "maintain", "write"} + + +def _parse_dt(value: Optional[str]) -> Optional[datetime]: + if not value: + return None + return datetime.fromisoformat(value.replace("Z", "+00:00")) + + +class PRStatusRule(Rule): + id = "R-PR-010" + field_name = "status" + doc_url = ( + "https://github.com/FilOzone/tpm-utils/blob/master/foc-board-rules/" + "pr-hygiene.md#r-pr-010-triage-prs-should-be-routed-to-the-correct-status" + ) + + def __init__(self, org: str = FILOZ_ORG, project_number: int = PROJECT_NUMBER): + self.org = org + self.project_number = project_number + # Cache of (owner, repo, username) -> permission, so a reviewer who + # shows up on several candidate PRs in one run only costs one lookup. + self._permission_cache: Dict[tuple, Optional[str]] = {} + + def select(self, session: requests.Session) -> List[Dict[str, Any]]: + items: List[Dict[str, Any]] = [] + cursor: Optional[str] = None + while True: + result = list_items( + session, + org=self.org, + project_number=self.project_number, + query=f'is:pr status:"{STATUS_TRIAGE}"', + fields=["Repository", "Id", "Title", "url"], + cursor=cursor, + ) + items.extend(result["items"]) + if not result["has_more"]: + break + cursor = result["next_cursor"] + return items + + def _has_write_access( + self, session: requests.Session, owner: str, repo: str, login: str + ) -> bool: + key = (owner, repo, login.lower()) + if key not in self._permission_cache: + self._permission_cache[key] = get_collaborator_permission( + session, owner=owner, repo=repo, username=login + ) + permission = self._permission_cache[key] + return permission in _WRITE_LEVELS + + def _explicitly_returned_to_triage(self, timeline: List[Dict[str, Any]]) -> bool: + """True if the most recent status change moved the item INTO Triage from + some other status (as opposed to the initial add-to-project default). + + ``timeline`` holds at most one event -- ``get_pr_review_context``'s + query already asks for just the last (most recent) status-changed + event, since that's the only one this guard cares about. + """ + if not timeline: + return False + last = timeline[-1] + return last.get("status") == STATUS_TRIAGE and bool(last.get("previousStatus")) + + def apply_one( + self, + session: requests.Session, + item: Dict[str, Any], + *, + dry_run: bool, + mutation_log: MutationLog, + ) -> ActionResult: + # The revert-to-Triage guard reads real GitHub Status history + # instead -- see this module's docstring. + del mutation_log + + repository = item.get("Repository", "") + number = str(item.get("Id", "")) + title = item.get("Title", "") + owner, repo = parse_repo_ref(repository) + item_ref = f"{owner}/{repo}#{number}" + node_id = item.get("_node_id", "") + + try: + pr = get_pr_review_context(session, owner=owner, repo=repo, number=number) + except (requests.HTTPError, GitHubAPIError) as exc: + return ActionResult( + item_ref=item_ref, + title=title, + status="error", + reason=f"failed to fetch PR review context: {exc}", + ) + if not pr: + return ActionResult( + item_ref=item_ref, + title=title, + status="error", + reason="PR not found via GraphQL (deleted or inaccessible?)", + ) + + timeline = (pr.get("timelineItems") or {}).get("nodes") or [] + if self._explicitly_returned_to_triage(timeline): + return ActionResult( + item_ref=item_ref, + title=title, + status="flagged", + reason=( + f"{self.id}: this PR was moved out of Triage before and a human " + "has since moved it back -- not auto-routing out again" + ), + ) + + draft = bool(pr.get("isDraft")) + author = (pr.get("author") or {}).get("login", "") + is_bot = is_bot_author(author) + is_release = bool(RELEASE_PR_TITLE_RE.match(title)) + + commit_nodes = (pr.get("commits") or {}).get("nodes") or [] + last_commit = _parse_dt( + (commit_nodes[0].get("commit") or {}).get("committedDate") + if commit_nodes + else None + ) + + requested_logins = { + (rr.get("requestedReviewer") or {}).get("login", "").lower() + for rr in (pr.get("reviewRequests") or {}).get("nodes") or [] + if (rr.get("requestedReviewer") or {}).get("login") + } + + # Latest qualifying (human, write-access, non-self) formal review per + # reviewer, oldest to newest, so later entries overwrite earlier ones. + latest_by_reviewer: Dict[str, Dict[str, Any]] = {} + feedback_timestamps: List[datetime] = [] + reviews = sorted( + (pr.get("reviews") or {}).get("nodes") or [], + key=lambda r: r.get("submittedAt") or "", + ) + for review in reviews: + review_author = review.get("author") or {} + login = review_author.get("login", "") + state = review.get("state") + submitted_at = _parse_dt(review.get("submittedAt")) + if not login or not state or submitted_at is None: + continue + if login.lower() == author.lower(): + continue # self-review + if is_bot_actor(login, review_author.get("__typename", "")): + continue + if not self._has_write_access(session, owner, repo, login): + continue + if state in ("APPROVED", "CHANGES_REQUESTED"): + latest_by_reviewer[login.lower()] = { + "state": state, + "submitted_at": submitted_at, + "login": login, + } + if state in ("CHANGES_REQUESTED", "COMMENTED"): + feedback_timestamps.append(submitted_at) + + authoritative_approval = any( + r["state"] == "APPROVED" for r in latest_by_reviewer.values() + ) + blocking_cr = False + for r in latest_by_reviewer.values(): + if r["state"] != "CHANGES_REQUESTED": + continue + re_requested = r["login"].lower() in requested_logins + superseded_by_commit = ( + last_commit is not None and last_commit > r["submitted_at"] + ) + if re_requested or not superseded_by_commit: + blocking_cr = True + + last_feedback = max(feedback_timestamps) if feedback_timestamps else None + + # Post-commit human comments aren't judged for substantiveness (see + # module docstring) -- their presence alone routes to a flag instead + # of an auto-applied status. + flagged_comments = False + for comment in (pr.get("comments") or {}).get("nodes") or []: + comment_author = comment.get("author") or {} + login = comment_author.get("login", "") + if not login or login.lower() == author.lower(): + continue + if is_bot_actor(login, comment_author.get("__typename", "")): + continue + created_at = _parse_dt(comment.get("createdAt")) + if created_at is None: + continue + if last_commit is None or created_at > last_commit: + flagged_comments = True + break + + # Comment substantiveness is only ambiguous for the timestamp-driven + # rows (4-6) -- a draft, bot/release, or authoritatively-approved PR + # routes the same way regardless of trailing comments (R-PR-005 has + # no comment carve-out, and neither do rows 2/3). + timing_driven = ( + not draft + and not (is_bot or is_release) + and not (authoritative_approval and not blocking_cr) + ) + + if draft: + target = STATUS_IN_PROGRESS + reason = "draft PR" + elif is_bot or is_release: + target = STATUS_TODO + reason = "bot-authored or release PR" + elif authoritative_approval and not blocking_cr: + target = STATUS_APPROVED + reason = "has an authoritative approval and no blocking changes-requested" + elif last_feedback is not None and ( + last_commit is None or last_feedback >= last_commit + ): + target = STATUS_IN_PROGRESS + reason = "unaddressed reviewer feedback is the most recent activity" + elif last_feedback is not None: + target = STATUS_AWAITING_REVIEW + reason = "author has pushed commits since the last reviewer feedback" + else: + target = STATUS_AWAITING_REVIEW + reason = "no reviewer feedback yet" + + if timing_driven and flagged_comments: + return ActionResult( + item_ref=item_ref, + title=title, + status="flagged", + reason=( + f"{self.id}: has human PR comments after the last commit that " + "weren't evaluated (comment substantiveness needs a human judgment " + "call) -- apply R-PR-006/R-SL-010 by hand" + ), + ) + + if dry_run: + return ActionResult( + item_ref=item_ref, + title=title, + status="applied", + reason=f"dry-run: would move to {target} ({reason})", + old_value=STATUS_TRIAGE, + new_value=target, + ) + + if not node_id: + # Per README.md's "API call pattern per rule" #3, mutations + # should always carry the item's real node ID -- list_items + # always includes it, so a miss here means something upstream + # is broken and silently falling back to item_ref would just + # mask that (and cost set_field_value_bulk an extra per-item + # lookup, or fail outright if the ref format ever changes). + return ActionResult( + item_ref=item_ref, + title=title, + status="error", + reason="no node ID for this item -- cannot queue a mutation", + ) + + return ActionResult( + item_ref=item_ref, + title=title, + status="pending", + old_value=STATUS_TRIAGE, + new_value=target, + node_id=node_id, + ) + + def mutate_pending( + self, session: requests.Session, pending: List[ActionResult] + ) -> List[ActionResult]: + finalized: List[ActionResult] = [] + by_value: Dict[str, List[ActionResult]] = {} + for p in pending: + by_value.setdefault(p.new_value, []).append(p) + + for new_value, group in by_value.items(): + bulk_result = set_field_value_bulk( + session, + org=self.org, + project_number=self.project_number, + item_refs=[p.node_id for p in group], + field_name="Status", + value=new_value, + ) + by_node_id = {r["item_ref"]: r for r in bulk_result["results"]} + for p in group: + r = by_node_id.get(p.node_id) + if not r or not r.get("success"): + error = (r or {}).get("error", "no result for this item") + finalized.append( + ActionResult( + item_ref=p.item_ref, + title=p.title, + status="error", + reason=f"failed to set status: {error}", + ) + ) + else: + finalized.append( + ActionResult( + item_ref=p.item_ref, + title=p.title, + status="applied", + old_value=r.get("old_value", p.old_value), + new_value=new_value, + ) + ) + return finalized diff --git a/foc-mechanical-rules/tests/test_cli.py b/foc-mechanical-rules/tests/test_cli.py index 703f5bb..6bd713b 100644 --- a/foc-mechanical-rules/tests/test_cli.py +++ b/foc-mechanical-rules/tests/test_cli.py @@ -28,7 +28,7 @@ def test_no_rule_flag_runs_every_registered_rule(capsys): _run_cli(["--dry-run", "--token", "x"], run_all_mock) ran_ids = {rule.id for rule in run_all_mock.call_args.args[1]} - assert ran_ids == {"R-PR-001", "R-FC-012", "R-FC-013"} + assert ran_ids == {"R-PR-001", "R-FC-012", "R-FC-013", "R-PR-010"} def test_rule_flag_filters_to_the_named_rule(capsys): diff --git a/foc-mechanical-rules/tests/test_pr_status_rule.py b/foc-mechanical-rules/tests/test_pr_status_rule.py new file mode 100644 index 0000000..175b9b0 --- /dev/null +++ b/foc-mechanical-rules/tests/test_pr_status_rule.py @@ -0,0 +1,363 @@ +"""Unit tests for R-PR-010 (Triage PR status routing) — mocked GitHub API, no live calls.""" + +from __future__ import annotations + +from unittest.mock import MagicMock, patch + +from foc_mechanical_rules.rule import ActionResult, Rule +from foc_mechanical_rules.rules.pr_status import PRStatusRule + +ITEM = { + "Repository": "FilOzone/dealbot", + "Id": "458", + "Title": "fix: something", + "url": "https://github.com/FilOzone/dealbot/pull/458", + "_node_id": "PVTI_abc123", +} + +# get_pr_review_context's query asks GitHub for just the last (most recent) +# status-changed event via `timelineItems(last: 1, ...)`, so these fixtures +# hold at most one element, matching what the real query returns. +NEVER_LEFT_TRIAGE = [ + {"createdAt": "2026-08-01T00:00:00Z", "previousStatus": "", "status": "📌 Triage"}, +] + +RETURNED_TO_TRIAGE = [ + { + "createdAt": "2026-08-10T00:00:00Z", + "previousStatus": "⌨️ In Progress", + "status": "📌 Triage", + }, +] + + +def _pr( + *, + is_draft=False, + author="alice", + last_commit=None, + reviews=None, + review_requests=None, + comments=None, + timeline=NEVER_LEFT_TRIAGE, +): + return { + "isDraft": is_draft, + "author": {"login": author}, + "commits": { + "nodes": [{"commit": {"committedDate": last_commit}}] if last_commit else [] + }, + "reviews": {"nodes": reviews or []}, + "reviewRequests": {"nodes": review_requests or []}, + "comments": {"nodes": comments or []}, + "timelineItems": {"nodes": timeline}, + } + + +def _review(login, state, submitted_at, typename="User"): + return { + "author": {"login": login, "__typename": typename}, + "state": state, + "submittedAt": submitted_at, + } + + +def _comment(login, created_at, typename="User"): + return {"author": {"login": login, "__typename": typename}, "createdAt": created_at} + + +@patch("foc_mechanical_rules.rules.pr_status.get_pr_review_context") +def test_explicit_return_to_triage_is_flagged(mock_ctx): + mock_ctx.return_value = _pr(is_draft=True, timeline=RETURNED_TO_TRIAGE) + + result = PRStatusRule().apply_one( + MagicMock(), ITEM, dry_run=False, mutation_log=None + ) + + assert result.status == "flagged" + assert "moved it back" in result.reason + + +@patch("foc_mechanical_rules.rules.pr_status.get_pr_review_context") +def test_draft_pr_moves_to_in_progress(mock_ctx): + mock_ctx.return_value = _pr(is_draft=True) + + result = PRStatusRule().apply_one( + MagicMock(), ITEM, dry_run=False, mutation_log=None + ) + + assert result.status == "pending" + assert result.new_value == "⌨️ In Progress" + assert result.node_id == "PVTI_abc123" + + +@patch("foc_mechanical_rules.rules.pr_status.get_pr_review_context") +def test_bot_authored_pr_moves_to_todo(mock_ctx): + mock_ctx.return_value = _pr(author="dependabot") + + result = PRStatusRule().apply_one( + MagicMock(), ITEM, dry_run=False, mutation_log=None + ) + + assert result.status == "pending" + assert result.new_value == "🐱 Todo" + + +@patch("foc_mechanical_rules.rules.pr_status.get_pr_review_context") +def test_release_pr_moves_to_todo(mock_ctx): + item = {**ITEM, "Title": "chore(master): release 1.2.3"} + mock_ctx.return_value = _pr(author="alice") + + result = PRStatusRule().apply_one( + MagicMock(), item, dry_run=False, mutation_log=None + ) + + assert result.status == "pending" + assert result.new_value == "🐱 Todo" + + +@patch( + "foc_mechanical_rules.rules.pr_status.get_collaborator_permission", + return_value="write", +) +@patch("foc_mechanical_rules.rules.pr_status.get_pr_review_context") +def test_authoritative_approval_moves_to_approved(mock_ctx, mock_perm): + mock_ctx.return_value = _pr( + last_commit="2026-08-01T00:00:00Z", + reviews=[_review("bob", "APPROVED", "2026-08-02T00:00:00Z")], + ) + + result = PRStatusRule().apply_one( + MagicMock(), ITEM, dry_run=False, mutation_log=None + ) + + assert result.status == "pending" + assert result.new_value == "✔️ Approved by reviewer" + + +@patch( + "foc_mechanical_rules.rules.pr_status.get_collaborator_permission", + return_value="read", +) +@patch("foc_mechanical_rules.rules.pr_status.get_pr_review_context") +def test_read_access_approval_does_not_count(mock_ctx, mock_perm): + mock_ctx.return_value = _pr( + last_commit="2026-08-01T00:00:00Z", + reviews=[_review("bob", "APPROVED", "2026-08-02T00:00:00Z")], + ) + + result = PRStatusRule().apply_one( + MagicMock(), ITEM, dry_run=False, mutation_log=None + ) + + # No qualifying feedback and no qualifying approval -> row 6, Awaiting review. + assert result.status == "pending" + assert result.new_value == "🔎 Awaiting review" + + +@patch( + "foc_mechanical_rules.rules.pr_status.get_collaborator_permission", + return_value="write", +) +@patch("foc_mechanical_rules.rules.pr_status.get_pr_review_context") +def test_blocking_changes_requested_moves_to_in_progress(mock_ctx, mock_perm): + mock_ctx.return_value = _pr( + last_commit="2026-08-01T00:00:00Z", + reviews=[_review("bob", "CHANGES_REQUESTED", "2026-08-02T00:00:00Z")], + ) + + result = PRStatusRule().apply_one( + MagicMock(), ITEM, dry_run=False, mutation_log=None + ) + + assert result.status == "pending" + assert result.new_value == "⌨️ In Progress" + + +@patch( + "foc_mechanical_rules.rules.pr_status.get_collaborator_permission", + return_value="write", +) +@patch("foc_mechanical_rules.rules.pr_status.get_pr_review_context") +def test_commit_after_changes_requested_moves_to_awaiting_review(mock_ctx, mock_perm): + mock_ctx.return_value = _pr( + last_commit="2026-08-03T00:00:00Z", + reviews=[_review("bob", "CHANGES_REQUESTED", "2026-08-02T00:00:00Z")], + ) + + result = PRStatusRule().apply_one( + MagicMock(), ITEM, dry_run=False, mutation_log=None + ) + + assert result.status == "pending" + assert result.new_value == "🔎 Awaiting review" + + +@patch( + "foc_mechanical_rules.rules.pr_status.get_collaborator_permission", + return_value="write", +) +@patch("foc_mechanical_rules.rules.pr_status.get_pr_review_context") +def test_re_requested_cr_reviewer_blocks_a_later_approval(mock_ctx, mock_perm): + # filecoin-pin-website#154 shape: bob's CR is superseded by a commit and + # carol's later approval, but bob has been re-requested -- his objection + # stays blocking, so the approval never becomes authoritative and row 3 + # doesn't fire (falls through to the timestamp rows instead). + mock_ctx.return_value = _pr( + last_commit="2026-08-02T00:00:00Z", + reviews=[ + _review("bob", "CHANGES_REQUESTED", "2026-08-01T00:00:00Z"), + _review("carol", "APPROVED", "2026-08-03T00:00:00Z"), + ], + review_requests=[{"requestedReviewer": {"login": "bob"}}], + ) + + result = PRStatusRule().apply_one( + MagicMock(), ITEM, dry_run=False, mutation_log=None + ) + + assert result.status == "pending" + assert result.new_value != "✔️ Approved by reviewer" + + +@patch("foc_mechanical_rules.rules.pr_status.get_pr_review_context") +def test_no_feedback_moves_to_awaiting_review(mock_ctx): + mock_ctx.return_value = _pr() + + result = PRStatusRule().apply_one( + MagicMock(), ITEM, dry_run=False, mutation_log=None + ) + + assert result.status == "pending" + assert result.new_value == "🔎 Awaiting review" + + +@patch("foc_mechanical_rules.rules.pr_status.get_pr_review_context") +def test_comments_after_last_commit_are_flagged_not_auto_routed(mock_ctx): + mock_ctx.return_value = _pr( + last_commit="2026-08-01T00:00:00Z", + comments=[_comment("bob", "2026-08-02T00:00:00Z")], + ) + + result = PRStatusRule().apply_one( + MagicMock(), ITEM, dry_run=False, mutation_log=None + ) + + assert result.status == "flagged" + assert "comments" in result.reason + + +@patch("foc_mechanical_rules.rules.pr_status.get_pr_review_context") +def test_bot_comment_after_last_commit_is_ignored(mock_ctx): + mock_ctx.return_value = _pr( + last_commit="2026-08-01T00:00:00Z", + comments=[ + _comment( + "copilot-pull-request-reviewer", "2026-08-02T00:00:00Z", typename="Bot" + ) + ], + ) + + result = PRStatusRule().apply_one( + MagicMock(), ITEM, dry_run=False, mutation_log=None + ) + + assert result.status == "pending" + assert result.new_value == "🔎 Awaiting review" + + +@patch("foc_mechanical_rules.rules.pr_status.get_pr_review_context") +def test_self_comment_after_last_commit_is_ignored(mock_ctx): + mock_ctx.return_value = _pr( + author="alice", + last_commit="2026-08-01T00:00:00Z", + comments=[_comment("alice", "2026-08-02T00:00:00Z")], + ) + + result = PRStatusRule().apply_one( + MagicMock(), ITEM, dry_run=False, mutation_log=None + ) + + assert result.status == "pending" + assert result.new_value == "🔎 Awaiting review" + + +@patch("foc_mechanical_rules.rules.pr_status.get_pr_review_context") +def test_trailing_comments_do_not_block_draft_routing(mock_ctx): + # R-PR-005 has no comment carve-out -- a draft PR routes to In Progress + # regardless of trailing comments. + mock_ctx.return_value = _pr( + is_draft=True, + last_commit="2026-08-01T00:00:00Z", + comments=[_comment("bob", "2026-08-02T00:00:00Z")], + ) + + result = PRStatusRule().apply_one( + MagicMock(), ITEM, dry_run=False, mutation_log=None + ) + + assert result.status == "pending" + assert result.new_value == "⌨️ In Progress" + + +@patch("foc_mechanical_rules.rules.pr_status.get_pr_review_context") +def test_dry_run_does_not_queue_a_mutation(mock_ctx): + mock_ctx.return_value = _pr(is_draft=True) + + result = PRStatusRule().apply_one( + MagicMock(), ITEM, dry_run=True, mutation_log=None + ) + + assert result.status == "applied" + assert result.new_value == "⌨️ In Progress" + + +@patch("foc_mechanical_rules.rules.pr_status.get_pr_review_context") +def test_pr_not_found_is_an_error(mock_ctx): + mock_ctx.return_value = {} + + result = PRStatusRule().apply_one( + MagicMock(), ITEM, dry_run=False, mutation_log=None + ) + + assert result.status == "error" + + +@patch("foc_mechanical_rules.rules.pr_status.set_field_value_bulk") +def test_mutate_pending_batches_by_target_value(mock_bulk): + mock_bulk.return_value = { + "results": [ + {"item_ref": "PVTI_1", "success": True, "old_value": "📌 Triage"}, + {"item_ref": "PVTI_2", "success": True, "old_value": "📌 Triage"}, + ] + } + pending = [ + ActionResult( + item_ref="FilOzone/dealbot#1", + title="a", + status="pending", + old_value="📌 Triage", + new_value="⌨️ In Progress", + node_id="PVTI_1", + ), + ActionResult( + item_ref="FilOzone/dealbot#2", + title="b", + status="pending", + old_value="📌 Triage", + new_value="⌨️ In Progress", + node_id="PVTI_2", + ), + ] + + finalized = PRStatusRule().mutate_pending(MagicMock(), pending) + + assert len(finalized) == 2 + assert all(r.status == "applied" for r in finalized) + mock_bulk.assert_called_once() + assert mock_bulk.call_args.kwargs["field_name"] == "Status" + assert mock_bulk.call_args.kwargs["item_refs"] == ["PVTI_1", "PVTI_2"] + + +def test_pr_status_rule_is_a_rule(): + assert isinstance(PRStatusRule(), Rule) diff --git a/github-projects-client/github_projects_client/__init__.py b/github-projects-client/github_projects_client/__init__.py index 9159791..e085dc6 100644 --- a/github-projects-client/github_projects_client/__init__.py +++ b/github-projects-client/github_projects_client/__init__.py @@ -1,6 +1,12 @@ """Context-efficient client library for GitHub Projects v2 boards.""" -from .api import graphql_query, list_field_ids_by_name, fetch_items_rest +from .api import ( + GitHubAPIError, + GitHubAuthError, + graphql_query, + list_field_ids_by_name, + fetch_items_rest, +) from .fields import list_field_options from .items import list_items, list_fields, get_item from .mutations import set_field_value, set_field_value_bulk @@ -8,6 +14,8 @@ from .views import resolve_view_url __all__ = [ + "GitHubAPIError", + "GitHubAuthError", "graphql_query", "list_field_ids_by_name", "fetch_items_rest",