fix(node): address Twigpine#465 review findings

Author: cairn-intern

CONTRIBUTING.md

@@ -81,7 +81,7 @@ time once updated.
 
 **Requirements:**
 - Rust stable (≥ 1.91) — install via [rustup](https://rustup.rs)
-- PostgreSQL — required for the node. Use the bundled `docker-compose.yml` for local dev.
+- PostgreSQL 16+ — required for the node (it uses `pg_input_is_valid`, which exists only on PostgreSQL 16 and later). Use the bundled `docker-compose.yml` for local dev.
 - Docker (optional, for full-stack local testing)
 
 **Environment variables:**

crates/gitlawb-node/src/api/events.rs

@@ -11,7 +11,8 @@ use crate::state::AppState;
 
 /// Hard ceiling on rows any ref-update feed returns in one request. Shared by the
 /// shared collector's clamp and the per-handler request caps so they can't drift.
-const MAX_VISIBLE_REF_UPDATES: i64 = 200;
+/// `pub(crate)` so the GraphQL complexity meter can price against the same bound.
+pub(crate) const MAX_VISIBLE_REF_UPDATES: i64 = 200;
 
 /// Collect up to `limit` ref-update rows visible to `caller`, newest first,
 /// paging past rows the feed gate drops. Filtering after a plain SQL `LIMIT`

crates/gitlawb-node/src/db/mod.rs

@@ -1549,6 +1549,20 @@ impl Db {
     /// mirrors `visibility::listable_at_root`; a differential test pins it.
     /// Malformed reader JSON denies that repo to non-owners without failing the page.
     /// Cursors only select a position and never confer read authority.
+    ///
+    /// Performance note: the page orders and keyset-filters on
+    /// `(owner_key, name)` under `COLLATE "C"`, but the existing
+    /// `idx_repos_owner_key_name` index is built on the database default
+    /// collation and cannot serve that ordering, so each page sorts the full
+    /// deduped repo set before the keyset filter applies: O(total repos) work
+    /// per page. This is accepted (pages are capped at
+    /// `MAX_VISIBLE_REPO_PAGE_SIZE` rows over a narrow projection). A
+    /// C-collation expression index was deliberately not added in a migration:
+    /// versions 27-35 are claimed by other in-flight branches (Gitlawb/node#384
+    /// claims 27-35, #464 claims 27-28) and the runner keys applied migrations
+    /// on the version integer alone, so a colliding entry would be silently
+    /// skipped on whichever side merges second. Revisit once that range lands
+    /// or clears.
     pub async fn list_visible_repos_page(
         &self,
         caller: Option<&str>,

crates/gitlawb-node/src/graphql/mod.rs

@@ -580,7 +580,7 @@ mod tests {
         let schema = build_schema(Arc::new(Db::for_testing(pool)), ref_tx, task_tx);
 
         // refUpdates(limit: 200):
-        // Single-field selection has child_complexity 1 -> cost 50 + 200 * 1 = 250 <= 400.
+        // Single-field selection has child_complexity 1 -> cost 100 + 200 + 1 = 301 <= 400.
         // It passes validation and reaches the resolver (which yields db error on lazy pool).
         let single_ref = schema.execute("{ refUpdates(limit: 200) { repo } }").await;
         assert!(
@@ -600,13 +600,29 @@ mod tests {
             single_ref.errors
         );
 
-        // Two-field selection has child_complexity 2 -> cost 50 + 200 * 2 = 450 > 400 (rejected before resolver).
+        // Two-field selection has child_complexity 2 -> cost 100 + 200 + 2 = 302 <= 400.
+        // The documented max-200 request stays reachable with multiple fields
+        // (the old multiplicative price rejected it at 450); it passes
+        // validation and reaches the resolver like the single-field case.
         let multi_ref = schema
             .execute("{ refUpdates(limit: 200) { repo refName } }")
             .await;
-        assert_eq!(multi_ref.data, async_graphql::Value::Null);
-        assert_eq!(multi_ref.errors.len(), 1);
-        assert_eq!(multi_ref.errors[0].message, "Query is too complex.");
+        assert!(
+            !multi_ref
+                .errors
+                .iter()
+                .any(|e| e.message == "Query is too complex."),
+            "two-field refUpdates at max limit 200 must pass complexity validation: {:?}",
+            multi_ref.errors
+        );
+        assert!(
+            multi_ref
+                .errors
+                .iter()
+                .any(|e| e.message == GRAPHQL_DB_ERROR_MESSAGE),
+            "two-field refUpdates at max limit 200 must reach the resolver: {:?}",
+            multi_ref.errors
+        );
 
         // tasks(limit: 200):
         // Single-field selection has child_complexity 1 -> cost 50 + 200 * 1 = 250 <= 400.

crates/gitlawb-node/src/graphql/query.rs

@@ -2,6 +2,7 @@ use async_graphql::{Context, Object, Result};
 use base64::{engine::general_purpose::URL_SAFE_NO_PAD, Engine};
 use std::sync::Arc;
 
+use crate::api::events::MAX_VISIBLE_REF_UPDATES;
 use crate::db::{Db, RepoRecord, MAX_VISIBLE_REPO_PAGE_SIZE};
 
 use super::types::{AgentTaskType, RefUpdateType, RepoPageType, RepoType};
@@ -49,8 +50,9 @@ impl QueryRoot {
     // DB-backed roots carry a base cost so aliases consume the request budget
     // even when each alias selects only one inexpensive response field.
     #[graphql(complexity = "50 + child_complexity")]
-    /// Complete visible repository list, up to 200 entries. Larger lists must
-    /// use reposPage; this field returns an error instead of truncating silently.
+    /// Complete visible repository list, up to MAX_VISIBLE_REPO_PAGE_SIZE entries.
+    /// Larger lists must use reposPage; this field returns an error instead of
+    /// truncating silently.
     async fn repos(&self, ctx: &Context<'_>) -> Result<Vec<RepoType>> {
         let db = ctx.data_unchecked::<Arc<Db>>();
         let caller = ctx
@@ -62,9 +64,10 @@ impl QueryRoot {
             .await
             .map_err(crate::graphql::graphql_db_err)?;
         if repos.len() > MAX_VISIBLE_REPO_PAGE_SIZE {
-            return Err(async_graphql::Error::new(
-                "repository list exceeds 200 entries; use reposPage with limit and after",
-            ));
+            return Err(async_graphql::Error::new(format!(
+                "repository list exceeds {} entries; use reposPage with limit and after",
+                MAX_VISIBLE_REPO_PAGE_SIZE
+            )));
         }
         // Preserve the legacy activity ordering for complete, small lists.
         repos.sort_by_key(|repo| std::cmp::Reverse(repo.updated_at));
@@ -73,19 +76,24 @@ impl QueryRoot {
 
     /// Bounded visible repositories ordered by owner and name. Continue with
     /// endCursor while hasNextPage is true. Pages are not a database snapshot.
-    #[graphql(complexity = "50 + (limit.clamp(1, 200) as usize) + child_complexity")]
+    #[graphql(
+        complexity = "50 + (limit.clamp(1, MAX_VISIBLE_REPO_PAGE_SIZE as i64) as usize) + child_complexity"
+    )]
     async fn repos_page(
         &self,
         ctx: &Context<'_>,
         #[graphql(
             default = 50,
-            desc = "Page size from 1 to 200; other values are rejected."
+            desc = "Page size from 1 to MAX_VISIBLE_REPO_PAGE_SIZE; other values are rejected."
         )]
         limit: i64,
         after: Option<String>,
     ) -> Result<RepoPageType> {
         if !(1..=MAX_VISIBLE_REPO_PAGE_SIZE as i64).contains(&limit) {
-            return Err(async_graphql::Error::new("limit must be between 1 and 200"));
+            return Err(async_graphql::Error::new(format!(
+                "limit must be between 1 and {}",
+                MAX_VISIBLE_REPO_PAGE_SIZE
+            )));
         }
         let after = after.as_deref().map(parse_repo_cursor).transpose()?;
         let caller = ctx
@@ -113,7 +121,15 @@ impl QueryRoot {
         })
     }
 
-    #[graphql(complexity = "50 + (limit.clamp(0, 200) as usize) * child_complexity")]
+    // Complexity is additive with a base reflecting the collector's fixed scan:
+    // every request loads the full deduped repo set, the quarantined set, and
+    // the visibility rules, then walks up to max(limit, 2048) event rows past
+    // withheld ones, regardless of `limit`. A multiplicative price both
+    // undercharged limit:1 (51 for that fixed work) and made the documented
+    // max-200 request unreachable (limit:200 with two fields cost 450 > 400).
+    #[graphql(
+        complexity = "100 + (limit.clamp(0, MAX_VISIBLE_REF_UPDATES) as usize) + child_complexity"
+    )]
     async fn ref_updates(
         &self,
         ctx: &Context<'_>,
@@ -453,6 +469,8 @@ mod tests {
             ("root-tie", true),
             ("odd-star", true),
             ("malformed-readers", true),
+            ("non-array-readers", true),
+            ("non-string-reader", true),
             ("quarantined", true),
         ] {
             db.create_repo(&repo(id, OWNER, id, public)).await.unwrap();
@@ -478,19 +496,40 @@ mod tests {
             ("root-tie", "/**", vec![]),
             ("odd-star", "/*", vec![]),
             ("malformed-readers", "/", vec![]),
+            ("non-array-readers", "/", vec![]),
+            ("non-string-reader", "/", vec![]),
         ] {
             db.set_visibility_rule(id, glob, VisibilityMode::B, &readers, OWNER)
                 .await
                 .unwrap();
         }
         // The typed setter cannot create malformed JSON, but existing TEXT rows
         // can contain it. Such a rule must deny this repo, not abort the page.
+        // All three deny arms of the reader_dids predicate are pinned here:
+        // (a) invalid JSON, (b) valid JSON that is not an array, (c) an array
+        // containing a non-string member. The Rust gate parses reader_dids as
+        // Vec<String> with unwrap_or_default(), so (b) and (c) also fail to
+        // parse and deny; the SQL CASE must agree on every shape.
         sqlx::query("UPDATE visibility_rules SET reader_dids = $1 WHERE repo_id = $2")
             .bind("not JSON")
             .bind("malformed-readers")
             .execute(db.pool())
             .await
             .unwrap();
+        sqlx::query("UPDATE visibility_rules SET reader_dids = $1 WHERE repo_id = $2")
+            .bind("\"just-a-string\"")
+            .bind("non-array-readers")
+            .execute(db.pool())
+            .await
+            .unwrap();
+        // The named reader is listed, but the non-string member poisons the
+        // whole list: both sides must still deny them.
+        sqlx::query("UPDATE visibility_rules SET reader_dids = $1 WHERE repo_id = $2")
+            .bind("[\"did:key:zReader\", 42]")
+            .bind("non-string-reader")
+            .execute(db.pool())
+            .await
+            .unwrap();
         let all = db.list_all_repos_deduped().await.unwrap();
         for caller in [
             None,
@@ -691,8 +730,12 @@ mod tests {
                     .await
                     .unwrap();
             assert_eq!(response.errors.len(), 1);
+            let expected_limit_message = format!(
+                "limit must be between 1 and {}",
+                crate::db::MAX_VISIBLE_REPO_PAGE_SIZE
+            );
             assert!(
-                response.errors[0].message == "limit must be between 1 and 200"
+                response.errors[0].message == expected_limit_message
                     || response.errors[0].message == "invalid repository cursor"
             );
         }

docs/RUN-A-NODE.md

@@ -8,6 +8,7 @@ Step-by-step guide to staking $GITLAWB, registering your node on-chain, and earn
 
 - A wallet with at least **10,000 $GITLAWB** (minimum stake) plus a small amount of ETH on Base for gas
 - Docker or Rust 1.91+ (for running the node process)
+- PostgreSQL 16+ — the node uses `pg_input_is_valid`, which exists only on PostgreSQL 16 and later. The bundled `docker-compose.yml` pins `postgres:16-alpine`; if you supply your own `DATABASE_URL`, the server must be 16+.
 - A public HTTP URL (your-host.com) — can be a VPS, Fly.io app, or anything reachable. A Fly.io config is provided at `infra/fly/fly.toml` (deploy from the repo root with `fly deploy -c infra/fly/fly.toml`)
 
 ---