michal/tit

Browse tree · Show commit · Download archive

Diff

ff182b9ee47dd2a3a4bfd3a5

README.md

Mode 100644100644; object e7c73d347841511acedc90aa

@@ -404,3 +404,18 @@
 work limits, and Web output. Read the
 [pull-request comparison architectural decision record](docs/adr/0023-pull-request-comparison.md)
 for the computation, limit, and cache contracts.
+
+## Milestone 5.3 gate
+
+Run the pull-request review gate:
+
+```text
+./scripts/check-m5-3
+```
+
+This command tests general comments, line comments, approvals, change requests,
+immutable byte-path anchors, outdated display, repository permission, atomic
+events, the chronological timeline, no-JavaScript Web forms, and historical
+schema migration. Read the
+[pull-request review architectural decision record](docs/adr/0024-pull-request-review.md)
+for the anchor, permission, event, and outdated-state contracts.

docs/adr/0024-pull-request-review.md

Mode 100644; object 4f9b5e24ba5b

@@ -1,0 +1,85 @@
+# Architectural decision record 0024: Pull-request review
+
+Status: Accepted
+
+Date: 2026-07-23
+
+## Context
+
+A review must keep its original meaning after a branch update. General comments
+need Markdown content. A line comment also needs an exact Git anchor. Approvals
+and change requests must remain in the history. Each metadata mutation must
+have one repository event in the same SQLite transaction.
+
+## Decision
+
+Store each review action as an append-only `pull_request_review` record with a
+random permanent ID. Every action refers to one immutable pull-request
+revision and one active account. Use these action kinds:
+
+- `comment` stores a general comment.
+- `line-comment` stores a line comment.
+- `approved` stores an approval.
+- `changes-requested` stores a change request.
+
+A line comment stores the exact commit ID, path bytes, side, and one-based line
+number. The side is `base` or `head`. The commit ID must equal that side of the
+selected revision. The path must be a changed, non-binary file in that
+revision. The selected side must contain the path and line. Perform the Git
+checks with the bounded comparison and blob reader. Recheck the revision and
+commit relation in the write transaction.
+
+Store raw Markdown with a maximum size of 256 KiB. Use the common safe Markdown
+renderer for Web output. A comment and a change request require nonempty text.
+An approval can have empty text.
+
+An active account that can read the repository can review. Thus, a repository
+reader can comment, approve, or request changes. Apply the current repository
+policy again in the write transaction. Do not accept review actions on a pull
+request that is not open.
+
+Append one specific repository event for each review action. The event payload
+contains the review ID, pull-request number, revision number, body, and optional
+line anchor. Repository event sequence gives one chronological timeline for
+creation, revisions, and review actions.
+
+## Outdated comments
+
+A line comment is current only while its revision is the current pull-request
+revision. Show `Outdated` after the pull request gets a later revision. Keep the
+original commit, path, side, line, and Markdown visible. General comments,
+approvals, and change requests stay in the timeline and do not get the
+`Outdated` label.
+
+This rule is conservative. It does not move an anchor to a similar line in a
+later commit. Automatic line mapping can change the meaning of a comment, so it
+needs a separate design and tests before use.
+
+## Failure and threat cases
+
+Reject an unknown revision, invalid kind, invalid or oversized body, missing
+path, binary file, unchanged path, absent side, absent line, line outside the
+file, and commit mismatch. Store path data as bytes so a Git path does not need
+UTF-8. The no-JavaScript Web form sends the path as hexadecimal text.
+
+If review insertion or event insertion fails, roll back both records. A later
+permission change can hide existing review data because reads use the current
+repository policy.
+
+## Evidence
+
+The integration test uses SHA-1 and SHA-256 repositories. A reader adds a
+general comment, approval, and line comment. An owner requests changes. The
+test checks the exact line anchor, rejects a missing line, records a later
+revision, verifies event order, and denies the reader after private access is
+removed.
+
+The Web integration test submits review actions without JavaScript, renders
+safe Markdown, shows the review events, and marks a line comment as outdated
+after a later revision. The migration test upgrades every committed schema.
+
+## Consequences
+
+Review history is durable and reproducible from SQLite and immutable Git IDs.
+The service does not silently move line comments. A later branch-rule milestone
+can consume approval and change-request records without changing their history.

scripts/check-m5-3

Mode 100755; object 305f497f83a6

@@ -1,0 +1,7 @@
+#!/bin/sh
+set -eu
+
+./scripts/check
+cargo test --locked --release --test pull_requests
+cargo test --locked --release --test public_routes runs_the_complete_issue_workflow_without_javascript
+cargo test --locked --release --test sqlite migrates_each_committed_historical_fixture

src/feed.rs

Mode 100644100644; object fd8f4f6926b46083ae4415b4

@@ -296,6 +296,10 @@
         "issue-unassigned" => issue_value_title(event, "unassigned", "assignee"),
         "pull-request-created" => pull_request_title(event, "opened"),
         "pull-request-revised" => pull_request_title(event, "revised"),
+        "pull-request-commented" => pull_request_title(event, "commented on"),
+        "pull-request-line-commented" => pull_request_title(event, "commented on a line in"),
+        "pull-request-approved" => pull_request_title(event, "approved"),
+        "pull-request-changes-requested" => pull_request_title(event, "requested changes on"),
         _ => "Repository event".to_owned(),
     }
 }

src/http/pull_requests.rs

Mode 100644100644; object acaacc3275be7caddf3b99d2

@@ -32,6 +32,10 @@
             "/{owner}/{repository}/pulls/{number}/revisions",
             post(revise_pull_request),
         )
+        .route(
+            "/{owner}/{repository}/pulls/{number}/reviews",
+            post(create_review),
+        )
         .layer(DefaultBodyLimit::max(MAX_PULL_REQUEST_BYTES))
 }
 
@@ -116,14 +120,122 @@
                     pull_request,
                     body_html: markdown::render(&pull_request.body),
                     revisions: &detail.revisions,
+                    reviews: detail
+                        .reviews
+                        .iter()
+                        .map(|review| ReviewView {
+                            id: &review.id,
+                            revision: review.revision,
+                            author: &review.author,
+                            kind: &review.kind,
+                            body_html: markdown::render(&review.body),
+                            has_body: !review.body.is_empty(),
+                            commit_object_id: review.commit_object_id.as_deref().unwrap_or(""),
+                            path: review
+                                .path
+                                .as_deref()
+                                .map(|path| String::from_utf8_lossy(path).into_owned())
+                                .unwrap_or_default(),
+                            side: review.side.as_deref().unwrap_or(""),
+                            line: review
+                                .line
+                                .map_or_else(String::new, |line| line.to_string()),
+                            outdated: review.kind == "line-comment"
+                                && review.revision != pull_request_revision(detail),
+                            created_at: review.created_at,
+                        })
+                        .collect(),
+                    timeline: detail
+                        .timeline
+                        .iter()
+                        .map(|event| TimelineView {
+                            sequence: event.sequence,
+                            kind: &event.kind,
+                            actor: &event.actor,
+                            created_at: event.created_at,
+                        })
+                        .collect(),
                     selected_revision: result.revision.number,
                     comparison: ComparisonView::from(&result.comparison),
                     csrf: &csrf,
                     can_revise: detail.can_revise && !csrf.is_empty(),
