From 1026b0e127ef2c339df6ed9a30ca3065fe5727f7 Mon Sep 17 00:00:00 2001 From: Bryan Gin-ge Chen Date: Sun, 12 Apr 2026 15:02:39 -0400 Subject: [PATCH 01/24] doc: add design decision for zulip close-pr command (#041) Covers command flow, permission check (PR author + collaborator write/admin via GitHub App), token service, confirmation form, post-close actions (sync, DM, repo log thread), and settings (ZULIP_CLOSE_PR_MUTATIONS_ENABLED, ZULIP_REPO_LOG). Notes required queueboard-assignment app permission upgrades: Pull requests: Read and write, Members: Read (org). Custom close-message follow-up planned as commit 3. Co-Authored-By: Claude Sonnet 4.6 --- .../041-zulip-close-pr-command.md | 171 ++++++++++++++++++ 1 file changed, 171 insertions(+) create mode 100644 docs/design-decisions/041-zulip-close-pr-command.md diff --git a/docs/design-decisions/041-zulip-close-pr-command.md b/docs/design-decisions/041-zulip-close-pr-command.md new file mode 100644 index 00000000..a5ea923b --- /dev/null +++ b/docs/design-decisions/041-zulip-close-pr-command.md @@ -0,0 +1,171 @@ +# Zulip Close-PR Command + +## Context +- Certain Queueboard users have GitHub write/maintain/admin access and sometimes need to close PRs without + their personal GitHub identity being visible (e.g. to keep moderation actions neutral). +- Existing Zulip bot commands use `ZULIP_COMMAND_POLICY` for access control, but that only gates which + Zulip users can invoke a command — it does not verify that the invoker actually has the corresponding + GitHub permission at runtime. +- Closing a PR is a destructive, hard-to-reverse action; it requires a confirmation step beyond a single + command invocation. +- The existing secure-link + web form pattern (used by `prefs` and registration) is a natural fit for a + confirmation flow. +- The `queueboard-assignment` GitHub App currently has `Issues: Read and write` and `Pull requests: + Read-only`. Closing a PR via `PATCH /repos/{owner}/{repo}/pulls/{number}` requires `Pull requests: + Read and write`. Checking a user's collaborator permission via + `GET /repos/{owner}/{repo}/collaborators/{username}/permission` requires `Members: Read` (org-level). + Both permissions must be added to the `queueboard-assignment` app. + +## Goals / Non-Goals +- Goals: + - Let authorized users close a PR from Zulip with their personal GitHub identity hidden (action + attributed to the GitHub App). + - Verify at command time that the invoker has GitHub write/maintain/admin on the target repo, or is the + PR author. + - Require explicit confirmation before any mutation. + - Log closes to a configurable per-repo Zulip stream/topic. +- Non-goals: + - Re-opening PRs. + - Closing PRs on repos not tracked by Queueboard. + - Storing or exposing the user's personal GitHub OAuth token. + +## Proposed Design + +### Command: `close-pr` +- Syntax: `close-pr ` +- Registered in `qb_site/zulip_bot/commands/close_pr.py`. +- Response mode: `PRIVATE` (link is sent only to the invoker). +- Flow: + 1. Parse exactly one PR URL from args / `rendered_content` (reuse `_parse_single_pr_ref` logic). + 2. Resolve invoker's `core.User` by `context.sender_id` (`zulip_user_id`); require `github_login`. + 3. Check permission (see below). + 4. Issue an encrypted `close-pr` token and send private link. +- Feature flag: `ZULIP_CLOSE_PR_MUTATIONS_ENABLED`. When disabled, command still validates and issues + the link but the form view blocks the actual close and returns a preflight-only message. + +### Permission Check (command time) +Performed using a GitHub App token for operation `close_pr` (mapped to `queueboard-assignment`): + +1. Fetch PR via `GET /repos/{owner}/{repo}/pulls/{number}`: + - Check PR is open; if not, return private "PR is not open" message (no link issued). + - Extract `user.login` to identify the PR author. +2. If `github_login == pr_author_login` (case-insensitive): **permitted**. +3. Otherwise call `GET /repos/{owner}/{repo}/collaborators/{github_login}/permission`: + - If `permission` in `{"write", "admin"}`: **permitted** (`maintain` role maps to `write` here). + - Otherwise: return private "you don't have permission to close this PR" message. + +### Token Service: `close_pr_links.py` +- Pattern mirrors `prefs_links.py` (Fernet encryption, `iat`/`exp` claims). +- Claims: `zulip_user_id`, `github_login`, `pr_owner`, `pr_repo`, `pr_number`. +- Settings: + - `ZULIP_CLOSE_PR_TOKEN_SECRET` (falls back to `SECRET_KEY`) + - `ZULIP_CLOSE_PR_TOKEN_SALT` (default: `"zulip_bot.close_pr"`) + - `ZULIP_CLOSE_PR_TOKEN_TTL_SECONDS` (default: `1800`) + - `ZULIP_CLOSE_PR_URL_BASE` (optional; falls back to relative path) + +### Execution Service: `close_pr_execution.py` +- `check_close_pr_permission(github_login, owner, repo, number)` → `PermissionCheckResult` + - Fetches PR + optionally checks collaborator permission. + - Returns structured result: `permitted`, `not_permitted`, `pr_not_open`, `token_unavailable`. +- `close_pull_request(owner, repo, number)` → success/error + - `PATCH /repos/{owner}/{repo}/pulls/{number}` with `{"state": "closed"}`. + - Uses `close_pr` operation token (same `queueboard-assignment` app). + +### Web Layer +- URL: `close-pr//` → `views.close_pr_form` +- View behavior: + - Validate token; show `close_pr_invalid.html` (HTTP 403) on expiry/invalid. + - Fetch live PR details (title, state) from GitHub to display in form. + - **GET**: render confirmation form. If PR is already closed/merged, show informational message + without a submit button (no redirect, no error page — same template, conditional rendering). + - **POST**: if `ZULIP_CLOSE_PR_MUTATIONS_ENABLED` is off, show preflight-only message. Otherwise: + 1. Close PR via execution service. + 2. Enqueue `sync_pr_task.delay(repository_id, pr_number)` (best-effort). + 3. Send DM to invoker confirming the close (best-effort; failure logged, not surfaced). + 4. Post to repo log thread if `ZULIP_REPO_LOG` has an entry for `owner/repo` (best-effort). + 5. Render success state in same template. + - `Cache-Control: no-store` on all responses. + +### Settings +- `ZULIP_CLOSE_PR_MUTATIONS_ENABLED` — feature flag (same pattern as assignment mutations flag). +- `ZULIP_REPO_LOG` — JSON object mapping `"owner/repo"` to `{"stream": "...", "topic": "..."}`. + If a repo has no entry, the log post step is skipped and a `WARNING` is emitted. + Designed to be reusable for future per-repo Zulip logging beyond close-PR. + + Example: + ```json + { + "leanprover-community/mathlib4": { + "stream": "mathlib4", + "topic": "bot actions" + } + } + ``` + +### GitHub App Changes (manual) +- Add to `queueboard-assignment`: + - `Pull requests: Read and write` (upgrade from read-only) + - `Members: Read` (org-level permission, new) +- Update `docs/github_app_setup.md` and `GITHUB_APP_TOKEN_CONFIG` operation map to include `close_pr`. + +## Subtleties / Invariants +- Permission check uses the GitHub App token, not the user's personal token. The close itself is also + performed by the GitHub App. The invoker's personal GitHub identity is never exposed. +- The collaborator permission endpoint returns `permission: "none"` for users with no access (not 404). + `maintain` role maps to `permission: "write"` in this endpoint's response. +- If `check_close_pr_permission` can't obtain a GitHub App token (app not installed on repo, config + missing), the command fails privately with a clear message — same behavior as assignment mutations. +- The form token embeds `github_login` at issuance time. If the user's GitHub login changes between + issuance and form submission, the close still proceeds using the embedded login for attribution + purposes (acceptable; the login change is rare and the token is short-lived). +- Sync enqueue, DM, and log post are all best-effort: failures are logged but do not cause the HTTP + response to indicate failure. The PR close itself is the authoritative outcome. +- The `ZULIP_REPO_LOG` setting is intentionally generic (not named `ZULIP_CLOSE_PR_LOG`) to allow + other future bot operations to reuse the same per-repo destination config. + +## Implementation Plan + +### Commit 1 (this doc) +- Add `docs/design-decisions/041-zulip-close-pr-command.md`. + +### Commit 2 (implementation) +- New files: + - `qb_site/zulip_bot/commands/close_pr.py` + - `qb_site/zulip_bot/services/close_pr_links.py` + - `qb_site/zulip_bot/services/close_pr_execution.py` + - `qb_site/templates/zulip_bot/close_pr_form.html` + - `qb_site/templates/zulip_bot/close_pr_invalid.html` +- Modified files: + - `qb_site/zulip_bot/views.py` — add `close_pr_form` view, import command module + - `qb_site/zulip_bot/urls.py` — add `close-pr//` + - `qb_site/zulip_bot/AGENTS.md` / `CLAUDE.md` — document new command and settings + - `docs/github_app_setup.md` — add new app permissions and `close_pr` operation + +### Commit 3 (follow-up: custom close message) +- Add textarea to confirmation form for optional close message. +- Add pre-written close message templates (rendered as selectable buttons/options). +- On submit, post the message as a PR comment (`POST /repos/{owner}/{repo}/issues/{number}/comments`) + before closing. No additional GitHub App permissions required (`Issues: Read and write` already + granted). + +## Validation Plan +- Tests: + - Token service: issue → validate round-trip; expiry; invalid/tampered tokens. + - Permission check: permitted as author; permitted as write collaborator; denied; PR not open; + token unavailable. + - Command handler: no linked user; linked but no GitHub login; not permitted; permitted (link issued); + feature-flag preflight path. + - View (GET): valid open PR; valid already-closed PR; expired token; invalid token. + - View (POST): successful close with sync enqueue + DM + log post; mutations disabled preflight; + GitHub error on close. + - `ZULIP_REPO_LOG` parsing: valid config; missing repo entry (skip + warn); malformed config. +- Manual checks: + - Verify `GET /repos/{owner}/{repo}/collaborators/{username}/permission` response shape and `"none"` + behavior for non-collaborators with the updated `queueboard-assignment` app credentials. + - Verify `PATCH /repos/{owner}/{repo}/pulls/{number}` with `{"state": "closed"}` succeeds with the + updated app token. + - End-to-end: issue command → receive link → open form → submit → PR closed → DM received → + log thread updated. + +## Progress Notes +- 2026-04-12: Design doc written; implementation not yet started. From c54ff74b58e777ce155ad71d31114f5eefff247e Mon Sep 17 00:00:00 2001 From: Bryan Gin-ge Chen Date: Sun, 12 Apr 2026 15:07:45 -0400 Subject: [PATCH 02/24] feat(zulip_bot): add close-pr command with permission check and confirmation form Adds a `close-pr` Zulip bot command that: - Checks the invoker's linked GitHub account has permission to close the target PR (PR author or write/admin collaborator), using a GitHub App token for the permission check - Issues a short-lived encrypted private link to a confirmation form - Closes the PR via PATCH /repos/{owner}/{repo}/pulls/{number} on form submission - Enqueues a sync task, sends a DM to the invoker, and posts to the per-repo log thread (ZULIP_REPO_LOG) on success New settings: - ZULIP_CLOSE_PR_MUTATIONS_ENABLED (feature flag) - ZULIP_REPO_LOG (per-repo stream/topic for bot action logging) - ZULIP_CLOSE_PR_TOKEN_{SECRET,SALT,TTL_SECONDS,URL_BASE} Requires queueboard-assignment GitHub App to be updated with: - Pull requests: Read and write (upgrade) - Members: Read (new org-level permission) and close_pr added to GITHUB_APP_TOKEN_CONFIG operation_app_map. Co-Authored-By: Claude Sonnet 4.6 --- docs/github_app_setup.md | 10 +- .../templates/zulip_bot/close_pr_form.html | 78 ++++++ .../templates/zulip_bot/close_pr_invalid.html | 16 ++ qb_site/zulip_bot/AGENTS.md | 13 +- qb_site/zulip_bot/commands/close_pr.py | 108 ++++++++ .../zulip_bot/services/close_pr_execution.py | 248 ++++++++++++++++++ qb_site/zulip_bot/services/close_pr_links.py | 133 ++++++++++ .../zulip_bot/tests/test_close_pr_command.py | 93 +++++++ .../tests/test_close_pr_execution.py | 238 +++++++++++++++++ qb_site/zulip_bot/tests/test_close_pr_form.py | 189 +++++++++++++ .../zulip_bot/tests/test_close_pr_links.py | 66 +++++ qb_site/zulip_bot/urls.py | 1 + qb_site/zulip_bot/views.py | 148 +++++++++++ 13 files changed, 1334 insertions(+), 7 deletions(-) create mode 100644 qb_site/templates/zulip_bot/close_pr_form.html create mode 100644 qb_site/templates/zulip_bot/close_pr_invalid.html create mode 100644 qb_site/zulip_bot/commands/close_pr.py create mode 100644 qb_site/zulip_bot/services/close_pr_execution.py create mode 100644 qb_site/zulip_bot/services/close_pr_links.py create mode 100644 qb_site/zulip_bot/tests/test_close_pr_command.py create mode 100644 qb_site/zulip_bot/tests/test_close_pr_execution.py create mode 100644 qb_site/zulip_bot/tests/test_close_pr_form.py create mode 100644 qb_site/zulip_bot/tests/test_close_pr_links.py diff --git a/docs/github_app_setup.md b/docs/github_app_setup.md index 5076d6f6..feab84f5 100644 --- a/docs/github_app_setup.md +++ b/docs/github_app_setup.md @@ -4,6 +4,7 @@ This guide covers how to create and configure GitHub Apps for Queueboard's opera ## Scope and Current Token Policy - Assignment commands (`assign_pr`, `unassign_pr`) require GitHub App installation tokens. +- Close-PR command (`close_pr`) requires a GitHub App installation token with `Pull requests: Read and write` and `Members: Read` (org-level). - Syncer operations (`syncer_repo_discovery`, `syncer_pr_read`, `syncer_ci_read`) try GitHub App tokens first and fall back to `GH_TOKEN`/`GITHUB_TOKEN` when no app token is available. - Runtime config is loaded from `GITHUB_APP_TOKEN_CONFIG` (JSON object). @@ -34,13 +35,13 @@ Notes: Set only the permissions needed by each operation set. -### App A: `queueboard-assignment` (assign/unassign) +### App A: `queueboard-assignment` (assign/unassign/close-pr) - Repository permissions: - `Issues: Read and write` (required for `POST/DELETE /repos/{owner}/{repo}/issues/{number}/assignees`) - - `Pull requests: Read-only` (required for live precondition checks on PR state/assignees) + - `Pull requests: Read and write` (required for close-PR mutation via `PATCH /repos/{owner}/{repo}/pulls/{number}`; also used for live precondition checks on PR state/assignees) - `Metadata: Read-only` (baseline repository visibility/lookup behavior) - Organization permissions: - - none required + - `Members: Read` (required for `GET /repos/{owner}/{repo}/collaborators/{username}/permission` to check if a user has write/admin access) ### App B: `queueboard-syncer-read` (syncer read operations) - Repository permissions: @@ -96,6 +97,7 @@ Example (two-app config): "operation_app_map": { "assign_pr": "queueboard-assignment", "unassign_pr": "queueboard-assignment", + "close_pr": "queueboard-assignment", "syncer_repo_discovery": "queueboard-syncer-read", "syncer_pr_read": "queueboard-syncer-read", "syncer_ci_read": "queueboard-syncer-read" @@ -106,7 +108,7 @@ Example (two-app config): "app_id": 123456, "private_key_path": "/run/secrets/queueboard-assignment.pem", "installation_lookup": "repo", - "operations": ["assign_pr", "unassign_pr"] + "operations": ["assign_pr", "unassign_pr", "close_pr"] }, { "name": "queueboard-syncer-read", diff --git a/qb_site/templates/zulip_bot/close_pr_form.html b/qb_site/templates/zulip_bot/close_pr_form.html new file mode 100644 index 00000000..5b4f623a --- /dev/null +++ b/qb_site/templates/zulip_bot/close_pr_form.html @@ -0,0 +1,78 @@ +{% load static %} + + + + + + Queueboard — close pull request + + + + +
+
+

Close Pull Request

+

{{ pr_owner }}/{{ pr_repo }}#{{ pr_number }}

+ {% if pr_title %} +

{{ pr_title }}

+ {% endif %} +
+ + {% if success %} +
+
+ Pull request {{ pr_owner }}/{{ pr_repo }}#{{ pr_number }} has been closed. + The close was attributed to the bot. A confirmation has been sent to you via Zulip DM. +
+
+ + {% elif mutations_disabled %} +
+
+ Preflight check passed — your account has permission to close this PR. + However, mutations are currently disabled on this server + (ZULIP_CLOSE_PR_MUTATIONS_ENABLED is not set). + Contact an administrator to enable the close-PR mutation. +
+
+ + {% elif not pr_is_open %} +
+
+ This pull request is already closed or merged. No action is needed. +
+
+ + {% elif close_error %} +
+ +

Please try again or contact an administrator.

+
+ + {% else %} +
+

+ You are about to close pull request + {{ pr_owner }}/{{ pr_repo }}#{{ pr_number }}{% if pr_title %}: {{ pr_title }}{% endif %}. +

+

+ This action will be attributed to the bot, not your personal GitHub account. + Closing a pull request is reversible (it can be re-opened), but please confirm you intend to proceed. +

+
+ {% csrf_token %} + +
+
+ +
+

+ This link expires at . +

+
+ {% endif %} +
+ + diff --git a/qb_site/templates/zulip_bot/close_pr_invalid.html b/qb_site/templates/zulip_bot/close_pr_invalid.html new file mode 100644 index 00000000..da5db2fc --- /dev/null +++ b/qb_site/templates/zulip_bot/close_pr_invalid.html @@ -0,0 +1,16 @@ + + + + + + Queueboard — close PR link unavailable + + +

Close PR link unavailable

+ {% if reason == "expired" %} +

This link has expired. Send close-pr <pr-url> to the bot again for a fresh link.

+ {% else %} +

This link is invalid. Send close-pr <pr-url> to the bot for a fresh link.

+ {% endif %} + + diff --git a/qb_site/zulip_bot/AGENTS.md b/qb_site/zulip_bot/AGENTS.md index a42a93fc..b77acc2f 100644 --- a/qb_site/zulip_bot/AGENTS.md +++ b/qb_site/zulip_bot/AGENTS.md @@ -17,27 +17,34 @@ cd qb_site/zulip_bot/frontend && npm test ``` ## Command Architecture Notes -- Commands live in `commands/`: `assign`, `unassign`, `assigned-prs`, `pr-info`, `prefs`, `help`, `echo`, `register_test`. +- Commands live in `commands/`: `assign`, `unassign`, `assigned-prs`, `pr-info`, `prefs`, `help`, `echo`, `register_test`, `close-pr`. - `pr-info`: parses GitHub PR links from Zulip `rendered_content`, reacts with 👀, then sends one stream message per PR (up to 10) with queue info sourced from `analyzer.services.pr_info`. - Assignment command flow (all under `services/`) is split for clarity: - parse: `assignment_command_parser.py`, - validate: `assignment_validation.py`, - preflight/mutation orchestration: `assignment_execution.py` + `assignment_preflight.py`. +- `close-pr` command: checks GitHub permission at command time, then issues a short-lived private link to a confirmation form. Services: `close_pr_links.py` (token), `close_pr_execution.py` (permission check + GitHub mutation). Feature flag: `ZULIP_CLOSE_PR_MUTATIONS_ENABLED`. Uses operation `close_pr` via the GitHub App token system. - Keep user-facing command responses explicit and safe for partial failures. - Prefer private failure responses for sensitive mutation/policy errors. ## Policy and Safety Notes - Command availability and context restrictions are controlled by `ZULIP_COMMAND_POLICY`. -- Mutation paths are feature-flagged (`ZULIP_ASSIGNMENT_MUTATIONS_ENABLED`) and depend on GitHub operation-token services. +- Mutation paths are feature-flagged (`ZULIP_ASSIGNMENT_MUTATIONS_ENABLED`, `ZULIP_CLOSE_PR_MUTATIONS_ENABLED`) and depend on GitHub operation-token services. - Do not log secrets/tokens or raw sensitive payload fragments. +## Per-Repo Zulip Log +- `ZULIP_REPO_LOG` is a JSON setting mapping `"owner/repo"` to `{"stream": "...", "topic": "..."}`. +- Used by `close-pr` (and intended for future operations) to post an audit log entry after a mutation. +- If a repo has no entry, the log post is skipped and a WARNING is emitted. + ## Registration and Preferences - Registration-link/state behavior is in `services/`: - `registration_links.py`, - `registration_oauth_state.py`, - `registration_linking.py`, - `registration_bootstrap.py` (initial bootstrap helpers), - - `prefs_links.py` (preference deep-link generation). + - `prefs_links.py` (preference deep-link generation), + - `close_pr_links.py` (close-PR confirmation link generation). - Zulip prefs form/UI behavior spans Django forms/views and `frontend/` tests; keep behavior parity across backend validation and frontend affordances. ## Testing Expectations diff --git a/qb_site/zulip_bot/commands/close_pr.py b/qb_site/zulip_bot/commands/close_pr.py new file mode 100644 index 00000000..7ff3394d --- /dev/null +++ b/qb_site/zulip_bot/commands/close_pr.py @@ -0,0 +1,108 @@ +from __future__ import annotations + +from datetime import timedelta + +from django.conf import settings +from django.utils import timezone + +from core.models import User +from zulip_bot.commands import CommandContext, CommandResult, ResponseMode, register_command +from zulip_bot.services.assignment_command_parser import AssignmentCommandParseError, _parse_single_pr_ref +from zulip_bot.services.close_pr_execution import PermissionOutcome, check_close_pr_permission +from zulip_bot.services.close_pr_links import ClosePRLinkClaims, build_close_pr_link + + +@register_command( + name="close-pr", + description="Open a private confirmation form to close a pull request (requires GitHub write access or PR authorship).", + response_mode=ResponseMode.PRIVATE, +) +def close_pr_command(context: CommandContext, args: str) -> CommandResult: + if context.sender_id is None: + return CommandResult( + content="Could not determine your Zulip identity from this message.", + response_mode=ResponseMode.PRIVATE, + ) + + try: + pr = _parse_single_pr_ref(args=args, rendered_content=context.rendered_content) + except AssignmentCommandParseError as exc: + return CommandResult( + content=f"Could not parse `close-pr` command: {exc.message}", + response_mode=ResponseMode.PRIVATE, + ) + + user = User.objects.filter(zulip_user_id=context.sender_id).only("id", "github_login").first() + if user is None: + return CommandResult( + content=("No GitHub account is linked to your Zulip profile. Use the `prefs` command to register."), + response_mode=ResponseMode.PRIVATE, + ) + + github_login = (user.github_login or "").strip() + if not github_login: + return CommandResult( + content="Your Queueboard profile does not have a GitHub login set.", + response_mode=ResponseMode.PRIVATE, + ) + + result = check_close_pr_permission( + github_login=github_login, + owner=pr.owner, + repo=pr.repo, + number=pr.number, + ) + + if result.outcome == PermissionOutcome.TOKEN_UNAVAILABLE: + return CommandResult( + content=(f"GitHub App token for `close_pr` is not available for `{pr.owner}/{pr.repo}`. Contact an administrator."), + response_mode=ResponseMode.PRIVATE, + ) + + if result.outcome == PermissionOutcome.GITHUB_ERROR: + return CommandResult( + content=(f"Could not fetch PR details for `{pr.owner}/{pr.repo}#{pr.number}` from GitHub. Please try again."), + response_mode=ResponseMode.PRIVATE, + ) + + if result.outcome == PermissionOutcome.PR_NOT_OPEN: + pr_ref = f"`{pr.owner}/{pr.repo}#{pr.number}`" + title_suffix = f' ("{result.pr_title}")' if result.pr_title else "" + return CommandResult( + content=f"Pull request {pr_ref}{title_suffix} is not open.", + response_mode=ResponseMode.PRIVATE, + ) + + if result.outcome == PermissionOutcome.NOT_PERMITTED: + return CommandResult( + content=( + f"Your GitHub account (`{github_login}`) does not have permission " + f"to close `{pr.owner}/{pr.repo}#{pr.number}`. " + "Only the PR author or a collaborator with write/admin access may close it." + ), + response_mode=ResponseMode.PRIVATE, + ) + + # PERMITTED — issue the confirmation link. + link = build_close_pr_link( + claims=ClosePRLinkClaims( + zulip_user_id=context.sender_id, + github_login=github_login, + pr_owner=pr.owner, + pr_repo=pr.repo, + pr_number=pr.number, + ) + ) + ttl_seconds = int(getattr(settings, "ZULIP_CLOSE_PR_TOKEN_TTL_SECONDS", 1800)) + expires_at = timezone.now() + timedelta(seconds=ttl_seconds) + expires_unix = int(expires_at.timestamp()) + pr_ref = f"`{pr.owner}/{pr.repo}#{pr.number}`" + title_suffix = f' ("{result.pr_title}")' if result.pr_title else "" + return CommandResult( + content=( + f"Use this private link to [confirm closing PR {pr_ref}{title_suffix}]({link}). " + f"It expires at . " + "The close will be attributed to the bot, not your personal GitHub account." + ), + response_mode=ResponseMode.PRIVATE, + ) diff --git a/qb_site/zulip_bot/services/close_pr_execution.py b/qb_site/zulip_bot/services/close_pr_execution.py new file mode 100644 index 00000000..fdb011c8 --- /dev/null +++ b/qb_site/zulip_bot/services/close_pr_execution.py @@ -0,0 +1,248 @@ +from __future__ import annotations + +import logging +from dataclasses import dataclass +from enum import Enum +from typing import Any + +import requests +from django.conf import settings + +from core.services.github_operation_tokens import resolve_github_app_operation_token + +log = logging.getLogger(__name__) + +_GITHUB_API_BASE = "https://api.github.com" +_CLOSE_PR_OPERATION = "close_pr" + + +class PermissionOutcome(str, Enum): + PERMITTED = "permitted" + NOT_PERMITTED = "not_permitted" + PR_NOT_OPEN = "pr_not_open" + TOKEN_UNAVAILABLE = "token_unavailable" + GITHUB_ERROR = "github_error" + + +@dataclass(frozen=True) +class PermissionCheckResult: + outcome: PermissionOutcome + pr_title: str | None = None + message: str | None = None + + +@dataclass(frozen=True) +class LivePRDetails: + title: str + is_open: bool + author_login: str + + +@dataclass(frozen=True) +class ClosePRError(RuntimeError): + code: str + message: str + + +def check_close_pr_permission( + *, + github_login: str, + owner: str, + repo: str, + number: int, +) -> PermissionCheckResult: + """Check whether github_login may close the given PR. + + Fetches PR details to verify it is open and extract the author login. + If the invoker is the PR author, permission is granted immediately. + Otherwise calls the collaborator permission endpoint to check for + write or admin access. + + Returns a PermissionCheckResult with one of the PermissionOutcome values. + """ + token = _get_token(owner=owner, repo=repo) + if not token: + return PermissionCheckResult( + outcome=PermissionOutcome.TOKEN_UNAVAILABLE, + message="GitHub App token for close_pr is not available for this repository.", + ) + + pr = _fetch_pr_details(token=token, owner=owner, repo=repo, number=number) + if pr is None: + return PermissionCheckResult( + outcome=PermissionOutcome.GITHUB_ERROR, + message=f"Could not fetch PR details for {owner}/{repo}#{number} from GitHub.", + ) + + if not pr.is_open: + return PermissionCheckResult( + outcome=PermissionOutcome.PR_NOT_OPEN, + pr_title=pr.title, + message=f"Pull request {owner}/{repo}#{number} is not open.", + ) + + if github_login.lower() == pr.author_login.lower(): + return PermissionCheckResult( + outcome=PermissionOutcome.PERMITTED, + pr_title=pr.title, + ) + + collab_permission = _fetch_collaborator_permission(token=token, owner=owner, repo=repo, github_login=github_login) + if collab_permission in {"write", "admin"}: + return PermissionCheckResult( + outcome=PermissionOutcome.PERMITTED, + pr_title=pr.title, + ) + + return PermissionCheckResult( + outcome=PermissionOutcome.NOT_PERMITTED, + pr_title=pr.title, + message=( + f"GitHub login `{github_login}` does not have permission to close " + f"{owner}/{repo}#{number} (not the PR author and not a write/admin collaborator)." + ), + ) + + +def fetch_pr_details_for_form(*, owner: str, repo: str, number: int) -> LivePRDetails | None: + """Fetch PR details for display on the confirmation form. + + Uses the close_pr operation token. Returns None if the token is unavailable + or the GitHub request fails. + """ + token = _get_token(owner=owner, repo=repo) + if not token: + return None + return _fetch_pr_details(token=token, owner=owner, repo=repo, number=number) + + +def close_pull_request(*, owner: str, repo: str, number: int) -> None: + """Close a pull request via the GitHub API. + + Raises ClosePRError on failure. + """ + token = _get_token(owner=owner, repo=repo) + if not token: + raise ClosePRError( + code="token_unavailable", + message="GitHub App token for close_pr is not available for this repository.", + ) + + api_base = _api_base_url() + url = f"{api_base}/repos/{owner}/{repo}/pulls/{number}" + headers = { + "Authorization": f"Bearer {token}", + "Accept": "application/vnd.github+json", + "X-GitHub-Api-Version": "2022-11-28", + } + try: + response = requests.patch(url, json={"state": "closed"}, headers=headers, timeout=20) + except requests.RequestException as exc: + raise ClosePRError(code="request_failed", message=f"GitHub request failed: {exc}") from exc + + if response.status_code < 400: + return + + details = _safe_json(response) + if response.status_code in {401, 403}: + raise ClosePRError(code="permission_denied", message="GitHub permission denied when closing pull request.") + if response.status_code == 404: + raise ClosePRError(code="pr_not_found", message="Pull request not found on GitHub.") + if response.status_code == 422: + raise ClosePRError(code="validation_failed", message=f"GitHub rejected the close request: {details}") + if response.status_code >= 500: + raise ClosePRError(code="github_transient", message="GitHub API temporarily failed.") + raise ClosePRError(code="github_error", message=f"GitHub API error when closing pull request: {details}") + + +def _get_token(*, owner: str, repo: str) -> str | None: + return resolve_github_app_operation_token( + operation=_CLOSE_PR_OPERATION, + owner=owner, + repo=repo, + ) + + +def _fetch_pr_details(*, token: str, owner: str, repo: str, number: int) -> LivePRDetails | None: + api_base = _api_base_url() + url = f"{api_base}/repos/{owner}/{repo}/pulls/{number}" + headers = { + "Authorization": f"Bearer {token}", + "Accept": "application/vnd.github+json", + "X-GitHub-Api-Version": "2022-11-28", + } + try: + response = requests.get(url, headers=headers, timeout=20) + except requests.RequestException: + log.warning("close_pr_fetch_pr_details_failed", extra={"owner": owner, "repo": repo, "number": number}) + return None + + if response.status_code >= 400: + log.warning( + "close_pr_fetch_pr_details_http_error", + extra={"owner": owner, "repo": repo, "number": number, "status": response.status_code}, + ) + return None + + payload = _safe_json(response) + title = str(payload.get("title") or "").strip() or f"{owner}/{repo}#{number}" + state = str(payload.get("state") or "").strip().lower() + merged_at = payload.get("merged_at") + is_open = state == "open" and not merged_at + author_login = str((payload.get("user") or {}).get("login") or "").strip() + return LivePRDetails(title=title, is_open=is_open, author_login=author_login) + + +def _fetch_collaborator_permission(*, token: str, owner: str, repo: str, github_login: str) -> str: + """Return the permission level string for github_login on owner/repo. + + Returns "none" on any error or if the user is not a collaborator. + The GitHub API returns "none" for users with no access (not 404). + Possible return values: "admin", "write", "read", "none". + Note: the "maintain" role maps to "write" in this endpoint's permission field. + """ + api_base = _api_base_url() + url = f"{api_base}/repos/{owner}/{repo}/collaborators/{github_login}/permission" + headers = { + "Authorization": f"Bearer {token}", + "Accept": "application/vnd.github+json", + "X-GitHub-Api-Version": "2022-11-28", + } + try: + response = requests.get(url, headers=headers, timeout=20) + except requests.RequestException: + log.warning( + "close_pr_collaborator_permission_request_failed", + extra={"owner": owner, "repo": repo, "github_login": github_login}, + ) + return "none" + + if response.status_code == 404: + # User is not a collaborator on this repo. + return "none" + + if response.status_code >= 400: + log.warning( + "close_pr_collaborator_permission_http_error", + extra={"owner": owner, "repo": repo, "github_login": github_login, "status": response.status_code}, + ) + return "none" + + payload = _safe_json(response) + permission = str(payload.get("permission") or "").strip().lower() + return permission or "none" + + +def _api_base_url() -> str: + configured = getattr(settings, "GITHUB_API_URL", "").strip().rstrip("/") + return configured or _GITHUB_API_BASE + + +def _safe_json(response: requests.Response) -> dict[str, Any]: + try: + payload = response.json() + except ValueError: + return {"raw": response.text} + if isinstance(payload, dict): + return payload + return {"raw": payload} diff --git a/qb_site/zulip_bot/services/close_pr_links.py b/qb_site/zulip_bot/services/close_pr_links.py new file mode 100644 index 00000000..a4d47f4c --- /dev/null +++ b/qb_site/zulip_bot/services/close_pr_links.py @@ -0,0 +1,133 @@ +from __future__ import annotations + +import base64 +import hashlib +import json +import time +from dataclasses import dataclass +from typing import Any +from urllib.parse import quote + +from cryptography.fernet import Fernet, InvalidToken +from django.conf import settings + + +@dataclass(frozen=True) +class ClosePRLinkClaims: + zulip_user_id: int + github_login: str + pr_owner: str + pr_repo: str + pr_number: int + iat: int | None = None + exp: int | None = None + + +class ClosePRTokenError(Exception): + pass + + +class ClosePRTokenExpired(ClosePRTokenError): + pass + + +class ClosePRTokenInvalid(ClosePRTokenError): + pass + + +def build_close_pr_link(*, claims: ClosePRLinkClaims) -> str: + token = issue_close_pr_token(claims=claims) + url_base = getattr(settings, "ZULIP_CLOSE_PR_URL_BASE", "").strip().rstrip("/") + path = f"/api/zulip/close-pr/{quote(token, safe='')}/" + if url_base: + return f"{url_base}{path}" + return path + + +def issue_close_pr_token(*, claims: ClosePRLinkClaims) -> str: + now_ts = int(time.time()) + payload = { + "zulip_user_id": claims.zulip_user_id, + "github_login": claims.github_login, + "pr_owner": claims.pr_owner, + "pr_repo": claims.pr_repo, + "pr_number": claims.pr_number, + "iat": now_ts, + "exp": now_ts + _token_ttl_seconds(), + } + encrypted = _fernet().encrypt(json.dumps(payload, separators=(",", ":"), sort_keys=True).encode("utf-8")) + return encrypted.decode("utf-8") + + +def validate_close_pr_token(token: str) -> ClosePRLinkClaims: + try: + decrypted = _fernet().decrypt(token.encode("utf-8")) + except (InvalidToken, ValueError, UnicodeEncodeError) as exc: + raise ClosePRTokenInvalid("invalid token") from exc + try: + payload = json.loads(decrypted.decode("utf-8")) + except (ValueError, UnicodeDecodeError) as exc: + raise ClosePRTokenInvalid("invalid payload") from exc + return _parse_claims(payload) + + +def _parse_claims(payload: Any) -> ClosePRLinkClaims: + if not isinstance(payload, dict): + raise ClosePRTokenInvalid("invalid payload") + + zulip_user_id = payload.get("zulip_user_id") + github_login = payload.get("github_login") + pr_owner = payload.get("pr_owner") + pr_repo = payload.get("pr_repo") + pr_number = payload.get("pr_number") + exp = payload.get("exp") + + if not isinstance(zulip_user_id, int) or zulip_user_id <= 0: + raise ClosePRTokenInvalid("invalid zulip_user_id") + if not isinstance(github_login, str) or not github_login.strip(): + raise ClosePRTokenInvalid("invalid github_login") + if not isinstance(pr_owner, str) or not pr_owner.strip(): + raise ClosePRTokenInvalid("invalid pr_owner") + if not isinstance(pr_repo, str) or not pr_repo.strip(): + raise ClosePRTokenInvalid("invalid pr_repo") + if not isinstance(pr_number, int) or pr_number <= 0: + raise ClosePRTokenInvalid("invalid pr_number") + if not isinstance(exp, int): + raise ClosePRTokenInvalid("invalid exp") + if int(time.time()) > exp: + raise ClosePRTokenExpired("token expired") + + iat = payload.get("iat") + if iat is not None and not isinstance(iat, int): + raise ClosePRTokenInvalid("invalid iat") + + return ClosePRLinkClaims( + zulip_user_id=zulip_user_id, + github_login=github_login, + pr_owner=pr_owner, + pr_repo=pr_repo, + pr_number=pr_number, + iat=iat, + exp=exp, + ) + + +def _token_secret() -> str: + custom = getattr(settings, "ZULIP_CLOSE_PR_TOKEN_SECRET", "").strip() + if custom: + return custom + return settings.SECRET_KEY + + +def _token_salt() -> str: + return getattr(settings, "ZULIP_CLOSE_PR_TOKEN_SALT", "zulip_bot.close_pr") + + +def _fernet() -> Fernet: + material = f"{_token_secret()}:{_token_salt()}".encode("utf-8") + key = base64.urlsafe_b64encode(hashlib.sha256(material).digest()) + return Fernet(key) + + +def _token_ttl_seconds() -> int: + return int(getattr(settings, "ZULIP_CLOSE_PR_TOKEN_TTL_SECONDS", 1800)) diff --git a/qb_site/zulip_bot/tests/test_close_pr_command.py b/qb_site/zulip_bot/tests/test_close_pr_command.py new file mode 100644 index 00000000..ad2b9b04 --- /dev/null +++ b/qb_site/zulip_bot/tests/test_close_pr_command.py @@ -0,0 +1,93 @@ +from __future__ import annotations + +from unittest.mock import patch + +from django.test import TestCase + +from core.models import User +from zulip_bot.commands import CommandContext +from zulip_bot.commands.close_pr import close_pr_command +from zulip_bot.services.close_pr_execution import PermissionCheckResult, PermissionOutcome + + +def _context(sender_id: int | None = 101) -> CommandContext: + return CommandContext( + sender_id=sender_id, + sender_email="reviewer@example.com", + sender_full_name="Reviewer User", + message_content="close-pr", + message_id=555, + stream_id=None, + topic=None, + is_private=True, + rendered_content=None, + allowed_command_names=frozenset({"close-pr"}), + ) + + +_PR_URL = "https://github.com/leanprover-community/mathlib4/pull/999" + + +class TestClosePRCommand(TestCase): + def setUp(self) -> None: + super().setUp() + self.user = User.objects.create(zulip_user_id=101, github_login="reviewer") + + def _patch_permission(self, result: PermissionCheckResult): + return patch( + "zulip_bot.commands.close_pr.check_close_pr_permission", + return_value=result, + ) + + def test_missing_sender_id(self) -> None: + result = close_pr_command(_context(sender_id=None), _PR_URL) + self.assertIn("Could not determine your Zulip identity", result.content) + + def test_no_pr_url(self) -> None: + result = close_pr_command(_context(), "") + self.assertIn("Could not parse `close-pr` command", result.content) + + def test_no_linked_user(self) -> None: + self.user.delete() + result = close_pr_command(_context(), _PR_URL) + self.assertIn("No GitHub account is linked", result.content) + + def test_no_github_login(self) -> None: + self.user.github_login = "" + self.user.save() + result = close_pr_command(_context(), _PR_URL) + self.assertIn("does not have a GitHub login set", result.content) + + def test_token_unavailable(self) -> None: + with self._patch_permission(PermissionCheckResult(outcome=PermissionOutcome.TOKEN_UNAVAILABLE)): + result = close_pr_command(_context(), _PR_URL) + self.assertIn("token for `close_pr` is not available", result.content) + + def test_github_error(self) -> None: + with self._patch_permission(PermissionCheckResult(outcome=PermissionOutcome.GITHUB_ERROR)): + result = close_pr_command(_context(), _PR_URL) + self.assertIn("Could not fetch PR details", result.content) + + def test_pr_not_open(self) -> None: + with self._patch_permission(PermissionCheckResult(outcome=PermissionOutcome.PR_NOT_OPEN, pr_title="Fix something")): + result = close_pr_command(_context(), _PR_URL) + self.assertIn("is not open", result.content) + self.assertIn("Fix something", result.content) + + def test_not_permitted(self) -> None: + with self._patch_permission(PermissionCheckResult(outcome=PermissionOutcome.NOT_PERMITTED)): + result = close_pr_command(_context(), _PR_URL) + self.assertIn("does not have permission to close", result.content) + + def test_permitted_issues_link(self) -> None: + with self._patch_permission(PermissionCheckResult(outcome=PermissionOutcome.PERMITTED, pr_title="Great PR")): + result = close_pr_command(_context(), _PR_URL) + self.assertIn("confirm closing PR", result.content) + self.assertIn("Great PR", result.content) + self.assertIn("attributed to the bot", result.content) + self.assertIn("/api/zulip/close-pr/", result.content) + + def test_permitted_link_includes_expiry(self) -> None: + with self._patch_permission(PermissionCheckResult(outcome=PermissionOutcome.PERMITTED)): + result = close_pr_command(_context(), _PR_URL) + self.assertIn(" MagicMock: + mock = MagicMock() + mock.status_code = status_code + if json_data is not None: + mock.json.return_value = json_data + else: + mock.json.side_effect = ValueError("no body") + mock.text = "" + return mock + + +class TestCheckClosePRPermission(SimpleTestCase): + def _patch_token(self, token: str | None = "test-token"): + return patch( + "zulip_bot.services.close_pr_execution.resolve_github_app_operation_token", + return_value=token, + ) + + def _patch_get(self, responses: list): + """Patch requests.get to return successive responses.""" + return patch("zulip_bot.services.close_pr_execution.requests.get", side_effect=responses) + + def test_token_unavailable(self) -> None: + with self._patch_token(None): + result = check_close_pr_permission( + github_login="reviewer", + owner="leanprover-community", + repo="mathlib4", + number=1, + ) + self.assertEqual(result.outcome, PermissionOutcome.TOKEN_UNAVAILABLE) + + def test_github_error_on_pr_fetch(self) -> None: + pr_response = _make_response(status_code=500) + with self._patch_token(), self._patch_get([pr_response]): + result = check_close_pr_permission( + github_login="reviewer", + owner="leanprover-community", + repo="mathlib4", + number=1, + ) + self.assertEqual(result.outcome, PermissionOutcome.GITHUB_ERROR) + + def test_pr_not_open(self) -> None: + pr_response = _make_response( + status_code=200, + json_data={"title": "Fix bug", "state": "closed", "merged_at": None, "user": {"login": "author"}}, + ) + with self._patch_token(), self._patch_get([pr_response]): + result = check_close_pr_permission( + github_login="reviewer", + owner="leanprover-community", + repo="mathlib4", + number=1, + ) + self.assertEqual(result.outcome, PermissionOutcome.PR_NOT_OPEN) + self.assertEqual(result.pr_title, "Fix bug") + + def test_permitted_as_pr_author(self) -> None: + pr_response = _make_response( + status_code=200, + json_data={"title": "My PR", "state": "open", "merged_at": None, "user": {"login": "reviewer"}}, + ) + with self._patch_token(), self._patch_get([pr_response]): + result = check_close_pr_permission( + github_login="reviewer", + owner="leanprover-community", + repo="mathlib4", + number=1, + ) + self.assertEqual(result.outcome, PermissionOutcome.PERMITTED) + self.assertEqual(result.pr_title, "My PR") + + def test_permitted_as_pr_author_case_insensitive(self) -> None: + pr_response = _make_response( + status_code=200, + json_data={"title": "My PR", "state": "open", "merged_at": None, "user": {"login": "Reviewer"}}, + ) + with self._patch_token(), self._patch_get([pr_response]): + result = check_close_pr_permission( + github_login="reviewer", + owner="leanprover-community", + repo="mathlib4", + number=1, + ) + self.assertEqual(result.outcome, PermissionOutcome.PERMITTED) + + def test_permitted_as_write_collaborator(self) -> None: + pr_response = _make_response( + status_code=200, + json_data={"title": "Someone's PR", "state": "open", "merged_at": None, "user": {"login": "author"}}, + ) + collab_response = _make_response(status_code=200, json_data={"permission": "write"}) + with self._patch_token(), self._patch_get([pr_response, collab_response]): + result = check_close_pr_permission( + github_login="reviewer", + owner="leanprover-community", + repo="mathlib4", + number=1, + ) + self.assertEqual(result.outcome, PermissionOutcome.PERMITTED) + + def test_permitted_as_admin_collaborator(self) -> None: + pr_response = _make_response( + status_code=200, + json_data={"title": "Someone's PR", "state": "open", "merged_at": None, "user": {"login": "author"}}, + ) + collab_response = _make_response(status_code=200, json_data={"permission": "admin"}) + with self._patch_token(), self._patch_get([pr_response, collab_response]): + result = check_close_pr_permission( + github_login="reviewer", + owner="leanprover-community", + repo="mathlib4", + number=1, + ) + self.assertEqual(result.outcome, PermissionOutcome.PERMITTED) + + def test_not_permitted_read_only(self) -> None: + pr_response = _make_response( + status_code=200, + json_data={"title": "Someone's PR", "state": "open", "merged_at": None, "user": {"login": "author"}}, + ) + collab_response = _make_response(status_code=200, json_data={"permission": "read"}) + with self._patch_token(), self._patch_get([pr_response, collab_response]): + result = check_close_pr_permission( + github_login="reviewer", + owner="leanprover-community", + repo="mathlib4", + number=1, + ) + self.assertEqual(result.outcome, PermissionOutcome.NOT_PERMITTED) + + def test_not_permitted_no_access(self) -> None: + pr_response = _make_response( + status_code=200, + json_data={"title": "Someone's PR", "state": "open", "merged_at": None, "user": {"login": "author"}}, + ) + collab_response = _make_response(status_code=200, json_data={"permission": "none"}) + with self._patch_token(), self._patch_get([pr_response, collab_response]): + result = check_close_pr_permission( + github_login="reviewer", + owner="leanprover-community", + repo="mathlib4", + number=1, + ) + self.assertEqual(result.outcome, PermissionOutcome.NOT_PERMITTED) + + def test_not_permitted_when_collaborator_endpoint_returns_404(self) -> None: + pr_response = _make_response( + status_code=200, + json_data={"title": "Someone's PR", "state": "open", "merged_at": None, "user": {"login": "author"}}, + ) + collab_response = _make_response(status_code=404) + with self._patch_token(), self._patch_get([pr_response, collab_response]): + result = check_close_pr_permission( + github_login="reviewer", + owner="leanprover-community", + repo="mathlib4", + number=1, + ) + self.assertEqual(result.outcome, PermissionOutcome.NOT_PERMITTED) + + def test_merged_pr_treated_as_not_open(self) -> None: + pr_response = _make_response( + status_code=200, + json_data={ + "title": "Merged PR", + "state": "closed", + "merged_at": "2024-01-01T00:00:00Z", + "user": {"login": "author"}, + }, + ) + with self._patch_token(), self._patch_get([pr_response]): + result = check_close_pr_permission( + github_login="reviewer", + owner="leanprover-community", + repo="mathlib4", + number=1, + ) + self.assertEqual(result.outcome, PermissionOutcome.PR_NOT_OPEN) + + +class TestClosePullRequest(SimpleTestCase): + def _patch_token(self, token: str | None = "test-token"): + return patch( + "zulip_bot.services.close_pr_execution.resolve_github_app_operation_token", + return_value=token, + ) + + def _patch_patch(self, response): + return patch("zulip_bot.services.close_pr_execution.requests.patch", return_value=response) + + def test_success(self) -> None: + response = _make_response(status_code=200, json_data={"state": "closed"}) + with self._patch_token(), self._patch_patch(response): + close_pull_request(owner="leanprover-community", repo="mathlib4", number=1) + # No exception raised. + + def test_token_unavailable(self) -> None: + with self._patch_token(None): + with self.assertRaises(ClosePRError) as cm: + close_pull_request(owner="leanprover-community", repo="mathlib4", number=1) + self.assertEqual(cm.exception.code, "token_unavailable") + + def test_permission_denied(self) -> None: + response = _make_response(status_code=403) + with self._patch_token(), self._patch_patch(response): + with self.assertRaises(ClosePRError) as cm: + close_pull_request(owner="leanprover-community", repo="mathlib4", number=1) + self.assertEqual(cm.exception.code, "permission_denied") + + def test_pr_not_found(self) -> None: + response = _make_response(status_code=404) + with self._patch_token(), self._patch_patch(response): + with self.assertRaises(ClosePRError) as cm: + close_pull_request(owner="leanprover-community", repo="mathlib4", number=1) + self.assertEqual(cm.exception.code, "pr_not_found") + + def test_github_transient_error(self) -> None: + response = _make_response(status_code=503) + with self._patch_token(), self._patch_patch(response): + with self.assertRaises(ClosePRError) as cm: + close_pull_request(owner="leanprover-community", repo="mathlib4", number=1) + self.assertEqual(cm.exception.code, "github_transient") diff --git a/qb_site/zulip_bot/tests/test_close_pr_form.py b/qb_site/zulip_bot/tests/test_close_pr_form.py new file mode 100644 index 00000000..181969a9 --- /dev/null +++ b/qb_site/zulip_bot/tests/test_close_pr_form.py @@ -0,0 +1,189 @@ +from __future__ import annotations + +from unittest.mock import patch + +from django.test import TestCase, override_settings +from django.urls import reverse + +from zulip_bot.services.close_pr_execution import ClosePRError, LivePRDetails +from zulip_bot.services.close_pr_links import ClosePRLinkClaims, issue_close_pr_token + + +def _token( + *, + zulip_user_id: int = 101, + github_login: str = "reviewer", + pr_owner: str = "leanprover-community", + pr_repo: str = "mathlib4", + pr_number: int = 999, +) -> str: + return issue_close_pr_token( + claims=ClosePRLinkClaims( + zulip_user_id=zulip_user_id, + github_login=github_login, + pr_owner=pr_owner, + pr_repo=pr_repo, + pr_number=pr_number, + ) + ) + + +def _url(token: str) -> str: + return reverse("zulip-close-pr-form", kwargs={"token": token}) + + +def _open_pr(title: str = "My PR") -> LivePRDetails: + return LivePRDetails(title=title, is_open=True, author_login="author") + + +def _closed_pr(title: str = "My PR") -> LivePRDetails: + return LivePRDetails(title=title, is_open=False, author_login="author") + + +def _patch_pr_details(pr: LivePRDetails | None): + return patch("zulip_bot.views.fetch_pr_details_for_form", return_value=pr) + + +def _patch_close(*, raises: ClosePRError | None = None): + if raises: + return patch("zulip_bot.views.close_pull_request", side_effect=raises) + return patch("zulip_bot.views.close_pull_request") + + +def _patch_post_actions(): + return patch("zulip_bot.views._enqueue_close_pr_post_actions") + + +class TestClosePRFormGet(TestCase): + def test_expired_token_returns_403(self) -> None: + with patch("zulip_bot.services.close_pr_links.time.time", return_value=1_700_000_000): + tok = _token() + with patch("zulip_bot.services.close_pr_links.time.time", return_value=1_700_000_000 + 1_900): + with _patch_pr_details(_open_pr()): + response = self.client.get(_url(tok)) + self.assertEqual(response.status_code, 403) + self.assertTemplateUsed(response, "zulip_bot/close_pr_invalid.html") + self.assertIn("expired", response.context["reason"]) + + def test_invalid_token_returns_403(self) -> None: + response = self.client.get(_url("not-a-valid-token")) + self.assertEqual(response.status_code, 403) + self.assertTemplateUsed(response, "zulip_bot/close_pr_invalid.html") + self.assertIn("invalid", response.context["reason"]) + + def test_valid_open_pr_shows_confirmation_form(self) -> None: + with _patch_pr_details(_open_pr("Fix the thing")): + response = self.client.get(_url(_token())) + self.assertEqual(response.status_code, 200) + self.assertTemplateUsed(response, "zulip_bot/close_pr_form.html") + self.assertContains(response, "Fix the thing") + self.assertContains(response, "Close this pull request") + self.assertFalse(response.context["success"]) + self.assertTrue(response.context["pr_is_open"]) + + def test_already_closed_pr_shows_informational_message_no_button(self) -> None: + with _patch_pr_details(_closed_pr()): + response = self.client.get(_url(_token())) + self.assertEqual(response.status_code, 200) + self.assertFalse(response.context["pr_is_open"]) + self.assertNotContains(response, "Close this pull request") + + def test_github_fetch_failure_assumes_open(self) -> None: + # If we can't fetch PR details, we still show the form (POST will validate). + with _patch_pr_details(None): + response = self.client.get(_url(_token())) + self.assertEqual(response.status_code, 200) + self.assertTrue(response.context["pr_is_open"]) + + @override_settings(ZULIP_CLOSE_PR_MUTATIONS_ENABLED="true") + def test_mutations_enabled_flag_in_context(self) -> None: + with _patch_pr_details(_open_pr()): + response = self.client.get(_url(_token())) + self.assertFalse(response.context["mutations_disabled"]) + + def test_mutations_disabled_flag_in_context(self) -> None: + with _patch_pr_details(_open_pr()): + response = self.client.get(_url(_token())) + self.assertTrue(response.context["mutations_disabled"]) + + +class TestClosePRFormPost(TestCase): + def test_mutations_disabled_shows_preflight_message(self) -> None: + with _patch_pr_details(_open_pr()), _patch_close() as mock_close: + response = self.client.post(_url(_token())) + mock_close.assert_not_called() + self.assertEqual(response.status_code, 200) + self.assertTrue(response.context["mutations_disabled"]) + + @override_settings(ZULIP_CLOSE_PR_MUTATIONS_ENABLED="true") + def test_successful_close(self) -> None: + with _patch_pr_details(_open_pr()), _patch_close(), _patch_post_actions() as mock_post: + response = self.client.post(_url(_token())) + self.assertEqual(response.status_code, 200) + self.assertTrue(response.context["success"]) + mock_post.assert_called_once() + + @override_settings(ZULIP_CLOSE_PR_MUTATIONS_ENABLED="true") + def test_github_close_error_shown_in_template(self) -> None: + error = ClosePRError(code="permission_denied", message="GitHub permission denied when closing pull request.") + with _patch_pr_details(_open_pr()), _patch_close(raises=error), _patch_post_actions(): + response = self.client.post(_url(_token())) + self.assertEqual(response.status_code, 200) + self.assertFalse(response.context["success"]) + self.assertIn("permission_denied", response.context["close_error"] or "") + + @override_settings(ZULIP_CLOSE_PR_MUTATIONS_ENABLED="true") + def test_post_actions_called_with_claims_and_title(self) -> None: + tok = _token(pr_number=42) + with _patch_pr_details(_open_pr("The Title")), _patch_close(), _patch_post_actions() as mock_post: + self.client.post(_url(tok)) + mock_post.assert_called_once() + kwargs = mock_post.call_args.kwargs + self.assertEqual(kwargs["claims"].pr_number, 42) + self.assertEqual(kwargs["pr_title"], "The Title") + + @override_settings(ZULIP_CLOSE_PR_MUTATIONS_ENABLED="true") + def test_expired_token_on_post_returns_403(self) -> None: + with patch("zulip_bot.services.close_pr_links.time.time", return_value=1_700_000_000): + tok = _token() + with patch("zulip_bot.services.close_pr_links.time.time", return_value=1_700_000_000 + 1_900): + response = self.client.post(_url(tok)) + self.assertEqual(response.status_code, 403) + + +class TestRepoLogResolution(TestCase): + def test_missing_setting_skips_log(self) -> None: + from zulip_bot.views import _resolve_repo_log_target + + result = _resolve_repo_log_target(owner="leanprover-community", repo="mathlib4") + self.assertIsNone(result) + + @override_settings(ZULIP_REPO_LOG={"leanprover-community/mathlib4": {"stream": "mathlib4", "topic": "bot log"}}) + def test_configured_repo_returns_target(self) -> None: + from zulip_bot.views import _resolve_repo_log_target + + result = _resolve_repo_log_target(owner="leanprover-community", repo="mathlib4") + self.assertIsNotNone(result) + self.assertEqual(result["stream"], "mathlib4") + self.assertEqual(result["topic"], "bot log") + + @override_settings(ZULIP_REPO_LOG={"other/repo": {"stream": "s", "topic": "t"}}) + def test_unconfigured_repo_returns_none(self) -> None: + from zulip_bot.views import _resolve_repo_log_target + + result = _resolve_repo_log_target(owner="leanprover-community", repo="mathlib4") + self.assertIsNone(result) + + @override_settings(ZULIP_REPO_LOG={"leanprover-community/mathlib4": {"stream": "", "topic": "bot log"}}) + def test_missing_stream_returns_none(self) -> None: + from zulip_bot.views import _resolve_repo_log_target + + result = _resolve_repo_log_target(owner="leanprover-community", repo="mathlib4") + self.assertIsNone(result) + + @override_settings(ZULIP_REPO_LOG="not-a-dict") + def test_malformed_setting_returns_none(self) -> None: + from zulip_bot.views import _resolve_repo_log_target + + result = _resolve_repo_log_target(owner="leanprover-community", repo="mathlib4") + self.assertIsNone(result) diff --git a/qb_site/zulip_bot/tests/test_close_pr_links.py b/qb_site/zulip_bot/tests/test_close_pr_links.py new file mode 100644 index 00000000..9ac8ffdb --- /dev/null +++ b/qb_site/zulip_bot/tests/test_close_pr_links.py @@ -0,0 +1,66 @@ +from __future__ import annotations + +from unittest.mock import patch + +from django.test import SimpleTestCase, override_settings + +from zulip_bot.services.close_pr_links import ( + ClosePRLinkClaims, + ClosePRTokenExpired, + ClosePRTokenInvalid, + build_close_pr_link, + issue_close_pr_token, + validate_close_pr_token, +) + + +class TestClosePRLinks(SimpleTestCase): + def _claims(self, **kwargs) -> ClosePRLinkClaims: + defaults = dict( + zulip_user_id=101, + github_login="reviewer", + pr_owner="leanprover-community", + pr_repo="mathlib4", + pr_number=999, + ) + defaults.update(kwargs) + return ClosePRLinkClaims(**defaults) + + def test_round_trip(self) -> None: + token = issue_close_pr_token(claims=self._claims()) + claims = validate_close_pr_token(token) + self.assertEqual(claims.zulip_user_id, 101) + self.assertEqual(claims.github_login, "reviewer") + self.assertEqual(claims.pr_owner, "leanprover-community") + self.assertEqual(claims.pr_repo, "mathlib4") + self.assertEqual(claims.pr_number, 999) + self.assertIsNotNone(claims.iat) + self.assertIsNotNone(claims.exp) + + @override_settings(ZULIP_CLOSE_PR_URL_BASE="https://queueboard.example") + def test_build_link_uses_url_base(self) -> None: + link = build_close_pr_link(claims=self._claims()) + self.assertTrue(link.startswith("https://queueboard.example/api/zulip/close-pr/")) + + def test_build_link_falls_back_to_relative_path(self) -> None: + link = build_close_pr_link(claims=self._claims()) + self.assertTrue(link.startswith("/api/zulip/close-pr/")) + + def test_rejects_invalid_token(self) -> None: + with self.assertRaises(ClosePRTokenInvalid): + validate_close_pr_token("not-a-valid-token") + + def test_rejects_expired_token(self) -> None: + with patch("zulip_bot.services.close_pr_links.time.time", return_value=1_700_000_000): + token = issue_close_pr_token(claims=self._claims()) + with patch("zulip_bot.services.close_pr_links.time.time", return_value=1_700_000_000 + 1_900): + with self.assertRaises(ClosePRTokenExpired): + validate_close_pr_token(token) + + def test_rejects_tampered_token(self) -> None: + token = issue_close_pr_token(claims=self._claims()) + # Flip a character in the middle. + mid = len(token) // 2 + flipped = token[:mid] + ("A" if token[mid] != "A" else "B") + token[mid + 1 :] + with self.assertRaises(ClosePRTokenInvalid): + validate_close_pr_token(flipped) diff --git a/qb_site/zulip_bot/urls.py b/qb_site/zulip_bot/urls.py index 84090269..3388ed43 100644 --- a/qb_site/zulip_bot/urls.py +++ b/qb_site/zulip_bot/urls.py @@ -10,4 +10,5 @@ path("register//github/", views.register_github_start, name="zulip-register-github-start"), path("register/github/callback/", views.register_github_callback, name="zulip-register-github-callback"), path("prefs//", views.prefs_form, name="zulip-prefs-form"), + path("close-pr//", views.close_pr_form, name="zulip-close-pr-form"), ] diff --git a/qb_site/zulip_bot/views.py b/qb_site/zulip_bot/views.py index 1786935b..1a2bb726 100644 --- a/qb_site/zulip_bot/views.py +++ b/qb_site/zulip_bot/views.py @@ -21,6 +21,7 @@ from zulip_bot.commands import CommandResult, ResponseMode, get_command from zulip_bot.commands import assign as _assign # noqa: F401 from zulip_bot.commands import assigned_prs as _assigned_prs # noqa: F401 +from zulip_bot.commands import close_pr as _close_pr # noqa: F401 from zulip_bot.commands import echo as _echo # noqa: F401 from zulip_bot.commands import help as _help # noqa: F401 from zulip_bot.commands import prefs as _prefs # noqa: F401 @@ -30,6 +31,12 @@ from zulip_bot.forms import ReviewerPreferenceForm from zulip_bot.services.registration_bootstrap import ensure_default_preferences_for_user from core.services.github_oauth import GitHubOAuthClient, GitHubOAuthError +from zulip_bot.services.close_pr_execution import ClosePRError, fetch_pr_details_for_form, close_pull_request +from zulip_bot.services.close_pr_links import ( + ClosePRTokenExpired, + ClosePRTokenInvalid, + validate_close_pr_token, +) from zulip_bot.services.prefs_links import ( PrefsLinkClaims, PrefsTokenExpired, @@ -350,6 +357,147 @@ def register_github_callback(request: HttpRequest) -> HttpResponse: return response +def close_pr_form(request: HttpRequest, token: str) -> HttpResponse: + try: + claims = validate_close_pr_token(token) + except ClosePRTokenExpired: + return _close_pr_invalid_response(request, reason="expired") + except ClosePRTokenInvalid: + return _close_pr_invalid_response(request, reason="invalid") + + from datetime import datetime, timezone as dt_timezone + + expires_at_utc = datetime.fromtimestamp(claims.exp, tz=dt_timezone.utc) + + # Fetch live PR details for display. + pr_details = fetch_pr_details_for_form( + owner=claims.pr_owner, + repo=claims.pr_repo, + number=claims.pr_number, + ) + pr_title = pr_details.title if pr_details else None + pr_is_open = pr_details.is_open if pr_details else True # assume open if fetch fails; POST will verify + + mutations_enabled = _close_pr_mutations_enabled() + context: dict = { + "pr_owner": claims.pr_owner, + "pr_repo": claims.pr_repo, + "pr_number": claims.pr_number, + "pr_title": pr_title, + "pr_is_open": pr_is_open, + "expires_at_iso": expires_at_utc.isoformat(), + "mutations_disabled": not mutations_enabled, + "success": False, + "close_error": None, + } + + if request.method == "POST": + if not mutations_enabled: + response = TemplateResponse(request, "zulip_bot/close_pr_form.html", context, status=200) + response["Cache-Control"] = "no-store" + return response + + try: + close_pull_request(owner=claims.pr_owner, repo=claims.pr_repo, number=claims.pr_number) + except ClosePRError as exc: + context["close_error"] = exc.message + response = TemplateResponse(request, "zulip_bot/close_pr_form.html", context, status=200) + response["Cache-Control"] = "no-store" + return response + + context["success"] = True + _enqueue_close_pr_post_actions(claims=claims, pr_title=pr_title) + + response = TemplateResponse(request, "zulip_bot/close_pr_form.html", context, status=200) + response["Cache-Control"] = "no-store" + return response + + +def _close_pr_mutations_enabled() -> bool: + value = str(getattr(settings, "ZULIP_CLOSE_PR_MUTATIONS_ENABLED", "")).strip().lower() + return value in {"1", "true", "yes", "on"} + + +def _enqueue_close_pr_post_actions(*, claims, pr_title: str | None) -> None: + from core.models import Repository + + pr_ref = f"{claims.pr_owner}/{claims.pr_repo}#{claims.pr_number}" + title_part = f' ("{pr_title}")' if pr_title else "" + + # 1. Best-effort sync. + repository = Repository.objects.filter(owner=claims.pr_owner, name=claims.pr_repo).only("id").first() + if repository is not None: + try: + from syncer.tasks.sync_tasks import sync_pr_task + + sync_pr_task.delay(repository.id, claims.pr_number) + except Exception: + logger.warning( + "close_pr_post_action_sync_enqueue_failed", + extra={"owner": claims.pr_owner, "repo": claims.pr_repo, "number": claims.pr_number}, + ) + + # 2. Best-effort DM to invoker. + dm_content = f"Pull request `{pr_ref}`{title_part} has been closed. The close was attributed to the bot." + try: + ZulipClient().send_direct_message(to=[claims.zulip_user_id], content=dm_content) + except ZulipApiError: + logger.warning( + "close_pr_dm_failed", + extra={"zulip_user_id": claims.zulip_user_id, "pr_ref": pr_ref}, + ) + + # 3. Best-effort log post. + log_target = _resolve_repo_log_target(owner=claims.pr_owner, repo=claims.pr_repo) + if log_target is not None: + log_content = f"Closed PR `{pr_ref}`{title_part} (via bot, requested by Zulip user {claims.zulip_user_id})." + try: + ZulipClient().send_stream_message( + stream=log_target["stream"], + topic=log_target["topic"], + content=log_content, + ) + except ZulipApiError: + logger.warning( + "close_pr_log_post_failed", + extra={"owner": claims.pr_owner, "repo": claims.pr_repo, "pr_ref": pr_ref}, + ) + + +def _resolve_repo_log_target(*, owner: str, repo: str) -> dict | None: + raw = getattr(settings, "ZULIP_REPO_LOG", None) + if not isinstance(raw, dict): + return None + key = f"{owner}/{repo}" + entry = raw.get(key) + if not isinstance(entry, dict): + logger.warning( + "close_pr_no_repo_log_configured", + extra={"owner": owner, "repo": repo}, + ) + return None + stream = str(entry.get("stream") or "").strip() + topic = str(entry.get("topic") or "").strip() + if not stream or not topic: + logger.warning( + "close_pr_repo_log_missing_stream_or_topic", + extra={"owner": owner, "repo": repo}, + ) + return None + return {"stream": stream, "topic": topic} + + +def _close_pr_invalid_response(request: HttpRequest, *, reason: str) -> HttpResponse: + response = TemplateResponse( + request, + "zulip_bot/close_pr_invalid.html", + {"reason": reason}, + status=403, + ) + response["Cache-Control"] = "no-store" + return response + + def _prefs_invalid_response(request: HttpRequest, *, reason: str) -> HttpResponse: response = TemplateResponse( request, From 96a4ebf3943088191af70786826fa20827c732b1 Mon Sep 17 00:00:00 2001 From: Bryan Gin-ge Chen Date: Sun, 12 Apr 2026 15:12:07 -0400 Subject: [PATCH 03/24] fix(zulip_bot): fix two test_close_pr_form assertions - test_valid_open_pr_shows_confirmation_form: add ZULIP_CLOSE_PR_MUTATIONS_ENABLED override so the form renders instead of the preflight-disabled branch - test_github_close_error_shown_in_template: close_error holds exc.message (human-readable), not exc.code; check for "permission denied" substring instead Co-Authored-By: Claude Sonnet 4.6 --- qb_site/zulip_bot/tests/test_close_pr_form.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/qb_site/zulip_bot/tests/test_close_pr_form.py b/qb_site/zulip_bot/tests/test_close_pr_form.py index 181969a9..149f840f 100644 --- a/qb_site/zulip_bot/tests/test_close_pr_form.py +++ b/qb_site/zulip_bot/tests/test_close_pr_form.py @@ -71,6 +71,7 @@ def test_invalid_token_returns_403(self) -> None: self.assertTemplateUsed(response, "zulip_bot/close_pr_invalid.html") self.assertIn("invalid", response.context["reason"]) + @override_settings(ZULIP_CLOSE_PR_MUTATIONS_ENABLED="true") def test_valid_open_pr_shows_confirmation_form(self) -> None: with _patch_pr_details(_open_pr("Fix the thing")): response = self.client.get(_url(_token())) @@ -130,7 +131,7 @@ def test_github_close_error_shown_in_template(self) -> None: response = self.client.post(_url(_token())) self.assertEqual(response.status_code, 200) self.assertFalse(response.context["success"]) - self.assertIn("permission_denied", response.context["close_error"] or "") + self.assertIn("permission denied", response.context["close_error"] or "") @override_settings(ZULIP_CLOSE_PR_MUTATIONS_ENABLED="true") def test_post_actions_called_with_claims_and_title(self) -> None: From 8f90ccfc0d1c1cca8a8549afa34a0043f07bbd52 Mon Sep 17 00:00:00 2001 From: Bryan Gin-ge Chen Date: Sun, 12 Apr 2026 15:15:53 -0400 Subject: [PATCH 04/24] fix(zulip_bot): normalise underscores to hyphens in command name parser Users may type `close_pr`, `assigned_prs`, or `pr_info` expecting them to work. The parser now replaces `_` with `-` in the command token before any lookup, so underscore variants resolve to the same command as their hyphen canonical names without needing per-command aliases. Co-Authored-By: Claude Sonnet 4.6 --- qb_site/zulip_bot/tests/test_payload_parser.py | 13 +++++++++++++ qb_site/zulip_bot/webhook/payload.py | 2 +- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/qb_site/zulip_bot/tests/test_payload_parser.py b/qb_site/zulip_bot/tests/test_payload_parser.py index ef854d8e..82640237 100644 --- a/qb_site/zulip_bot/tests/test_payload_parser.py +++ b/qb_site/zulip_bot/tests/test_payload_parser.py @@ -25,6 +25,19 @@ def test_parse_command_with_slash_prefix(self) -> None: self.assertEqual(parsed.name, "help") self.assertEqual(parsed.args, "") + def test_parse_command_normalizes_underscores_to_hyphens(self) -> None: + parsed = parse_command("close_pr https://github.com/org/repo/pull/1") + self.assertIsNotNone(parsed) + assert parsed is not None + self.assertEqual(parsed.name, "close-pr") + + def test_parse_command_normalizes_underscores_preserves_args(self) -> None: + parsed = parse_command("assigned_prs") + self.assertIsNotNone(parsed) + assert parsed is not None + self.assertEqual(parsed.name, "assigned-prs") + self.assertEqual(parsed.args, "") + def test_parse_command_returns_none_when_only_mention(self) -> None: self.assertIsNone(parse_command("@**queueboard-bot**")) diff --git a/qb_site/zulip_bot/webhook/payload.py b/qb_site/zulip_bot/webhook/payload.py index f506c9c9..61f519de 100644 --- a/qb_site/zulip_bot/webhook/payload.py +++ b/qb_site/zulip_bot/webhook/payload.py @@ -102,7 +102,7 @@ def parse_command(message_content: str) -> ParsedCommand | None: content = content[1:] parts = content.split(maxsplit=1) - name = parts[0].lower() + name = parts[0].lower().replace("_", "-") args = parts[1] if len(parts) > 1 else "" return ParsedCommand(name=name, args=args) From d72db9ce8e25476c18c8c3774b83c13fbc55a10c Mon Sep 17 00:00:00 2001 From: Bryan Gin-ge Chen Date: Sun, 12 Apr 2026 15:16:35 -0400 Subject: [PATCH 05/24] chore(zulip_bot): remove redundant underscore aliases from pr-info and assigned-prs The parser now normalises underscores to hyphens before command lookup, making these aliases unnecessary. Co-Authored-By: Claude Sonnet 4.6 --- qb_site/zulip_bot/commands/assigned_prs.py | 1 - qb_site/zulip_bot/commands/pr_info.py | 1 - 2 files changed, 2 deletions(-) diff --git a/qb_site/zulip_bot/commands/assigned_prs.py b/qb_site/zulip_bot/commands/assigned_prs.py index e1db8a1a..e965867e 100644 --- a/qb_site/zulip_bot/commands/assigned_prs.py +++ b/qb_site/zulip_bot/commands/assigned_prs.py @@ -28,7 +28,6 @@ name="assigned-prs", description="Show your assigned open PRs with queue-time status.", response_mode=ResponseMode.PRIVATE, - aliases=("assigned_prs",), ) def assigned_prs_command(context: CommandContext, args: str) -> CommandResult: del args diff --git a/qb_site/zulip_bot/commands/pr_info.py b/qb_site/zulip_bot/commands/pr_info.py index 558dcb90..092415d7 100644 --- a/qb_site/zulip_bot/commands/pr_info.py +++ b/qb_site/zulip_bot/commands/pr_info.py @@ -24,7 +24,6 @@ name="pr-info", description="Show queue info for one or more GitHub PR links (up to 10). Zulip linkifiers also work.", response_mode=ResponseMode.STREAM, - aliases=("pr_info",), ) def pr_info_command(context: CommandContext, args: str) -> CommandResult: refs = _parse_pr_refs(context, args) From c1add8b563c24c6714b23c6ad255c0aabbc125be Mon Sep 17 00:00:00 2001 From: Bryan Gin-ge Chen Date: Sun, 12 Apr 2026 17:40:49 -0400 Subject: [PATCH 06/24] doc(041): update progress notes and add operational deployment notes Mark implementation commits as complete, note the two follow-up fix commits, and add a new Operational Deployment Notes section covering the GitHub App permission upgrade steps, required env vars (GITHUB_APP_TOKEN_CONFIG, ZULIP_REPO_LOG, ZULIP_CLOSE_PR_MUTATIONS_ENABLED), and a manual verification checklist for post-deployment testing. Co-Authored-By: Claude Sonnet 4.6 --- .../041-zulip-close-pr-command.md | 66 +++++++++++++++++-- 1 file changed, 61 insertions(+), 5 deletions(-) diff --git a/docs/design-decisions/041-zulip-close-pr-command.md b/docs/design-decisions/041-zulip-close-pr-command.md index a5ea923b..8b8df53b 100644 --- a/docs/design-decisions/041-zulip-close-pr-command.md +++ b/docs/design-decisions/041-zulip-close-pr-command.md @@ -125,10 +125,10 @@ Performed using a GitHub App token for operation `close_pr` (mapped to `queueboa ## Implementation Plan -### Commit 1 (this doc) +### Commit 1 (this doc) ✓ - Add `docs/design-decisions/041-zulip-close-pr-command.md`. -### Commit 2 (implementation) +### Commit 2 (implementation) ✓ - New files: - `qb_site/zulip_bot/commands/close_pr.py` - `qb_site/zulip_bot/services/close_pr_links.py` @@ -140,8 +140,15 @@ Performed using a GitHub App token for operation `close_pr` (mapped to `queueboa - `qb_site/zulip_bot/urls.py` — add `close-pr//` - `qb_site/zulip_bot/AGENTS.md` / `CLAUDE.md` — document new command and settings - `docs/github_app_setup.md` — add new app permissions and `close_pr` operation - -### Commit 3 (follow-up: custom close message) +- Also included two follow-up fixes: + - `fix(zulip_bot): normalise underscores to hyphens in command name parser` — so + `close_pr`, `assigned_prs`, `pr_info` variants resolve to the canonical hyphenated names + without per-command aliases. + - `fix(zulip_bot): fix two test_close_pr_form assertions` — missing + `ZULIP_CLOSE_PR_MUTATIONS_ENABLED` override in one test and a wrong error-substring check + in another. + +### Commit 3 (follow-up: custom close message) — not yet started - Add textarea to confirmation form for optional close message. - Add pre-written close message templates (rendered as selectable buttons/options). - On submit, post the message as a PR comment (`POST /repos/{owner}/{repo}/issues/{number}/comments`) @@ -167,5 +174,54 @@ Performed using a GitHub App token for operation `close_pr` (mapped to `queueboa - End-to-end: issue command → receive link → open form → submit → PR closed → DM received → log thread updated. +## Operational Deployment Notes + +### Before enabling in production +1. **Update the `queueboard-assignment` GitHub App** (manual step in the GitHub UI): + - Upgrade `Pull requests` from Read-only to **Read and write**. + - Add `Members: Read` as a new org-level permission. + - After saving, GitHub will ask any installation admins to approve the expanded permissions. + Existing `assign`/`unassign` operations continue with the old token scope until approved, + so coordinate approval with minimal downtime. +2. **Update `GITHUB_APP_TOKEN_CONFIG`** to include `close_pr` in the `operation_app_map` and + in the `queueboard-assignment` app's `operations` list. See `docs/github_app_setup.md` for + the full example config. +3. **Add `ZULIP_REPO_LOG`** to settings/env for each repo you want audit-log posts in Zulip: + ```json + { + "leanprover-community/mathlib4": { + "stream": "mathlib4", + "topic": "bot actions" + } + } + ``` + If a repo has no entry, closes still work but a WARNING is logged and no Zulip log post is + sent. The setting is intentionally generic for reuse by future operations. +4. **Add `ZULIP_CLOSE_PR_MUTATIONS_ENABLED = True`** to enable actual PR closes. Without this + flag the form renders and validates but the POST returns a preflight-only message and does not + call GitHub. Useful for end-to-end smoke testing without side effects. +5. **Optional token settings** (all have reasonable defaults): + - `ZULIP_CLOSE_PR_TOKEN_SECRET` (falls back to `SECRET_KEY`) + - `ZULIP_CLOSE_PR_TOKEN_SALT` (default: `"zulip_bot.close_pr"`) + - `ZULIP_CLOSE_PR_TOKEN_TTL_SECONDS` (default: `1800`) + - `ZULIP_CLOSE_PR_URL_BASE` (falls back to relative path) + +### Manual verification checklist (after app permissions are updated) +- [ ] `GET /repos/{owner}/{repo}/collaborators/{username}/permission` returns `{"permission": + "none"}` for a non-collaborator (not 404). +- [ ] `GET /repos/{owner}/{repo}/collaborators/{username}/permission` returns `"write"` for a + user with the `maintain` role. +- [ ] `PATCH /repos/{owner}/{repo}/pulls/{number}` with `{"state": "closed"}` succeeds using + the updated `queueboard-assignment` installation token. +- [ ] End-to-end: issue `close-pr ` in Zulip → receive private link → open form → + confirm → PR closed on GitHub → DM received → log thread updated in Zulip. +- [ ] Issue command as a non-collaborator who is not the PR author → receive "you don't have + permission" private message, no link issued. +- [ ] Issue command for an already-closed PR → receive "PR is not open" private message. +- [ ] Open confirmation link after TTL expires → see 403 invalid-token page. + ## Progress Notes -- 2026-04-12: Design doc written; implementation not yet started. +- 2026-04-12: Design doc written. +- 2026-04-12: Implementation complete (commits 1 and 2 landed on `close-pr-command` branch). + All unit tests pass under Compose. GitHub App permission upgrade and production env vars are + the remaining manual steps before the feature can be enabled. From d144fc83f08be9f1f85814a3a71c1214476a0f8f Mon Sep 17 00:00:00 2001 From: Bryan Gin-ge Chen Date: Wed, 15 Apr 2026 15:29:45 -0400 Subject: [PATCH 07/24] refactor(zulip_bot): split collaborator check onto separate queueboard-org-read app Introduce a dedicated `check_collaborator_permission` operation so the `Members: Read` org-level permission can be carried by a separate `queueboard-org-read` GitHub App rather than `queueboard-assignment`. The execution service falls back to the `close_pr` token when `check_collaborator_permission` is not mapped, keeping single-app setups working without config changes. Update docs/github_app_setup.md and 041 design doc to reflect the three-app layout and new operation name. Co-Authored-By: Claude Sonnet 4.6 --- .../041-zulip-close-pr-command.md | 25 +++++++++---- docs/github_app_setup.md | 35 ++++++++++++++++--- .../zulip_bot/services/close_pr_execution.py | 12 ++++++- 3 files changed, 59 insertions(+), 13 deletions(-) diff --git a/docs/design-decisions/041-zulip-close-pr-command.md b/docs/design-decisions/041-zulip-close-pr-command.md index 8b8df53b..5fd48650 100644 --- a/docs/design-decisions/041-zulip-close-pr-command.md +++ b/docs/design-decisions/041-zulip-close-pr-command.md @@ -14,7 +14,10 @@ Read-only`. Closing a PR via `PATCH /repos/{owner}/{repo}/pulls/{number}` requires `Pull requests: Read and write`. Checking a user's collaborator permission via `GET /repos/{owner}/{repo}/collaborators/{username}/permission` requires `Members: Read` (org-level). - Both permissions must be added to the `queueboard-assignment` app. + To limit blast radius, a separate `queueboard-org-read` app carries `Members: Read` and handles + the `check_collaborator_permission` operation; `queueboard-assignment` only needs `Pull requests: + Read and write` (no org-level permissions). The close-PR code falls back to the `close_pr` token + if `check_collaborator_permission` is not mapped, for single-app setups. ## Goals / Non-Goals - Goals: @@ -105,8 +108,13 @@ Performed using a GitHub App token for operation `close_pr` (mapped to `queueboa ### GitHub App Changes (manual) - Add to `queueboard-assignment`: - `Pull requests: Read and write` (upgrade from read-only) - - `Members: Read` (org-level permission, new) -- Update `docs/github_app_setup.md` and `GITHUB_APP_TOKEN_CONFIG` operation map to include `close_pr`. +- Create new app `queueboard-org-read`: + - `Metadata: Read-only` (baseline) + - `Members: Read` (org-level permission) + - Install on the org with owner-level installation lookup +- Update `docs/github_app_setup.md` and `GITHUB_APP_TOKEN_CONFIG` operation map to include + `close_pr` (→ `queueboard-assignment`) and `check_collaborator_permission` + (→ `queueboard-org-read`). ## Subtleties / Invariants - Permission check uses the GitHub App token, not the user's personal token. The close itself is also @@ -179,13 +187,16 @@ Performed using a GitHub App token for operation `close_pr` (mapped to `queueboa ### Before enabling in production 1. **Update the `queueboard-assignment` GitHub App** (manual step in the GitHub UI): - Upgrade `Pull requests` from Read-only to **Read and write**. - - Add `Members: Read` as a new org-level permission. - After saving, GitHub will ask any installation admins to approve the expanded permissions. Existing `assign`/`unassign` operations continue with the old token scope until approved, so coordinate approval with minimal downtime. -2. **Update `GITHUB_APP_TOKEN_CONFIG`** to include `close_pr` in the `operation_app_map` and - in the `queueboard-assignment` app's `operations` list. See `docs/github_app_setup.md` for - the full example config. +2. **Create the `queueboard-org-read` GitHub App** (manual step): + - Set `Metadata: Read-only` and org-level `Members: Read`. + - Install on the org (owner-level installation). + - Generate a private key and store it in your secret store. +3. **Update `GITHUB_APP_TOKEN_CONFIG`** to include `close_pr` (→ `queueboard-assignment`) and + `check_collaborator_permission` (→ `queueboard-org-read`) in the `operation_app_map`, and add + both apps to the `apps` list. See `docs/github_app_setup.md` for the full example config. 3. **Add `ZULIP_REPO_LOG`** to settings/env for each repo you want audit-log posts in Zulip: ```json { diff --git a/docs/github_app_setup.md b/docs/github_app_setup.md index feab84f5..0fa5ab4b 100644 --- a/docs/github_app_setup.md +++ b/docs/github_app_setup.md @@ -9,9 +9,14 @@ This guide covers how to create and configure GitHub Apps for Queueboard's opera - Runtime config is loaded from `GITHUB_APP_TOKEN_CONFIG` (JSON object). ## Recommended App Layout -- Use two apps (recommended): - - `queueboard-assignment`: minimal write permissions for assignee mutations. +- Use three apps (recommended): + - `queueboard-assignment`: minimal write permissions for assignee mutations and PR closes. + - `queueboard-org-read`: org-level read permission for collaborator/membership checks. - `queueboard-syncer-read`: read-only permissions for sync/discovery. +- Keeping `queueboard-org-read` separate avoids granting org-level `Members: Read` to the + assignment app, limiting blast radius. If you prefer simplicity, `queueboard-assignment` can + cover `check_collaborator_permission` as well — the code falls back to the `close_pr` token + when `check_collaborator_permission` is not mapped. - One app for all operations also works, but increases blast radius and over-permission risk. ## 1) Create GitHub Apps Under the Organization @@ -40,10 +45,15 @@ Set only the permissions needed by each operation set. - `Issues: Read and write` (required for `POST/DELETE /repos/{owner}/{repo}/issues/{number}/assignees`) - `Pull requests: Read and write` (required for close-PR mutation via `PATCH /repos/{owner}/{repo}/pulls/{number}`; also used for live precondition checks on PR state/assignees) - `Metadata: Read-only` (baseline repository visibility/lookup behavior) +- Organization permissions: none (collaborator checks are delegated to `queueboard-org-read`) + +### App B: `queueboard-org-read` (collaborator/membership checks) +- Repository permissions: + - `Metadata: Read-only` (baseline) - Organization permissions: - - `Members: Read` (required for `GET /repos/{owner}/{repo}/collaborators/{username}/permission` to check if a user has write/admin access) + - `Members: Read` (required for `GET /repos/{owner}/{repo}/collaborators/{username}/permission`) -### App B: `queueboard-syncer-read` (syncer read operations) +### App C: `queueboard-syncer-read` (syncer read operations) - Repository permissions: - If Queueboard only targets public repositories, you can leave repository permissions at `No access`. - Recommended default (safer when repo visibility or API usage changes): @@ -88,7 +98,7 @@ Set these values in `.env` or your deployment secret manager: - `private_key` (PEM string; use `\\n` escapes in env JSON; preferred for current Heroku deployment), or - `private_key_path` (path to PEM file visible to the process) -Example (two-app config): +Example (three-app config): ```json { @@ -98,6 +108,7 @@ Example (two-app config): "assign_pr": "queueboard-assignment", "unassign_pr": "queueboard-assignment", "close_pr": "queueboard-assignment", + "check_collaborator_permission": "queueboard-org-read", "syncer_repo_discovery": "queueboard-syncer-read", "syncer_pr_read": "queueboard-syncer-read", "syncer_ci_read": "queueboard-syncer-read" @@ -110,6 +121,15 @@ Example (two-app config): "installation_lookup": "repo", "operations": ["assign_pr", "unassign_pr", "close_pr"] }, + { + "name": "queueboard-org-read", + "app_id": 345678, + "private_key_path": "/run/secrets/queueboard-org-read.pem", + "installation_lookup": "owner", + "installation_owner_type": "org", + "installation_owner": "leanprover-community", + "operations": ["check_collaborator_permission"] + }, { "name": "queueboard-syncer-read", "app_id": 234567, @@ -123,6 +143,11 @@ Example (two-app config): } ``` +Note: if `check_collaborator_permission` is not present in `operation_app_map` (e.g. during a +phased rollout or single-app setup), the close-PR command falls back to using the `close_pr` +token for the collaborator check — but that requires `queueboard-assignment` to also have +`Members: Read`. + Helper command for generating/maintaining this JSON: ```bash diff --git a/qb_site/zulip_bot/services/close_pr_execution.py b/qb_site/zulip_bot/services/close_pr_execution.py index fdb011c8..0ad7af99 100644 --- a/qb_site/zulip_bot/services/close_pr_execution.py +++ b/qb_site/zulip_bot/services/close_pr_execution.py @@ -14,6 +14,7 @@ _GITHUB_API_BASE = "https://api.github.com" _CLOSE_PR_OPERATION = "close_pr" +_CHECK_COLLABORATOR_PERMISSION_OPERATION = "check_collaborator_permission" class PermissionOutcome(str, Enum): @@ -87,7 +88,8 @@ def check_close_pr_permission( pr_title=pr.title, ) - collab_permission = _fetch_collaborator_permission(token=token, owner=owner, repo=repo, github_login=github_login) + member_token = _get_member_check_token(owner=owner, repo=repo) or token + collab_permission = _fetch_collaborator_permission(token=member_token, owner=owner, repo=repo, github_login=github_login) if collab_permission in {"write", "admin"}: return PermissionCheckResult( outcome=PermissionOutcome.PERMITTED, @@ -163,6 +165,14 @@ def _get_token(*, owner: str, repo: str) -> str | None: ) +def _get_member_check_token(*, owner: str, repo: str) -> str | None: + return resolve_github_app_operation_token( + operation=_CHECK_COLLABORATOR_PERMISSION_OPERATION, + owner=owner, + repo=repo, + ) + + def _fetch_pr_details(*, token: str, owner: str, repo: str, number: int) -> LivePRDetails | None: api_base = _api_base_url() url = f"{api_base}/repos/{owner}/{repo}/pulls/{number}" From 04727764caf52b08519259f543770242b08573f8 Mon Sep 17 00:00:00 2001 From: Bryan Gin-ge Chen Date: Wed, 15 Apr 2026 15:40:17 -0400 Subject: [PATCH 08/24] feat(zulip_bot): run post-actions in dry-run mode when mutations disabled When ZULIP_CLOSE_PR_MUTATIONS_ENABLED is unset, the confirmation form now shows a dry-run warning and still allows submission. On POST it skips the actual close_pull_request() call but runs all post-actions (sync enqueue, DM, log post) normally, allowing a full end-to-end test without touching the PR. Template: replaces the old static preflight-only message with a dry-run callout on the form (GET) and a preflight_only success message (POST). View: restructures the POST guard so mutations_enabled controls only the close call, not the post-actions. Test updated accordingly. Co-Authored-By: Claude Sonnet 4.6 --- .../templates/zulip_bot/close_pr_form.html | 33 +++++++++++-------- qb_site/zulip_bot/tests/test_close_pr_form.py | 8 +++-- qb_site/zulip_bot/views.py | 22 ++++++------- 3 files changed, 34 insertions(+), 29 deletions(-) diff --git a/qb_site/templates/zulip_bot/close_pr_form.html b/qb_site/templates/zulip_bot/close_pr_form.html index 5b4f623a..b5988a66 100644 --- a/qb_site/templates/zulip_bot/close_pr_form.html +++ b/qb_site/templates/zulip_bot/close_pr_form.html @@ -20,20 +20,18 @@

Close Pull Request

{% if success %}
-
- Pull request {{ pr_owner }}/{{ pr_repo }}#{{ pr_number }} has been closed. - The close was attributed to the bot. A confirmation has been sent to you via Zulip DM. -
-
- - {% elif mutations_disabled %} -
-
- Preflight check passed — your account has permission to close this PR. - However, mutations are currently disabled on this server - (ZULIP_CLOSE_PR_MUTATIONS_ENABLED is not set). - Contact an administrator to enable the close-PR mutation. -
+ {% if preflight_only %} +
+ Dry-run complete — the pull request was not closed + (ZULIP_CLOSE_PR_MUTATIONS_ENABLED is not set). + Post-actions (sync enqueue, Zulip DM, log post) were triggered normally. +
+ {% else %} +
+ Pull request {{ pr_owner }}/{{ pr_repo }}#{{ pr_number }} has been closed. + The close was attributed to the bot. A confirmation has been sent to you via Zulip DM. +
+ {% endif %}
{% elif not pr_is_open %} @@ -53,6 +51,13 @@

Close Pull Request

{% else %}
+ {% if mutations_disabled %} +
+ Dry-run mode: ZULIP_CLOSE_PR_MUTATIONS_ENABLED is not set. + Submitting will trigger all post-actions (sync enqueue, Zulip DM, log post) but will + not close the pull request. +
+ {% endif %}

You are about to close pull request {{ pr_owner }}/{{ pr_repo }}#{{ pr_number }}{% if pr_title %}: {{ pr_title }}{% endif %}. diff --git a/qb_site/zulip_bot/tests/test_close_pr_form.py b/qb_site/zulip_bot/tests/test_close_pr_form.py index 149f840f..7faf657f 100644 --- a/qb_site/zulip_bot/tests/test_close_pr_form.py +++ b/qb_site/zulip_bot/tests/test_close_pr_form.py @@ -109,12 +109,14 @@ def test_mutations_disabled_flag_in_context(self) -> None: class TestClosePRFormPost(TestCase): - def test_mutations_disabled_shows_preflight_message(self) -> None: - with _patch_pr_details(_open_pr()), _patch_close() as mock_close: + def test_mutations_disabled_skips_close_but_runs_post_actions(self) -> None: + with _patch_pr_details(_open_pr()), _patch_close() as mock_close, _patch_post_actions() as mock_post: response = self.client.post(_url(_token())) mock_close.assert_not_called() + mock_post.assert_called_once() self.assertEqual(response.status_code, 200) - self.assertTrue(response.context["mutations_disabled"]) + self.assertTrue(response.context["success"]) + self.assertTrue(response.context["preflight_only"]) @override_settings(ZULIP_CLOSE_PR_MUTATIONS_ENABLED="true") def test_successful_close(self) -> None: diff --git a/qb_site/zulip_bot/views.py b/qb_site/zulip_bot/views.py index 1a2bb726..6e9d32bf 100644 --- a/qb_site/zulip_bot/views.py +++ b/qb_site/zulip_bot/views.py @@ -388,24 +388,22 @@ def close_pr_form(request: HttpRequest, token: str) -> HttpResponse: "expires_at_iso": expires_at_utc.isoformat(), "mutations_disabled": not mutations_enabled, "success": False, + "preflight_only": False, "close_error": None, } if request.method == "POST": - if not mutations_enabled: - response = TemplateResponse(request, "zulip_bot/close_pr_form.html", context, status=200) - response["Cache-Control"] = "no-store" - return response - - try: - close_pull_request(owner=claims.pr_owner, repo=claims.pr_repo, number=claims.pr_number) - except ClosePRError as exc: - context["close_error"] = exc.message - response = TemplateResponse(request, "zulip_bot/close_pr_form.html", context, status=200) - response["Cache-Control"] = "no-store" - return response + if mutations_enabled: + try: + close_pull_request(owner=claims.pr_owner, repo=claims.pr_repo, number=claims.pr_number) + except ClosePRError as exc: + context["close_error"] = exc.message + response = TemplateResponse(request, "zulip_bot/close_pr_form.html", context, status=200) + response["Cache-Control"] = "no-store" + return response context["success"] = True + context["preflight_only"] = not mutations_enabled _enqueue_close_pr_post_actions(claims=claims, pr_title=pr_title) response = TemplateResponse(request, "zulip_bot/close_pr_form.html", context, status=200) From 35f84eb9485607c31be5f39b8ca010388d151410 Mon Sep 17 00:00:00 2001 From: Bryan Gin-ge Chen Date: Wed, 15 Apr 2026 15:48:22 -0400 Subject: [PATCH 09/24] doc(041): add ZULIP_COMMAND_POLICY step to deployment notes Without a close-pr entry in ZULIP_COMMAND_POLICY the command is silently blocked for all users. Add step 4 with an example policy snippet and a pointer to manage.py zulip_policy. Also fix the duplicate step-3 numbering introduced in an earlier edit, and update the ZULIP_CLOSE_PR_MUTATIONS_ENABLED description to reflect the new dry-run behaviour. Co-Authored-By: Claude Sonnet 4.6 --- .../041-zulip-close-pr-command.md | 23 +++++++++++++++---- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/docs/design-decisions/041-zulip-close-pr-command.md b/docs/design-decisions/041-zulip-close-pr-command.md index 5fd48650..73debc76 100644 --- a/docs/design-decisions/041-zulip-close-pr-command.md +++ b/docs/design-decisions/041-zulip-close-pr-command.md @@ -197,7 +197,20 @@ Performed using a GitHub App token for operation `close_pr` (mapped to `queueboa 3. **Update `GITHUB_APP_TOKEN_CONFIG`** to include `close_pr` (→ `queueboard-assignment`) and `check_collaborator_permission` (→ `queueboard-org-read`) in the `operation_app_map`, and add both apps to the `apps` list. See `docs/github_app_setup.md` for the full example config. -3. **Add `ZULIP_REPO_LOG`** to settings/env for each repo you want audit-log posts in Zulip: +4. **Add `close-pr` to `ZULIP_COMMAND_POLICY`**. Without a policy entry the command is silently + ignored for all users. Restrict `allowed_groups` to the Zulip group(s) whose members should be + able to invoke the command (GitHub permission is still checked at runtime). Allow both DM and + stream contexts so the command can be used from either: + ```json + { + "close-pr": { + "allowed_groups": [], + "allowed_contexts": ["dm", "stream:*"] + } + } + ``` + Use `manage.py zulip_policy` to build and validate the JSON before setting the env var. +5. **Add `ZULIP_REPO_LOG`** to settings/env for each repo you want audit-log posts in Zulip: ```json { "leanprover-community/mathlib4": { @@ -208,10 +221,10 @@ Performed using a GitHub App token for operation `close_pr` (mapped to `queueboa ``` If a repo has no entry, closes still work but a WARNING is logged and no Zulip log post is sent. The setting is intentionally generic for reuse by future operations. -4. **Add `ZULIP_CLOSE_PR_MUTATIONS_ENABLED = True`** to enable actual PR closes. Without this - flag the form renders and validates but the POST returns a preflight-only message and does not - call GitHub. Useful for end-to-end smoke testing without side effects. -5. **Optional token settings** (all have reasonable defaults): +6. **Add `ZULIP_CLOSE_PR_MUTATIONS_ENABLED = True`** to enable actual PR closes. Without this + flag the form renders and the POST runs all post-actions (sync enqueue, DM, log post) but does + not call GitHub — useful for end-to-end smoke testing without closing the PR. +7. **Optional token settings** (all have reasonable defaults): - `ZULIP_CLOSE_PR_TOKEN_SECRET` (falls back to `SECRET_KEY`) - `ZULIP_CLOSE_PR_TOKEN_SALT` (default: `"zulip_bot.close_pr"`) - `ZULIP_CLOSE_PR_TOKEN_TTL_SECONDS` (default: `1800`) From f2fbbf292a1963144fd8a88e532ddafcbe111ad0 Mon Sep 17 00:00:00 2001 From: Bryan Gin-ge Chen Date: Wed, 15 Apr 2026 15:53:46 -0400 Subject: [PATCH 10/24] refactor(zulip_bot): use ZULIP_PREFS_URL_BASE for close-pr links Replace the close-pr-specific ZULIP_CLOSE_PR_URL_BASE with the shared ZULIP_PREFS_URL_BASE that prefs and registration links already use. There is no reason to have separate base URL settings for different Zulip forms on the same server. Co-Authored-By: Claude Sonnet 4.6 --- docs/design-decisions/041-zulip-close-pr-command.md | 2 +- qb_site/zulip_bot/services/close_pr_links.py | 2 +- qb_site/zulip_bot/tests/test_close_pr_links.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/design-decisions/041-zulip-close-pr-command.md b/docs/design-decisions/041-zulip-close-pr-command.md index 73debc76..d390af9c 100644 --- a/docs/design-decisions/041-zulip-close-pr-command.md +++ b/docs/design-decisions/041-zulip-close-pr-command.md @@ -64,7 +64,7 @@ Performed using a GitHub App token for operation `close_pr` (mapped to `queueboa - `ZULIP_CLOSE_PR_TOKEN_SECRET` (falls back to `SECRET_KEY`) - `ZULIP_CLOSE_PR_TOKEN_SALT` (default: `"zulip_bot.close_pr"`) - `ZULIP_CLOSE_PR_TOKEN_TTL_SECONDS` (default: `1800`) - - `ZULIP_CLOSE_PR_URL_BASE` (optional; falls back to relative path) + - `ZULIP_PREFS_URL_BASE` (shared with prefs/registration links; falls back to relative path) ### Execution Service: `close_pr_execution.py` - `check_close_pr_permission(github_login, owner, repo, number)` → `PermissionCheckResult` diff --git a/qb_site/zulip_bot/services/close_pr_links.py b/qb_site/zulip_bot/services/close_pr_links.py index a4d47f4c..1abb8cb9 100644 --- a/qb_site/zulip_bot/services/close_pr_links.py +++ b/qb_site/zulip_bot/services/close_pr_links.py @@ -37,7 +37,7 @@ class ClosePRTokenInvalid(ClosePRTokenError): def build_close_pr_link(*, claims: ClosePRLinkClaims) -> str: token = issue_close_pr_token(claims=claims) - url_base = getattr(settings, "ZULIP_CLOSE_PR_URL_BASE", "").strip().rstrip("/") + url_base = getattr(settings, "ZULIP_PREFS_URL_BASE", "").strip().rstrip("/") path = f"/api/zulip/close-pr/{quote(token, safe='')}/" if url_base: return f"{url_base}{path}" diff --git a/qb_site/zulip_bot/tests/test_close_pr_links.py b/qb_site/zulip_bot/tests/test_close_pr_links.py index 9ac8ffdb..21f2c9d8 100644 --- a/qb_site/zulip_bot/tests/test_close_pr_links.py +++ b/qb_site/zulip_bot/tests/test_close_pr_links.py @@ -37,7 +37,7 @@ def test_round_trip(self) -> None: self.assertIsNotNone(claims.iat) self.assertIsNotNone(claims.exp) - @override_settings(ZULIP_CLOSE_PR_URL_BASE="https://queueboard.example") + @override_settings(ZULIP_PREFS_URL_BASE="https://queueboard.example") def test_build_link_uses_url_base(self) -> None: link = build_close_pr_link(claims=self._claims()) self.assertTrue(link.startswith("https://queueboard.example/api/zulip/close-pr/")) From 0da09d9d38619c5fe8495e17c6aa05781122ca45 Mon Sep 17 00:00:00 2001 From: Bryan Gin-ge Chen Date: Wed, 15 Apr 2026 15:55:08 -0400 Subject: [PATCH 11/24] chore: add close-pr settings to .env.example Add ZULIP_CLOSE_PR_MUTATIONS_ENABLED and ZULIP_REPO_LOG entries. Update GITHUB_APP_TOKEN_CONFIG inline example to include close_pr and check_collaborator_permission operations and the queueboard-org-read app. Co-Authored-By: Claude Sonnet 4.6 --- .env.example | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/.env.example b/.env.example index 0a48a121..03651276 100644 --- a/.env.example +++ b/.env.example @@ -31,6 +31,14 @@ ZULIP_ASSIGNMENT_SUCCESS_EMOJI=thumbs_up # Enable live GitHub assign/unassign mutations (true/false, 1/0, yes/no) ZULIP_ASSIGNMENT_MUTATIONS_ENABLED= +# Zulip close-pr command behavior +# Enable live GitHub PR close mutations (true/false, 1/0, yes/no). +# When unset, the form still renders and post-actions run (dry-run mode) but the PR is not closed. +ZULIP_CLOSE_PR_MUTATIONS_ENABLED= +# Per-repo Zulip audit log destinations (JSON). If a repo has no entry, log posts are skipped. +# Example: {"leanprover-community/mathlib4": {"stream": "mathlib4", "topic": "bot actions"}} +ZULIP_REPO_LOG= + # Core Django settings DJANGO_SETTINGS_MODULE=qb_site.settings.local DJANGO_SECRET_KEY=change-me @@ -85,11 +93,11 @@ GITHUB_OAUTH_SCOPE=read:user # Shared secret used to validate GitHub webhook signatures (X-Hub-Signature-256). GITHUB_WEBHOOK_SECRET= # JSON object describing GitHub App credentials and operation mapping. -# Assignment commands require app-token resolution for assign/unassign operations. -# Syncer operations try app tokens first and fall back to GH_TOKEN/GITHUB_TOKEN. (see `manage.py github_app_config`) +# Assignment and close-pr commands require app-token resolution. Syncer operations try app tokens +# first and fall back to GH_TOKEN/GITHUB_TOKEN. (see `manage.py github_app_config`) GITHUB_APP_TOKEN_CONFIG= # Example: -# GITHUB_APP_TOKEN_CONFIG={"api_base_url":"https://api.github.com","cache_skew_seconds":60,"operation_app_map":{"assign_pr":"queueboard-assignment","unassign_pr":"queueboard-assignment","syncer_repo_discovery":"queueboard-syncer-read","syncer_pr_read":"queueboard-syncer-read","syncer_ci_read":"queueboard-syncer-read"},"apps":[{"name":"queueboard-assignment","app_id":123456,"private_key_path":"/run/secrets/queueboard-assignment.pem","installation_lookup":"repo","operations":["assign_pr","unassign_pr"]},{"name":"queueboard-syncer-read","app_id":234567,"private_key_path":"/run/secrets/queueboard-syncer-read.pem","installation_lookup":"owner","installation_owner_type":"org","installation_owner":"leanprover-community","operations":["syncer_repo_discovery","syncer_pr_read","syncer_ci_read"]}]} +# GITHUB_APP_TOKEN_CONFIG={"api_base_url":"https://api.github.com","cache_skew_seconds":60,"operation_app_map":{"assign_pr":"queueboard-assignment","unassign_pr":"queueboard-assignment","close_pr":"queueboard-assignment","check_collaborator_permission":"queueboard-org-read","syncer_repo_discovery":"queueboard-syncer-read","syncer_pr_read":"queueboard-syncer-read","syncer_ci_read":"queueboard-syncer-read"},"apps":[{"name":"queueboard-assignment","app_id":123456,"private_key_path":"/run/secrets/queueboard-assignment.pem","installation_lookup":"repo","operations":["assign_pr","unassign_pr","close_pr"]},{"name":"queueboard-org-read","app_id":345678,"private_key_path":"/run/secrets/queueboard-org-read.pem","installation_lookup":"owner","installation_owner_type":"org","installation_owner":"leanprover-community","operations":["check_collaborator_permission"]},{"name":"queueboard-syncer-read","app_id":234567,"private_key_path":"/run/secrets/queueboard-syncer-read.pem","installation_lookup":"owner","installation_owner_type":"org","installation_owner":"leanprover-community","operations":["syncer_repo_discovery","syncer_pr_read","syncer_ci_read"]}]} # Syncer scheduling defaults # How far back to look for changed PRs each tick (minutes) From f2763f44910aa8fec8e1f4ecd95c21e69fa54c06 Mon Sep 17 00:00:00 2001 From: Bryan Gin-ge Chen Date: Wed, 15 Apr 2026 16:02:55 -0400 Subject: [PATCH 12/24] fix(zulip_bot): parse ZULIP_REPO_LOG from JSON env var in settings ZULIP_REPO_LOG was read via getattr(settings, ...) but never parsed from the env var in settings/base.py, so it remained a raw string and _resolve_repo_log_target silently returned None on every call. Add the same JSON-parse-and-validate pattern used by GITHUB_APP_TOKEN_CONFIG and ZULIP_COMMAND_POLICY. Co-Authored-By: Claude Sonnet 4.6 --- qb_site/qb_site/settings/base.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/qb_site/qb_site/settings/base.py b/qb_site/qb_site/settings/base.py index 38bcbb64..88b9e876 100644 --- a/qb_site/qb_site/settings/base.py +++ b/qb_site/qb_site/settings/base.py @@ -190,6 +190,13 @@ def env_optional_bounded_int(name: str, *, minimum: int, maximum: int) -> int | if not isinstance(parsed_policy, dict): raise RuntimeError("ZULIP_COMMAND_POLICY env var must be a JSON object") ZULIP_COMMAND_POLICY = parsed_policy +ZULIP_REPO_LOG: dict[str, dict[str, str]] = {} +_ZULIP_REPO_LOG_ENV = os.getenv("ZULIP_REPO_LOG", "").strip() +if _ZULIP_REPO_LOG_ENV: + parsed_repo_log = json.loads(_ZULIP_REPO_LOG_ENV) + if not isinstance(parsed_repo_log, dict): + raise RuntimeError("ZULIP_REPO_LOG env var must be a JSON object") + ZULIP_REPO_LOG = parsed_repo_log # Celery configuration From ddf729dc21a39ccc6029e9ee9ad987a4a09c7de0 Mon Sep 17 00:00:00 2001 From: Bryan Gin-ge Chen Date: Wed, 15 Apr 2026 16:07:07 -0400 Subject: [PATCH 13/24] feat(zulip_bot): improve close-pr log and DM message formatting - Link the PR (markdown link to github.com) instead of bare backtick ref in both the DM and the log post. - Use a Zulip silent mention (@_**Name|id**) for the requester in the log post, looked up from User.zulip_full_name; falls back to `github_login` if the name is not populated. Co-Authored-By: Claude Sonnet 4.6 --- qb_site/zulip_bot/views.py | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/qb_site/zulip_bot/views.py b/qb_site/zulip_bot/views.py index 6e9d32bf..e3a6e708 100644 --- a/qb_site/zulip_bot/views.py +++ b/qb_site/zulip_bot/views.py @@ -417,9 +417,11 @@ def _close_pr_mutations_enabled() -> bool: def _enqueue_close_pr_post_actions(*, claims, pr_title: str | None) -> None: - from core.models import Repository + from core.models import Repository, User pr_ref = f"{claims.pr_owner}/{claims.pr_repo}#{claims.pr_number}" + pr_url = f"https://github.com/{claims.pr_owner}/{claims.pr_repo}/pull/{claims.pr_number}" + pr_link = f"[{pr_ref}]({pr_url})" title_part = f' ("{pr_title}")' if pr_title else "" # 1. Best-effort sync. @@ -436,7 +438,7 @@ def _enqueue_close_pr_post_actions(*, claims, pr_title: str | None) -> None: ) # 2. Best-effort DM to invoker. - dm_content = f"Pull request `{pr_ref}`{title_part} has been closed. The close was attributed to the bot." + dm_content = f"Pull request {pr_link}{title_part} has been closed. The close was attributed to the bot." try: ZulipClient().send_direct_message(to=[claims.zulip_user_id], content=dm_content) except ZulipApiError: @@ -448,7 +450,13 @@ def _enqueue_close_pr_post_actions(*, claims, pr_title: str | None) -> None: # 3. Best-effort log post. log_target = _resolve_repo_log_target(owner=claims.pr_owner, repo=claims.pr_repo) if log_target is not None: - log_content = f"Closed PR `{pr_ref}`{title_part} (via bot, requested by Zulip user {claims.zulip_user_id})." + user = User.objects.filter(zulip_user_id=claims.zulip_user_id).only("zulip_full_name").first() + zulip_full_name = user.zulip_full_name if user and user.zulip_full_name else None + if zulip_full_name: + requester = f"@_**{zulip_full_name}|{claims.zulip_user_id}**" + else: + requester = f"`{claims.github_login}`" + log_content = f"Closed PR {pr_link}{title_part} (via bot, requested by {requester})." try: ZulipClient().send_stream_message( stream=log_target["stream"], From ec479ed6df5d764843ad734a6ef69ce6b6b8089a Mon Sep 17 00:00:00 2001 From: Bryan Gin-ge Chen Date: Fri, 17 Apr 2026 03:52:26 -0400 Subject: [PATCH 14/24] doc: fix ref to deleted env var --- docs/design-decisions/041-zulip-close-pr-command.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/design-decisions/041-zulip-close-pr-command.md b/docs/design-decisions/041-zulip-close-pr-command.md index d390af9c..025ea4e1 100644 --- a/docs/design-decisions/041-zulip-close-pr-command.md +++ b/docs/design-decisions/041-zulip-close-pr-command.md @@ -228,7 +228,7 @@ Performed using a GitHub App token for operation `close_pr` (mapped to `queueboa - `ZULIP_CLOSE_PR_TOKEN_SECRET` (falls back to `SECRET_KEY`) - `ZULIP_CLOSE_PR_TOKEN_SALT` (default: `"zulip_bot.close_pr"`) - `ZULIP_CLOSE_PR_TOKEN_TTL_SECONDS` (default: `1800`) - - `ZULIP_CLOSE_PR_URL_BASE` (falls back to relative path) + - `ZULIP_PREFS_URL_BASE` (shared with prefs/registration links; falls back to relative path) ### Manual verification checklist (after app permissions are updated) - [ ] `GET /repos/{owner}/{repo}/collaborators/{username}/permission` returns `{"permission": From 8310e1a70240db0cca053adccfe53f8e6cc7d607 Mon Sep 17 00:00:00 2001 From: Bryan Gin-ge Chen Date: Fri, 17 Apr 2026 04:16:44 -0400 Subject: [PATCH 15/24] feat(zulip_bot): add optional close message with markdown presets to close-pr form Adds a textarea to the close-PR confirmation form where users can type a message that is posted as a GitHub comment before the PR is closed. Preset messages are loaded from .md files in close_pr_presets/ (first # heading = button label, remainder = body; sorted by filename). No code change is needed to add or edit presets. If the comment post fails the PR is not closed and an inline error is shown with the message preserved for retry. The error state now renders inline in the form rather than as a dead-end page in all cases. Co-Authored-By: Claude Sonnet 4.6 --- .../041-zulip-close-pr-command.md | 22 +++- .../templates/zulip_bot/close_pr_form.html | 36 ++++-- .../close_pr_presets/01-superseded.md | 3 + .../zulip_bot/close_pr_presets/02-stale.md | 3 + .../close_pr_presets/03-out-of-scope.md | 3 + .../zulip_bot/services/close_pr_execution.py | 38 +++++++ .../zulip_bot/services/close_pr_presets.py | 36 ++++++ .../static/zulip_bot/close_pr_form.css | 64 +++++++++++ qb_site/zulip_bot/tests/test_close_pr_form.py | 107 ++++++++++++++++++ qb_site/zulip_bot/views.py | 21 +++- 10 files changed, 318 insertions(+), 15 deletions(-) create mode 100644 qb_site/zulip_bot/close_pr_presets/01-superseded.md create mode 100644 qb_site/zulip_bot/close_pr_presets/02-stale.md create mode 100644 qb_site/zulip_bot/close_pr_presets/03-out-of-scope.md create mode 100644 qb_site/zulip_bot/services/close_pr_presets.py create mode 100644 qb_site/zulip_bot/static/zulip_bot/close_pr_form.css diff --git a/docs/design-decisions/041-zulip-close-pr-command.md b/docs/design-decisions/041-zulip-close-pr-command.md index 025ea4e1..af23b047 100644 --- a/docs/design-decisions/041-zulip-close-pr-command.md +++ b/docs/design-decisions/041-zulip-close-pr-command.md @@ -156,12 +156,21 @@ Performed using a GitHub App token for operation `close_pr` (mapped to `queueboa `ZULIP_CLOSE_PR_MUTATIONS_ENABLED` override in one test and a wrong error-substring check in another. -### Commit 3 (follow-up: custom close message) — not yet started -- Add textarea to confirmation form for optional close message. -- Add pre-written close message templates (rendered as selectable buttons/options). -- On submit, post the message as a PR comment (`POST /repos/{owner}/{repo}/issues/{number}/comments`) - before closing. No additional GitHub App permissions required (`Issues: Read and write` already - granted). +### Commit 3 (follow-up: custom close message) ✓ +- Textarea on confirmation form for an optional close message. +- Preset buttons load pre-written messages into the textarea. Presets are stored as `.md` files in + `qb_site/zulip_bot/close_pr_presets/`; the first `# Heading` line becomes the button label, the + remainder becomes the body. Files are sorted by filename so numbering controls display order. No + code change required to add or edit presets. +- On submit, if a message is provided it is posted as a PR comment + (`POST /repos/{owner}/{repo}/issues/{number}/comments`) before the PR is closed. If the comment + post fails, an inline error is shown and the PR is not closed (user can retry or clear the + message). No additional GitHub App permissions required (`Issues: Read and write` already granted). +- New service: `qb_site/zulip_bot/services/close_pr_presets.py` (`load_close_pr_presets`). +- New execution helper: `post_pr_comment` in `close_pr_execution.py`. +- New CSS: `qb_site/zulip_bot/static/zulip_bot/close_pr_form.css`. +- The error state now renders inline in the form (rather than a dead-end page) so the user's + typed message is preserved for retry. ## Validation Plan - Tests: @@ -249,3 +258,4 @@ Performed using a GitHub App token for operation `close_pr` (mapped to `queueboa - 2026-04-12: Implementation complete (commits 1 and 2 landed on `close-pr-command` branch). All unit tests pass under Compose. GitHub App permission upgrade and production env vars are the remaining manual steps before the feature can be enabled. +- 2026-04-17: Commit 3 complete — optional close message with markdown presets added. diff --git a/qb_site/templates/zulip_bot/close_pr_form.html b/qb_site/templates/zulip_bot/close_pr_form.html index b5988a66..678c5c13 100644 --- a/qb_site/templates/zulip_bot/close_pr_form.html +++ b/qb_site/templates/zulip_bot/close_pr_form.html @@ -7,6 +7,7 @@ Queueboard — close pull request +

@@ -41,16 +42,13 @@

Close Pull Request

- {% elif close_error %} -
- -

Please try again or contact an administrator.

-
- {% else %}
+ {% if close_error %} + + {% endif %} {% if mutations_disabled %}
Dry-run mode: ZULIP_CLOSE_PR_MUTATIONS_ENABLED is not set. @@ -68,6 +66,21 @@

Close Pull Request

{% csrf_token %} +
+ {% if presets %} +
+ Presets: + {% for preset in presets %} + + {% endfor %} +
+ {% endif %} + + +
@@ -79,5 +92,12 @@

Close Pull Request

{% endif %} + diff --git a/qb_site/zulip_bot/close_pr_presets/01-superseded.md b/qb_site/zulip_bot/close_pr_presets/01-superseded.md new file mode 100644 index 00000000..5a161057 --- /dev/null +++ b/qb_site/zulip_bot/close_pr_presets/01-superseded.md @@ -0,0 +1,3 @@ +# Superseded + +This PR is being closed because it has been superseded by another PR. Thank you for your contribution! diff --git a/qb_site/zulip_bot/close_pr_presets/02-stale.md b/qb_site/zulip_bot/close_pr_presets/02-stale.md new file mode 100644 index 00000000..242bd3cd --- /dev/null +++ b/qb_site/zulip_bot/close_pr_presets/02-stale.md @@ -0,0 +1,3 @@ +# Stale + +This PR is being closed due to inactivity. Please feel free to reopen it or open a new PR if you'd like to continue working on this. diff --git a/qb_site/zulip_bot/close_pr_presets/03-out-of-scope.md b/qb_site/zulip_bot/close_pr_presets/03-out-of-scope.md new file mode 100644 index 00000000..c36055bb --- /dev/null +++ b/qb_site/zulip_bot/close_pr_presets/03-out-of-scope.md @@ -0,0 +1,3 @@ +# Out of scope + +This PR is being closed as it falls outside the current scope of the project. Thank you for the contribution — we encourage you to discuss the direction on Zulip before opening a new PR. diff --git a/qb_site/zulip_bot/services/close_pr_execution.py b/qb_site/zulip_bot/services/close_pr_execution.py index 0ad7af99..e7db51a5 100644 --- a/qb_site/zulip_bot/services/close_pr_execution.py +++ b/qb_site/zulip_bot/services/close_pr_execution.py @@ -106,6 +106,44 @@ def check_close_pr_permission( ) +def post_pr_comment(*, owner: str, repo: str, number: int, body: str) -> None: + """Post a comment on a pull request before closing it. + + Uses the close_pr operation token (Issues: Read and write already granted). + Raises ClosePRError on failure. + """ + token = _get_token(owner=owner, repo=repo) + if not token: + raise ClosePRError( + code="token_unavailable", + message="GitHub App token for close_pr is not available for this repository.", + ) + + api_base = _api_base_url() + url = f"{api_base}/repos/{owner}/{repo}/issues/{number}/comments" + headers = { + "Authorization": f"Bearer {token}", + "Accept": "application/vnd.github+json", + "X-GitHub-Api-Version": "2022-11-28", + } + try: + response = requests.post(url, json={"body": body}, headers=headers, timeout=20) + except requests.RequestException as exc: + raise ClosePRError(code="request_failed", message=f"GitHub request failed: {exc}") from exc + + if response.status_code < 400: + return + + details = _safe_json(response) + if response.status_code in {401, 403}: + raise ClosePRError(code="permission_denied", message="GitHub permission denied when posting comment.") + if response.status_code == 404: + raise ClosePRError(code="pr_not_found", message="Pull request not found on GitHub.") + if response.status_code >= 500: + raise ClosePRError(code="github_transient", message="GitHub API temporarily failed.") + raise ClosePRError(code="github_error", message=f"GitHub API error when posting comment: {details}") + + def fetch_pr_details_for_form(*, owner: str, repo: str, number: int) -> LivePRDetails | None: """Fetch PR details for display on the confirmation form. diff --git a/qb_site/zulip_bot/services/close_pr_presets.py b/qb_site/zulip_bot/services/close_pr_presets.py new file mode 100644 index 00000000..6e3b97c7 --- /dev/null +++ b/qb_site/zulip_bot/services/close_pr_presets.py @@ -0,0 +1,36 @@ +from __future__ import annotations + +import re +from pathlib import Path + +_PRESETS_DIR = Path(__file__).parent.parent / "close_pr_presets" + + +def load_close_pr_presets() -> list[dict[str, str]]: + """Return preset close messages loaded from markdown files in close_pr_presets/. + + Each .md file's first line may be a markdown heading (``# Title``) used as + the display name; otherwise the filename stem (digits-prefix stripped, + hyphens/underscores replaced with spaces, title-cased) is used as the name. + Files are sorted by filename so numbering them controls display order. + Returns a list of ``{"name": str, "body": str}`` dicts (files with empty + bodies after stripping are skipped). + """ + if not _PRESETS_DIR.is_dir(): + return [] + presets = [] + for path in sorted(_PRESETS_DIR.glob("*.md")): + text = path.read_text(encoding="utf-8").strip() + if not text: + continue + lines = text.splitlines() + if lines[0].startswith("# "): + name = lines[0][2:].strip() + body = "\n".join(lines[1:]).strip() + else: + stem = re.sub(r"^\d+-", "", path.stem) + name = stem.replace("-", " ").replace("_", " ").title() + body = text + if body: + presets.append({"name": name, "body": body}) + return presets diff --git a/qb_site/zulip_bot/static/zulip_bot/close_pr_form.css b/qb_site/zulip_bot/static/zulip_bot/close_pr_form.css new file mode 100644 index 00000000..af1a96c1 --- /dev/null +++ b/qb_site/zulip_bot/static/zulip_bot/close_pr_form.css @@ -0,0 +1,64 @@ +.message-field { + margin-top: 16px; +} + +.message-label { + display: block; + font-weight: 600; + margin-bottom: 6px; + font-size: 0.95rem; +} + +.optional { + font-weight: 400; + color: var(--muted); + font-size: 0.88rem; +} + +.preset-bar { + display: flex; + flex-wrap: wrap; + gap: 6px; + align-items: center; + margin-bottom: 8px; +} + +.preset-label { + font-size: 0.88rem; + color: var(--muted); + font-weight: 600; +} + +.preset-btn { + font-size: 0.82rem; + padding: 3px 8px; + border: 1px solid var(--border); + border-radius: 6px; + background: var(--bg); + color: var(--ink); + cursor: pointer; + font-family: inherit; +} + +.preset-btn:hover { + background: var(--border); +} + +.message-textarea { + width: 100%; + min-height: 100px; + border: 1px solid var(--border); + border-radius: 8px; + padding: 8px 10px; + font-size: 0.92rem; + font-family: inherit; + color: var(--ink); + background: var(--card); + resize: vertical; + margin-bottom: 12px; +} + +.message-textarea:focus { + outline: 2px solid var(--accent); + outline-offset: 1px; +} diff --git a/qb_site/zulip_bot/tests/test_close_pr_form.py b/qb_site/zulip_bot/tests/test_close_pr_form.py index 7faf657f..44f4111c 100644 --- a/qb_site/zulip_bot/tests/test_close_pr_form.py +++ b/qb_site/zulip_bot/tests/test_close_pr_form.py @@ -7,6 +7,7 @@ from zulip_bot.services.close_pr_execution import ClosePRError, LivePRDetails from zulip_bot.services.close_pr_links import ClosePRLinkClaims, issue_close_pr_token +from zulip_bot.services.close_pr_presets import load_close_pr_presets def _token( @@ -50,10 +51,20 @@ def _patch_close(*, raises: ClosePRError | None = None): return patch("zulip_bot.views.close_pull_request") +def _patch_post_comment(*, raises: ClosePRError | None = None): + if raises: + return patch("zulip_bot.views.post_pr_comment", side_effect=raises) + return patch("zulip_bot.views.post_pr_comment") + + def _patch_post_actions(): return patch("zulip_bot.views._enqueue_close_pr_post_actions") +def _patch_presets(presets=None): + return patch("zulip_bot.views.load_close_pr_presets", return_value=presets or []) + + class TestClosePRFormGet(TestCase): def test_expired_token_returns_403(self) -> None: with patch("zulip_bot.services.close_pr_links.time.time", return_value=1_700_000_000): @@ -154,6 +165,102 @@ def test_expired_token_on_post_returns_403(self) -> None: self.assertEqual(response.status_code, 403) +class TestClosePRFormPostWithMessage(TestCase): + @override_settings(ZULIP_CLOSE_PR_MUTATIONS_ENABLED="true") + def test_close_message_posts_comment_before_close(self) -> None: + with ( + _patch_pr_details(_open_pr()), + _patch_post_comment() as mock_comment, + _patch_close() as mock_close, + _patch_post_actions(), + ): + response = self.client.post(_url(_token()), data={"close_message": "Superseded by #1000."}) + self.assertEqual(response.status_code, 200) + self.assertTrue(response.context["success"]) + mock_comment.assert_called_once() + mock_close.assert_called_once() + + @override_settings(ZULIP_CLOSE_PR_MUTATIONS_ENABLED="true") + def test_comment_failure_shows_error_and_skips_close(self) -> None: + error = ClosePRError(code="permission_denied", message="GitHub permission denied when posting comment.") + with ( + _patch_pr_details(_open_pr()), + _patch_post_comment(raises=error), + _patch_close() as mock_close, + _patch_post_actions(), + ): + response = self.client.post(_url(_token()), data={"close_message": "Some message."}) + self.assertEqual(response.status_code, 200) + self.assertFalse(response.context["success"]) + self.assertIn("permission denied", response.context["close_error"] or "") + mock_close.assert_not_called() + + @override_settings(ZULIP_CLOSE_PR_MUTATIONS_ENABLED="true") + def test_empty_message_skips_comment(self) -> None: + with ( + _patch_pr_details(_open_pr()), + _patch_post_comment() as mock_comment, + _patch_close(), + _patch_post_actions(), + ): + response = self.client.post(_url(_token()), data={"close_message": ""}) + self.assertTrue(response.context["success"]) + mock_comment.assert_not_called() + + def test_mutations_disabled_skips_comment_even_with_message(self) -> None: + with ( + _patch_pr_details(_open_pr()), + _patch_post_comment() as mock_comment, + _patch_close() as mock_close, + _patch_post_actions(), + ): + response = self.client.post(_url(_token()), data={"close_message": "Some message."}) + self.assertTrue(response.context["success"]) + self.assertTrue(response.context["preflight_only"]) + mock_comment.assert_not_called() + mock_close.assert_not_called() + + @override_settings(ZULIP_CLOSE_PR_MUTATIONS_ENABLED="true") + def test_close_message_preserved_in_context_on_error(self) -> None: + error = ClosePRError(code="github_transient", message="GitHub API temporarily failed.") + with ( + _patch_pr_details(_open_pr()), + _patch_post_comment(), + _patch_close(raises=error), + _patch_post_actions(), + ): + response = self.client.post(_url(_token()), data={"close_message": "My message."}) + self.assertFalse(response.context["success"]) + self.assertEqual(response.context["close_message"], "My message.") + + +class TestClosePRPresets(TestCase): + def test_load_presets_returns_list(self) -> None: + presets = load_close_pr_presets() + self.assertIsInstance(presets, list) + + def test_presets_have_name_and_body(self) -> None: + presets = load_close_pr_presets() + for preset in presets: + self.assertIn("name", preset) + self.assertIn("body", preset) + self.assertTrue(preset["name"]) + self.assertTrue(preset["body"]) + + def test_presets_passed_to_template_context(self) -> None: + sample = [{"name": "Superseded", "body": "This PR is superseded."}] + with _patch_presets(sample), _patch_pr_details(_open_pr()): + response = self.client.get(_url(_token())) + self.assertEqual(response.context["presets"], sample) + + def test_preset_buttons_rendered_in_template(self) -> None: + sample = [{"name": "Stale", "body": "Closing as stale."}] + with _patch_presets(sample), _patch_pr_details(_open_pr()): + response = self.client.get(_url(_token())) + self.assertContains(response, "Stale") + self.assertContains(response, "Closing as stale.") + + class TestRepoLogResolution(TestCase): def test_missing_setting_skips_log(self) -> None: from zulip_bot.views import _resolve_repo_log_target diff --git a/qb_site/zulip_bot/views.py b/qb_site/zulip_bot/views.py index e3a6e708..58511cf2 100644 --- a/qb_site/zulip_bot/views.py +++ b/qb_site/zulip_bot/views.py @@ -31,7 +31,8 @@ from zulip_bot.forms import ReviewerPreferenceForm from zulip_bot.services.registration_bootstrap import ensure_default_preferences_for_user from core.services.github_oauth import GitHubOAuthClient, GitHubOAuthError -from zulip_bot.services.close_pr_execution import ClosePRError, fetch_pr_details_for_form, close_pull_request +from zulip_bot.services.close_pr_execution import ClosePRError, fetch_pr_details_for_form, close_pull_request, post_pr_comment +from zulip_bot.services.close_pr_presets import load_close_pr_presets from zulip_bot.services.close_pr_links import ( ClosePRTokenExpired, ClosePRTokenInvalid, @@ -390,10 +391,28 @@ def close_pr_form(request: HttpRequest, token: str) -> HttpResponse: "success": False, "preflight_only": False, "close_error": None, + "close_message": "", + "presets": load_close_pr_presets(), } if request.method == "POST": + close_message = request.POST.get("close_message", "").strip() + context["close_message"] = close_message + if mutations_enabled: + if close_message: + try: + post_pr_comment( + owner=claims.pr_owner, + repo=claims.pr_repo, + number=claims.pr_number, + body=close_message, + ) + except ClosePRError as exc: + context["close_error"] = f"Could not post comment: {exc.message}" + response = TemplateResponse(request, "zulip_bot/close_pr_form.html", context, status=200) + response["Cache-Control"] = "no-store" + return response try: close_pull_request(owner=claims.pr_owner, repo=claims.pr_repo, number=claims.pr_number) except ClosePRError as exc: From 466ca68245fa2a73a9e8eccab77dfab78bbe105e Mon Sep 17 00:00:00 2001 From: Bryan Gin-ge Chen Date: Fri, 17 Apr 2026 08:07:07 -0400 Subject: [PATCH 16/24] feat(zulip_bot): polish close-pr form with PR details, links, and expiry countdown - Show PR author, opened/updated dates, labels (colored chips), and body preview in a compact info card above the confirmation form. - Link all owner/repo#number occurrences to the GitHub PR URL. - Replace static "expires at" text with a live countdown that disables the submit button when the token expires, matching prefs-form behavior. Shared getExpiryState/formatRemaining logic imported from prefs_form.js. - Style close_pr_invalid.html with shared_pages.css for consistency. - Add close_pr_form.js (module) and move preset-button logic out of the inline + diff --git a/qb_site/templates/zulip_bot/close_pr_invalid.html b/qb_site/templates/zulip_bot/close_pr_invalid.html index da5db2fc..f4137087 100644 --- a/qb_site/templates/zulip_bot/close_pr_invalid.html +++ b/qb_site/templates/zulip_bot/close_pr_invalid.html @@ -1,16 +1,25 @@ +{% load static %} Queueboard — close PR link unavailable + + -

Close PR link unavailable

- {% if reason == "expired" %} -

This link has expired. Send close-pr <pr-url> to the bot again for a fresh link.

- {% else %} -

This link is invalid. Send close-pr <pr-url> to the bot for a fresh link.

- {% endif %} +
+
+

Close PR link unavailable

+
+
+ {% if reason == "expired" %} +

This link has expired. Send close-pr <pr-url> to the bot again for a fresh link.

+ {% else %} +

This link is invalid. Send close-pr <pr-url> to the bot for a fresh link.

+ {% endif %} +
+
diff --git a/qb_site/zulip_bot/frontend/tests/close_pr_form.test.js b/qb_site/zulip_bot/frontend/tests/close_pr_form.test.js new file mode 100644 index 00000000..9a854b80 --- /dev/null +++ b/qb_site/zulip_bot/frontend/tests/close_pr_form.test.js @@ -0,0 +1,46 @@ +import { describe, expect, it } from "vitest"; + +import { mountClosePrForm } from "../../static/zulip_bot/close_pr_form.js"; + +describe("mountClosePrForm", () => { + function buildDom(expUnix) { + document.body.innerHTML = ` +
+ + + +
+
+ + +
+ `; + } + + it("disables submit when link is already expired", () => { + buildDom(1); + const unmount = mountClosePrForm(document); + const submit = document.getElementById("close-pr-submit"); + const text = document.getElementById("close-pr-countdown-text"); + expect(submit.disabled).toBe(true); + expect(text.textContent).toContain("expired"); + unmount(); + }); + + it("enables submit when link is not yet expired", () => { + buildDom(Math.floor(Date.now() / 1000) + 3600); + const unmount = mountClosePrForm(document); + const submit = document.getElementById("close-pr-submit"); + expect(submit.disabled).toBe(false); + unmount(); + }); + + it("loads preset text into textarea on preset button click", () => { + buildDom(Math.floor(Date.now() / 1000) + 3600); + const unmount = mountClosePrForm(document); + document.querySelector(".preset-btn").click(); + const textarea = document.getElementById("close_message"); + expect(textarea.value).toBe("preset text"); + unmount(); + }); +}); diff --git a/qb_site/zulip_bot/services/close_pr_execution.py b/qb_site/zulip_bot/services/close_pr_execution.py index e7db51a5..5cddd40d 100644 --- a/qb_site/zulip_bot/services/close_pr_execution.py +++ b/qb_site/zulip_bot/services/close_pr_execution.py @@ -37,6 +37,10 @@ class LivePRDetails: title: str is_open: bool author_login: str + body: str | None = None + opened_at: str | None = None + updated_at: str | None = None + labels: tuple[tuple[str, str], ...] = () @dataclass(frozen=True) @@ -238,7 +242,22 @@ def _fetch_pr_details(*, token: str, owner: str, repo: str, number: int) -> Live merged_at = payload.get("merged_at") is_open = state == "open" and not merged_at author_login = str((payload.get("user") or {}).get("login") or "").strip() - return LivePRDetails(title=title, is_open=is_open, author_login=author_login) + body = str(payload.get("body") or "").strip() or None + opened_at = str(payload.get("created_at") or "").strip() or None + updated_at = str(payload.get("updated_at") or "").strip() or None + raw_labels = payload.get("labels") if isinstance(payload.get("labels"), list) else [] + labels: tuple[tuple[str, str], ...] = tuple( + (str(lbl.get("name", "")), str(lbl.get("color", ""))) for lbl in raw_labels if isinstance(lbl, dict) and lbl.get("name") + ) + return LivePRDetails( + title=title, + is_open=is_open, + author_login=author_login, + body=body, + opened_at=opened_at, + updated_at=updated_at, + labels=labels, + ) def _fetch_collaborator_permission(*, token: str, owner: str, repo: str, github_login: str) -> str: diff --git a/qb_site/zulip_bot/static/zulip_bot/close_pr_form.css b/qb_site/zulip_bot/static/zulip_bot/close_pr_form.css index af1a96c1..0bdfed9a 100644 --- a/qb_site/zulip_bot/static/zulip_bot/close_pr_form.css +++ b/qb_site/zulip_bot/static/zulip_bot/close_pr_form.css @@ -1,3 +1,88 @@ +.meta-link { + color: #d8f3f2; + text-decoration: none; +} + +.meta-link:hover { + text-decoration: underline; +} + +.pr-info { + margin-bottom: 16px; +} + +.pr-meta-row { + display: flex; + flex-wrap: wrap; + gap: 4px 12px; + font-size: 0.88rem; + color: var(--muted); + margin-bottom: 8px; +} + +.pr-meta-row a { + color: var(--accent); + text-decoration: none; +} + +.pr-meta-row a:hover { + text-decoration: underline; +} + +.pr-labels { + display: flex; + flex-wrap: wrap; + gap: 6px; + margin-bottom: 10px; +} + +.pr-label { + display: inline-block; + padding: 2px 8px; + border-radius: 12px; + font-size: 0.8rem; + font-weight: 500; + background: var(--border); + color: var(--ink); + border: 1px solid transparent; +} + +.pr-body { + white-space: pre-wrap; + overflow-y: auto; + max-height: 180px; + font-size: 0.88rem; + color: var(--muted); + margin: 0; + line-height: 1.5; + padding: 8px 10px; + background: var(--bg); + border-radius: 6px; + border: 1px solid var(--border); +} + +.countdown { + display: flex; + flex-wrap: wrap; + gap: 8px 14px; + align-items: center; + background: var(--card); + border: 1px solid var(--border); + border-radius: 10px; + padding: 11px 12px; + margin-bottom: 16px; + color: var(--muted); + font-size: 0.95rem; +} + +.countdown strong { + color: var(--ink); +} + +.card a:not(.cta) { + color: var(--accent); +} + .message-field { margin-top: 16px; } diff --git a/qb_site/zulip_bot/static/zulip_bot/close_pr_form.js b/qb_site/zulip_bot/static/zulip_bot/close_pr_form.js new file mode 100644 index 00000000..77271f5b --- /dev/null +++ b/qb_site/zulip_bot/static/zulip_bot/close_pr_form.js @@ -0,0 +1,52 @@ +import { formatRemaining, getExpiryState } from "./prefs_form.js"; + +export function mountClosePrForm(root = document) { + const form = root.getElementById("close-pr-form"); + const submitButton = root.getElementById("close-pr-submit"); + const countdownText = root.getElementById("close-pr-countdown-text"); + const countdownLabel = root.getElementById("close-pr-countdown-label"); + if (!form || !submitButton || !countdownText || !countdownLabel) { + return () => undefined; + } + + const expUnix = Number(form.dataset.expUnix || "0"); + + const update = () => { + const state = getExpiryState(expUnix); + if (state.expired) { + countdownLabel.textContent = "Expired:"; + countdownText.textContent = "This link has expired. Request a fresh one in Zulip."; + submitButton.disabled = true; + } else { + countdownLabel.textContent = "Expires in:"; + countdownText.textContent = formatRemaining(state.remainingMs); + submitButton.disabled = false; + } + }; + update(); + const intervalId = window.setInterval(update, 1000); + + for (const btn of root.querySelectorAll(".preset-btn")) { + btn.addEventListener("click", () => { + const textarea = root.getElementById("close_message"); + if (textarea) { + textarea.value = btn.getAttribute("data-body") || ""; + } + }); + } + + form.addEventListener("submit", (event) => { + update(); + if (submitButton.disabled) { + event.preventDefault(); + } + }); + + return () => window.clearInterval(intervalId); +} + +if (typeof window !== "undefined") { + window.addEventListener("DOMContentLoaded", () => { + mountClosePrForm(document); + }); +} diff --git a/qb_site/zulip_bot/views.py b/qb_site/zulip_bot/views.py index 58511cf2..753b1336 100644 --- a/qb_site/zulip_bot/views.py +++ b/qb_site/zulip_bot/views.py @@ -379,14 +379,22 @@ def close_pr_form(request: HttpRequest, token: str) -> HttpResponse: pr_title = pr_details.title if pr_details else None pr_is_open = pr_details.is_open if pr_details else True # assume open if fetch fails; POST will verify + pr_url = f"https://github.com/{claims.pr_owner}/{claims.pr_repo}/pull/{claims.pr_number}" mutations_enabled = _close_pr_mutations_enabled() context: dict = { "pr_owner": claims.pr_owner, "pr_repo": claims.pr_repo, "pr_number": claims.pr_number, + "pr_url": pr_url, "pr_title": pr_title, "pr_is_open": pr_is_open, + "pr_author_login": pr_details.author_login if pr_details else None, + "pr_body": pr_details.body if pr_details else None, + "pr_opened_at": _fmt_date(pr_details.opened_at) if pr_details else None, + "pr_updated_at": _fmt_date(pr_details.updated_at) if pr_details else None, + "pr_labels": _label_badge_context(pr_details.labels) if pr_details else [], "expires_at_iso": expires_at_utc.isoformat(), + "expires_at_unix": claims.exp, "mutations_disabled": not mutations_enabled, "success": False, "preflight_only": False, @@ -430,6 +438,39 @@ def close_pr_form(request: HttpRequest, token: str) -> HttpResponse: return response +def _fmt_date(iso: str | None) -> str | None: + if not iso: + return None + try: + dt = datetime.fromisoformat(iso.replace("Z", "+00:00")) + return f"{dt.strftime('%b')} {dt.day}, {dt.year}" + except ValueError: + return iso + + +def _label_text_color(hex6: str) -> str: + try: + r, g, b = int(hex6[0:2], 16), int(hex6[2:4], 16), int(hex6[4:6], 16) + return "#0f172a" if 0.299 * r + 0.587 * g + 0.114 * b > 160 else "#f8fafc" + except (ValueError, IndexError): + return "#0f172a" + + +def _label_badge_context(labels: tuple) -> list[dict]: + result = [] + for name, color in labels: + if not name: + continue + result.append( + { + "name": name, + "bg": f"#{color}" if color else None, + "text": _label_text_color(color) if color else "#0f172a", + } + ) + return result + + def _close_pr_mutations_enabled() -> bool: value = str(getattr(settings, "ZULIP_CLOSE_PR_MUTATIONS_ENABLED", "")).strip().lower() return value in {"1", "true", "yes", "on"} From fb28bfe2045d1ea869a8aeddc715aff89da894a2 Mon Sep 17 00:00:00 2001 From: Bryan Gin-ge Chen Date: Fri, 17 Apr 2026 08:35:41 -0400 Subject: [PATCH 17/24] feat(zulip_bot): improve close-pr form layout, timestamps, and full body display - Consolidate layout: PR title moves from hero into the PR card so all identity info (title, author, dates, labels, body) lives in one place; hero now shows only the page heading and owner/repo#number. - Remove attribution paragraph from the action card (PR card above makes the target obvious); keep only the bot-attribution warning. - Show full PR body with no character truncation or max-height cap. - Always show updated_at (dropped the same-day equality suppression). - Pass raw ISO strings (pr_opened_at_iso, pr_updated_at_iso) to template; JS formats