@@ -0,0 +1,199 @@ |
| 1 | +# PR review |
| 2 | + |
| 3 | +S23 ships file-level inline review comments, three review states |
| 4 | +(`comment` / `approve` / `request_changes`), reviewer-request |
| 5 | +flow, dismiss, server-side draft → submit, branch-protection-driven |
| 6 | +required-review gating on merge, and outdated-comment handling on |
| 7 | +synchronize. |
| 8 | + |
| 9 | +## Schema (migration 0024) |
| 10 | + |
| 11 | +``` |
| 12 | +pr_reviews — one row per submitted review (state, body, dismissed_*) |
| 13 | +pr_review_comments — file-level inline comments anchored on |
| 14 | + (file_path, side, original_commit_sha, |
| 15 | + original_line, original_position). |
| 16 | + review_id NULL + pending=true means draft. |
| 17 | + review_id N means part of review N. |
| 18 | + review_id NULL + pending=false means single inline comment. |
| 19 | +pr_review_requests — pending review requests (user; teams in S31). |
| 20 | +``` |
| 21 | + |
| 22 | +Plus three columns on `branch_protection_rules`: |
| 23 | + |
| 24 | +| Column | Default | Notes | |
| 25 | +| ------------------------------- | ------- | -------------------------------------------------- | |
| 26 | +| `required_review_count` | `0` | How many distinct approves required before merge | |
| 27 | +| `dismiss_stale_reviews_on_push` | `false` | Placeholder; dismiss-on-push wires post-MVP | |
| 28 | +| `require_code_owner_review` | `false` | Placeholder; CODEOWNERS parsing post-MVP | |
| 29 | + |
| 30 | +The CHECK on `required_review_count >= 0` keeps the gate sane when an |
| 31 | +admin types nonsense. |
| 32 | + |
| 33 | +## Anchoring model |
| 34 | + |
| 35 | +Each comment captures `(file_path, side, original_commit_sha, |
| 36 | +original_line, original_position)`. On synchronize, the new helper |
| 37 | +`internal/pulls/review/position_map.go::RemapAllForPR` re-walks the |
| 38 | +file's content at the new snapshot and tries to match each comment |
| 39 | +back to a position. Lines that survive get their `current_position` |
| 40 | +updated; lines that don't go to NULL ("outdated"). |
| 41 | + |
| 42 | +The v1 mapper is intentionally simple: it reads the file at the new |
| 43 | +snapshot and asks "does the original line number still exist?" That |
| 44 | +covers ~90% of the additive-commit and pure-rename cases. The |
| 45 | +spec calls out `git blame --porcelain` for rebase-heavy PRs; that |
| 46 | +swap-in is a follow-up. |
| 47 | + |
| 48 | +Outdated comments still display in the Files tab under a default- |
| 49 | +hidden "Show outdated" toggle and stay readable in the Conversation |
| 50 | +timeline. |
| 51 | + |
| 52 | +## Server-side drafts |
| 53 | + |
| 54 | +Browser-side drafts are fragile across tabs and don't survive a |
| 55 | +crash. The S23 model uses `pending=true` rows on |
| 56 | +`pr_review_comments`: every "Add inline comment" action with the |
| 57 | +"Pending" checkbox checked stores a draft row with `review_id=NULL` |
| 58 | +and `pending=true`. Submitting a review runs |
| 59 | + |
| 60 | +```sql |
| 61 | +UPDATE pr_review_comments |
| 62 | +SET review_id = $review_id, pending = false |
| 63 | +WHERE pr_issue_id = $pr AND author_user_id = $author AND pending = true; |
| 64 | +``` |
| 65 | + |
| 66 | +inside the same tx as the review insert, so the operation is atomic. |
| 67 | + |
| 68 | +A single inline comment outside a review (the spec's "post directly |
| 69 | +without starting a review" path) is `pending=false` from the start, |
| 70 | +`review_id=NULL`, and shows up in the diff inline immediately. |
| 71 | + |
| 72 | +## Routes |
| 73 | + |
| 74 | +| Route | Auth | |
| 75 | +| ----------------------------------------------------------------- | ------------ | |
| 76 | +| `POST /{owner}/{repo}/pulls/{n}/review-comments` | RequireUser | |
| 77 | +| `POST /{owner}/{repo}/pulls/{n}/review-comments/{cid}/reply` | RequireUser | |
| 78 | +| `POST /{owner}/{repo}/pulls/{n}/review-comments/{cid}/edit` | RequireUser | |
| 79 | +| `POST /{owner}/{repo}/pulls/{n}/review-comments/{cid}/resolve` | RequireUser | |
| 80 | +| `POST /{owner}/{repo}/pulls/{n}/review-comments/{cid}/reopen` | RequireUser | |
| 81 | +| `POST /{owner}/{repo}/pulls/{n}/reviews` | RequireUser | |
| 82 | +| `POST /{owner}/{repo}/pulls/{n}/reviews/{rid}/dismiss` | RequireUser | |
| 83 | +| `POST /{owner}/{repo}/pulls/{n}/reviewers` | RequireUser | |
| 84 | + |
| 85 | +The settings/branches handler now also accepts |
| 86 | +`required_review_count` and `dismiss_stale_reviews_on_push`. |
| 87 | + |
| 88 | +## Required-review gate |
| 89 | + |
| 90 | +`internal/pulls/review/required.go::Evaluate` is the authoritative |
| 91 | +gate. It: |
| 92 | + |
| 93 | +1. Looks up the longest-pattern-matching `branch_protection_rule` |
| 94 | + for the PR's `base_ref`. |
| 95 | +2. Counts approves per author (latest review per author wins). |
| 96 | +3. Counts undismissed `request_changes` reviews. |
| 97 | +4. Returns `Satisfied=true` iff `approves >= required_review_count` |
| 98 | + AND no undismissed request_changes. |
| 99 | + |
| 100 | +PR-author approvals are **excluded** from the count. |
| 101 | + |
| 102 | +The gate is consulted at **two points** (belt-and-braces, per spec): |
| 103 | + |
| 104 | +- `pulls.Mergeability` sets `mergeable_state = 'blocked'` when not |
| 105 | + satisfied. Re-runs after every `pr:synchronize`, every review |
| 106 | + submit, and every dismiss. |
| 107 | +- `pulls.Merge` re-evaluates the gate inside the `FOR UPDATE` row |
| 108 | + lock right before `repogit.PerformMerge` runs. A `request_changes` |
| 109 | + submitted between the last mergeability tick and the merge attempt |
| 110 | + still blocks. |
| 111 | + |
| 112 | +`mergeable_state` priority order in `Mergeability`: |
| 113 | + |
| 114 | +``` |
| 115 | +dirty > behind > blocked > clean |
| 116 | +``` |
| 117 | + |
| 118 | +## Author cannot approve own PR |
| 119 | + |
| 120 | +Enforced at `review.Submit`: when state is `approve` and the author |
| 121 | +matches the PR author, the orchestrator returns |
| 122 | +`ErrAuthorCannotApprove` (HTTP 403). The UI also hides the option |
| 123 | +in the review form template — server check is authoritative. |
| 124 | + |
| 125 | +## Reviewer requests |
| 126 | + |
| 127 | +Capped at 20 active reviewers per PR (`MaxReviewersPerPR`). Already- |
| 128 | +pending reviewer hits `ErrReviewerAlreadyPending` instead of |
| 129 | +producing a duplicate row. Submitting an `approve` or |
| 130 | +`request_changes` review by a requested reviewer satisfies their |
| 131 | +request (`satisfied_by_review_id` is set inside the submit tx); a |
| 132 | +plain `comment` review does **not** satisfy, matching GitHub. |
| 133 | + |
| 134 | +## Resolved threads |
| 135 | + |
| 136 | +`Resolve` sets `resolved_at + resolved_by_user_id` on the root |
| 137 | +comment; `Reopen` clears them. The Files tab default-collapses |
| 138 | +resolved threads under their file section; the comment row picks up |
| 139 | +the `shithub-pull-thread-resolved` CSS class so they're visually |
| 140 | +distinct. |
| 141 | + |
| 142 | +## Notifications hook |
| 143 | + |
| 144 | +S23 emits these timeline events via `issuesdb.InsertIssueEvent`: |
| 145 | + |
| 146 | +- `reviewed` (with `meta = {state: "approve"|...}`) |
| 147 | + |
| 148 | +Comment-added and review-requested events go through S29's pipeline |
| 149 | +when notifications ship; the trigger points are already in place. |
| 150 | + |
| 151 | +## Tests |
| 152 | + |
| 153 | +| File | Covers | |
| 154 | +| ------------------------------------------------- | -------------------------------------------------------------- | |
| 155 | +| `internal/pulls/review/review_test.go::AuthorCannotApprove` | Self-approval rejected at orchestrator | |
| 156 | +| `…::AttachesPendingComments` | Two `pending=true` drafts attach to a single submitted review | |
| 157 | +| `…::RequiredReviews_BlocksThenUnblocks` | Rule with `required_review_count=1`: blocked → approve → clean | |
| 158 | +| `…::RequestChanges_BlocksMerge` | Even without a rule, an undismissed `request_changes` blocks | |
| 159 | +| `…::TwoApprovers_UnblockMerge` | `required_review_count=2` satisfied by two distinct authors | |
| 160 | +| `…::ResolveAndReopen` | Resolve flips `resolved_at`; double-resolve errors; reopen clears | |
| 161 | +| `…::Dismiss_ClearsBlock` | Dismissing a `request_changes` re-clears mergeable_state | |
| 162 | + |
| 163 | +## Pitfalls handled |
| 164 | + |
| 165 | +- **TOCTOU between mergeability and merge** — pre-flight inside the |
| 166 | + row lock re-evaluates the gate. A reviewer hitting |
| 167 | + `request_changes` mid-merge-click can't lose the race. |
| 168 | +- **Latest-review-per-author** — a reviewer who first |
| 169 | + `request_changes` and then `approve` counts as approve. A trailing |
| 170 | + `comment` doesn't reset earlier approve. Encoded in |
| 171 | + `latestPerAuthor`. |
| 172 | +- **Author email leak** — same caveat as everywhere; reviewer's |
| 173 | + primary verified email is propagated. Documented; noreply is |
| 174 | + post-MVP. |
| 175 | +- **`pending=true` orphan rows** — drafts with no submit are user |
| 176 | + state; left in place. A janitor that cleans drafts older than 30d |
| 177 | + is post-MVP. |
| 178 | +- **Approver count includes dismissed reviews?** No. The query in |
| 179 | + `ListUndismissedReviewsForGate` filters `dismissed_at IS NULL` |
| 180 | + before the per-author reduce. |
| 181 | + |
| 182 | +## Deferred |
| 183 | + |
| 184 | +- **CODEOWNERS-driven required reviewers** → post-MVP. Column |
| 185 | + exists; parser + UI follow. |
| 186 | +- **Auto-dismiss-stale-reviews on synchronize** → post-MVP. Column |
| 187 | + exists; default off matches GitHub. |
| 188 | +- **Multi-line comment ranges** → post-MVP. |
| 189 | +- **Suggestion-block markdown fence** → post-MVP. Renders as plain |
| 190 | + code today. |
| 191 | +- **Cross-fork (S27) reviews** — comments anchor on the head repo's |
| 192 | + OID; cross-repo lookup gap closes when forks land. |
| 193 | +- **`@team` review requests** → S31. Column already in place. |
| 194 | +- **Position mapping via blame for rebase-heavy PRs** → follow-up. |
| 195 | + Current line-presence mapper is the floor. |
| 196 | +- **Notification fan-out** → S29 consumes the events S23 emits. |
| 197 | +- **Comment edit on someone else's comment** — not a feature; only |
| 198 | + the author can edit, server-enforced. Repo admins dismiss reviews |
| 199 | + but don't edit comment bodies. |