+                    can_review: detail.can_review
+                        && !csrf.is_empty()
+                        && result.revision.number == pull_request_revision(detail),
                 },
             )
         }
         Err(error) => read_error(error, &request_id.0),
+    }
+}
+
+async fn create_review(
+    State(state): State<WebState>,
+    Extension(request_id): Extension<RequestId>,
+    Path(path): Path<PullRequestPath>,
+    headers: HeaderMap,
+    body: Bytes,
+) -> Response {
+    let fields = match parse_named_form(
+        &headers,
+        &body,
+        &[
+            "csrf", "revision", "kind", "body", "path-hex", "side", "line",
+        ],
+    ) {
+        Ok(fields) => fields,
+        Err(()) => return bad_request(&request_id.0),
+    };
+    let actor =
+        match authenticate_mutation(state.clone(), &headers, &fields[0], &request_id.0).await {
+            Ok(actor) => actor,
+            Err(response) => return response,
+        };
+    let revision = match fields[1].parse::<i64>() {
+        Ok(revision) => revision,
+        Err(_) => return bad_request(&request_id.0),
+    };
+    let path_bytes = if fields[4].is_empty() {
+        None
+    } else {
+        match decode_hex(&fields[4]) {
+            Some(path) => Some(path),
+            None => return bad_request(&request_id.0),
+        }
+    };
+    let side = (!fields[5].is_empty()).then(|| fields[5].clone());
+    let line = if fields[6].is_empty() {
+        None
+    } else {
+        match fields[6].parse::<i64>() {
+            Ok(line) => Some(line),
+            Err(_) => return bad_request(&request_id.0),
+        }
+    };
+    let Some(service) = state.pull_requests.clone() else {
+        return internal(&request_id.0);
+    };
+    let owner = path.owner.clone();
+    let repository = path.repository.clone();
+    let number = path.number;
+    let result = job(state, move || {
+        service.review(
+            &owner,
+            &repository,
+            number,
+            revision,
+            &actor,
+            &fields[2],
+            &fields[3],
+            path_bytes.as_deref(),
+            side.as_deref(),
+            line,
+        )
+    })
+    .await;
+    match result {
+        Ok(_) => redirect(&path.owner, &path.repository, number),
+        Err(error) => mutation_error(error, &request_id.0),
     }
 }
 
@@ -245,6 +357,12 @@
             "Pull-request error",
             "You cannot change pull requests in this repository.",
         ),
+        PullRequestError::Store(StoreError::PullRequestState) => render_error(
+            StatusCode::CONFLICT,
+            request_id,
+            "Pull-request conflict",
+            "The pull request is not open.",
+        ),
         PullRequestError::Store(
             StoreError::RepositoryNotFound(_, _)
             | StoreError::PullRequestNotFound(_, _, _)
@@ -255,11 +373,38 @@
         | PullRequestError::Branch
         | PullRequestError::Number
         | PullRequestError::Unchanged
+        | PullRequestError::Revision
+        | PullRequestError::ReviewKind
+        | PullRequestError::ReviewBody
+        | PullRequestError::ReviewAnchor
+        | PullRequestError::Store(StoreError::PullRequestRevisionNotFound)
+        | PullRequestError::Store(StoreError::PullRequestReviewAnchor)
         | PullRequestError::Git(crate::git::repository::GitRepositoryError::MissingReference(_)) => {
             bad_request(request_id)
         }
         _ => internal(request_id),
     }
+}
+
+fn pull_request_revision(detail: &crate::store::PullRequestDetail) -> i64 {
+    detail
+        .revisions
+        .last()
+        .map_or(0, |revision| revision.number)
+}
+
+fn decode_hex(value: &str) -> Option<Vec<u8>> {
+    if value.is_empty() || !value.len().is_multiple_of(2) {
+        return None;
+    }
+    value
+        .as_bytes()
+        .chunks_exact(2)
+        .map(|pair| {
+            let pair = std::str::from_utf8(pair).ok()?;
+            u8::from_str_radix(pair, 16).ok()
+        })
+        .collect()
 }
 
 fn bad_request(request_id: &str) -> Response {
@@ -338,10 +483,35 @@
     pull_request: &'a crate::store::PullRequestRecord,
     body_html: RenderedMarkdown,
     revisions: &'a [crate::store::PullRequestRevisionRecord],
+    reviews: Vec<ReviewView<'a>>,
+    timeline: Vec<TimelineView<'a>>,
     selected_revision: i64,
     comparison: ComparisonView,
     csrf: &'a str,
     can_revise: bool,
+    can_review: bool,
+}
+
+struct ReviewView<'a> {
+    id: &'a str,
+    revision: i64,
+    author: &'a str,
+    kind: &'a str,
+    body_html: RenderedMarkdown,
+    has_body: bool,
+    commit_object_id: &'a str,
+    path: String,
+    side: &'a str,
+    line: String,
+    outdated: bool,
+    created_at: i64,
+}
+
+struct TimelineView<'a> {
+    sequence: i64,
+    kind: &'a str,
+    actor: &'a str,
+    created_at: i64,
 }
 
 struct ComparisonView {
@@ -359,7 +529,10 @@
 
 struct DiffView {
     path: String,
+    path_hex: String,
     binary: bool,
+    has_base: bool,
+    has_head: bool,
     hunks: String,
 }
 
@@ -396,10 +569,22 @@
                 .iter()
                 .map(|file| DiffView {
                     path: String::from_utf8_lossy(&file.path).into_owned(),
+                    path_hex: encode_hex(&file.path),
                     binary: file.binary,
+                    has_base: file.old_id.is_some(),
+                    has_head: file.new_id.is_some(),
                     hunks: String::from_utf8_lossy(&file.hunks).into_owned(),
                 })
                 .collect(),
         }
     }
+}
+
+fn encode_hex(bytes: &[u8]) -> String {
+    let mut encoded = String::with_capacity(bytes.len().saturating_mul(2));
+    for byte in bytes {
+        use std::fmt::Write;
+        write!(encoded, "{byte:02x}").expect("a string write cannot fail");
+    }
+    encoded
 }

src/pull_request.rs

Mode 100644100644; object 103ba52412fce1fb41428065

@@ -14,12 +14,13 @@
 };
 use crate::git::repository::{GitRepository, GitRepositoryError};
 use crate::store::{
-    NewPullRequestRefIntent, PullRequestDetail, PullRequestRecord, PullRequestRefIntentRecord,
-    PullRequestRevisionRecord, Store, StoreError,
+    NewPullRequestRefIntent, NewPullRequestReview, PullRequestDetail, PullRequestRecord,
+    PullRequestRefIntentRecord, PullRequestRevisionRecord, Store, StoreError,
 };
 
 pub(crate) const MAX_TITLE_BYTES: usize = 200;
 pub(crate) const MAX_BODY_BYTES: usize = 256 * 1024;
