fix(node): opaque GraphQL DB error messages (#250)

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

@@ -14,6 +14,57 @@ use subscription::SubscriptionRoot;
 
 pub type GitlawbSchema = Schema<QueryRoot, MutationRoot, SubscriptionRoot>;
 
+/// Client-facing message for GraphQL resolver failures that wrap a real
+/// `sqlx::Error`. The real error is logged server-side; never put sqlx/Postgres
+/// detail in the GraphQL `errors` array (#250).
+///
+/// Kept as its own constant on this PR's base (main still renders
+/// `AppError::Db` with `e.to_string()`). If/when #247's `DB_ERROR_MESSAGE`
+/// lands, fold this into that shared constant.
+pub const GRAPHQL_DB_ERROR_MESSAGE: &str = "a database error occurred";
+
+fn anyhow_has_sqlx(e: &anyhow::Error) -> bool {
+    e.chain().any(|c| c.downcast_ref::<sqlx::Error>().is_some())
+}
+
+/// Map an `anyhow` failure from the db layer to a GraphQL error.
+///
+/// - Real DB faults (`sqlx::Error` anywhere in the chain) → opaque client
+///   message + `error!` log with the full `{e:#}` cause chain.
+/// - Application/business errors (e.g. claim race, not-in-claimed-state) →
+///   keep the actionable message; log at `warn!` so they are not mistaken for
+///   infrastructure failures (#250 review).
+pub(crate) fn graphql_db_err(e: anyhow::Error) -> async_graphql::Error {
+    if anyhow_has_sqlx(&e) {
+        tracing::error!(error = %format!("{e:#}"), "graphql database error");
+        async_graphql::Error::new(GRAPHQL_DB_ERROR_MESSAGE)
+    } else {
+        tracing::warn!(error = %format!("{e:#}"), "graphql application error");
+        async_graphql::Error::new(e.to_string())
+    }
+}
+
+/// Map an `AppError` from a shared collector (e.g. ref-update feed) to a
+/// GraphQL error.
+///
+/// - `Db` → opaque via [`graphql_db_err`] (sqlx detail stays in logs).
+/// - `Internal` → opaque (may carry raw anyhow/sqlx text on this base).
+/// - Other variants already carry safe, actionable messages → surface them
+///   and log at `warn!` (same posture as business errors in `graphql_db_err`).
+pub(crate) fn graphql_app_err(e: crate::error::AppError) -> async_graphql::Error {
+    match e {
+        crate::error::AppError::Db(sql) => graphql_db_err(sql.into()),
+        crate::error::AppError::Internal(err) => {
+            tracing::error!(error = %format!("{err:#}"), "graphql internal error");
+            async_graphql::Error::new(GRAPHQL_DB_ERROR_MESSAGE)
+        }
+        other => {
+            tracing::warn!(error = %other, "graphql application error");
+            async_graphql::Error::new(other.to_string())
+        }
+    }
+}
+
 pub fn build_schema(
     db: Arc<Db>,
     ref_update_tx: tokio::sync::broadcast::Sender<RefUpdateBroadcast>,
@@ -25,3 +76,59 @@ pub fn build_schema(
         .data(task_event_tx)
         .finish()
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn graphql_db_err_opaques_sqlx_chain() {
+        let leak = "error returned from database: column \"is_public\" does not exist";
+        let err = graphql_db_err(anyhow::Error::from(sqlx::Error::Protocol(leak.into())));
+        assert_eq!(err.message, GRAPHQL_DB_ERROR_MESSAGE);
+        assert!(!err.message.contains("is_public"));
+        assert!(!err.message.contains(leak));
+    }
+
+    #[test]
+    fn graphql_db_err_keeps_business_message() {
+        let msg = "task not claimable: not found or already claimed";
+        let err = graphql_db_err(anyhow::anyhow!("{msg}"));
+        assert_eq!(err.message, msg);
+    }
+
+    #[test]
+    fn graphql_app_err_opaques_db_and_internal() {
+        let leak = "column \"is_public\" does not exist";
+        let db_err = graphql_app_err(crate::error::AppError::Db(sqlx::Error::Protocol(
+            leak.into(),
+        )));
+        assert_eq!(db_err.message, GRAPHQL_DB_ERROR_MESSAGE);
+        assert!(!db_err.message.contains("is_public"));
+
+        let internal = graphql_app_err(crate::error::AppError::Internal(anyhow::anyhow!(
+            "loading repo: {leak}"
+        )));
+        assert_eq!(internal.message, GRAPHQL_DB_ERROR_MESSAGE);
+        assert!(!internal.message.contains("is_public"));
+    }
+
+    #[test]
+    fn graphql_app_err_keeps_safe_variant_messages() {
+        let err = graphql_app_err(crate::error::AppError::NotFound("widget".into()));
+        assert!(
+            err.message.contains("widget"),
+            "safe NotFound message must reach the client: {}",
+            err.message
+        );
+        assert_ne!(err.message, GRAPHQL_DB_ERROR_MESSAGE);
+
+        let err = graphql_app_err(crate::error::AppError::BadRequest("bad cid".into()));
+        assert!(
+            err.message.contains("bad cid"),
+            "safe BadRequest message must reach the client: {}",
+            err.message
+        );
+        assert_ne!(err.message, GRAPHQL_DB_ERROR_MESSAGE);
+    }
+}

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

@@ -54,7 +54,7 @@ impl MutationRoot {
         };
         db.create_task(&task)
             .await
-            .map_err(|e| async_graphql::Error::new(e.to_string()))?;
+            .map_err(crate::graphql::graphql_db_err)?;
         Ok(AgentTaskType::from(task))
     }
 
@@ -76,7 +76,7 @@ impl MutationRoot {
         let task = db
             .claim_task(&id, &assignee_did)
             .await
-            .map_err(|e| async_graphql::Error::new(e.to_string()))?;
+            .map_err(crate::graphql::graphql_db_err)?;
         let _ = tx.send(TaskEventBroadcast {
             task_id: id,
             old_status: "pending".to_string(),
@@ -108,7 +108,7 @@ impl MutationRoot {
         let existing = db
             .get_task(&id)
             .await
-            .map_err(|e| async_graphql::Error::new(e.to_string()))?
+            .map_err(crate::graphql::graphql_db_err)?
             .ok_or_else(|| async_graphql::Error::new("task not found"))?;
         if !crate::api::did_matches(caller, existing.assignee_did.as_deref().unwrap_or_default()) {
             return Err(async_graphql::Error::new(
@@ -118,7 +118,7 @@ impl MutationRoot {
         let task = db
             .finish_task(&id, "completed", input.result.as_deref())
             .await
-            .map_err(|e| async_graphql::Error::new(e.to_string()))?;
+            .map_err(crate::graphql::graphql_db_err)?;
         let _ = tx.send(TaskEventBroadcast {
             task_id: id,
             old_status: "claimed".to_string(),
@@ -149,7 +149,7 @@ impl MutationRoot {
         let existing = db
             .get_task(&id)
             .await
-            .map_err(|e| async_graphql::Error::new(e.to_string()))?
+            .map_err(crate::graphql::graphql_db_err)?
             .ok_or_else(|| async_graphql::Error::new("task not found"))?;
         if !crate::api::did_matches(caller, existing.assignee_did.as_deref().unwrap_or_default()) {
             return Err(async_graphql::Error::new(
@@ -160,7 +160,7 @@ impl MutationRoot {
         let task = db
             .finish_task(&id, "failed", Some(&reason))
             .await
-            .map_err(|e| async_graphql::Error::new(e.to_string()))?;
+            .map_err(crate::graphql::graphql_db_err)?;
         let _ = tx.send(TaskEventBroadcast {
             task_id: id,
             old_status: "claimed".to_string(),
@@ -218,8 +218,9 @@ mod tests {
             errors(&resp)
         );
 
-        // 3. Signed as the claimed assignee → passes the auth gate (any remaining
-        //    error is the missing task, not an auth error).
+        // 3. Signed as the claimed assignee → passes the auth gate. The missing
+        //    task is a business error from claim_task, not a sqlx fault, so the
+        //    actionable message must survive (not the opaque DB string) (#250).
         let resp = schema
             .execute(Request::new(&q).data(AuthenticatedDid(assignee.into())))
             .await;
@@ -228,6 +229,47 @@ mod tests {
             !errs.contains("authentication required") && !errs.contains("authenticated signer"),
             "matching signer must pass the auth gate: {errs}"
         );
+        assert!(
+            errs.contains("task not claimable"),
+            "claim race / missing task must keep its business message: {errs}"
+        );
+        assert!(
+            !errs.contains(crate::graphql::GRAPHQL_DB_ERROR_MESSAGE),
+            "business error must not be rewritten as opaque DB error: {errs}"
+        );
+    }
+
+    /// #250: mutation DB faults must be opaque; create_task hits agent_tasks.
+    #[sqlx::test]
+    async fn create_task_db_error_message_is_opaque(pool: PgPool) {
+        let state = crate::test_support::test_state(pool.clone()).await;
+        sqlx::query("ALTER TABLE agent_tasks DROP COLUMN status")
+            .execute(&pool)
+            .await
+            .unwrap();
+
+        let delegator = "did:key:zGQLDELEGATORAAAAAAAAAAAAAAAAAAAAAAAAAA";
+        let q = format!(
+            r#"mutation {{
+                createTask(
+                    delegatorDid: "{delegator}",
+                    input: {{ kind: "build", capability: "repo:write" }}
+                ) {{ id }}
+            }}"#
+        );
+        let resp = state
+            .graphql_schema
+            .execute(Request::new(&q).data(AuthenticatedDid(delegator.into())))
+            .await;
+        let errs = errors(&resp);
+        assert!(
+            errs.contains(crate::graphql::GRAPHQL_DB_ERROR_MESSAGE),
+            "sqlx fault must be opaque: {errs}"
+        );
+        assert!(
+            !errs.contains("column") && !errs.contains("status"),
+            "schema text leaked: {errs}"
+        );
     }
 
     /// Adversarial-review GATE-1 (GraphQL): completing a task requires being its

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

@@ -14,7 +14,7 @@ impl QueryRoot {
         let repos = db
             .list_all_repos_deduped()
             .await
-            .map_err(|e| async_graphql::Error::new(e.to_string()))?;
+            .map_err(crate::graphql::graphql_db_err)?;
 
         // Apply the same "/" visibility gate the REST/per-repo endpoints use so
         // this surface does not enumerate private repos (#97). The caller DID is
@@ -27,7 +27,7 @@ impl QueryRoot {
         let rules_by_repo = db
             .list_visibility_rules_for_repos(&ids)
             .await
-            .map_err(|e| async_graphql::Error::new(e.to_string()))?;
+            .map_err(crate::graphql::graphql_db_err)?;
 
         Ok(repos
             .into_iter()
@@ -71,7 +71,7 @@ impl QueryRoot {
         let updates =
             crate::api::events::collect_visible_ref_updates(db, repo.as_deref(), limit, caller)
                 .await
-                .map_err(|e| async_graphql::Error::new(e.to_string()))?;
+                .map_err(crate::graphql::graphql_app_err)?;
 
         // Resolve the trusted display owner_did per row, identical to the REST
         // feed: the stored wire value is untrusted, so it is echoed only when it
@@ -86,7 +86,7 @@ impl QueryRoot {
         let owner_dids = db
             .resolve_ref_update_owner_dids(&pairs)
             .await
-            .map_err(|e| async_graphql::Error::new(e.to_string()))?;
+            .map_err(crate::graphql::graphql_db_err)?;
 
         let resolved: Vec<RefUpdateType> = updates
             .into_iter()
@@ -110,13 +110,21 @@ impl QueryRoot {
         ctx: &Context<'_>,
         status: Option<String>,
         assignee_did: Option<String>,
-        #[graphql(default = 50)] limit: i64,
+        #[graphql(
+            default = 50,
+            desc = "Max 200; larger requests are clamped to 200 (no error). Negative values clamp to 0."
+        )]
+        limit: i64,
     ) -> Result<Vec<AgentTaskType>> {
         let db = ctx.data_unchecked::<Arc<Db>>();
+        // Clamp before SQL: a negative LIMIT is a client fault that Postgres
+        // rejects with 2201W, which would otherwise trip the opaque DB path
+        // and write an error-level log on every probe (#250 review).
+        let limit = limit.clamp(0, 200);
         let tasks = db
             .list_tasks(status.as_deref(), assignee_did.as_deref(), limit)
             .await
-            .map_err(|e| async_graphql::Error::new(e.to_string()))?;
+            .map_err(crate::graphql::graphql_db_err)?;
         Ok(tasks.into_iter().map(AgentTaskType::from).collect())
     }
 
@@ -125,7 +133,7 @@ impl QueryRoot {
         let t = db
             .get_task(&id)
             .await
-            .map_err(|e| async_graphql::Error::new(e.to_string()))?;
+            .map_err(crate::graphql::graphql_db_err)?;
         Ok(t.map(AgentTaskType::from))
     }
 }
@@ -419,4 +427,62 @@ mod tests {
         let q = r#"{ refUpdates { repo } }"#;
         assert_eq!(count(&authed(&schema, q, "did:key:z6MkQuar").await), 0);
     }
+
+    /// #250: anonymous GraphQL query DB failures must not leak sqlx/schema text.
+    #[sqlx::test]
+    async fn repos_query_db_error_message_is_opaque(pool: PgPool) {
+        let db = db(pool.clone()).await;
+        db.create_repo(&repo("r1", OWNER, "widget", true))
+            .await
+            .unwrap();
+        sqlx::query("ALTER TABLE repos DROP COLUMN is_public")
+            .execute(&pool)
+            .await
+            .unwrap();
+
+        let schema = schema(db);
+        let resp = anon(&schema, "{ repos { name ownerDid } }").await;
+        assert!(
+            !resp.errors.is_empty(),
+            "DB failure must surface as a GraphQL error"
+        );
+        for err in &resp.errors {
+            assert_eq!(
+                err.message,
+                crate::graphql::GRAPHQL_DB_ERROR_MESSAGE,
+                "raw DB detail leaked into GraphQL error: {}",
+                err.message
+            );
+            assert!(
+                !err.message.contains("is_public") && !err.message.contains("column"),
+                "schema text leaked: {}",
+                err.message
+            );
+        }
+    }
+
+    /// #250: negative tasks(limit) must not hit Postgres (and must not 500-log).
+    #[sqlx::test]
+    async fn tasks_negative_limit_clamped(pool: PgPool) {
+        let db = db(pool).await;
+        let schema = schema(db);
+        let resp = anon(&schema, "{ tasks(limit: -1) { id } }").await;
+        assert!(
+            resp.errors.is_empty(),
+            "negative limit must clamp, not fail: {:?}",
+            resp.errors
+        );
+        assert_eq!(count_tasks(&resp), 0);
+    }
+
+    fn count_tasks(resp: &async_graphql::Response) -> usize {
+        assert!(resp.errors.is_empty(), "graphql errors: {:?}", resp.errors);
+        let async_graphql::Value::Object(obj) = &resp.data else {
+            panic!("data not an object: {:?}", resp.data);
+        };
+        let async_graphql::Value::List(rows) = obj.get("tasks").expect("tasks key") else {
+            panic!("tasks not a list");
+        };
+        rows.len()
+    }
 }