From 5c1b595a21c7acdad53b1ff49f9803353fb2ce80 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 14 Jun 2026 17:08:45 +0000 Subject: [PATCH 1/3] feat(soup): add notification filters to foreign entity (GitHub PR) queries Foreign entities (GitHub PRs) fetched from soup can now be filtered by the requesting user's notification state, matching the done/seen filters already available for documents, chats, emails, projects, and channels. - Add `notification_filters` to `ForeignEntityFilters` and expand it into `nd`/`ns` AST literals. - In the foreign entity repo, hoist the notification literals (alongside `includes_me`) out of the metadata jsonpath and apply them as `EXISTS` predicates against `notification`/`user_notification`, scoped to the requesting user and the `foreign_entity` event type. A notification filter with no requesting user matches nothing. - Regenerate the storage/search OpenAPI + TypeScript clients and the sqlx cache. https://claude.ai/code/session_01V3PQwKGcmnjAdhQDhkG1De --- .../generated/models/foreignEntityFilters.ts | 4 + .../service-search/openapi.json | 4 + .../generated/schemas/foreignEntityFilters.ts | 4 + .../service-storage/generated/zod.ts | 17 ++ .../service-storage/openapi.json | 4 + ...f2d433780b1d32441dcddc6350501de54260.json} | 7 +- .../src/outbound/pg_foreign_entity_repo.rs | 177 ++++++++++--- .../outbound/pg_foreign_entity_repo/tests.rs | 240 ++++++++++++++++++ .../item_filters/src/ast/foreign_entity.rs | 29 ++- .../item_filters/src/ast/tests.rs | 67 +++++ rust/cloud-storage/item_filters/src/lib.rs | 7 + .../soup/src/domain/service/tests.rs | 8 +- 12 files changed, 525 insertions(+), 43 deletions(-) rename rust/cloud-storage/.sqlx/{query-e4eb2173ecfc39987f998d5075237c0b8aaf00c3eae94c06f069591f72445e14.json => query-d45423d79a30cd22b475c082ccb2f2d433780b1d32441dcddc6350501de54260.json} (58%) diff --git a/js/app/packages/service-clients/service-search/generated/models/foreignEntityFilters.ts b/js/app/packages/service-clients/service-search/generated/models/foreignEntityFilters.ts index 6bea6ed0a2..0367780948 100644 --- a/js/app/packages/service-clients/service-search/generated/models/foreignEntityFilters.ts +++ b/js/app/packages/service-clients/service-search/generated/models/foreignEntityFilters.ts @@ -4,6 +4,7 @@ * search_service * OpenAPI spec version: 0.1.0 */ +import type { NotificationFilters } from './notificationFilters'; /** * Filters for foreign entity records. @@ -19,4 +20,7 @@ export interface ForeignEntityFilters { participant (GitHub `involves:me` semantics for `github_pull_request` records). False or absent applies no filter. Serialized in filter ASTs as the `"me"` literal. */ includes_me?: boolean; + /** Filter by the notification state of the foreign entity (e.g. whether the requesting user's +GitHub PR notification is done/seen). */ + notification_filters?: NotificationFilters; } diff --git a/js/app/packages/service-clients/service-search/openapi.json b/js/app/packages/service-clients/service-search/openapi.json index 422bd8f38d..dd325162e3 100644 --- a/js/app/packages/service-clients/service-search/openapi.json +++ b/js/app/packages/service-clients/service-search/openapi.json @@ -2105,6 +2105,10 @@ "includes_me": { "type": "boolean", "description": "When true, only return foreign entities whose metadata lists the requesting user as a\nparticipant (GitHub `involves:me` semantics for `github_pull_request` records). False or\nabsent applies no filter. Serialized in filter ASTs as the `\"me\"` literal." + }, + "notification_filters": { + "$ref": "#/components/schemas/NotificationFilters", + "description": "Filter by the notification state of the foreign entity (e.g. whether the requesting user's\nGitHub PR notification is done/seen)." } } }, diff --git a/js/app/packages/service-clients/service-storage/generated/schemas/foreignEntityFilters.ts b/js/app/packages/service-clients/service-storage/generated/schemas/foreignEntityFilters.ts index a6a64604bb..f744e54969 100644 --- a/js/app/packages/service-clients/service-storage/generated/schemas/foreignEntityFilters.ts +++ b/js/app/packages/service-clients/service-storage/generated/schemas/foreignEntityFilters.ts @@ -4,6 +4,7 @@ * document_storage_service * OpenAPI spec version: 0.1.0 */ +import type { NotificationFilters } from './notificationFilters'; /** * Filters for foreign entity records. @@ -19,4 +20,7 @@ export interface ForeignEntityFilters { participant (GitHub `involves:me` semantics for `github_pull_request` records). False or absent applies no filter. Serialized in filter ASTs as the `"me"` literal. */ includes_me?: boolean; + /** Filter by the notification state of the foreign entity (e.g. whether the requesting user's +GitHub PR notification is done/seen). */ + notification_filters?: NotificationFilters; } diff --git a/js/app/packages/service-clients/service-storage/generated/zod.ts b/js/app/packages/service-clients/service-storage/generated/zod.ts index ebda929fd0..652215e7d0 100644 --- a/js/app/packages/service-clients/service-storage/generated/zod.ts +++ b/js/app/packages/service-clients/service-storage/generated/zod.ts @@ -7998,6 +7998,23 @@ export const postItemsSoupBody = zod .describe( 'When true, only return foreign entities whose metadata lists the requesting user as a\nparticipant (GitHub `involves:me` semantics for `github_pull_request` records). False or\nabsent applies no filter. Serialized in filter ASTs as the `\"me\"` literal.' ), + notification_filters: zod + .object({ + done: zod + .boolean() + .nullish() + .describe( + 'Filter by notification done state. `Some(true)` selects done\nnotifications; `Some(false)` selects not-done notifications.' + ), + seen: zod + .boolean() + .nullish() + .describe( + 'Filter by notification seen state. `Some(true)` selects seen\nnotifications; `Some(false)` selects not-seen notifications.' + ), + }) + .optional() + .describe('Notification state filters for channel message queries.'), }) .optional() .describe('Filters for foreign entity records.'), diff --git a/js/app/packages/service-clients/service-storage/openapi.json b/js/app/packages/service-clients/service-storage/openapi.json index 2e85463d68..4d4c87f9bd 100644 --- a/js/app/packages/service-clients/service-storage/openapi.json +++ b/js/app/packages/service-clients/service-storage/openapi.json @@ -14059,6 +14059,10 @@ "includes_me": { "type": "boolean", "description": "When true, only return foreign entities whose metadata lists the requesting user as a\nparticipant (GitHub `involves:me` semantics for `github_pull_request` records). False or\nabsent applies no filter. Serialized in filter ASTs as the `\"me\"` literal." + }, + "notification_filters": { + "$ref": "#/components/schemas/NotificationFilters", + "description": "Filter by the notification state of the foreign entity (e.g. whether the requesting user's\nGitHub PR notification is done/seen)." } } }, diff --git a/rust/cloud-storage/.sqlx/query-e4eb2173ecfc39987f998d5075237c0b8aaf00c3eae94c06f069591f72445e14.json b/rust/cloud-storage/.sqlx/query-d45423d79a30cd22b475c082ccb2f2d433780b1d32441dcddc6350501de54260.json similarity index 58% rename from rust/cloud-storage/.sqlx/query-e4eb2173ecfc39987f998d5075237c0b8aaf00c3eae94c06f069591f72445e14.json rename to rust/cloud-storage/.sqlx/query-d45423d79a30cd22b475c082ccb2f2d433780b1d32441dcddc6350501de54260.json index 0c2c26e399..9d7cbd8b72 100644 --- a/rust/cloud-storage/.sqlx/query-e4eb2173ecfc39987f998d5075237c0b8aaf00c3eae94c06f069591f72445e14.json +++ b/rust/cloud-storage/.sqlx/query-d45423d79a30cd22b475c082ccb2f2d433780b1d32441dcddc6350501de54260.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "\n WITH source_ids AS (\n SELECT DISTINCT stored_for_id, stored_for_auth_entity\n FROM UNNEST($1::text[], $2::text[])\n AS source_rows(stored_for_id, stored_for_auth_entity)\n ),\n deduped AS (\n SELECT DISTINCT ON (fe.foreign_entity_source, fe.foreign_entity_id)\n fe.id,\n fe.foreign_entity_id,\n fe.foreign_entity_source,\n fe.metadata,\n fe.stored_for_id,\n fe.stored_for_auth_entity,\n fe.created_at,\n fe.updated_at,\n CASE $3::text\n WHEN 'created_at' THEN fe.created_at\n ELSE fe.updated_at\n END AS sort_at\n FROM foreign_entity fe\n WHERE EXISTS (\n SELECT 1\n FROM source_ids s\n WHERE s.stored_for_id = fe.stored_for_id\n AND s.stored_for_auth_entity = fe.stored_for_auth_entity\n )\n AND (\n $4::text IS NULL\n OR jsonb_path_match(\n jsonb_build_object(\n 'id', fe.id::text,\n 'foreignEntityId', fe.foreign_entity_id,\n 'foreignEntitySource', fe.foreign_entity_source\n ),\n ($4::text)::jsonpath\n )\n )\n AND (\n $8::text IS NULL\n OR (fe.metadata -> 'participantGithubUserIds') ? $8::text\n )\n ORDER BY fe.foreign_entity_source, fe.foreign_entity_id, sort_at DESC, fe.id DESC\n )\n SELECT\n id as \"id!: Uuid\",\n foreign_entity_id as \"foreign_entity_id!: String\",\n foreign_entity_source as \"foreign_entity_source!: String\",\n metadata as \"metadata!: serde_json::Value\",\n stored_for_id as \"stored_for_id!: String\",\n stored_for_auth_entity as \"stored_for_auth_entity!: String\",\n created_at as \"created_at!: DateTime\",\n updated_at as \"updated_at!: DateTime\"\n FROM deduped\n WHERE $5::timestamptz IS NULL\n OR (sort_at, id) < ($5::timestamptz, $6::uuid)\n ORDER BY sort_at DESC, id DESC\n LIMIT $7\n ", + "query": "\n WITH source_ids AS (\n SELECT DISTINCT stored_for_id, stored_for_auth_entity\n FROM UNNEST($1::text[], $2::text[])\n AS source_rows(stored_for_id, stored_for_auth_entity)\n ),\n deduped AS (\n SELECT DISTINCT ON (fe.foreign_entity_source, fe.foreign_entity_id)\n fe.id,\n fe.foreign_entity_id,\n fe.foreign_entity_source,\n fe.metadata,\n fe.stored_for_id,\n fe.stored_for_auth_entity,\n fe.created_at,\n fe.updated_at,\n CASE $3::text\n WHEN 'created_at' THEN fe.created_at\n ELSE fe.updated_at\n END AS sort_at\n FROM foreign_entity fe\n WHERE EXISTS (\n SELECT 1\n FROM source_ids s\n WHERE s.stored_for_id = fe.stored_for_id\n AND s.stored_for_auth_entity = fe.stored_for_auth_entity\n )\n AND (\n $4::text IS NULL\n OR jsonb_path_match(\n jsonb_build_object(\n 'id', fe.id::text,\n 'foreignEntityId', fe.foreign_entity_id,\n 'foreignEntitySource', fe.foreign_entity_source\n ),\n ($4::text)::jsonpath\n )\n )\n AND (\n $8::text IS NULL\n OR (fe.metadata -> 'participantGithubUserIds') ? $8::text\n )\n AND (\n $9::bool IS NULL\n OR EXISTS (\n SELECT 1\n FROM notification n\n JOIN user_notification un ON un.notification_id = n.id\n WHERE un.user_id = $11::text\n AND un.deleted_at IS NULL\n AND n.event_item_type = 'foreign_entity'\n AND n.event_item_id = fe.id::text\n AND un.done = $9::bool\n )\n )\n AND (\n $10::bool IS NULL\n OR EXISTS (\n SELECT 1\n FROM notification n\n JOIN user_notification un ON un.notification_id = n.id\n WHERE un.user_id = $11::text\n AND un.deleted_at IS NULL\n AND n.event_item_type = 'foreign_entity'\n AND n.event_item_id = fe.id::text\n AND (un.seen_at IS NOT NULL) = $10::bool\n )\n )\n ORDER BY fe.foreign_entity_source, fe.foreign_entity_id, sort_at DESC, fe.id DESC\n )\n SELECT\n id as \"id!: Uuid\",\n foreign_entity_id as \"foreign_entity_id!: String\",\n foreign_entity_source as \"foreign_entity_source!: String\",\n metadata as \"metadata!: serde_json::Value\",\n stored_for_id as \"stored_for_id!: String\",\n stored_for_auth_entity as \"stored_for_auth_entity!: String\",\n created_at as \"created_at!: DateTime\",\n updated_at as \"updated_at!: DateTime\"\n FROM deduped\n WHERE $5::timestamptz IS NULL\n OR (sort_at, id) < ($5::timestamptz, $6::uuid)\n ORDER BY sort_at DESC, id DESC\n LIMIT $7\n ", "describe": { "columns": [ { @@ -53,6 +53,9 @@ "Timestamptz", "Uuid", "Int8", + "Text", + "Bool", + "Bool", "Text" ] }, @@ -67,5 +70,5 @@ false ] }, - "hash": "e4eb2173ecfc39987f998d5075237c0b8aaf00c3eae94c06f069591f72445e14" + "hash": "d45423d79a30cd22b475c082ccb2f2d433780b1d32441dcddc6350501de54260" } diff --git a/rust/cloud-storage/foreign_entity/src/outbound/pg_foreign_entity_repo.rs b/rust/cloud-storage/foreign_entity/src/outbound/pg_foreign_entity_repo.rs index 5b9b0d28ba..0238c087b5 100644 --- a/rust/cloud-storage/foreign_entity/src/outbound/pg_foreign_entity_repo.rs +++ b/rust/cloud-storage/foreign_entity/src/outbound/pg_foreign_entity_repo.rs @@ -19,6 +19,13 @@ struct ForeignEntityBatchQuery<'a> { sort_method: SimpleSortMethod, filter_jsonpath: Option<&'a str>, participant_github_user_id: Option<&'a str>, + /// Macro user id used to scope the per-user notification done/seen predicates. + /// When a notification filter is requested but this is `None`, nothing matches. + notification_user_id: Option<&'a str>, + /// `Some(true)`/`Some(false)` keeps entities whose notification is/ isn't done; `None` ignores. + notification_done: Option, + /// `Some(true)`/`Some(false)` keeps entities whose notification is/ isn't seen; `None` ignores. + notification_seen: Option, cursor_id: Option, cursor_value: Option>, limit: i64, @@ -31,43 +38,97 @@ fn source_id_parts(source_ids: &[SourceId]) -> (Vec, Vec) { .unzip() } -/// Marker error for filters that place [`ForeignEntityLiteral::IncludesMe`] somewhere it cannot -/// be hoisted into the dedicated SQL predicate (under `Or`/`Not`). -struct UnsupportedIncludesMe; +/// Marker error for filters that place a hoisted literal (see [`is_hoisted_literal`]) somewhere it +/// cannot be lifted into a dedicated SQL predicate (under `Or`/`Not`). +struct UnsupportedHoistedFilter; -fn contains_includes_me(expr: &Expr) -> bool { +/// Filters lifted off the top-level AND spine into dedicated SQL predicates because they cannot be +/// expressed in the metadata jsonpath (they need indexed-containment or notification-table joins). +#[derive(Default)] +struct HoistedForeignEntityFilters { + /// Whether the requesting user must be a participant in the entity's metadata. + includes_me: bool, + /// Notification done filter for the requesting user (`None` ignores). + notification_done: Option, + /// Notification seen filter for the requesting user (`None` ignores). + notification_seen: Option, + /// jsonpath for the residual (non-hoisted) filter, if any. + jsonpath: Option, +} + +impl HoistedForeignEntityFilters { + /// Combine two extracted filter halves taken from the two sides of an `And`. + fn and(self, other: Self) -> Self { + let jsonpath = match (self.jsonpath, other.jsonpath) { + (Some(left), Some(right)) => Some(format!("({left} && {right})")), + (left, right) => left.or(right), + }; + Self { + includes_me: self.includes_me || other.includes_me, + notification_done: self.notification_done.or(other.notification_done), + notification_seen: self.notification_seen.or(other.notification_seen), + jsonpath, + } + } +} + +/// Literals that cannot be represented in the metadata jsonpath and must be lifted into dedicated +/// SQL predicates instead. +fn is_hoisted_literal(literal: &ForeignEntityLiteral) -> bool { + matches!( + literal, + ForeignEntityLiteral::IncludesMe + | ForeignEntityLiteral::NotificationDone(_) + | ForeignEntityLiteral::NotificationSeen(_) + ) +} + +fn contains_hoisted_literal(expr: &Expr) -> bool { match expr { Expr::And(left, right) | Expr::Or(left, right) => { - contains_includes_me(left) || contains_includes_me(right) + contains_hoisted_literal(left) || contains_hoisted_literal(right) } - Expr::Not(inner) => contains_includes_me(inner), - Expr::Literal(literal) => matches!(literal, ForeignEntityLiteral::IncludesMe), + Expr::Not(inner) => contains_hoisted_literal(inner), + Expr::Literal(literal) => is_hoisted_literal(literal), } } -/// Strip [`ForeignEntityLiteral::IncludesMe`] literals off the top-level AND spine of a filter -/// tree, returning the flag and the jsonpath for the residual filter. The literal cannot be -/// expressed in the jsonpath (it needs the indexed metadata containment predicate), so any -/// occurrence under `Or`/`Not` is an error. -fn extract_includes_me( +/// Strip hoisted literals ([`ForeignEntityLiteral::IncludesMe`], notification done/seen) off the +/// top-level AND spine of a filter tree, returning them alongside the jsonpath for the residual +/// filter. Hoisted literals cannot be expressed in the jsonpath (they need the indexed metadata +/// containment predicate or a join against the notification tables), so any occurrence under +/// `Or`/`Not` is an error. +fn extract_hoisted_filters( expr: &Expr, -) -> Result<(bool, Option), UnsupportedIncludesMe> { +) -> Result { match expr { Expr::And(left, right) => { - let (left_me, left_jsonpath) = extract_includes_me(left)?; - let (right_me, right_jsonpath) = extract_includes_me(right)?; - let jsonpath = match (left_jsonpath, right_jsonpath) { - (Some(left), Some(right)) => Some(format!("({left} && {right})")), - (left, right) => left.or(right), - }; - Ok((left_me || right_me, jsonpath)) + Ok(extract_hoisted_filters(left)?.and(extract_hoisted_filters(right)?)) + } + Expr::Literal(ForeignEntityLiteral::IncludesMe) => Ok(HoistedForeignEntityFilters { + includes_me: true, + ..Default::default() + }), + Expr::Literal(ForeignEntityLiteral::NotificationDone(done)) => { + Ok(HoistedForeignEntityFilters { + notification_done: Some(*done), + ..Default::default() + }) + } + Expr::Literal(ForeignEntityLiteral::NotificationSeen(seen)) => { + Ok(HoistedForeignEntityFilters { + notification_seen: Some(*seen), + ..Default::default() + }) } - Expr::Literal(ForeignEntityLiteral::IncludesMe) => Ok((true, None)), other => { - if contains_includes_me(other) { - Err(UnsupportedIncludesMe) + if contains_hoisted_literal(other) { + Err(UnsupportedHoistedFilter) } else { - Ok((false, Some(foreign_entity_expr_jsonpath(other)))) + Ok(HoistedForeignEntityFilters { + jsonpath: Some(foreign_entity_expr_jsonpath(other)), + ..Default::default() + }) } } } @@ -97,9 +158,12 @@ fn foreign_entity_literal_jsonpath(literal: &ForeignEntityLiteral) -> String { ForeignEntityLiteral::ForeignEntitySource(source) => { jsonpath_text_eq("foreignEntitySource", source) } - // IncludesMe is hoisted into a dedicated SQL predicate by extract_includes_me and never - // reaches the jsonpath; if one slips through, match nothing rather than everything. - ForeignEntityLiteral::IncludesMe => "(1 == 0)".to_string(), + // IncludesMe and the notification literals are hoisted into dedicated SQL predicates by + // extract_hoisted_filters and never reach the jsonpath; if one slips through, match nothing + // rather than everything. + ForeignEntityLiteral::IncludesMe + | ForeignEntityLiteral::NotificationDone(_) + | ForeignEntityLiteral::NotificationSeen(_) => "(1 == 0)".to_string(), } } @@ -131,6 +195,9 @@ impl PgForeignEntityRepo { sort_method, filter_jsonpath, participant_github_user_id, + notification_user_id, + notification_done, + notification_seen, cursor_id, cursor_value, limit, @@ -181,6 +248,32 @@ impl PgForeignEntityRepo { $8::text IS NULL OR (fe.metadata -> 'participantGithubUserIds') ? $8::text ) + AND ( + $9::bool IS NULL + OR EXISTS ( + SELECT 1 + FROM notification n + JOIN user_notification un ON un.notification_id = n.id + WHERE un.user_id = $11::text + AND un.deleted_at IS NULL + AND n.event_item_type = 'foreign_entity' + AND n.event_item_id = fe.id::text + AND un.done = $9::bool + ) + ) + AND ( + $10::bool IS NULL + OR EXISTS ( + SELECT 1 + FROM notification n + JOIN user_notification un ON un.notification_id = n.id + WHERE un.user_id = $11::text + AND un.deleted_at IS NULL + AND n.event_item_type = 'foreign_entity' + AND n.event_item_id = fe.id::text + AND (un.seen_at IS NOT NULL) = $10::bool + ) + ) ORDER BY fe.foreign_entity_source, fe.foreign_entity_id, sort_at DESC, fe.id DESC ) SELECT @@ -206,6 +299,9 @@ impl PgForeignEntityRepo { cursor_id, limit, participant_github_user_id, + notification_done, + notification_seen, + notification_user_id, ) .fetch_all(&self.pool) .await @@ -297,26 +393,31 @@ impl ForeignEntityRepository for PgForeignEntityRepo { return Ok(Vec::new()); } - let (includes_me, filter_jsonpath) = match query + let HoistedForeignEntityFilters { + includes_me, + notification_done, + notification_seen, + jsonpath: filter_jsonpath, + } = match query .filter() .as_deref() - .map(extract_includes_me) + .map(extract_hoisted_filters) .transpose() { - Ok(split) => split.unwrap_or((false, None)), - Err(UnsupportedIncludesMe) => { + Ok(hoisted) => hoisted.unwrap_or_default(), + Err(UnsupportedHoistedFilter) => { tracing::warn!( - "IncludesMe under Or/Not in a foreign entity filter is unsupported; returning no results" + "IncludesMe/notification literal under Or/Not in a foreign entity filter is unsupported; returning no results" ); return Ok(Vec::new()); } }; let participant_github_user_id = if includes_me { - let Some(requesting_user) = requesting_user else { + let Some(requesting_user) = requesting_user.as_deref() else { return Ok(Vec::new()); }; - match self.github_user_id_for_macro_user(&requesting_user).await? { + match self.github_user_id_for_macro_user(requesting_user).await? { Some(github_user_id) => Some(github_user_id), // No linked GitHub identity: the user participates in nothing. None => return Ok(Vec::new()), @@ -325,6 +426,11 @@ impl ForeignEntityRepository for PgForeignEntityRepo { None }; + // Notification done/seen are scoped to the requesting user's per-user notification row. + // Without a requesting user the predicate matches nothing, so an active notification + // filter yields no results (consistent with the participant filter above). + let notification_user_id = requesting_user.as_deref(); + let (source_ids, source_auth_entities) = source_id_parts(&source_ids); let (cursor_id, cursor_value) = query.vals(); @@ -334,6 +440,9 @@ impl ForeignEntityRepository for PgForeignEntityRepo { sort_method: *query.sort_method(), filter_jsonpath: filter_jsonpath.as_deref(), participant_github_user_id: participant_github_user_id.as_deref(), + notification_user_id, + notification_done, + notification_seen, cursor_id: cursor_id.copied(), cursor_value: cursor_value.copied(), limit: limit as i64, diff --git a/rust/cloud-storage/foreign_entity/src/outbound/pg_foreign_entity_repo/tests.rs b/rust/cloud-storage/foreign_entity/src/outbound/pg_foreign_entity_repo/tests.rs index f5204bcdc8..f1a96c760f 100644 --- a/rust/cloud-storage/foreign_entity/src/outbound/pg_foreign_entity_repo/tests.rs +++ b/rust/cloud-storage/foreign_entity/src/outbound/pg_foreign_entity_repo/tests.rs @@ -739,6 +739,246 @@ async fn get_for_user_includes_me_under_not_fails_closed(pool: PgPool) { assert!(entities.is_empty()); } +/// Insert a `foreign_entity`-scoped notification and the matching per-user row so the +/// notification done/seen predicates have something to match against. +async fn insert_foreign_entity_notification( + pool: &PgPool, + foreign_entity_id: Uuid, + user_id: &str, + done: bool, + seen: bool, +) { + let notification_id = Uuid::now_v7(); + sqlx::query( + r#" + INSERT INTO notification (id, notification_event_type, event_item_id, event_item_type, service_sender) + VALUES ($1, 'github_pr_status_changed', $2, 'foreign_entity', 'test') + "#, + ) + .bind(notification_id) + .bind(foreign_entity_id.to_string()) + .execute(pool) + .await + .expect("notification row should be inserted"); + + let seen_at: Option = seen.then(|| Utc::now().naive_utc()); + sqlx::query( + r#" + INSERT INTO user_notification (user_id, notification_id, done, seen_at) + VALUES ($1, $2, $3, $4) + "#, + ) + .bind(user_id) + .bind(notification_id) + .bind(done) + .bind(seen_at) + .execute(pool) + .await + .expect("user_notification row should be inserted"); +} + +fn notification_done_filter(done: bool) -> LiteralTree { + Some(Arc::new(Expr::val(ForeignEntityLiteral::NotificationDone( + done, + )))) +} + +fn notification_seen_filter(seen: bool) -> LiteralTree { + Some(Arc::new(Expr::val(ForeignEntityLiteral::NotificationSeen( + seen, + )))) +} + +#[sqlx::test(migrator = "MACRO_DB_MIGRATIONS")] +async fn get_for_user_notification_done_filters_by_done_state(pool: PgPool) { + let repo = PgForeignEntityRepo::new(pool.clone()); + let macro_id = "macro|user@example.com"; + + let done = + insert_foreign_entity_for_source(&repo, "done-pr", "github_pull_request", macro_id, "user") + .await; + let not_done = insert_foreign_entity_for_source( + &repo, + "not-done-pr", + "github_pull_request", + macro_id, + "user", + ) + .await; + // No notification at all: matches neither done=true nor done=false. + insert_foreign_entity_for_source( + &repo, + "no-notif-pr", + "github_pull_request", + macro_id, + "user", + ) + .await; + + insert_foreign_entity_notification(&pool, done.id, macro_id, true, false).await; + insert_foreign_entity_notification(&pool, not_done.id, macro_id, false, false).await; + + let done_matches = repo + .get_foreign_entities_for_user( + Some(macro_id.to_string()), + vec![SourceId::user(macro_id)], + 10, + filter_query(notification_done_filter(true)), + ) + .await + .expect("done=true filter should be applied"); + + let not_done_matches = repo + .get_foreign_entities_for_user( + Some(macro_id.to_string()), + vec![SourceId::user(macro_id)], + 10, + filter_query(notification_done_filter(false)), + ) + .await + .expect("done=false filter should be applied"); + + assert_eq!(done_matches, vec![done]); + assert_eq!(not_done_matches, vec![not_done]); +} + +#[sqlx::test(migrator = "MACRO_DB_MIGRATIONS")] +async fn get_for_user_notification_seen_filters_by_seen_state(pool: PgPool) { + let repo = PgForeignEntityRepo::new(pool.clone()); + let macro_id = "macro|user@example.com"; + + let seen = + insert_foreign_entity_for_source(&repo, "seen-pr", "github_pull_request", macro_id, "user") + .await; + let unseen = insert_foreign_entity_for_source( + &repo, + "unseen-pr", + "github_pull_request", + macro_id, + "user", + ) + .await; + + insert_foreign_entity_notification(&pool, seen.id, macro_id, false, true).await; + insert_foreign_entity_notification(&pool, unseen.id, macro_id, false, false).await; + + let seen_matches = repo + .get_foreign_entities_for_user( + Some(macro_id.to_string()), + vec![SourceId::user(macro_id)], + 10, + filter_query(notification_seen_filter(true)), + ) + .await + .expect("seen=true filter should be applied"); + + let unseen_matches = repo + .get_foreign_entities_for_user( + Some(macro_id.to_string()), + vec![SourceId::user(macro_id)], + 10, + filter_query(notification_seen_filter(false)), + ) + .await + .expect("seen=false filter should be applied"); + + assert_eq!(seen_matches, vec![seen]); + assert_eq!(unseen_matches, vec![unseen]); +} + +#[sqlx::test(migrator = "MACRO_DB_MIGRATIONS")] +async fn get_for_user_notification_done_composes_with_source(pool: PgPool) { + let repo = PgForeignEntityRepo::new(pool.clone()); + let macro_id = "macro|user@example.com"; + + let github = insert_foreign_entity_for_source( + &repo, + "github-pr", + "github_pull_request", + macro_id, + "user", + ) + .await; + let linear = + insert_foreign_entity_for_source(&repo, "linear-issue", "linear_issue", macro_id, "user") + .await; + + insert_foreign_entity_notification(&pool, github.id, macro_id, true, false).await; + insert_foreign_entity_notification(&pool, linear.id, macro_id, true, false).await; + + let filter = Some(Arc::new(Expr::and( + Expr::val(ForeignEntityLiteral::ForeignEntitySource( + "github_pull_request".to_string(), + )), + Expr::val(ForeignEntityLiteral::NotificationDone(true)), + ))); + + let matches = repo + .get_foreign_entities_for_user( + Some(macro_id.to_string()), + vec![SourceId::user(macro_id)], + 10, + filter_query(filter), + ) + .await + .expect("source AND notification done filter should be applied"); + + assert_eq!(matches, vec![github]); +} + +#[sqlx::test(migrator = "MACRO_DB_MIGRATIONS")] +async fn get_for_user_notification_done_scopes_to_requesting_user(pool: PgPool) { + let repo = PgForeignEntityRepo::new(pool.clone()); + let macro_id = "macro|user@example.com"; + let other_id = "macro|other@example.com"; + + let entity = insert_foreign_entity_for_source( + &repo, + "shared-pr", + "github_pull_request", + macro_id, + "user", + ) + .await; + // The done notification belongs to a different user. + insert_foreign_entity_notification(&pool, entity.id, other_id, true, false).await; + + let matches = repo + .get_foreign_entities_for_user( + Some(macro_id.to_string()), + vec![SourceId::user(macro_id)], + 10, + filter_query(notification_done_filter(true)), + ) + .await + .expect("notification filter scoped to requesting user should be applied"); + + assert!(matches.is_empty()); +} + +#[sqlx::test(migrator = "MACRO_DB_MIGRATIONS")] +async fn get_for_user_notification_filter_without_requesting_user_returns_empty(pool: PgPool) { + let repo = PgForeignEntityRepo::new(pool.clone()); + let macro_id = "macro|user@example.com"; + + let entity = + insert_foreign_entity_for_source(&repo, "done-pr", "github_pull_request", macro_id, "user") + .await; + insert_foreign_entity_notification(&pool, entity.id, macro_id, true, false).await; + + let matches = repo + .get_foreign_entities_for_user( + None, + vec![SourceId::user(macro_id)], + 10, + filter_query(notification_done_filter(true)), + ) + .await + .expect("notification filter without a requesting user should succeed"); + + assert!(matches.is_empty()); +} + #[sqlx::test(migrator = "MACRO_DB_MIGRATIONS")] async fn patch_updates_selected_fields_preserves_others_and_refreshes_updated_at(pool: PgPool) { let repo = PgForeignEntityRepo::new(pool.clone()); diff --git a/rust/cloud-storage/item_filters/src/ast/foreign_entity.rs b/rust/cloud-storage/item_filters/src/ast/foreign_entity.rs index aca7ef0743..c27df939f1 100644 --- a/rust/cloud-storage/item_filters/src/ast/foreign_entity.rs +++ b/rust/cloud-storage/item_filters/src/ast/foreign_entity.rs @@ -19,6 +19,12 @@ pub enum ForeignEntityLiteral { /// Filter to entities whose metadata participant list contains the requesting user. #[serde(rename = "me")] IncludesMe, + /// Filter by the requesting user's notification done state for this foreign entity. + #[serde(rename = "nd")] + NotificationDone(bool), + /// Filter by the requesting user's notification seen state for this foreign entity. + #[serde(rename = "ns")] + NotificationSeen(bool), } impl ExpandFrame for ForeignEntityFilters { @@ -32,6 +38,7 @@ impl ExpandFrame for ForeignEntityFilters { foreign_entity_ids, foreign_entity_sources, includes_me, + notification_filters, } = filter_request; let ids = ids @@ -49,10 +56,22 @@ impl ExpandFrame for ForeignEntityFilters { let includes_me = includes_me.then_some(Expr::Literal(ForeignEntityLiteral::IncludesMe)); - Ok( - [ids, foreign_entity_ids, foreign_entity_sources, includes_me] - .into_iter() - .fold_with(Expr::and), - ) + let notification_done = notification_filters + .done + .map(|done| Expr::Literal(ForeignEntityLiteral::NotificationDone(done))); + let notification_seen = notification_filters + .seen + .map(|seen| Expr::Literal(ForeignEntityLiteral::NotificationSeen(seen))); + + Ok([ + ids, + foreign_entity_ids, + foreign_entity_sources, + includes_me, + notification_done, + notification_seen, + ] + .into_iter() + .fold_with(Expr::and)) } } diff --git a/rust/cloud-storage/item_filters/src/ast/tests.rs b/rust/cloud-storage/item_filters/src/ast/tests.rs index 75f80c354e..54f9958d4c 100644 --- a/rust/cloud-storage/item_filters/src/ast/tests.rs +++ b/rust/cloud-storage/item_filters/src/ast/tests.rs @@ -1051,6 +1051,73 @@ fn foreign_entity_includes_me_ands_with_sources() { assert_eq!(json, exp); } +#[test] +fn foreign_entity_notification_filters_expand_as_literals() { + let f = EntityFilters { + foreign_entity_filters: ForeignEntityFilters { + notification_filters: crate::NotificationFilters { + done: Some(true), + seen: Some(false), + }, + ..Default::default() + }, + ..Default::default() + }; + + let ast = Arc::into_inner( + EntityFilterAst::new_from_filters(f) + .unwrap() + .unwrap() + .foreign_entity_filter + .unwrap(), + ) + .unwrap(); + + let json = serde_json::to_string(&ast).unwrap(); + assert!( + json.contains(r#""nd":true"#), + "expected done literal: {json}" + ); + assert!( + json.contains(r#""ns":false"#), + "expected seen literal: {json}" + ); +} + +#[test] +fn foreign_entity_notification_done_ands_with_source() { + let f = EntityFilters { + foreign_entity_filters: ForeignEntityFilters { + foreign_entity_sources: vec!["github_pull_request".to_string()], + notification_filters: crate::NotificationFilters { + done: Some(true), + seen: None, + }, + ..Default::default() + }, + ..Default::default() + }; + + let ast = Arc::into_inner( + EntityFilterAst::new_from_filters(f) + .unwrap() + .unwrap() + .foreign_entity_filter + .unwrap(), + ) + .unwrap(); + + let json = serde_json::to_value(ast).unwrap(); + let exp = json!({ + "&": [ + { "l": { "fes": "github_pull_request" } }, + { "l": { "nd": true } } + ] + }); + + assert_eq!(json, exp); +} + #[test] fn foreign_entity_invalid_id_returns_uuid_error() { let f = EntityFilters { diff --git a/rust/cloud-storage/item_filters/src/lib.rs b/rust/cloud-storage/item_filters/src/lib.rs index 0bc5c50872..3b13fce10d 100644 --- a/rust/cloud-storage/item_filters/src/lib.rs +++ b/rust/cloud-storage/item_filters/src/lib.rs @@ -387,6 +387,11 @@ pub struct ForeignEntityFilters { /// absent applies no filter. Serialized in filter ASTs as the `"me"` literal. #[serde(default, skip_serializing_if = "std::ops::Not::not")] pub includes_me: bool, + + /// Filter by the notification state of the foreign entity (e.g. whether the requesting user's + /// GitHub PR notification is done/seen). + #[serde(default, skip_serializing_if = "NotificationFilters::is_empty")] + pub notification_filters: NotificationFilters, } impl IsEmpty for ForeignEntityFilters { @@ -396,11 +401,13 @@ impl IsEmpty for ForeignEntityFilters { foreign_entity_ids, foreign_entity_sources, includes_me, + notification_filters, } = self; ids.is_empty() && foreign_entity_ids.is_empty() && foreign_entity_sources.is_empty() && !*includes_me + && notification_filters.is_empty() } } diff --git a/rust/cloud-storage/soup/src/domain/service/tests.rs b/rust/cloud-storage/soup/src/domain/service/tests.rs index a4d105c78b..754eb7fcb7 100644 --- a/rust/cloud-storage/soup/src/domain/service/tests.rs +++ b/rust/cloud-storage/soup/src/domain/service/tests.rs @@ -210,8 +210,12 @@ fn foreign_entity_matches_literal(entity: &ForeignEntity, literal: &ForeignEntit ForeignEntityLiteral::ForeignEntitySource(source) => { entity.foreign_entity_source.as_str() == source.as_str() } - // "me" resolution happens in the repository; the fake cannot resolve it, so fail closed. - ForeignEntityLiteral::IncludesMe => false, + // "me" and notification done/seen resolution happen in the repository (against the + // metadata participant list and the notification tables); the fake cannot resolve them, + // so fail closed. + ForeignEntityLiteral::IncludesMe + | ForeignEntityLiteral::NotificationDone(_) + | ForeignEntityLiteral::NotificationSeen(_) => false, } } From b14984d7e28c8c65b6a781c3f1cbbbdf21977324 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 15 Jun 2026 00:16:55 +0000 Subject: [PATCH 2/3] feat(soup): surface GitHub PR notifications in the inbox Signal tab Foreign entities (GitHub PRs) now appear in the inbox Signal tab, gated on the user's not-done notifications like the other notification-backed types. - Add `foreignEntityDone`/`foreignEntitySeen` filter-store fields mapping to the `fef` target (`nd`/`ns`), backed by the notification filters added to `ForeignEntityFilters`. - Reference `foreignEntityDone: false` in the inbox signal query so foreign entities are fetched (and opted past defineQueryFilters' exclusion of unreferenced entity types) and gated on not-done notifications. - Classify foreign entities as signal client-side (`signalFilter`). Rendering stays gated on the existing supported-foreign-entities flag. https://claude.ai/code/session_01V3PQwKGcmnjAdhQDhkG1De --- .../app/component/app-sidebar/soup-filter-presets.ts | 5 +++++ .../app/component/next-soup/filters/configs/general.ts | 1 + .../app/component/next-soup/filters/filter-store/compile.ts | 2 ++ .../app/component/next-soup/filters/filter-store/types.ts | 2 ++ .../app/component/next-soup/filters/inbox-filters.ts | 4 +++- 5 files changed, 13 insertions(+), 1 deletion(-) diff --git a/js/app/packages/app/component/app-sidebar/soup-filter-presets.ts b/js/app/packages/app/component/app-sidebar/soup-filter-presets.ts index 1397e466f0..7a60fc522e 100644 --- a/js/app/packages/app/component/app-sidebar/soup-filter-presets.ts +++ b/js/app/packages/app/component/app-sidebar/soup-filter-presets.ts @@ -64,6 +64,11 @@ const getInboxSignalFilters = () => { chatUpdatedAt: { gte: twoWeeksAgo }, folderDone: false, folderUpdatedAt: { gte: twoWeeksAgo }, + // Foreign entities (e.g. GitHub PRs) with a not-done notification. + // Referencing `fef` also opts them into the signal query (otherwise + // defineQueryFilters excludes unreferenced entity types). Rendering is + // still gated on the supported-foreign-entities flag client-side. + foreignEntityDone: false, emailShared: 'exclude', }, exclude: getDisabledSnippetSubtypeExclude(), diff --git a/js/app/packages/app/component/next-soup/filters/configs/general.ts b/js/app/packages/app/component/next-soup/filters/configs/general.ts index b416842638..6b784ff8ca 100644 --- a/js/app/packages/app/component/next-soup/filters/configs/general.ts +++ b/js/app/packages/app/component/next-soup/filters/configs/general.ts @@ -26,6 +26,7 @@ export const inboxFilter = config({ channelDone: false, chatDone: false, folderDone: false, + foreignEntityDone: false, }, emailView: 'inbox', }, diff --git a/js/app/packages/app/component/next-soup/filters/filter-store/compile.ts b/js/app/packages/app/component/next-soup/filters/filter-store/compile.ts index 5b5ed9bdbb..acbf0c14bb 100644 --- a/js/app/packages/app/component/next-soup/filters/filter-store/compile.ts +++ b/js/app/packages/app/component/next-soup/filters/filter-store/compile.ts @@ -114,6 +114,8 @@ const FIELD_CONFIG: Record< callStatus: { target: 'callf', field: 'Status' }, callAttended: { target: 'callf', field: 'Attended' }, foreignEntityRecordId: { target: 'fef', field: 'id' }, + foreignEntitySeen: { target: 'fef', field: 'ns' }, + foreignEntityDone: { target: 'fef', field: 'nd' }, crmCompanyId: { target: 'ccf', field: 'id' }, crmCompanyHidden: { target: 'ccf', field: 'hidden' }, }; diff --git a/js/app/packages/app/component/next-soup/filters/filter-store/types.ts b/js/app/packages/app/component/next-soup/filters/filter-store/types.ts index 833fd57824..910344ac20 100644 --- a/js/app/packages/app/component/next-soup/filters/filter-store/types.ts +++ b/js/app/packages/app/component/next-soup/filters/filter-store/types.ts @@ -66,6 +66,8 @@ export type ScalarFieldFilters = { chatDone?: boolean; folderSeen?: boolean; folderDone?: boolean; + foreignEntitySeen?: boolean; + foreignEntityDone?: boolean; callStatus?: CallStatus; callAttended?: boolean; crmCompanyHidden?: boolean; diff --git a/js/app/packages/app/component/next-soup/filters/inbox-filters.ts b/js/app/packages/app/component/next-soup/filters/inbox-filters.ts index 6ce247637d..886240a75a 100644 --- a/js/app/packages/app/component/next-soup/filters/inbox-filters.ts +++ b/js/app/packages/app/component/next-soup/filters/inbox-filters.ts @@ -128,7 +128,9 @@ export function signalFilter(entity: EntityData): boolean { // Automations only show in the Agents > Scheduled tab, not Inbox. return false; case 'foreign': - return false; + // Foreign entities (e.g. GitHub PRs) are gated by the inbox query on the + // user's not-done notifications, so they're signal whenever returned. + return true; } } From 88164a3b1659ea231c675b5e374d2473ae2ff801 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 15 Jun 2026 13:50:05 +0000 Subject: [PATCH 3/3] fix(soup): treat contradictory foreign entity notification filters as unsatisfiable Combining hoisted notification predicates with `Option::or` silently dropped one side, so a contradictory AST like `NotificationDone(true) AND NotificationDone(false)` collapsed to a single-sided `done = true` predicate instead of matching nothing. Detect conflicting done/seen predicates while folding the AND spine and short-circuit to an empty result. Only reachable via a hand-crafted AST (the typed ForeignEntityFilters emits at most one done/seen literal each), but the silent collapse was misleading. https://claude.ai/code/session_01V3PQwKGcmnjAdhQDhkG1De --- .../src/outbound/pg_foreign_entity_repo.rs | 34 ++++++++++++++- .../outbound/pg_foreign_entity_repo/tests.rs | 41 +++++++++++++++++++ 2 files changed, 73 insertions(+), 2 deletions(-) diff --git a/rust/cloud-storage/foreign_entity/src/outbound/pg_foreign_entity_repo.rs b/rust/cloud-storage/foreign_entity/src/outbound/pg_foreign_entity_repo.rs index 0238c087b5..4dc81bc779 100644 --- a/rust/cloud-storage/foreign_entity/src/outbound/pg_foreign_entity_repo.rs +++ b/rust/cloud-storage/foreign_entity/src/outbound/pg_foreign_entity_repo.rs @@ -52,21 +52,44 @@ struct HoistedForeignEntityFilters { notification_done: Option, /// Notification seen filter for the requesting user (`None` ignores). notification_seen: Option, + /// True when the AND spine carries contradictory predicates (e.g. done=true AND done=false), + /// in which case the whole filter is unsatisfiable and must match nothing. + unsatisfiable: bool, /// jsonpath for the residual (non-hoisted) filter, if any. jsonpath: Option, } impl HoistedForeignEntityFilters { + /// Combine two `Option` predicates taken from the two sides of an `And`. Returns the + /// merged value plus whether the two sides contradicted each other (true AND false). + fn merge_bool_filter(a: Option, b: Option) -> (Option, bool) { + match (a, b) { + (Some(x), Some(y)) if x != y => (None, true), + (Some(x), Some(_)) => (Some(x), false), + (Some(x), None) | (None, Some(x)) => (Some(x), false), + (None, None) => (None, false), + } + } + /// Combine two extracted filter halves taken from the two sides of an `And`. fn and(self, other: Self) -> Self { + let (notification_done, done_conflict) = + Self::merge_bool_filter(self.notification_done, other.notification_done); + let (notification_seen, seen_conflict) = + Self::merge_bool_filter(self.notification_seen, other.notification_seen); + let jsonpath = match (self.jsonpath, other.jsonpath) { (Some(left), Some(right)) => Some(format!("({left} && {right})")), (left, right) => left.or(right), }; Self { includes_me: self.includes_me || other.includes_me, - notification_done: self.notification_done.or(other.notification_done), - notification_seen: self.notification_seen.or(other.notification_seen), + notification_done, + notification_seen, + unsatisfiable: self.unsatisfiable + || other.unsatisfiable + || done_conflict + || seen_conflict, jsonpath, } } @@ -397,6 +420,7 @@ impl ForeignEntityRepository for PgForeignEntityRepo { includes_me, notification_done, notification_seen, + unsatisfiable, jsonpath: filter_jsonpath, } = match query .filter() @@ -413,6 +437,12 @@ impl ForeignEntityRepository for PgForeignEntityRepo { } }; + // Contradictory predicates on the AND spine (e.g. done=true AND done=false) can never + // match, so short-circuit before doing any work. + if unsatisfiable { + return Ok(Vec::new()); + } + let participant_github_user_id = if includes_me { let Some(requesting_user) = requesting_user.as_deref() else { return Ok(Vec::new()); diff --git a/rust/cloud-storage/foreign_entity/src/outbound/pg_foreign_entity_repo/tests.rs b/rust/cloud-storage/foreign_entity/src/outbound/pg_foreign_entity_repo/tests.rs index f1a96c760f..ce0155acac 100644 --- a/rust/cloud-storage/foreign_entity/src/outbound/pg_foreign_entity_repo/tests.rs +++ b/rust/cloud-storage/foreign_entity/src/outbound/pg_foreign_entity_repo/tests.rs @@ -979,6 +979,47 @@ async fn get_for_user_notification_filter_without_requesting_user_returns_empty( assert!(matches.is_empty()); } +#[sqlx::test(migrator = "MACRO_DB_MIGRATIONS")] +async fn get_for_user_contradictory_notification_done_matches_nothing(pool: PgPool) { + let repo = PgForeignEntityRepo::new(pool.clone()); + let macro_id = "macro|user@example.com"; + + let entity = + insert_foreign_entity_for_source(&repo, "done-pr", "github_pull_request", macro_id, "user") + .await; + insert_foreign_entity_notification(&pool, entity.id, macro_id, true, false).await; + + // Sanity check: done=true alone matches the entity. + let one_sided = repo + .get_foreign_entities_for_user( + Some(macro_id.to_string()), + vec![SourceId::user(macro_id)], + 10, + filter_query(notification_done_filter(true)), + ) + .await + .expect("done=true filter should be applied"); + assert_eq!(one_sided, vec![entity.clone()]); + + // done=true AND done=false is contradictory and must match nothing rather than + // collapsing to a single-sided predicate. + let contradiction = Some(Arc::new(Expr::and( + Expr::val(ForeignEntityLiteral::NotificationDone(true)), + Expr::val(ForeignEntityLiteral::NotificationDone(false)), + ))); + let matches = repo + .get_foreign_entities_for_user( + Some(macro_id.to_string()), + vec![SourceId::user(macro_id)], + 10, + filter_query(contradiction), + ) + .await + .expect("contradictory notification filter should be applied"); + + assert!(matches.is_empty()); +} + #[sqlx::test(migrator = "MACRO_DB_MIGRATIONS")] async fn patch_updates_selected_fields_preserves_others_and_refreshes_updated_at(pool: PgPool) { let repo = PgForeignEntityRepo::new(pool.clone());