+pub(crate) const MAX_REVIEW_BODY_BYTES: usize = 256 * 1024;
 
 #[derive(Clone)]
 pub(crate) struct PullRequestService {
@@ -237,6 +238,96 @@
     }
 
     #[allow(
+        clippy::too_many_arguments,
+        reason = "a review action includes its repository, revision, content, and optional line anchor"
+    )]
+    pub(crate) fn review(
+        &self,
+        owner: &str,
+        repository: &str,
+        number: i64,
+        revision_number: i64,
+        actor: &str,
+        kind: &str,
+        body: &str,
+        path: Option<&[u8]>,
+        side: Option<&str>,
+        line: Option<i64>,
+    ) -> Result<String, PullRequestError> {
+        validate_context(owner, repository, actor)?;
+        if number < 1 || revision_number < 1 {
+            return Err(PullRequestError::Number);
+        }
+        validate_review(kind, body, path, side, line)?;
+        let _operation = self
+            .operations
+            .lock()
+            .unwrap_or_else(std::sync::PoisonError::into_inner);
+        self.recover_inner()?;
+        let detail =
+            Store::open(&self.database)?.pull_request(owner, repository, number, Some(actor))?;
+        let revision = detail
+            .revisions
+            .iter()
+            .find(|revision| revision.number == revision_number)
+            .ok_or(PullRequestError::Revision)?;
+        let commit_object_id = if kind == "line-comment" {
+            let path = path.ok_or(PullRequestError::ReviewAnchor)?;
+            let side = side.ok_or(PullRequestError::ReviewAnchor)?;
+            let line = line.ok_or(PullRequestError::ReviewAnchor)?;
+            let path_to_repository = self.repository_path(&detail.repository.id)?;
+            let reader = RepositoryReadService::open(&path_to_repository, ReadLimits::default())?;
+            let base = parse_id(&revision.base_object_id)?;
+            let head = parse_id(&revision.head_object_id)?;
+            let comparison = reader.comparison(base, head, &ReadCancellation::default())?;
+            let file = comparison
+                .files
+                .iter()
+                .find(|file| file.path == path)
+                .filter(|file| !file.binary)
+                .ok_or(PullRequestError::ReviewAnchor)?;
+            let (commit, exists) = match side {
+                "base" => (base, file.old_id.is_some()),
+                "head" => (head, file.new_id.is_some()),
+                _ => return Err(PullRequestError::ReviewAnchor),
+            };
+            if !exists {
+                return Err(PullRequestError::ReviewAnchor);
+            }
+            let blob = reader.blob(commit, path, &ReadCancellation::default())?;
+            let line_count = if blob.data.is_empty() {
+                0
+            } else {
+                blob.data.split(|byte| *byte == b'\n').count()
+                    - usize::from(blob.data.ends_with(b"\n"))
+            };
+            let line = usize::try_from(line).map_err(|_| PullRequestError::ReviewAnchor)?;
+            if line == 0 || line > line_count {
+                return Err(PullRequestError::ReviewAnchor);
+            }
+            Some(commit.to_string())
+        } else {
+            None
+        };
+        Store::open(&self.database)?
+            .create_pull_request_review(&NewPullRequestReview {
+                owner,
+                repository,
+                number,
+                revision: revision_number,
+                actor,
+                kind,
+                body,
+                commit_object_id: commit_object_id.as_deref(),
+                path,
+                side,
+                line,
+                created_at: timestamp()?,
+            })
+            .map_err(Into::into)
+    }
+
+    #[allow(
         dead_code,
         reason = "some integration tests compile the service without the Web list route"
     )]
@@ -361,6 +452,48 @@
     Ok(())
 }
 
+fn validate_review(
+    kind: &str,
+    body: &str,
+    path: Option<&[u8]>,
+    side: Option<&str>,
+    line: Option<i64>,
+) -> Result<(), PullRequestError> {
+    if body.len() > MAX_REVIEW_BODY_BYTES
+        || body
+            .chars()
+            .any(|character| character.is_control() && !matches!(character, '\n' | '\r' | '\t'))
+    {
+        return Err(PullRequestError::ReviewBody);
+    }
+    match kind {
+        "comment" | "changes-requested" if !body.trim().is_empty() => {
+            if path.is_none() && side.is_none() && line.is_none() {
+                Ok(())
+            } else {
+                Err(PullRequestError::ReviewAnchor)
+            }
+        }
+        "approved" if path.is_none() && side.is_none() && line.is_none() => Ok(()),
+        "line-comment" if !body.trim().is_empty() => {
+            let path = path.ok_or(PullRequestError::ReviewAnchor)?;
+            if path.is_empty()
+                || path.len() > ReadLimits::default().max_path_bytes
+                || path.contains(&0)
+                || !matches!(side, Some("base" | "head"))
+                || line.is_none_or(|line| line < 1)
+            {
+                return Err(PullRequestError::ReviewAnchor);
+            }
+            Ok(())
+        }
+        "comment" | "line-comment" | "approved" | "changes-requested" => {
+            Err(PullRequestError::ReviewBody)
+        }
+        _ => Err(PullRequestError::ReviewKind),
+    }
+}
+
 fn pull_request_ref(number: i64) -> String {
     format!("refs/pull/{number}/head")
 }
@@ -425,6 +558,12 @@
     Number,
     #[error("pull-request revision does not exist")]
     Revision,
+    #[error("pull-request review kind is not valid")]
+    ReviewKind,
+    #[error("pull-request review body is not valid")]
+    ReviewBody,
+    #[error("pull-request review line anchor is not valid")]
+    ReviewAnchor,
     #[error("pull-request refs have not changed")]
     Unchanged,
     #[error("stored pull-request object ID is not valid")]

src/store/event.rs

Mode 100644100644; object 2fd644f2bd5698afed30bd53

@@ -24,6 +24,10 @@
     IssueUnassigned,
     PullRequestCreated,
     PullRequestRevised,
+    PullRequestCommented,
+    PullRequestLineCommented,
+    PullRequestApproved,
+    PullRequestChangesRequested,
 }
 
 impl EventKind {
@@ -49,7 +53,52 @@
             Self::IssueUnassigned => "issue-unassigned",
             Self::PullRequestCreated => "pull-request-created",
             Self::PullRequestRevised => "pull-request-revised",
+            Self::PullRequestCommented => "pull-request-commented",
+            Self::PullRequestLineCommented => "pull-request-line-commented",
+            Self::PullRequestApproved => "pull-request-approved",
+            Self::PullRequestChangesRequested => "pull-request-changes-requested",
         }
