feat: SR DCI server/client end-to-end + CEL metric composition fix#223
Draft
gonzalesedwin1123 wants to merge 12 commits into
Draft
feat: SR DCI server/client end-to-end + CEL metric composition fix#223gonzalesedwin1123 wants to merge 12 commits into
gonzalesedwin1123 wants to merge 12 commits into
Conversation
Prepares the social module to serve as a standalone SR DCI server: - accept the namespaced reg_type DCI clients send (ns:org:RegistryType:Social) alongside the legacy bare form; previously every client search was rejected - idtype-value queries match the identifier type by namespace URI or vocabulary code (clients resolve identifiers as short codes); malformed queries missing type/value now reject with FILTER_INVALID - sync search route resolves the verified sender and passes it to the search service so consent filtering engages; it silently disengaged because the consent adapter no-ops without a sender - remove unmounted, unauthenticated routers (social_search, sr_alias) and their tests; the only search surface is spp_dci_server's authenticated route - delete notifications carry external identifiers snapshotted before unlink, never raw database ids; legacy queued jobs degrade to empty identifier lists - document deployment prerequisites (endpoint user group, queue_job worker/channels, auth surface, client base URL) Suites: spp_dci_server 306, spp_dci_server_social 64 (63 +5 new -4 removed with the dead routers).
…cords (Phase B) - spp_dci: add ProgramEnrollment schema (programme_name, enrolment_status, enrolment_date; SPDCI social profile naming, no database ids) and an optional enrolled_programs field on Person - spp_dci_server_social: _to_dci_person emits active enrollments (enrolled/paused states; draft/exited/not_eligible/duplicated excluded) from program_membership_ids - explicit spp_programs dependency - the service now reads program memberships directly, so the previously transitive dependency is real Unblocks sr.dci.program_count and sr.dci.has_programs: the SR client already reads enrolled_programs from person records. Suites: spp_dci 20, spp_dci_server 306, spp_dci_server_social 66.
…se C) - spp_dci: HouseholdInfo schema (household_size, is_household_head) as an optional Person field - an OpenSPP extension so one person search answers household-level questions without a second group query - spp_dci_server_social: _build_household_info from the person's first active group membership; size counts active members, headship via the seeded urn:openspp:vocab:group-membership-type 'head' code; omitted entirely for group-less persons Unblocks sr.dci.household_size, is_head_of_household and large_household. Suites: spp_dci 20, spp_dci_server 306, spp_dci_server_social 68.
…ables (Phase D) One Social Registry person search feeds every metric: - sr.dci.is_registered: record exists (not-found is a meaningful False) - sr.dci.program_count / has_programs: from enrolled_programs - sr.dci.household_size / is_head_of_household / large_household (>5 per the seeded variable): from household_info; size is skipped when the person has no household summary, headship/large default to False Person-not-found skips every metric except is_registered (no cache row, expressions simply don't match). Suite: spp_dci_indicators 78 (65 + 13 new).
…fixes
- mount registry routers at /registry/* relative to the deployment base
URL per the SPDCI spec (registry type travels in the message reg_type,
not in a URL segment); the /social/registry long form stays as a legacy
alias for existing deployments
- sender-registry lookup on the sync path uses a narrow sudo: the
endpoint user (often low-privilege) cannot read the sender registry,
which surfaced as AccessError in live smoke testing
- regression test for a low-privilege endpoint user driving the search
Live-verified over HTTP: bearer-authenticated idtype-value search at
{base}/registry/sync/search returns the person with programme
enrollments and household summary.
ruff-format had wrapped the sender-registry sudo call so the suppression comment landed outside the matched expression, re-triggering the semgrep finding. Assign the sudo'd model first so the comment stays inline.
…rdcoded one search_person, search_household and check_connection overrode the registry type with the ad-hoc string 'ns:registry_type:social_registry', which spec-compliant servers reject - every SR search returned no results. Drop the overrides so the client defaults to the namespaced RegistryType configured on the data source. Found during the local client-server e2e: searches succeeded over curl but returned None through SRService. Suite: spp_dci_client_sr 134.
…person The CEL SQL fast path requires a complete cache - a row per candidate subject. Skipping not-found/household-less persons left cache holes, so every sr.dci.* comparison except is_registered fell back to Python and matched nothing (found during the local client-server e2e: cache status 'incomplete', base=30 have=20). Not found in SR now yields semantic defaults: 0 programs, household size 0, not a head, not a large household. Suite: spp_dci_indicators 78.
The fresh-cache SQL shortcut for a metric comparison returned no ids and stashed an override domain for compile_and_preview - valid only when the comparison IS the whole plan. Composed under and/or, the first metric's override replaced the entire result, silently dropping every other conjunct (live-found: metricA == true and metricB == true matched everyone matching metricA alone). The shortcut now applies only at the plan root (as_root); composed metric nodes materialize their matching ids so set composition stays correct. Side effects fixed for free: require_coverage can no longer be bypassed by the leaked override, and compile_for_batch / compile_count_only no longer receive empty ids for fresh-cached metrics. Suite: spp_cel_domain 576 (573 + 3 new composition tests).
…ameter The base executor gained as_root for correct metric composition; the event executor's override broke compile with an unexpected-kwarg error on stacks where spp_cel_event is installed (invisible to spp_cel_domain's own test DB - signature changes must be checked against overrides repo-wide). Suite: spp_cel_event 170.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the DCI (Distributed Consent and Identity) and CEL (Common Expression Language) integration by supporting metric conjunctions, introducing social registry (SR) indicators, and aligning the API endpoints with the SPDCI specification. Key updates include adding ProgramEnrollment and HouseholdInfo schemas, implementing SR fetch handlers, resolving verified senders for consent filtering, and snapshotting external identifiers for delete notifications to avoid exposing raw database IDs. Feedback focuses on adding defensive checks to prevent Pydantic validation errors when related records (like program_id or id_type_id) are missing.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+609
to
+610
| for membership in partner.program_membership_ids | ||
| if membership.state in ("enrolled", "paused") |
There was a problem hiding this comment.
Defensively filter out memberships where program_id is missing. Since programme_name is a required field in the ProgramEnrollment schema, if program_id is empty, membership.program_id.name will return False (or None), causing a Pydantic validation error and crashing the search request with a 500 Internal Server Error.
Suggested change
| for membership in partner.program_membership_ids | |
| if membership.state in ("enrolled", "paused") | |
| for membership in partner.program_membership_ids | |
| if membership.state in ("enrolled", "paused") and membership.program_id |
References
- When processing records that should have related detail records, defensively filter out any records where the detail record is missing to handle edge cases like newly created records.
Member
Author
There was a problem hiding this comment.
Applied in 328ca85. One note for the record: program_id is required=True (NOT NULL + restrict), so the empty case is unreachable through the ORM — and a Pydantic failure here is caught by the record-conversion handler (record skipped + warning), not a 500. The guard is still worth its one token against pathological data, so taken as suggested.
Comment on lines
+112
to
+113
| for reg_id in partner.reg_ids | ||
| if reg_id.value |
There was a problem hiding this comment.
Defensively ensure reg_id.id_type_id is present before including it in the delete notification payload. If id_type_id is missing, identifier_type will evaluate to False, which violates the DCI schema expecting a string URI or code.
Suggested change
| for reg_id in partner.reg_ids | |
| if reg_id.value | |
| for reg_id in partner.reg_ids | |
| if reg_id.value and reg_id.id_type_id |
References
- When processing records that should have related detail records, defensively filter out any records where the detail record is missing to handle edge cases like newly created records.
Member
Author
There was a problem hiding this comment.
Applied in 328ca85. Same note as the sibling thread: id_type_id is required=True, so this is defense-in-depth against pathological data rather than a reachable bug — taken as suggested.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 19.0 #223 +/- ##
==========================================
+ Coverage 73.06% 74.84% +1.77%
==========================================
Files 1069 1083 +14
Lines 62105 62785 +680
==========================================
+ Hits 45377 46991 +1614
+ Misses 16728 15794 -934
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…nces Guard membership.program_id and reg_id.id_type_id before building wire payloads (gemini review). Both fields are required=True so the branches are unreachable through the ORM, but the guards are cheap and protect against pathological data per the False-vs-None defensive pattern. Suite: spp_dci_server_social 68.
Event tests early-return on event nodes and plain plans take the SQL path, so the override's super() passthrough (the as_root thread-through) was the one uncovered changed line on the PR. Drive a LeafDomain plan through the override both with and without as_root. Suite: spp_cel_event 171.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Continuation of #221: makes the Social Registry (
sr.dci.*) CEL variables work end-to-end — an OpenSPP instance serving SR data over DCI, and the MIS fetching it into CEL eligibility — plus a core CEL engine fix for composed metric comparisons.SR server (
spp_dci_server_social/spp_dci_server)reg_typeDCI clients actually send (ns:org:RegistryType:Social); previously every client search was rejectedidtype-valuematches identifier types by namespace URI or vocabulary code; malformed queries reject withFILTER_INVALIDinstead of silently matching nothingsudo()for the sender lookup so low-privilege endpoint users work/registry/*mount at the deployment base URL (the/socialprefix was a non-spec OpenSPP-ism; kept as legacy alias). Registry type travels in the message, not the URLsocial_search,sr_alias) and their tests — the only search surface is the authenticated routehousehold_size,is_household_head) so one search answers all SR metricsSR client (
spp_dci_client_sr/spp_dci_indicators)sr.dci.is_registered,program_count,has_programs,household_size,is_head_of_household,large_household(>5)SRServiceno longer overrides the registry type with a hardcoded ad-hoc string that spec-compliant servers rejectCEL engine (
spp_cel_domain/spp_cel_event)metric() and metric()composed incorrectly: the fresh-cache SQL shortcut stashed an override domain valid only for single-comparison plans; under and/or the first metric's override replaced the whole result, silently dropping other conjuncts. The shortcut now applies only at the plan root; composed nodes materialize idsrequire_coveragecan no longer be bypassed by the leaked override;compile_for_batch/compile_count_onlyno longer get empty ids for fresh-cached metricsspp_cel_event's_execute_planoverride aligned with the new signatureOpenProject
Test plan