feat(forge): P38 F4b pull requests, reviews, comments and retention refs

ROADMAP.md

@@ -706,7 +706,8 @@ skeleton, users, organizations, tokens, `admin` CLI), F2a (repositories API, lif
 and reconciler, collaborators, branches) and F2b (smart-HTTP git transport via `git http-backend`,
 `pre-receive` ref policy, admission control, threat-model pass #1) and F3a (content reads: refs, tags, commits,
 trees, contents, archives) and F3b (contents writes: create, update, delete as server-made commits) and F4a (branch protection
-enforced on push, contents API and branch delete; commit statuses) are built. Hosted repos are
+enforced on push, contents API and branch delete; commit statuses) and F4b (pull requests, reviews, comments, retention
+refs) are built. Hosted repos are
 bare Git and get none of the `.sc/` features.
 
 Other follow-up work is tracked as GitHub issues and listed under Deferred

crates/forge/conformance/FINDINGS.md

@@ -99,7 +99,7 @@ every failure is a difference listed in the last table.
   An empty repo whose first push is a branch other than the default still reports `empty:true`.
 
 **O and P (PR lifecycle, pagination)**
-- Force-pushing the head moves `head.sha`. Deleting the head branch leaves the PR open;
+- Force-pushing the head moves `head.sha`. Deleting the head branch **closes the PR** (O#6-O#10: `state:"closed"`, `head.ref:"refs/pull/N/head"`);
   `refs/pull/N/head` stays visible and merge is 404. A merged PR's `head.sha` is **not** frozen: it
   follows a recreated branch. Pushing the head commit straight into base does **not** mark the PR
   merged (still open after 15 s). Default page size is 30, max 50; `limit=0`, `-1` and `abc` fall back

crates/forge/conformance/forge-check.sh

@@ -25,4 +25,4 @@ for _ in $(seq 50); do
 done
 python3 -I "$here/run.py" check --base-url "http://127.0.0.1:$port" --admin-token "$token" \
   --fixtures "$here/fixtures/1.22.6" --departures "$here/departures.json" --pending "$here/slice-pending.json" \
-  --only A,B,C,D,E,H,I,J,K,L,M,N,Q,R,V,W "$@" || { echo "--- server log"; cat "$data/serve.log"; exit 1; }
+  --only A,B,C,D,E,F,H,I,J,K,L,M,N,O,P,Q,R,V,W "$@" || { echo "--- server log"; cat "$data/serve.log"; exit 1; }

crates/forge/conformance/slice-pending.json

@@ -1,12 +1,4 @@
 {
- "D": {
-  "defer": [
-   [
-    13,
-    "GET /api/v1/repos/acme/blank/pulls?state=all"
-   ]
-  ]
- },
  "K": {
   "defer": [
    [
@@ -21,8 +13,54 @@
  },
  "Q": {
   "stop": [
-   5,
-   "expected [201]"
+   8,
+   "origin/main~1"
+  ],
+  "defer": [
+   [
+    5,
+    "POST /api/v1/repos/acme/bypass/pulls/<id:path:pulls#1>/merge"
+   ]
+  ]
+ },
+ "O": {
+  "defer": [
+   [
+    9,
+    "POST /api/v1/repos/acme/life/pulls/<id:path:pulls#2>/merge"
+   ],
+   [
+    19,
+    "POST /api/v1/repos/acme/life/pulls/<id:path:pulls#3>/merge"
+   ],
+   [
+    20,
+    "GET /api/v1/repos/acme/life/pulls/<id:path:pulls#3>"
+   ],
+   [
+    21,
+    "merged_pr_head_sha_frozen"
+   ],
+   [
+    22,
+    "merged_pr_head_sha_follows_new_branch"
+   ],
+   [
+    23,
+    "PATCH /api/v1/repos/acme/life/pulls/<id:path:pulls#4>"
+   ],
+   [
+    25,
+    "GET /api/v1/repos/acme/life/pulls/<id:path:pulls#4>"
+   ],
+   [
+    26,
+    "PATCH /api/v1/repos/acme/life/pulls/<id:path:pulls#4>"
+   ],
+   [
+    27,
+    "GET /api/v1/repos/acme/life/pulls/<id:path:pulls#4>"
+   ]
   ]
  }
 }

crates/forge/src/api.rs

@@ -44,6 +44,20 @@ pub(crate) fn user_json(base: &str, u: &User) -> Value {
     })
 }
 
+/// The user object Gitea nests inside statuses, reviews and comments: inactive, never a site
+/// admin, no login name. `mask_email` replaces the stored address with `<login>@noreply.localhost`
+/// (recorded for statuses and comments; reviews show the stored one).
+pub(crate) fn nested_user_json(base: &str, u: &User, mask_email: bool) -> Value {
+    let mut j = user_json(base, u);
+    j["active"] = json!(false);
+    j["is_admin"] = json!(false);
+    j["login_name"] = json!("");
+    if mask_email {
+        j["email"] = json!(format!("{}@noreply.localhost", u.login));
+    }
+    j
+}
+
 pub(crate) fn org_json(base: &str, o: &User) -> Value {
     json!({
         "id": o.id,
@@ -119,7 +133,7 @@ pub(crate) fn paged(
     limit: i64,
     items: Vec<Value>,
 ) -> Resp {
-    paged_with(ctx, path, total, page, limit, &[], items)
+    paged_with(ctx, path, total, page, limit, items)
 }
 
 /// Percent-encode everything except the RFC 3986 unreserved characters, so a value can sit inside
@@ -136,79 +150,67 @@ pub(crate) fn pct_encode(s: &str) -> String {
     out
 }
 
-/// Like [`paged`], carrying `extra` query parameters (already strictly percent-encoded with [`pct_encode`], in the order
-/// Gitea emits them: alphabetical after `limit` and `page`) into every `Link` URL, so following
-/// a page link repeats the same query.
+/// Like [`paged`]; the page links repeat the request's own query (see [`link_header`]).
 pub(crate) fn paged_with(
     ctx: &Ctx,
     path: &str,
     total: i64,
     page: i64,
     limit: i64,
-    extra: &[(&str, String)],
     items: Vec<Value>,
 ) -> Resp {
     let mut r = Resp::json(200, &Value::Array(items)).with("x-total-count", total.to_string());
-    if let Some(l) = link_header(ctx, path, total, page, limit, extra) {
+    if let Some(l) = link_header(ctx, path, total, page, limit) {
         r = r.with("link", l);
     }
     r
 }
 
 /// Gitea's `Link` header value for page `page` of `total` rows, or `None` when there is one page.
+/// Each link repeats the request's own query parameters (first value of a repeated one, sorted by
+/// name) with `page` replaced, and carries `limit` only when the request did (recorded P#19-32:
+/// `?page=2` for a bare request, `?limit=0&page=2` when the client sent `limit=0`). The page is
+/// clamped to the last one, as Gitea does when building the links.
 pub(crate) fn link_header(
     ctx: &Ctx,
     path: &str,
     total: i64,
     page: i64,
     limit: i64,
-    extra: &[(&str, String)],
 ) -> Option<String> {
-    let tail: String = extra.iter().map(|(k, v)| format!("&{k}={v}")).collect();
-    let last = ((total + limit - 1) / limit).max(1);
-    let url = |p: i64, rel: &str| {
-        format!(
-            "<{}/api/v1{path}?limit={limit}&page={p}{tail}>; rel=\"{rel}\"",
-            ctx.app.base
-        )
-    };
-    let mut links = Vec::new();
-    if page < last {
-        links.push(url(page + 1, "next"));
-        links.push(url(last, "last"));
-    }
-    if page > 1 {
-        links.push(url(1, "first"));
-        links.push(url(page - 1, "prev"));
-    }
-    (!links.is_empty()).then(|| links.join(","))
+    links(ctx, path, total, page, limit, false)
 }
 
-/// Like [`paged`], but a page link repeats *every* request parameter, alphabetically by name (what
-/// Gitea does for the commits listing), with `limit` and `page` set to the link's own values.
-pub(crate) fn paged_sorted(
+fn links(
     ctx: &Ctx,
     path: &str,
     total: i64,
     page: i64,
     limit: i64,
-    items: Vec<Value>,
-) -> Resp {
+    with_limit: bool,
+) -> Option<String> {
     let mut q: std::collections::BTreeMap<String, String> = ctx
         .req
         .query
         .split('&')
-        .filter(|kv| !kv.is_empty())
-        .map(|kv| {
-            let (k, v) = kv.split_once('=').unwrap_or((kv, ""));
-            (k.to_string(), v.to_string())
+        // Parsed exactly like `Req::param` (raw names, `name=value` pairs only, first value wins),
+        // so the next page selects the rows this one did.
+        .filter_map(|kv| kv.split_once('='))
+        .map(|(k, v)| {
+            // Re-encoded strictly, never echoed raw: a `>` or `"` in the request must not close the
+            // link target or start another entry. Names stay raw-equivalent (not decoded).
+            let dec = |t: &str| {
+                crate::routes::percent_decode(&t.replace('+', " "))
+                    .map_or_else(|| pct_encode(t), |d| pct_encode(&d))
+            };
+            (pct_encode(k), dec(v))
         })
         .rev() // the request parser reads the first value of a repeated parameter; the last insert wins
         .collect();
-    q.insert("limit".into(), limit.to_string());
+    if with_limit {
+        q.insert("limit".into(), limit.to_string());
+    }
     let last = ((total + limit - 1) / limit).max(1);
-    // Gitea builds the links from the clamped page: asking past the end still points `prev` at the
-    // page before the last.
     let page = page.min(last);
     let mut url = |p: i64, rel: &str| {
         q.insert("page".into(), p.to_string());
@@ -228,9 +230,21 @@ pub(crate) fn paged_sorted(
         links.push(url(1, "first"));
         links.push(url(page - 1, "prev"));
     }
+    (!links.is_empty()).then(|| links.join(","))
+}
+
+/// Like [`paged`], but every link also carries `limit` (what Gitea does for the commits listing).
+pub(crate) fn paged_sorted(
+    ctx: &Ctx,
+    path: &str,
+    total: i64,
+    page: i64,
+    limit: i64,
+    items: Vec<Value>,
+) -> Resp {
     let mut r = Resp::json(200, &Value::Array(items)).with("x-total-count", total.to_string());
-    if !links.is_empty() {
-        r = r.with("link", links.join(","));
+    if let Some(l) = links(ctx, path, total, page, limit, true) {
+        r = r.with("link", l);
     }
     r
 }

crates/forge/src/cgi.rs

@@ -811,6 +811,9 @@ impl Supervise {
         let status = child.wait();
         let ok = reason == "exited" && status.as_ref().is_ok_and(|s| s.success());
         let _ = verdict.send(ok);
+        // The child is gone: release the transport registration, so a repository delete waiting
+        // for it is not also waiting on the sync below (which takes the shared lifecycle lock).
+        drop(reg);
         let err = tail.and_then(|t| t.join().ok()).unwrap_or_default();
         if !ok {
             eprintln!(
@@ -821,6 +824,7 @@ impl Supervise {
         } else if receive && marker.exists() {
             if let Ok(conn) = crate::db::connect(&app.data_dir) {
                 let _ = repos::touch(&conn, repo_id, crate::timefmt::now());
+                crate::pulls::sync(&app.life, &conn, repo_id, crate::timefmt::now());
             }
         }
         let _ = std::fs::remove_file(&marker);

crates/forge/src/cli.rs

@@ -313,6 +313,8 @@ fn run_serve(a: ServeArgs) -> Result<()> {
     if report != scl_forge::lifecycle::Report::default() {
         eprintln!("sc-forge: reconciled {report:?}");
     }
+    // Pull request refs and rows agree again before anything is served.
+    scl_forge::pulls::reconcile_all(&life, &db::connect(&a.data_dir)?, scl_forge::timefmt::now());
     let tcfg = scl_forge::cgi::Cfg {
         max_transports: a.max_transports,
         per_repo: a.max_transports_per_repo,

crates/forge/src/content_api.rs

@@ -72,7 +72,12 @@ fn no_refs(ctx: &Ctx, a: &Access) -> Result<bool, Resp> {
     all_refs(ctx, a).map(|rows| rows.is_empty())
 }
 
-fn resolve(ctx: &Ctx, a: &Access, rev: &str, peel: Peel) -> Result<Option<Resolved>, Resp> {
+pub(crate) fn resolve(
+    ctx: &Ctx,
+    a: &Access,
+    rev: &str,
+    peel: Peel,
+) -> Result<Option<Resolved>, Resp> {
     gitobj::resolve(
         &ctx.app.life.git,
         &dir(ctx, a),
@@ -83,28 +88,28 @@ fn resolve(ctx: &Ctx, a: &Access, rev: &str, peel: Peel) -> Result<Option<Resolv
     .map_err(|e| read_err(ctx, a, e))
 }
 
-fn git_err<T>(ctx: &Ctx, a: &Access, r: Result<T, GitError>) -> Result<T, Resp> {
+pub(crate) fn git_err<T>(ctx: &Ctx, a: &Access, r: Result<T, GitError>) -> Result<T, Resp> {
     r.map_err(|e| read_err(ctx, a, e))
 }
 
 // ---- JSON pieces ----
 
 /// Commit authors resolved to accounts by email, once per request.
-struct Users<'a> {
+pub(crate) struct Users<'a> {
     ctx: &'a Ctx<'a>,
     cache: HashMap<String, Value>,
 }
 
 impl<'a> Users<'a> {
-    fn new(ctx: &'a Ctx<'a>) -> Self {
+    pub(crate) fn new(ctx: &'a Ctx<'a>) -> Self {
         Users {
             ctx,
             cache: HashMap::new(),
         }
     }
 
     /// The account owning `email`, or `null` (recorded: commits by unknown addresses).
-    fn actor(&mut self, email: &str) -> Result<Value, Resp> {
+    pub(crate) fn actor(&mut self, email: &str) -> Result<Value, Resp> {
         if let Some(v) = self.cache.get(&email.to_lowercase()) {
             return Ok(v.clone());
         }
@@ -127,7 +132,7 @@ pub(crate) fn verification() -> Value {
     json!({"verified": false, "reason": "gpg.error.not_signed_commit", "signature": "", "signer": null, "payload": ""})
 }
 
-fn person_json(p: &gitread::Person) -> Value {
+pub(crate) fn person_json(p: &gitread::Person) -> Value {
     json!({"name": p.name, "email": p.email, "date": rfc3339(p.when)})
 }
 
@@ -136,10 +141,10 @@ fn commit_meta(ctx: &Ctx, a: &Access, sha: &str, when: i64) -> Value {
 }
 
 /// Files and line counts a commit response may carry (`None` for each the client did not ask for).
-type Changes = (Option<Vec<gitobj::FileChange>>, Option<gitobj::Stats>);
+pub(crate) type Changes = (Option<Vec<gitobj::FileChange>>, Option<gitobj::Stats>);
 
 /// The Gitea commit object. `changes` fills `files` and `stats` (`None` leaves them `null`).
-fn commit_json(
+pub(crate) fn commit_json(
     ctx: &Ctx,
     a: &Access,
     users: &mut Users,
@@ -318,7 +323,6 @@ pub fn list_tags(ctx: &mut Ctx) -> Resp {
             total,
             page,
             limit,
-            &[],
             items,
         )
     })
@@ -336,7 +340,7 @@ pub fn list_commits(ctx: &mut Ctx) -> Resp {
 }
 
 /// Changed files one `commits` response may carry in total (a few hundred bytes of JSON each).
-const MAX_PAGE_FILES: usize = 50_000;
+pub(crate) const MAX_PAGE_FILES: usize = 50_000;
 
 fn list_commits_inner(ctx: &Ctx, a: &Access) -> Resp {
     if ctx.req.param("not").is_some() {

crates/forge/src/content_write.rs

@@ -486,6 +486,8 @@ fn published(ctx: &Ctx, a: &Access, resp: Resp) -> Resp {
             a.repo.id
         );
     }
+    // The branch moved: PRs on it follow (the write gate is held).
+    crate::pulls_api::sync_held(ctx, a);
     resp
 }