+    }
+}
+
+#[allow(
+    clippy::too_many_arguments,
+    reason = "the event payload stores the complete immutable review anchor"
+)]
+pub(super) fn pull_request_review(
+    kind: EventKind,
+    pull_request_id: &str,
+    number: i64,
+    review_id: &str,
+    revision: i64,
+    body: &str,
+    commit_object_id: Option<&str>,
+    path: Option<&[u8]>,
+    side: Option<&str>,
+    line: Option<i64>,
+) -> VersionedEvent {
+    debug_assert!(matches!(
+        kind,
+        EventKind::PullRequestCommented
+            | EventKind::PullRequestLineCommented
+            | EventKind::PullRequestApproved
+            | EventKind::PullRequestChangesRequested
+    ));
+    VersionedEvent {
+        kind,
+        payload: json!({
+            "version": PAYLOAD_VERSION,
+            "pull_request_id": pull_request_id,
+            "number": number,
+            "review_id": review_id,
+            "revision": revision,
+            "body": body,
+            "commit_object_id": commit_object_id,
+            "path_hex": path.map(encode_hex),
+            "side": side,
+            "line": line,
+        })
+        .to_string(),
     }
 }
 

src/store/migrations/016_pull_request_reviews.sql

Mode 100644; object ae9219c9bd48

@@ -1,0 +1,150 @@
+CREATE TABLE pull_request_review (
+    id TEXT PRIMARY KEY
+        CHECK (
+            length(id) = 32
+            AND id = lower(id)
+            AND id NOT GLOB '*[^0-9a-f]*'
+        ),
+    pull_request_id TEXT NOT NULL
+        REFERENCES pull_request (id) ON DELETE RESTRICT,
+    revision_id TEXT NOT NULL
+        REFERENCES pull_request_revision (id) ON DELETE RESTRICT,
+    author_account_id INTEGER NOT NULL
+        REFERENCES account (id) ON DELETE RESTRICT,
+    kind TEXT NOT NULL
+        CHECK (kind IN ('comment', 'line-comment', 'approved', 'changes-requested')),
+    body TEXT NOT NULL CHECK (length(CAST(body AS BLOB)) <= 262144),
+    commit_object_id TEXT
+        CHECK (
+            commit_object_id IS NULL
+            OR (length(commit_object_id) IN (40, 64)
+                AND commit_object_id = lower(commit_object_id)
+                AND commit_object_id NOT GLOB '*[^0-9a-f]*')
+        ),
+    path BLOB CHECK (path IS NULL OR (length(path) BETWEEN 1 AND 4096 AND instr(path, X'00') = 0)),
+    side TEXT CHECK (side IS NULL OR side IN ('base', 'head')),
+    line INTEGER CHECK (line IS NULL OR line >= 1),
+    created_at INTEGER NOT NULL CHECK (created_at >= 0),
+    CHECK (
+        (kind = 'line-comment'
+            AND length(CAST(body AS BLOB)) >= 1
+            AND commit_object_id IS NOT NULL
+            AND path IS NOT NULL
+            AND side IS NOT NULL
+            AND line IS NOT NULL)
+        OR
+        (kind != 'line-comment'
+            AND commit_object_id IS NULL
+            AND path IS NULL
+            AND side IS NULL
+            AND line IS NULL)
+    ),
+    CHECK (kind NOT IN ('comment', 'changes-requested') OR length(CAST(body AS BLOB)) >= 1)
+) STRICT;
+
+CREATE INDEX pull_request_review_timeline
+ON pull_request_review (pull_request_id, created_at, id);
+
+CREATE INDEX pull_request_review_status
+ON pull_request_review (pull_request_id, author_account_id, created_at DESC, id DESC)
+WHERE kind IN ('approved', 'changes-requested');
+
+DROP INDEX repository_event_feed;
+DROP INDEX repository_event_issue_timeline;
+DROP INDEX repository_event_pull_request_timeline;
+ALTER TABLE repository_event RENAME TO repository_event_v15;
+
+CREATE TABLE repository_event (
+    id INTEGER PRIMARY KEY,
+    event_id TEXT NOT NULL UNIQUE
+        CHECK (
+            length(event_id) = 32
+            AND event_id = lower(event_id)
+            AND event_id NOT GLOB '*[^0-9a-f]*'
+        ),
+    repository_id TEXT NOT NULL
+        REFERENCES repository (id) ON DELETE RESTRICT,
+    sequence INTEGER NOT NULL CHECK (sequence >= 1),
+    source_intent_id TEXT
+        REFERENCES git_operation_intent (id) ON DELETE RESTRICT,
+    source_ordinal INTEGER CHECK (source_ordinal IS NULL OR source_ordinal >= 0),
+    issue_id TEXT REFERENCES issue (id) ON DELETE RESTRICT,
+    pull_request_id TEXT REFERENCES pull_request (id) ON DELETE RESTRICT,
+    kind TEXT NOT NULL
+        CHECK (kind IN (
+            'repository-created', 'repository-imported', 'push',
+            'ref-created', 'ref-updated', 'ref-deleted',
+            'tag-created', 'tag-updated', 'tag-deleted',
+            'issue-created', 'issue-edited', 'issue-commented',
+            'issue-closed', 'issue-reopened',
+            'issue-labeled', 'issue-unlabeled',
+            'issue-assigned', 'issue-unassigned',
+            'pull-request-created', 'pull-request-revised',
+            'pull-request-commented', 'pull-request-line-commented',
+            'pull-request-approved', 'pull-request-changes-requested'
+        )),
+    actor TEXT NOT NULL CHECK (length(actor) BETWEEN 1 AND 256),
+    ref_name BLOB,
+    old_target TEXT,
+    new_target TEXT,
+    payload_version INTEGER NOT NULL CHECK (payload_version = 1),
+    payload TEXT NOT NULL
+        CHECK (
+            length(payload) BETWEEN 1 AND 1048576
+            AND CASE WHEN json_valid(payload) THEN
+                json_type(payload) = 'object'
+                AND coalesce(json_extract(payload, '$.version') = payload_version, 0)
+            ELSE 0 END
+        ),
+    created_at INTEGER NOT NULL CHECK (created_at >= 0),
+    UNIQUE (repository_id, sequence),
+    UNIQUE (source_intent_id, source_ordinal),
+    CHECK (
+        (source_intent_id IS NULL AND source_ordinal IS NULL)
+        OR (source_intent_id IS NOT NULL AND source_ordinal IS NOT NULL)
+    ),
+    CHECK (
+        (kind IN ('repository-created', 'repository-imported', 'push')
+            AND issue_id IS NULL AND pull_request_id IS NULL
+            AND ref_name IS NULL AND old_target IS NULL AND new_target IS NULL)
+        OR
+        ((kind LIKE 'ref-%' OR kind LIKE 'tag-%')
+            AND issue_id IS NULL AND pull_request_id IS NULL
+            AND ref_name IS NOT NULL
+            AND (old_target IS NOT NULL OR new_target IS NOT NULL))
+        OR
+        (kind LIKE 'issue-%'
+            AND issue_id IS NOT NULL AND pull_request_id IS NULL
+            AND source_intent_id IS NULL AND source_ordinal IS NULL
+            AND ref_name IS NULL AND old_target IS NULL AND new_target IS NULL)
+        OR
+        (kind LIKE 'pull-request-%'
+            AND issue_id IS NULL AND pull_request_id IS NOT NULL
+            AND source_intent_id IS NULL AND source_ordinal IS NULL
+            AND ref_name IS NULL AND old_target IS NULL AND new_target IS NULL)
+    )
+) STRICT;
+
+INSERT INTO repository_event
+    (id, event_id, repository_id, sequence, source_intent_id, source_ordinal,
+     issue_id, pull_request_id, kind, actor, ref_name, old_target, new_target,
+     payload_version, payload, created_at)
+SELECT
+    id, event_id, repository_id, sequence, source_intent_id, source_ordinal,
+    issue_id, pull_request_id, kind, actor, ref_name, old_target, new_target,
+    payload_version, payload, created_at
+FROM repository_event_v15
+ORDER BY id;
+
+DROP TABLE repository_event_v15;
+
+CREATE INDEX repository_event_feed
+ON repository_event (repository_id, sequence DESC);
+
+CREATE INDEX repository_event_issue_timeline
+ON repository_event (issue_id, sequence)
+WHERE issue_id IS NOT NULL;
+
+CREATE INDEX repository_event_pull_request_timeline
+ON repository_event (pull_request_id, sequence)
+WHERE pull_request_id IS NOT NULL;

