From 40b92557af0492ac8188d4f0dd5a3e343ae8c84a Mon Sep 17 00:00:00 2001 From: Joana Maia Date: Mon, 29 Jun 2026 17:37:08 +0100 Subject: [PATCH 1/6] fix: link email-only maintainers on incremental runs (DE-980) Two related bugs caused email-only maintainers (e.g. Joshua Crofts in the linux MAINTAINERS file) to never get linked to memberIdentities after the first run for a repo: - compare_and_update_maintainers keyed both current and new maintainers by github_username. Extractions where the AI returns "unknown" (~3959 entries in the kernel file) collapse into a single dict slot, silently dropping the rest. Now keyed by (identityId, role), matching the unique index on maintainersInternal. - get_maintainers_for_repo filtered current maintainers to platform='github' AND type='username', excluding maintainers linked via email. The comparison set was therefore incomplete. Filter relaxed to only deletedAt IS NULL. Identity resolution is also unified: a single _resolve_identity helper falls back from github username to email when the username is missing or "unknown", so the email path is exercised on both first and incremental runs. Co-Authored-By: Claude Signed-off-by: Joana Maia --- .../src/crowdgit/database/crud.py | 2 +- .../services/maintainer/maintainer_service.py | 149 ++++++++---------- 2 files changed, 67 insertions(+), 84 deletions(-) diff --git a/services/apps/git_integration/src/crowdgit/database/crud.py b/services/apps/git_integration/src/crowdgit/database/crud.py index fd0fec6e13..f52d6c76b4 100644 --- a/services/apps/git_integration/src/crowdgit/database/crud.py +++ b/services/apps/git_integration/src/crowdgit/database/crud.py @@ -409,7 +409,7 @@ async def get_maintainers_for_repo(repo_id: str): SELECT mi.role, mi."originalRole", mi."repoUrl", mi."repoId", mi."identityId", mem.value as github_username FROM "maintainersInternal" mi JOIN "memberIdentities" mem ON mi."identityId" = mem.id - WHERE mi."repoId" = $1 AND mem.platform = 'github' AND mem.type = 'username' and mem.verified = True AND mem."deletedAt" is null + WHERE mi."repoId" = $1 AND mem."deletedAt" is null """ return await query( maintainers_sql_query, diff --git a/services/apps/git_integration/src/crowdgit/services/maintainer/maintainer_service.py b/services/apps/git_integration/src/crowdgit/services/maintainer/maintainer_service.py index a88b667d74..ffcb6bc9fb 100644 --- a/services/apps/git_integration/src/crowdgit/services/maintainer/maintainer_service.py +++ b/services/apps/git_integration/src/crowdgit/services/maintainer/maintainer_service.py @@ -153,43 +153,51 @@ def make_role(self, title: str): ) return slugify(title) + async def _resolve_identity( + self, github_username: str | None, email: str | None + ) -> str | None: + # Fall back from github username to email so a wrong/missing username + # does not prevent an email match. + if github_username and github_username != "unknown": + identity_id = await find_github_identity(github_username) + if identity_id: + return identity_id + if email and email != "unknown": + return await find_maintainer_identity_by_email(email) + return None + + async def _resolve_maintainers( + self, maintainers: list[MaintainerInfoItem] + ) -> list[tuple[MaintainerInfoItem, str]]: + semaphore = asyncio.Semaphore(3) + + async def resolve(m: MaintainerInfoItem) -> tuple[MaintainerInfoItem, str | None]: + async with semaphore: + identity_id = await self._resolve_identity(m.github_username, m.email) + return m, identity_id + + results = await asyncio.gather(*[resolve(m) for m in maintainers]) + + resolved: list[tuple[MaintainerInfoItem, str]] = [] + for m, identity_id in results: + if identity_id is None: + self.logger.warning(f"Identity not found for maintainer: {m}") + continue + resolved.append((m, identity_id)) + return resolved + async def insert_new_maintainers( self, repo_url: str, repo_id: str, maintainers: list[MaintainerInfoItem] ): - async def process_maintainer(maintainer: MaintainerInfoItem): - self.logger.info(f"Processing maintainer: {maintainer.github_username}") + resolved = await self._resolve_maintainers(maintainers) + for maintainer, identity_id in resolved: role = maintainer.normalized_title original_role = self.make_role(maintainer.title) - # Find the identity in the database - github_username = maintainer.github_username - email = maintainer.email - - if github_username == "unknown" and email == "unknown": - self.logger.warning("username & email with value 'unknown' aborting") - return - identity_id = ( - await find_github_identity(github_username) - if github_username != "unknown" - else await find_maintainer_identity_by_email(email) - ) - self.logger.debug( - f"Found identity_id for {github_username}: {identity_id} (type: {type(identity_id)})" + await upsert_maintainer(repo_id, identity_id, repo_url, role, original_role) + self.logger.info( + f"Successfully upserted maintainer {maintainer.github_username} " + f"with identity_id {identity_id}" ) - if identity_id: - await upsert_maintainer(repo_id, identity_id, repo_url, role, original_role) - self.logger.info( - f"Successfully upserted maintainer {github_username} with identity_id {identity_id}" - ) - else: - self.logger.warning(f"Identity not found for GitHub user: {maintainer}") - - semaphore = asyncio.Semaphore(3) - - async def process_with_semaphore(maintainer: MaintainerInfoItem): - async with semaphore: - await process_maintainer(maintainer) - - await asyncio.gather(*[process_with_semaphore(maintainer) for maintainer in maintainers]) async def compare_and_update_maintainers( self, @@ -200,63 +208,38 @@ async def compare_and_update_maintainers( ): self.logger.info(f"Comparing and updating maintainers for repo: {repo_id}") current_maintainers = await get_maintainers_for_repo(repo_id) - current_maintainers_dict = {m["github_username"]: m for m in current_maintainers} - new_maintainers_dict = {m.github_username: m for m in maintainers} - for github_username, maintainer in new_maintainers_dict.items(): - role = maintainer.normalized_title - original_role = self.make_role(maintainer.title) - if github_username == "unknown" and maintainer.email in ("unknown", None): - self.logger.warning( - f"Skipping unknown github_username & email with title {maintainer.title}" - ) + # Key by (identityId, role) — the natural unique tuple for a linked maintainer. + # Keying by github_username collapses every "unknown" extraction into one slot, + # which silently drops most email-only maintainers (e.g. linux MAINTAINERS). + current_by_key: dict[tuple[str, str], dict] = { + (m["identityId"], m["role"]): m for m in current_maintainers + } + + resolved = await self._resolve_maintainers(maintainers) + new_by_key: dict[tuple[str, str], MaintainerInfoItem] = { + (identity_id, m.normalized_title): m for m, identity_id in resolved + } + + for (identity_id, role), maintainer in new_by_key.items(): + if (identity_id, role) in current_by_key: continue - elif github_username not in current_maintainers_dict: - # New maintainer - identity_id = ( - await find_github_identity(github_username) - if github_username != "unknown" - else await find_maintainer_identity_by_email(maintainer.email) - ) - self.logger.info(f"Found new maintainer {github_username} to be inserted") - if identity_id: - await upsert_maintainer( - repo_id, identity_id, repo_url, role, original_role, start_date=change_date - ) - self.logger.info( - f"Successfully inserted new maintainer {github_username} with identity_id {identity_id}" - ) - else: - # will happen for new users if their identity isn't created yet but should be fixed on the next iteration - self.logger.warning(f"Identity not found for username: {github_username}") - else: - # Existing maintainer - current_maintainer = current_maintainers_dict[github_username] - if current_maintainer["role"] != role: - # Role has changed: we update maintainer - self.logger.info( - f"Role changed from {current_maintainer['role']} to {role} for maintainer {current_maintainer['identityId']}" - ) - await upsert_maintainer( - repo_id, - current_maintainer["identityId"], - repo_url, - role, - original_role, - change_date, - ) + original_role = self.make_role(maintainer.title) + await upsert_maintainer( + repo_id, identity_id, repo_url, role, original_role, start_date=change_date + ) + self.logger.info( + f"Inserted new maintainer {maintainer.github_username} " + f"with identity_id {identity_id} role {role}" + ) - for github_username, current_maintainer in current_maintainers_dict.items(): - if github_username not in new_maintainers_dict: + for identity_id, role in current_by_key: + if (identity_id, role) not in new_by_key: self.logger.info( - f"Maintainer {github_username} with identity {current_maintainer['identityId']} no longer exists, updating its endDate..." - ) - await set_maintainer_end_date( - repo_id, - current_maintainer["identityId"], - current_maintainer["role"], - change_date, + f"Maintainer with identity {identity_id} role {role} no longer exists, " + f"updating its endDate..." ) + await set_maintainer_end_date(repo_id, identity_id, role, change_date) async def save_maintainers( self, From 052a630f2e2bb778038ef7ba47f8fe4a03542c78 Mon Sep 17 00:00:00 2001 From: Joana Maia Date: Mon, 29 Jun 2026 17:58:11 +0100 Subject: [PATCH 2/6] docs: explain rationale for maintainer identity comparison changes (DE-980) Adds inline comments to the fix from the previous commit so future readers understand: - why get_maintainers_for_repo intentionally has no platform/type/verified filters (the read must mirror what the write path persists), - why _resolve_identity falls back from github username to email, - why _resolve_maintainers is a shared helper, and - why compare_and_update_maintainers keys by (identityId, role) instead of github_username. Co-Authored-By: Claude Signed-off-by: Joana Maia --- .../src/crowdgit/database/crud.py | 5 +++++ .../services/maintainer/maintainer_service.py | 20 ++++++++++++++----- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/services/apps/git_integration/src/crowdgit/database/crud.py b/services/apps/git_integration/src/crowdgit/database/crud.py index f52d6c76b4..0e6e300882 100644 --- a/services/apps/git_integration/src/crowdgit/database/crud.py +++ b/services/apps/git_integration/src/crowdgit/database/crud.py @@ -405,6 +405,11 @@ async def update_maintainer_run(repo_id: str, maintainer_file: str): async def get_maintainers_for_repo(repo_id: str): + # No extra platform/type/verified filter here on purpose: the read set must + # mirror what the write path (find_github_identity / find_maintainer_identity_by_email) + # actually persists. A stricter read filter hides rows from the incremental diff, + # which leads to spurious re-inserts and missed end-dates for email-linked + # maintainers (e.g. platform='git' rows on the linux kernel repo). maintainers_sql_query = """ SELECT mi.role, mi."originalRole", mi."repoUrl", mi."repoId", mi."identityId", mem.value as github_username FROM "maintainersInternal" mi diff --git a/services/apps/git_integration/src/crowdgit/services/maintainer/maintainer_service.py b/services/apps/git_integration/src/crowdgit/services/maintainer/maintainer_service.py index ffcb6bc9fb..36aedc87c0 100644 --- a/services/apps/git_integration/src/crowdgit/services/maintainer/maintainer_service.py +++ b/services/apps/git_integration/src/crowdgit/services/maintainer/maintainer_service.py @@ -156,8 +156,10 @@ def make_role(self, title: str): async def _resolve_identity( self, github_username: str | None, email: str | None ) -> str | None: - # Fall back from github username to email so a wrong/missing username - # does not prevent an email match. + # Try github username first, fall back to email. The fallback exists because + # extraction frequently emits github_username="unknown" (e.g. the linux + # MAINTAINERS file has ~4k entries with only an email and no github handle), + # and because a wrong/stale username should not block a valid email match. if github_username and github_username != "unknown": identity_id = await find_github_identity(github_username) if identity_id: @@ -169,6 +171,9 @@ async def _resolve_identity( async def _resolve_maintainers( self, maintainers: list[MaintainerInfoItem] ) -> list[tuple[MaintainerInfoItem, str]]: + # Centralised resolver used by both the first-run and incremental paths so + # they share identical lookup + fallback semantics. The semaphore caps DB + # concurrency at 3 (large MAINTAINERS files can carry thousands of entries). semaphore = asyncio.Semaphore(3) async def resolve(m: MaintainerInfoItem) -> tuple[MaintainerInfoItem, str | None]: @@ -209,13 +214,18 @@ async def compare_and_update_maintainers( self.logger.info(f"Comparing and updating maintainers for repo: {repo_id}") current_maintainers = await get_maintainers_for_repo(repo_id) - # Key by (identityId, role) — the natural unique tuple for a linked maintainer. - # Keying by github_username collapses every "unknown" extraction into one slot, - # which silently drops most email-only maintainers (e.g. linux MAINTAINERS). + # Key by (identityId, role) — matches the unique index on maintainersInternal + # (repoId, identityId, role) so the diff aligns with what the DB can actually + # hold. The previous github_username key collapsed every "unknown" extraction + # into a single slot, which on the linux MAINTAINERS file (~4k unknowns) + # silently dropped most email-only maintainers and left them unlinked. current_by_key: dict[tuple[str, str], dict] = { (m["identityId"], m["role"]): m for m in current_maintainers } + # Resolve first so the comparison is identity-based, not username-based: + # the same person may extract with different github_username strings across + # runs (or "unknown") yet still resolve to the same memberIdentity. resolved = await self._resolve_maintainers(maintainers) new_by_key: dict[tuple[str, str], MaintainerInfoItem] = { (identity_id, m.normalized_title): m for m, identity_id in resolved From e8d64e566eb9b7d0011c1789816acfdc61f40179 Mon Sep 17 00:00:00 2001 From: Joana Maia Date: Mon, 29 Jun 2026 18:17:57 +0100 Subject: [PATCH 3/6] fix: address PR review on maintainer identity comparison (DE-980) Four follow-ups from PR #4277 review, all aimed at preserving prior behaviour rather than changing the feature: - compare_and_update_maintainers: guard the end-date pass with a "still mentioned in source" check. The old github_username-keyed dict always included extracted maintainers regardless of identity-lookup outcome, so a transient resolution failure never wrongly end-dated a maintainer still present in the file. Restore that invariant under the new identity-based keying. - get_maintainers_for_repo: add mi."endDate" IS NULL. Reinstates the reactivation path the old platform='github' filter incidentally provided for email-linked maintainers (end-dated rows fell out of the comparison set, hit the "new maintainer" branch, and got reactivated via upsert's ON CONFLICT ... SET "endDate" = NULL). - insert_new_maintainers: restore concurrent upserts via asyncio.gather + Semaphore(MAX_CONCURRENT_CHUNKS). The earlier refactor accidentally serialised them, regressing first-run performance on large MAINTAINERS files. - _resolve_maintainers: use self.MAX_CONCURRENT_CHUNKS instead of the hardcoded literal so concurrency tuning stays in one place. Co-Authored-By: Claude Signed-off-by: Joana Maia --- .../src/crowdgit/database/crud.py | 8 ++- .../services/maintainer/maintainer_service.py | 61 ++++++++++++++----- 2 files changed, 52 insertions(+), 17 deletions(-) diff --git a/services/apps/git_integration/src/crowdgit/database/crud.py b/services/apps/git_integration/src/crowdgit/database/crud.py index 0e6e300882..b92c8e819d 100644 --- a/services/apps/git_integration/src/crowdgit/database/crud.py +++ b/services/apps/git_integration/src/crowdgit/database/crud.py @@ -410,11 +410,17 @@ async def get_maintainers_for_repo(repo_id: str): # actually persists. A stricter read filter hides rows from the incremental diff, # which leads to spurious re-inserts and missed end-dates for email-linked # maintainers (e.g. platform='git' rows on the linux kernel repo). + # + # endDate IS NULL keeps the comparison set limited to active maintainers, matching + # the previous behaviour for email-linked maintainers (where the old platform filter + # incidentally excluded them and let the "new maintainer" branch reactivate them + # via the upsert's `endDate = NULL` on conflict). Without this, end-dated rows would + # sit in the comparison set and block reactivation when the maintainer reappears. maintainers_sql_query = """ SELECT mi.role, mi."originalRole", mi."repoUrl", mi."repoId", mi."identityId", mem.value as github_username FROM "maintainersInternal" mi JOIN "memberIdentities" mem ON mi."identityId" = mem.id - WHERE mi."repoId" = $1 AND mem."deletedAt" is null + WHERE mi."repoId" = $1 AND mi."endDate" IS NULL AND mem."deletedAt" is null """ return await query( maintainers_sql_query, diff --git a/services/apps/git_integration/src/crowdgit/services/maintainer/maintainer_service.py b/services/apps/git_integration/src/crowdgit/services/maintainer/maintainer_service.py index 36aedc87c0..675991f75f 100644 --- a/services/apps/git_integration/src/crowdgit/services/maintainer/maintainer_service.py +++ b/services/apps/git_integration/src/crowdgit/services/maintainer/maintainer_service.py @@ -173,8 +173,9 @@ async def _resolve_maintainers( ) -> list[tuple[MaintainerInfoItem, str]]: # Centralised resolver used by both the first-run and incremental paths so # they share identical lookup + fallback semantics. The semaphore caps DB - # concurrency at 3 (large MAINTAINERS files can carry thousands of entries). - semaphore = asyncio.Semaphore(3) + # concurrency at MAX_CONCURRENT_CHUNKS (large MAINTAINERS files can carry + # thousands of entries). + semaphore = asyncio.Semaphore(self.MAX_CONCURRENT_CHUNKS) async def resolve(m: MaintainerInfoItem) -> tuple[MaintainerInfoItem, str | None]: async with semaphore: @@ -195,14 +196,21 @@ async def insert_new_maintainers( self, repo_url: str, repo_id: str, maintainers: list[MaintainerInfoItem] ): resolved = await self._resolve_maintainers(maintainers) - for maintainer, identity_id in resolved: - role = maintainer.normalized_title - original_role = self.make_role(maintainer.title) - await upsert_maintainer(repo_id, identity_id, repo_url, role, original_role) - self.logger.info( - f"Successfully upserted maintainer {maintainer.github_username} " - f"with identity_id {identity_id}" - ) + # Preserve the previous behaviour of running upserts concurrently so first-run + # processing of large MAINTAINERS files (thousands of entries) doesn't serialise. + semaphore = asyncio.Semaphore(self.MAX_CONCURRENT_CHUNKS) + + async def upsert(maintainer: MaintainerInfoItem, identity_id: str) -> None: + async with semaphore: + role = maintainer.normalized_title + original_role = self.make_role(maintainer.title) + await upsert_maintainer(repo_id, identity_id, repo_url, role, original_role) + self.logger.info( + f"Successfully upserted maintainer {maintainer.github_username} " + f"with identity_id {identity_id}" + ) + + await asyncio.gather(*[upsert(m, identity_id) for m, identity_id in resolved]) async def compare_and_update_maintainers( self, @@ -243,13 +251,34 @@ async def compare_and_update_maintainers( f"with identity_id {identity_id} role {role}" ) - for identity_id, role in current_by_key: - if (identity_id, role) not in new_by_key: - self.logger.info( - f"Maintainer with identity {identity_id} role {role} no longer exists, " - f"updating its endDate..." + # Defensive guard for the end-date pass: the previous implementation keyed + # the "new" set by github_username, so any maintainer present in the source + # was unconditionally in that set and never wrongly end-dated. The identity- + # based key now only contains successfully resolved entries, so resolution + # failures (transient DB issues, stale extraction values) could end-date a + # maintainer who is still in the file. Skip end-dating when the current + # row's identifying value still appears in the extracted source. + mentioned_values = set() + for m in maintainers: + for v in (m.github_username, m.email): + if v and v != "unknown": + mentioned_values.add(v.lower()) + + for (identity_id, role), current in current_by_key.items(): + if (identity_id, role) in new_by_key: + continue + current_value = (current.get("github_username") or "").lower() + if current_value and current_value in mentioned_values: + self.logger.warning( + f"Maintainer with identity {identity_id} role {role} could not be " + f"re-resolved but is still mentioned in the source; skipping end-date" ) - await set_maintainer_end_date(repo_id, identity_id, role, change_date) + continue + self.logger.info( + f"Maintainer with identity {identity_id} role {role} no longer exists, " + f"updating its endDate..." + ) + await set_maintainer_end_date(repo_id, identity_id, role, change_date) async def save_maintainers( self, From 17f1852fee40042bfd4c4f50d0278038e36e63ee Mon Sep 17 00:00:00 2001 From: Joana Maia Date: Mon, 29 Jun 2026 18:35:35 +0100 Subject: [PATCH 4/6] docs: simplify maintainer identity comparison comments (DE-980) Trims the explanatory comments added in the earlier docs commit down to one-line whys, dropping the restatement of code behaviour. No behaviour change. Co-Authored-By: Claude Signed-off-by: Joana Maia --- .../src/crowdgit/database/crud.py | 15 ++------ .../services/maintainer/maintainer_service.py | 37 ++++++------------- 2 files changed, 16 insertions(+), 36 deletions(-) diff --git a/services/apps/git_integration/src/crowdgit/database/crud.py b/services/apps/git_integration/src/crowdgit/database/crud.py index b92c8e819d..4722e4135e 100644 --- a/services/apps/git_integration/src/crowdgit/database/crud.py +++ b/services/apps/git_integration/src/crowdgit/database/crud.py @@ -405,17 +405,10 @@ async def update_maintainer_run(repo_id: str, maintainer_file: str): async def get_maintainers_for_repo(repo_id: str): - # No extra platform/type/verified filter here on purpose: the read set must - # mirror what the write path (find_github_identity / find_maintainer_identity_by_email) - # actually persists. A stricter read filter hides rows from the incremental diff, - # which leads to spurious re-inserts and missed end-dates for email-linked - # maintainers (e.g. platform='git' rows on the linux kernel repo). - # - # endDate IS NULL keeps the comparison set limited to active maintainers, matching - # the previous behaviour for email-linked maintainers (where the old platform filter - # incidentally excluded them and let the "new maintainer" branch reactivate them - # via the upsert's `endDate = NULL` on conflict). Without this, end-dated rows would - # sit in the comparison set and block reactivation when the maintainer reappears. + # Read filter mirrors the write path: no platform/type/verified narrowing + # (otherwise email-linked rows like platform='git' are hidden from the diff). + # endDate IS NULL keeps only active rows so reappearing maintainers hit the + # "new" branch and get reactivated by upsert_maintainer's ON CONFLICT clause. maintainers_sql_query = """ SELECT mi.role, mi."originalRole", mi."repoUrl", mi."repoId", mi."identityId", mem.value as github_username FROM "maintainersInternal" mi diff --git a/services/apps/git_integration/src/crowdgit/services/maintainer/maintainer_service.py b/services/apps/git_integration/src/crowdgit/services/maintainer/maintainer_service.py index 675991f75f..625d58f056 100644 --- a/services/apps/git_integration/src/crowdgit/services/maintainer/maintainer_service.py +++ b/services/apps/git_integration/src/crowdgit/services/maintainer/maintainer_service.py @@ -156,10 +156,8 @@ def make_role(self, title: str): async def _resolve_identity( self, github_username: str | None, email: str | None ) -> str | None: - # Try github username first, fall back to email. The fallback exists because - # extraction frequently emits github_username="unknown" (e.g. the linux - # MAINTAINERS file has ~4k entries with only an email and no github handle), - # and because a wrong/stale username should not block a valid email match. + # Fall back to email when github_username is missing/"unknown" — the AI + # extractor emits "unknown" for ~4k entries on the linux MAINTAINERS file. if github_username and github_username != "unknown": identity_id = await find_github_identity(github_username) if identity_id: @@ -171,10 +169,7 @@ async def _resolve_identity( async def _resolve_maintainers( self, maintainers: list[MaintainerInfoItem] ) -> list[tuple[MaintainerInfoItem, str]]: - # Centralised resolver used by both the first-run and incremental paths so - # they share identical lookup + fallback semantics. The semaphore caps DB - # concurrency at MAX_CONCURRENT_CHUNKS (large MAINTAINERS files can carry - # thousands of entries). + # Shared by first-run and incremental paths so lookup semantics stay identical. semaphore = asyncio.Semaphore(self.MAX_CONCURRENT_CHUNKS) async def resolve(m: MaintainerInfoItem) -> tuple[MaintainerInfoItem, str | None]: @@ -196,8 +191,7 @@ async def insert_new_maintainers( self, repo_url: str, repo_id: str, maintainers: list[MaintainerInfoItem] ): resolved = await self._resolve_maintainers(maintainers) - # Preserve the previous behaviour of running upserts concurrently so first-run - # processing of large MAINTAINERS files (thousands of entries) doesn't serialise. + # Concurrent upserts: large MAINTAINERS files carry thousands of entries. semaphore = asyncio.Semaphore(self.MAX_CONCURRENT_CHUNKS) async def upsert(maintainer: MaintainerInfoItem, identity_id: str) -> None: @@ -222,18 +216,15 @@ async def compare_and_update_maintainers( self.logger.info(f"Comparing and updating maintainers for repo: {repo_id}") current_maintainers = await get_maintainers_for_repo(repo_id) - # Key by (identityId, role) — matches the unique index on maintainersInternal - # (repoId, identityId, role) so the diff aligns with what the DB can actually - # hold. The previous github_username key collapsed every "unknown" extraction - # into a single slot, which on the linux MAINTAINERS file (~4k unknowns) - # silently dropped most email-only maintainers and left them unlinked. + # Key by (identityId, role) — keying by github_username collapsed every + # "unknown" extraction into one slot, silently dropping most email-only + # maintainers (~4k of 4216 entries on the linux MAINTAINERS file). current_by_key: dict[tuple[str, str], dict] = { (m["identityId"], m["role"]): m for m in current_maintainers } - # Resolve first so the comparison is identity-based, not username-based: - # the same person may extract with different github_username strings across - # runs (or "unknown") yet still resolve to the same memberIdentity. + # Resolve before keying so the comparison is identity-based: the same + # person may extract with different github_username values across runs. resolved = await self._resolve_maintainers(maintainers) new_by_key: dict[tuple[str, str], MaintainerInfoItem] = { (identity_id, m.normalized_title): m for m, identity_id in resolved @@ -251,13 +242,9 @@ async def compare_and_update_maintainers( f"with identity_id {identity_id} role {role}" ) - # Defensive guard for the end-date pass: the previous implementation keyed - # the "new" set by github_username, so any maintainer present in the source - # was unconditionally in that set and never wrongly end-dated. The identity- - # based key now only contains successfully resolved entries, so resolution - # failures (transient DB issues, stale extraction values) could end-date a - # maintainer who is still in the file. Skip end-dating when the current - # row's identifying value still appears in the extracted source. + # Safety guard: a maintainer whose identity resolution fails (transient DB + # issue, stale extraction value) is absent from new_by_key but may still be + # in the source. Don't end-date if their identifying value is still present. mentioned_values = set() for m in maintainers: for v in (m.github_username, m.email): From 74c70d47dadb0aeb6fce534a2e10b9bd57ddf1d2 Mon Sep 17 00:00:00 2001 From: Joana Maia Date: Tue, 30 Jun 2026 12:34:31 +0100 Subject: [PATCH 5/6] fix: tighten maintainer identity comparison and end-date guard (DE-980) - Scope the safety guard to extractor entries whose identity resolution failed this run, and match by identifier kind (github-username vs email) so role changes correctly end-date the stale (identityId, role) row and a same-named handle on another platform cannot shield an unrelated identity from end-dating. - Restore verified=TRUE on the read path and add it to find_github_identity so reads and writes are symmetric; return mem.platform/mem.type so the guard can disambiguate identifier kinds. - Rename the misleading github_username alias to identity_value and add a dedicated github-username query for the fork/parent-repo filter path. Signed-off-by: Joana Maia --- .../src/crowdgit/database/crud.py | 36 ++++++++++++--- .../services/maintainer/maintainer_service.py | 46 +++++++++++++------ 2 files changed, 62 insertions(+), 20 deletions(-) diff --git a/services/apps/git_integration/src/crowdgit/database/crud.py b/services/apps/git_integration/src/crowdgit/database/crud.py index 4722e4135e..786b6e2ddb 100644 --- a/services/apps/git_integration/src/crowdgit/database/crud.py +++ b/services/apps/git_integration/src/crowdgit/database/crud.py @@ -334,6 +334,7 @@ async def find_github_identity(github_username: str): WHERE platform = 'github' AND value = $1 + AND "verified" = TRUE AND "deletedAt" is null LIMIT 1 """ @@ -405,15 +406,21 @@ async def update_maintainer_run(repo_id: str, maintainer_file: str): async def get_maintainers_for_repo(repo_id: str): - # Read filter mirrors the write path: no platform/type/verified narrowing - # (otherwise email-linked rows like platform='git' are hidden from the diff). - # endDate IS NULL keeps only active rows so reappearing maintainers hit the - # "new" branch and get reactivated by upsert_maintainer's ON CONFLICT clause. + # Active rows only (endDate IS NULL) — reappearing maintainers hit the "new" + # branch and get reactivated by upsert_maintainer's ON CONFLICT clause. + # verified=TRUE mirrors find_github_identity / find_maintainer_identity_by_email. + # platform/type are returned so the diff's safety guard can match identifiers + # by kind and avoid cross-platform value collisions (e.g. a GitHub username + # "foo" colliding with a same-named handle on another platform). maintainers_sql_query = """ - SELECT mi.role, mi."originalRole", mi."repoUrl", mi."repoId", mi."identityId", mem.value as github_username + SELECT mi.role, mi."originalRole", mi."repoUrl", mi."repoId", mi."identityId", + mem.value as identity_value, mem.platform, mem.type FROM "maintainersInternal" mi JOIN "memberIdentities" mem ON mi."identityId" = mem.id - WHERE mi."repoId" = $1 AND mi."endDate" IS NULL AND mem."deletedAt" is null + WHERE mi."repoId" = $1 + AND mi."endDate" IS NULL + AND mem."verified" = TRUE + AND mem."deletedAt" is null """ return await query( maintainers_sql_query, @@ -421,6 +428,23 @@ async def get_maintainers_for_repo(repo_id: str): ) +async def get_github_maintainer_usernames_for_repo(repo_id: str) -> set[str]: + """Return GitHub usernames of active maintainers for fork/parent-repo filtering.""" + sql_query = """ + SELECT mem.value + FROM "maintainersInternal" mi + JOIN "memberIdentities" mem ON mi."identityId" = mem.id + WHERE mi."repoId" = $1 + AND mi."endDate" IS NULL + AND mem.platform = 'github' + AND mem.type = 'username' + AND mem."verified" = TRUE + AND mem."deletedAt" is null + """ + rows = await query(sql_query, (repo_id,)) + return {row["value"] for row in rows} + + async def set_maintainer_end_date( repo_id: str, identity_id: str, role: str, change_date: datetime ): diff --git a/services/apps/git_integration/src/crowdgit/services/maintainer/maintainer_service.py b/services/apps/git_integration/src/crowdgit/services/maintainer/maintainer_service.py index 625d58f056..2271955a96 100644 --- a/services/apps/git_integration/src/crowdgit/services/maintainer/maintainer_service.py +++ b/services/apps/git_integration/src/crowdgit/services/maintainer/maintainer_service.py @@ -11,6 +11,7 @@ from crowdgit.database.crud import ( find_github_identity, find_maintainer_identity_by_email, + get_github_maintainer_usernames_for_repo, get_maintainers_for_repo, save_service_execution, set_maintainer_end_date, @@ -242,20 +243,36 @@ async def compare_and_update_maintainers( f"with identity_id {identity_id} role {role}" ) - # Safety guard: a maintainer whose identity resolution fails (transient DB - # issue, stale extraction value) is absent from new_by_key but may still be - # in the source. Don't end-date if their identifying value is still present. - mentioned_values = set() + # Safety guard scoped to entries whose identity resolution FAILED this run. + # A maintainer who resolves but ends up under a different (identityId, role) + # — i.e. a role change — must still be end-dated on the old role row, so we + # only protect values from extractor entries that did not resolve. Matching + # is kind-aware so a GitHub username "foo" cannot collide with a same-named + # handle on another platform (different person). + resolved_ids = {id(m) for m, _ in resolved} + unresolved_usernames: set[str] = set() + unresolved_emails: set[str] = set() for m in maintainers: - for v in (m.github_username, m.email): - if v and v != "unknown": - mentioned_values.add(v.lower()) + if id(m) in resolved_ids: + continue + if m.github_username and m.github_username != "unknown": + unresolved_usernames.add(m.github_username.lower()) + if m.email and m.email != "unknown": + unresolved_emails.add(m.email.lower()) for (identity_id, role), current in current_by_key.items(): if (identity_id, role) in new_by_key: continue - current_value = (current.get("github_username") or "").lower() - if current_value and current_value in mentioned_values: + current_value = (current.get("identity_value") or "").lower() + current_platform = current.get("platform") + current_type = current.get("type") + is_github_username = current_platform == "github" and current_type == "username" + is_email = current_type == "email" + skip_end_date = bool(current_value) and ( + (is_github_username and current_value in unresolved_usernames) + or (is_email and current_value in unresolved_emails) + ) + if skip_end_date: self.logger.warning( f"Maintainer with identity {identity_id} role {role} could not be " f"re-resolved but is still mentioned in the source; skipping end-date" @@ -950,13 +967,14 @@ async def exclude_parent_repo_maintainers( if not parent_repo or not extracted_maintainers: return extracted_maintainers - parent_repo_maintainers = await get_maintainers_for_repo(parent_repo.id) - if not parent_repo_maintainers: - self.logger.info(f"No maintainers found for parent repo {parent_repo.url}") + # Dedicated github-username lookup: get_maintainers_for_repo now returns any + # identity type (email-linked rows included), but this filter compares against + # extracted github_username values, so we must narrow to platform='github'/type='username'. + parent_github_usernames = await get_github_maintainer_usernames_for_repo(parent_repo.id) + if not parent_github_usernames: + self.logger.info(f"No github-username maintainers found for parent repo {parent_repo.url}") return extracted_maintainers - parent_github_usernames = {m["github_username"] for m in parent_repo_maintainers} - fork_only_maintainers = [ maintainer for maintainer in extracted_maintainers From bcf793ff6da4efe7857159662aebdc9cd0b44c7c Mon Sep 17 00:00:00 2001 From: Joana Maia Date: Tue, 30 Jun 2026 12:36:58 +0100 Subject: [PATCH 6/6] style: ruff-format long log line (DE-980) Signed-off-by: Joana Maia --- .../src/crowdgit/services/maintainer/maintainer_service.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/services/apps/git_integration/src/crowdgit/services/maintainer/maintainer_service.py b/services/apps/git_integration/src/crowdgit/services/maintainer/maintainer_service.py index 2271955a96..8d4029d787 100644 --- a/services/apps/git_integration/src/crowdgit/services/maintainer/maintainer_service.py +++ b/services/apps/git_integration/src/crowdgit/services/maintainer/maintainer_service.py @@ -972,7 +972,9 @@ async def exclude_parent_repo_maintainers( # extracted github_username values, so we must narrow to platform='github'/type='username'. parent_github_usernames = await get_github_maintainer_usernames_for_repo(parent_repo.id) if not parent_github_usernames: - self.logger.info(f"No github-username maintainers found for parent repo {parent_repo.url}") + self.logger.info( + f"No github-username maintainers found for parent repo {parent_repo.url}" + ) return extracted_maintainers fork_only_maintainers = [