src/store/mod.rs

Mode 100644100644; object 78abc9d772a5988f9fcd3f7c

@@ -12,7 +12,7 @@
 const BUSY_TIMEOUT: Duration = Duration::from_secs(5);
 const BUSY_TIMEOUT_MILLISECONDS: i64 = 5_000;
 const MAX_ACTIVE_FEED_TOKENS: i64 = 32;
-const SCHEMA_VERSION: i64 = 15;
+const SCHEMA_VERSION: i64 = 16;
 #[allow(
     dead_code,
     reason = "the integration test imports this module without the CLI operation"
@@ -22,7 +22,7 @@
     dead_code,
     reason = "M1A proves migrations before the M2 server calls them"
 )]
-const MIGRATIONS: [&str; 15] = [
+const MIGRATIONS: [&str; 16] = [
     include_str!("migrations/001_initial.sql"),
     include_str!("migrations/002_state.sql"),
     include_str!("migrations/003_git_intents.sql"),
@@ -38,6 +38,7 @@
     include_str!("migrations/013_watches.sql"),
     include_str!("migrations/014_feed_tokens.sql"),
     include_str!("migrations/015_pull_requests.sql"),
+    include_str!("migrations/016_pull_request_reviews.sql"),
 ];
 
 #[allow(
@@ -142,6 +143,10 @@
     PullRequestHidden,
     #[error("pull request is not open")]
     PullRequestState,
+    #[error("pull-request revision does not exist")]
+    PullRequestRevisionNotFound,
+    #[error("pull-request review anchor does not match its revision")]
+    PullRequestReviewAnchor,
     #[error("pull-request ref intent {0} is not in the required state")]
     PullRequestIntentState(String),
     #[error("repository watch access is not authorized")]
@@ -1802,13 +1807,172 @@
                 })
             })?
             .collect::<Result<Vec<_>, _>>()?;
+        let reviews = {
+            let mut statement = self.connection.prepare(
+                "SELECT pull_request_review.id, pull_request_revision.number,
+                        author.username, pull_request_review.kind,
+                        pull_request_review.body, pull_request_review.commit_object_id,
+                        pull_request_review.path, pull_request_review.side,
+                        pull_request_review.line, pull_request_review.created_at
+                 FROM pull_request_review
+                 JOIN pull_request_revision
+                   ON pull_request_revision.id = pull_request_review.revision_id
+                 JOIN account AS author
+                   ON author.id = pull_request_review.author_account_id
+                 WHERE pull_request_review.pull_request_id = ?1
+                 ORDER BY pull_request_review.created_at, pull_request_review.id",
+            )?;
+            statement
+                .query_map([&pull_request.id], pull_request_review_from_row)?
+                .collect::<Result<Vec<_>, _>>()?
+        };
+        let timeline = {
+            let mut statement = self.connection.prepare(
+                "SELECT sequence, kind, actor, payload, created_at
+                 FROM repository_event
+                 WHERE pull_request_id = ?1 ORDER BY sequence",
+            )?;
+            statement
+                .query_map([&pull_request.id], |row| {
+                    Ok(PullRequestTimelineRecord {
+                        sequence: row.get(0)?,
+                        kind: row.get(1)?,
+                        actor: row.get(2)?,
+                        payload: row.get(3)?,
+                        created_at: row.get(4)?,
+                    })
+                })?
+                .collect::<Result<Vec<_>, _>>()?
+        };
         let can_revise = access.can_write_repository();
         Ok(PullRequestDetail {
+            can_review: access.active_actor_id.is_some() && access.can_read(),
             repository: access.repository,
             pull_request,
             revisions,
+            reviews,
+            timeline,
             can_revise,
         })
+    }
+
+    pub(crate) fn create_pull_request_review(
+        &mut self,
+        review: &NewPullRequestReview<'_>,
+    ) -> Result<String, StoreError> {
+        let transaction = self
+            .connection
+            .transaction_with_behavior(TransactionBehavior::Immediate)?;
+        let access = repository_issue_access(
+            &transaction,
+            review.owner,
+            review.repository,
+            Some(review.actor),
+        )?;
+        if !access.can_read() {
+            return Err(StoreError::PullRequestHidden);
+        }
+        let actor_id = access
+            .active_actor_id
+            .ok_or(StoreError::PullRequestDenied)?;
+        let pull_request = transaction
+            .query_row(
+                "SELECT id, state FROM pull_request
+                 WHERE repository_id = ?1 AND number = ?2",
+                rusqlite::params![access.repository.id, review.number],
+                |row| Ok((row.get::<_, String>(0)?, row.get::<_, String>(1)?)),
+            )
+            .optional()?
+            .ok_or_else(|| {
+                StoreError::PullRequestNotFound(
+                    review.owner.to_owned(),
+                    review.repository.to_owned(),
+                    review.number,
+                )
+            })?;
+        if pull_request.1 != "open" {
+            return Err(StoreError::PullRequestState);
+        }
+        let revision = transaction
+            .query_row(
+                "SELECT id, base_object_id, head_object_id
+                 FROM pull_request_revision
+                 WHERE pull_request_id = ?1 AND number = ?2",
+                rusqlite::params![pull_request.0, review.revision],
+                |row| {
+                    Ok((
+                        row.get::<_, String>(0)?,
+                        row.get::<_, String>(1)?,
+                        row.get::<_, String>(2)?,
+                    ))
+                },
+            )
+            .optional()?
+            .ok_or(StoreError::PullRequestRevisionNotFound)?;
+        if review.kind == "line-comment" {
+            let expected = match review.side {
+                Some("base") => revision.1.as_str(),
+                Some("head") => revision.2.as_str(),
+                _ => return Err(StoreError::PullRequestReviewAnchor),
+            };
+            if review.commit_object_id != Some(expected) {
+                return Err(StoreError::PullRequestReviewAnchor);
+            }
+        }
+        let id: String =
+            transaction.query_row("SELECT lower(hex(randomblob(16)))", [], |row| row.get(0))?;
+        transaction.execute(
+            "INSERT INTO pull_request_review
+             (id, pull_request_id, revision_id, author_account_id, kind, body,
+              commit_object_id, path, side, line, created_at)
+             VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11)",
+            rusqlite::params![
+                id,
+                pull_request.0,
+                revision.0,
+                actor_id,
+                review.kind,
+                review.body,
+                review.commit_object_id,
+                review.path,
+                review.side,
+                review.line,
+                review.created_at,
+            ],
+        )?;
+        transaction.execute(
+            "UPDATE pull_request SET updated_at = ?2 WHERE id = ?1",
+            rusqlite::params![pull_request.0, review.created_at],
+        )?;
+        let kind = match review.kind {
+            "comment" => event::EventKind::PullRequestCommented,
+            "line-comment" => event::EventKind::PullRequestLineCommented,
+            "approved" => event::EventKind::PullRequestApproved,
+            "changes-requested" => event::EventKind::PullRequestChangesRequested,
+            _ => return Err(StoreError::PullRequestReviewAnchor),
+        };
+        let event = event::pull_request_review(
+            kind,
+            &pull_request.0,
+            review.number,
+            &id,
+            review.revision,
+            review.body,
+            review.commit_object_id,
+            review.path,
+            review.side,
+            review.line,
+        );
+        insert_pull_request_event(
+            &transaction,
+            &access.repository.id,
+            &pull_request.0,
+            review.actor,
+            review.created_at,
+            &event,
+        )?;
+        transaction.commit()?;
+        Ok(id)
     }
 
     pub(crate) fn pull_requests(
@@ -3043,11 +3207,52 @@
     pub(crate) created_at: i64,
 }
 
+#[derive(Clone, Debug, Eq, PartialEq)]
+pub(crate) struct PullRequestReviewRecord {
+    pub(crate) id: String,
+    pub(crate) revision: i64,
+    pub(crate) author: String,
+    pub(crate) kind: String,
+    pub(crate) body: String,
+    pub(crate) commit_object_id: Option<String>,
+    pub(crate) path: Option<Vec<u8>>,
+    pub(crate) side: Option<String>,
+    pub(crate) line: Option<i64>,
+    pub(crate) created_at: i64,
+}
+
+#[derive(Clone, Debug, Eq, PartialEq)]
+pub(crate) struct PullRequestTimelineRecord {
+    pub(crate) sequence: i64,
+    pub(crate) kind: String,
+    pub(crate) actor: String,
+    pub(crate) payload: String,
+    pub(crate) created_at: i64,
+}
+
 pub(crate) struct PullRequestDetail {
     pub(crate) repository: RepositoryRecord,
     pub(crate) pull_request: PullRequestRecord,
     pub(crate) revisions: Vec<PullRequestRevisionRecord>,
+    pub(crate) reviews: Vec<PullRequestReviewRecord>,
+    pub(crate) timeline: Vec<PullRequestTimelineRecord>,
     pub(crate) can_revise: bool,
+    pub(crate) can_review: bool,
+}
+
+pub(crate) struct NewPullRequestReview<'a> {
+    pub(crate) owner: &'a str,
+    pub(crate) repository: &'a str,
+    pub(crate) number: i64,
+    pub(crate) revision: i64,
+    pub(crate) actor: &'a str,
+    pub(crate) kind: &'a str,
+    pub(crate) body: &'a str,
+    pub(crate) commit_object_id: Option<&'a str>,
+    pub(crate) path: Option<&'a [u8]>,
+    pub(crate) side: Option<&'a str>,
+    pub(crate) line: Option<i64>,
+    pub(crate) created_at: i64,
 }
 
 pub(crate) struct NewPullRequestRefIntent<'a> {
@@ -3461,6 +3666,23 @@
     })
 }
 
+fn pull_request_review_from_row(
+    row: &rusqlite::Row<'_>,
+) -> rusqlite::Result<PullRequestReviewRecord> {
+    Ok(PullRequestReviewRecord {
+        id: row.get(0)?,
+        revision: row.get(1)?,
+        author: row.get(2)?,
+        kind: row.get(3)?,
+        body: row.get(4)?,
+        commit_object_id: row.get(5)?,
+        path: row.get(6)?,
+        side: row.get(7)?,
+        line: row.get(8)?,
+        created_at: row.get(9)?,
+    })
+}
+
 fn validate_feed_scope(scope: &str, repository: Option<(&str, &str)>) -> Result<(), StoreError> {
     match (scope, repository) {
         ("repository", Some(_)) | ("watched" | "assignments" | "mentions", None) => Ok(()),
@@ -3759,6 +3981,32 @@
             source_ordinal: None,
             issue_id: Some(issue_id),
             pull_request_id: None,
+            event,
+            actor,
+            ref_name: None,
+            old_target: None,
+            new_target: None,
+            created_at,
+        },
+    )
+}
+
+fn insert_pull_request_event(
+    transaction: &rusqlite::Transaction<'_>,
+    repository_id: &str,
+    pull_request_id: &str,
+    actor: &str,
+    created_at: i64,
+    event: &event::VersionedEvent,
+) -> Result<(), StoreError> {
+    insert_domain_event(
+        transaction,
+        &NewDomainEvent {
+            repository_id,
+            source_intent_id: None,
+            source_ordinal: None,
+            issue_id: None,
+            pull_request_id: Some(pull_request_id),
             event,
             actor,
             ref_name: None,

templates/pull_request.html

Mode 100644100644; object 523008b402f9039e4b5a5ac6

@@ -37,6 +37,39 @@
     </ol>
   </section>
 
+  <section aria-labelledby="reviews-heading">
+    <h2 id="reviews-heading">Reviews</h2>
+{% if reviews.is_empty() %}
+    <p>This pull request has no review activity.</p>
+{% else %}
+{% for review in reviews %}
+    <article id="review-{{ review.id }}">
+      <h3>{{ review.author }}: {{ review.kind }} at <time>{{ review.created_at }}</time></h3>
+      <p>Revision {{ review.revision }}
+{% if review.kind == "line-comment" %}
+        · <code>{{ review.commit_object_id }}</code> · <code>{{ review.path }}</code> · {{ review.side }} line {{ review.line }}
+{% if review.outdated %}
+        · <strong>Outdated</strong>
+{% endif %}
+{% endif %}
+      </p>
+{% if review.has_body %}
+      <div class="markdown">{{ review.body_html|safe }}</div>
+{% endif %}
+    </article>
+{% endfor %}
+{% endif %}
+  </section>
+
+  <section aria-labelledby="timeline-heading">
+    <h2 id="timeline-heading">Timeline</h2>
+    <ol>
+{% for event in timeline %}
+      <li value="{{ event.sequence }}">{{ event.actor }}: {{ event.kind }} at <time>{{ event.created_at }}</time></li>
+{% endfor %}
+    </ol>
+  </section>
+
   <section aria-labelledby="comparison-heading">
     <h2 id="comparison-heading">Comparison for revision {{ selected_revision }}</h2>
     <p>Merge base: <code>{{ comparison.merge_base }}</code></p>
@@ -72,10 +105,58 @@
       <p>This file has binary content.</p>
 {% else %}
       <pre>{{ file.hunks }}</pre>
+{% if can_review %}
+      <form method="post" action="/{{ owner }}/{{ repository }}/pulls/{{ pull_request.number }}/reviews">
+        <input type="hidden" name="csrf" value="{{ csrf }}">
+        <input type="hidden" name="revision" value="{{ selected_revision }}">
+        <input type="hidden" name="kind" value="line-comment">
+        <input type="hidden" name="path-hex" value="{{ file.path_hex }}">
+        <div class="field">
+          <label for="review-side-{{ file.path_hex }}">Side</label>
+          <select id="review-side-{{ file.path_hex }}" name="side">
+{% if file.has_base %}
+            <option value="base">Base</option>
+{% endif %}
+{% if file.has_head %}
+            <option value="head">Head</option>
+{% endif %}
+          </select>
+        </div>
+        <div class="field">
+          <label for="review-line-{{ file.path_hex }}">Line</label>
+          <input id="review-line-{{ file.path_hex }}" name="line" type="number" min="1" required>
+        </div>
+        <div class="field">
+          <label for="review-body-{{ file.path_hex }}">Line comment (Markdown)</label>
+          <textarea id="review-body-{{ file.path_hex }}" name="body" maxlength="262144" rows="4" required></textarea>
+        </div>
+        <button type="submit">Add line comment</button>
+      </form>
+{% endif %}
 {% endif %}
     </article>
 {% endfor %}
   </section>
+
+{% if can_review %}
+  <section aria-labelledby="review-heading">
+    <h2 id="review-heading">Review this revision</h2>
+    <form method="post" action="/{{ owner }}/{{ repository }}/pulls/{{ pull_request.number }}/reviews">
+      <input type="hidden" name="csrf" value="{{ csrf }}">
+      <input type="hidden" name="revision" value="{{ selected_revision }}">
+      <input type="hidden" name="path-hex" value="">
+      <input type="hidden" name="side" value="">
+      <input type="hidden" name="line" value="">
+      <div class="field">
+        <label for="review-body">Review comment (Markdown)</label>
+        <textarea id="review-body" name="body" maxlength="262144" rows="8"></textarea>
+      </div>
+      <button name="kind" value="comment" type="submit">Add comment</button>
+      <button name="kind" value="approved" type="submit">Approve</button>
+      <button name="kind" value="changes-requested" type="submit">Request changes</button>
+    </form>
+  </section>
+{% endif %}
 
 {% if can_revise %}
   <form method="post" action="/{{ owner }}/{{ repository }}/pulls/{{ pull_request.number }}/revisions">

tests/cli.rs

Mode 100644100644; object fd0b76536be032e32f6467b4

@@ -24,7 +24,8 @@
     include_str!("../src/store/migrations/013_watches.sql"),
     include_str!("../src/store/migrations/014_feed_tokens.sql"),
     include_str!("../src/store/migrations/015_pull_requests.sql"),
-    "PRAGMA user_version = 15;\n",
+    include_str!("../src/store/migrations/016_pull_request_reviews.sql"),
+    "PRAGMA user_version = 16;\n",
 );
 
 #[test]

tests/public_routes.rs

Mode 100644100644; object feb01e7cf40cd146e2787ded

@@ -848,6 +848,69 @@
     assert_eq!(first_revision.status, 200);
     assert!(first_revision.text().contains("Comparison for revision 1"));
     assert!(!first_revision.text().contains("pull-request.txt"));
+    for fields in [
+        vec![
+            ("csrf", csrf.as_str()),
+            ("revision", "2"),
+            ("kind", "comment"),
+            ("body", "A **general review**."),
+            ("path-hex", ""),
+            ("side", ""),
+            ("line", ""),
+        ],
+        vec![
+            ("csrf", csrf.as_str()),
+            ("revision", "2"),
+            ("kind", "approved"),
+            ("body", ""),
+            ("path-hex", ""),
+            ("side", ""),
+            ("line", ""),
+        ],
+        vec![
+            ("csrf", csrf.as_str()),
+            ("revision", "2"),
+            ("kind", "line-comment"),
+            ("body", "Review this line."),
+            ("path-hex", "70756c6c2d726571756573742e747874"),
+            ("side", "head"),
+            ("line", "1"),
+        ],
+    ] {
+        let response = request(
+            server.address(),
+            "POST",
+            "/alice/example/pulls/1/reviews",
+            &headers,
+            form(&fields).as_bytes(),
+        );
+        assert_eq!(response.status, 303);
+    }
+    let reviewed = request(server.address(), "GET", "/alice/example/pulls/1", &[], &[]);
+    assert!(reviewed.text().contains("<strong>general review</strong>"));
+    assert!(reviewed.text().contains("pull-request-approved"));
+    assert!(reviewed.text().contains("head line 1"));
+    assert!(!reviewed.text().contains("<strong>Outdated</strong>"));
+
+    fs::write(worktree.join("pull-request.txt"), b"later revision\n")
+        .expect("write a later pull-request revision");
+    commit_all(&worktree, "later pull-request revision");
+    run(Command::new("git")
+        .arg("-C")
+        .arg(&worktree)
+        .args(["push", "-q"])
+        .arg(&bare)
+        .arg("feature"));
+    let revised_again = request(
+        server.address(),
+        "POST",
+        "/alice/example/pulls/1/revisions",
+        &headers,
+        revision.as_bytes(),
+    );
+    assert_eq!(revised_again.status, 303);
+    let outdated = request(server.address(), "GET", "/alice/example/pulls/1", &[], &[]);
+    assert!(outdated.text().contains("<strong>Outdated</strong>"));
     let search_page = request(server.address(), "GET", "/search", &[], &[]);
     assert_eq!(search_page.status, 200);
     assert!(

tests/pull_requests.rs

Mode 100644100644; object 62d23979f9ee0a643fa75935

@@ -25,6 +25,7 @@
 mod store;
 
 use std::fs;
+use std::os::unix::ffi::OsStringExt;
 use std::path::{Path, PathBuf};
 use std::process::Command;
 use std::sync::Arc;
@@ -386,6 +387,158 @@
     }
 }
 
+#[test]
+fn records_review_actions_and_immutable_line_anchors_for_both_hashes() {
+    for (index, object_format) in ["sha1", "sha256"].into_iter().enumerate() {
+        let fixture = Fixture::new(object_format, index + 20);
+        let byte_path = "review-å.txt".as_bytes();
+        fixture.commit_bytes_on("feature", byte_path, b"byte path\n", "add a byte path");
+        let service = PullRequestService::new(&fixture.database, &fixture.repositories);
+        service
+            .open(
+                "alice",
+                "project",
+                "alice",
+                "Review anchors",
+                "Keep each review action.",
+                "refs/heads/main",
+                "refs/heads/feature",
+            )
+            .expect("open a reviewed pull request");
+        service
+            .review(
+                "alice",
+                "project",
+                1,
+                1,
+                "bob",
+                "comment",
+                "A **general** comment.",
+                None,
+                None,
+                None,
+            )
+            .expect("add a reader comment");
+        service
+            .review(
+                "alice", "project", 1, 1, "bob", "approved", "", None, None, None,
+            )
+            .expect("approve the revision");
+        service
+            .review(
+                "alice",
+                "project",
+                1,
+                1,
+                "alice",
+                "changes-requested",
+                "Change this line.",
+                None,
+                None,
+                None,
+            )
+            .expect("request changes");
+        let line_id = service
+            .review(
+                "alice",
+                "project",
+                1,
+                1,
+                "bob",
+                "line-comment",
+                "Use a clearer value.",
+                Some(byte_path),
+                Some("head"),
+                Some(1),
+            )
+            .expect("add a line comment");
+        assert!(matches!(
+            service.review(
+                "alice",
+                "project",
+                1,
+                1,
+                "bob",
+                "line-comment",
+                "This line does not exist.",
+                Some(byte_path),
+                Some("head"),
+                Some(2),
+            ),
+            Err(PullRequestError::ReviewAnchor)
+        ));
+
+        fixture.commit_feature("make the line comment outdated");
+        service
+            .revise("alice", "project", 1, "alice")
+            .expect("record a new revision");
+        let detail = service
+            .get("alice", "project", 1, Some("bob"))
+            .expect("read review activity");
+        assert_eq!(detail.revisions.len(), 2);
+        assert_eq!(detail.reviews.len(), 4);
+        let line = detail
+            .reviews
+            .iter()
+            .find(|review| review.id == line_id)
+            .expect("find the line review");
+        assert_eq!(line.revision, 1);
+        assert_eq!(line.path.as_deref(), Some(byte_path));
+        assert_eq!(line.side.as_deref(), Some("head"));
+        assert_eq!(line.line, Some(1));
+        assert_eq!(
+            line.commit_object_id.as_deref(),
+            Some(detail.revisions[0].head_object_id.as_str())
+        );
+        assert_eq!(
+            detail
+                .timeline
+                .iter()
+                .map(|event| event.kind.as_str())
+                .collect::<Vec<_>>(),
+            [
+                "pull-request-created",
+                "pull-request-commented",
+                "pull-request-approved",
+                "pull-request-changes-requested",
+                "pull-request-line-commented",
+                "pull-request-revised",
+            ]
+        );
+
+        let store = Store::open(&fixture.database).expect("open the review policy store");
+        store
+            .connection()
+            .execute(
+                "UPDATE repository SET visibility = 'private' WHERE slug = 'project'",
+                [],
+            )
+            .expect("make the reviewed repository private");
+        store
+            .connection()
+            .execute(
+                "DELETE FROM repository_collaborator WHERE account_id = 2",
+                [],
+            )
+            .expect("remove the reader");
+        assert!(matches!(
+            service.review(
+                "alice",
+                "project",
+                1,
+                2,
+                "bob",
+                "comment",
+                "This must stay hidden.",
+                None,
+                None,
+                None,
+            ),
+            Err(PullRequestError::Store(StoreError::PullRequestHidden))
+        ));
+    }
+}
+
 struct Fixture {
     _directory: TempDir,
     database: PathBuf,
@@ -500,6 +653,27 @@
             Command::new("git").args(["switch", "-q", branch]),
         );
         fs::write(self.worktree.join(path), content).expect("write branch content");
+        git_commit(&self.worktree, message);
+        run(
+            &self.worktree,
+            Command::new("git")
+                .args(["push", "-q"])
+                .arg(&self.bare)
+                .arg(branch),
+        );
+    }
+
+    fn commit_bytes_on(&self, branch: &str, path: &[u8], content: &[u8], message: &str) {
+        run(
+            &self.worktree,
+            Command::new("git").args(["switch", "-q", branch]),
+        );
+        fs::write(
+            self.worktree
+                .join(std::ffi::OsString::from_vec(path.to_vec())),
+            content,
+        )
+        .expect("write byte-path content");
         git_commit(&self.worktree, message);
         run(
             &self.worktree,

tests/sqlite.rs

Mode 100644100644; object f93ba12b080d9a654b0702c3

@@ -79,6 +79,18 @@
     include_str!("../src/store/migrations/014_feed_tokens.sql"),
     "PRAGMA user_version = 14;\n",
 );
+const V15_FIXTURE: &str = concat!(
+    include_str!("fixtures/sqlite/v7.sql"),
+    include_str!("../src/store/migrations/008_web_sessions.sql"),
+    include_str!("../src/store/migrations/009_repository_authorization.sql"),
+    include_str!("../src/store/migrations/010_audit_history.sql"),
+    include_str!("../src/store/migrations/011_domain_events.sql"),
+    include_str!("../src/store/migrations/012_issues.sql"),
+    include_str!("../src/store/migrations/013_watches.sql"),
+    include_str!("../src/store/migrations/014_feed_tokens.sql"),
+    include_str!("../src/store/migrations/015_pull_requests.sql"),
+    "PRAGMA user_version = 15;\n",
+);
 
 fn database(directory: &TempDir, name: &str) -> std::path::PathBuf {
     directory.path().join(name)
@@ -196,7 +208,7 @@
     let directory = TempDir::new().expect("create a temporary directory");
     let store = Store::open(&database(&directory, "store.sqlite")).expect("open the store");
 
-    assert_eq!(store.schema_version().expect("read the schema version"), 15);
+    assert_eq!(store.schema_version().expect("read the schema version"), 16);
     assert_eq!(
         store
             .connection()
@@ -1559,13 +1571,14 @@
         (V12_FIXTURE, 12),
         (V13_FIXTURE, 13),
         (V14_FIXTURE, 14),
+        (V15_FIXTURE, 15),
     ] {
         let directory = TempDir::new().expect("create a temporary directory");
         let path = database(&directory, "tit.sqlite3");
         create_fixture(&path, fixture);
 
         let store = Store::open(&path).expect("migrate the fixture");
-        assert_eq!(store.schema_version().expect("read the schema version"), 15);
+        assert_eq!(store.schema_version().expect("read the schema version"), 16);
         store.integrity_check().expect("check migrated integrity");
         let state: String = store
             .connection()
@@ -1638,7 +1651,7 @@
 
 #[test]
 fn recovers_complete_schema_versions_after_a_process_kill_during_migration() {
-    for (mode, expected_version) in [("migration-uncommitted", 1), ("migration-committed", 15)] {
+    for (mode, expected_version) in [("migration-uncommitted", 1), ("migration-committed", 16)] {
         let directory = TempDir::new().expect("create a temporary directory");
         let path = database(&directory, "fixture.sqlite");
         create_fixture(&path, V1_FIXTURE);