fix: case-insensitive channel join in health score v2 pipes (IN-1307) - #4626
gaspergrom wants to merge 16 commits into
Conversation
…re v2 joins (IN-1307) Signed-off-by: Gašper Grom <gasper.grom@gmail.com>
…(IN-1307) Signed-off-by: Gašper Grom <gasper.grom@gmail.com>
PR SummaryMedium Risk Overview Case-insensitive matching: Activity, PR, issue, vulnerability, and health-score pipes now compare channels with Canonical repo per URL (IN-1307): No schema or ingestion changes—SQL-only updates under Reviewed by Cursor Bugbot for commit 228b843. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
Pull request overview
Scopes Health Score v2 channel joins case-insensitively to recover GitHub signals affected by URL casing differences.
Changes:
- Applies
lower()to repository-channel joins across four health-score pipes. - Updates raw snapshots and maintainer, development, and lifecycle calculations.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
health_score_v2_raw_inputs_snapshot.pipe |
Normalizes raw-input channel joins. |
health_score_v2_maintainer.pipe |
Normalizes maintainer-signal joins. |
health_score_v2_lifecycle.pipe |
Normalizes lifecycle-signal joins. |
health_score_v2_development.pipe |
Normalizes development-signal joins. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…IN-1307) Signed-off-by: Gašper Grom <gasper.grom@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 34 changed files in this pull request and generated 3 comments.
Suppressed comments (4)
services/libs/tinybird/pipes/health_score_v2_maintainer.pipe:203
repos_channels_dsalso contains Gerrit/q/project:channels (repos_channels_copy.pipe:23-43), and Gerrit project names are case-sensitive. Lowercasing the complete URL can therefore make activity for one Gerrit project join a different repository whose path differs only by case, duplicating or misattributing every derived signal. Restrict case folding to GitHub/GitLab channels (ideally via a platform-aware normalized key) while preserving exact matching for Gerrit and other case-sensitive hosts.
INNER JOIN repos_channels_ds ch ON lower(ar.channel) = lower(ch.channel)
services/libs/tinybird/pipes/health_score_v2_development.pipe:206
repos_channels_dsalso contains Gerrit/q/project:channels (repos_channels_copy.pipe:23-43), and Gerrit project names are case-sensitive. Lowercasing the complete URL can therefore make activity for one Gerrit project join a different repository whose path differs only by case, duplicating or misattributing every derived signal. Restrict case folding to GitHub/GitLab channels (ideally via a platform-aware normalized key) while preserving exact matching for Gerrit and other case-sensitive hosts.
INNER JOIN repos_channels_ds ch ON lower(ar.channel) = lower(ch.channel)
services/libs/tinybird/pipes/health_score_v2_lifecycle.pipe:130
repos_channels_dsalso contains Gerrit/q/project:channels (repos_channels_copy.pipe:23-43), and Gerrit project names are case-sensitive. Lowercasing the complete URL can therefore make activity for one Gerrit project join a different repository whose path differs only by case, duplicating or misattributing every derived signal. Restrict case folding to GitHub/GitLab channels (ideally via a platform-aware normalized key) while preserving exact matching for Gerrit and other case-sensitive hosts.
INNER JOIN repos_channels_ds ch ON lower(ar.channel) = lower(ch.channel)
services/libs/tinybird/pipes/health_score_v2_raw_inputs_snapshot.pipe:88
repos_channels_dsalso contains Gerrit/q/project:channels (repos_channels_copy.pipe:23-43), and Gerrit project names are case-sensitive. Lowercasing the complete URL can therefore make activity for one Gerrit project join a different repository whose path differs only by case, duplicating or misattributing every snapshotted signal. Restrict case folding to GitHub/GitLab channels (ideally via a platform-aware normalized key) while preserving exact matching for Gerrit and other case-sensitive hosts.
INNER JOIN repos_channels_ds ch ON lower(ar.channel) = lower(ch.channel)
…1307) Signed-off-by: Gašper Grom <gasper.grom@gmail.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 36 changed files in this pull request and generated 7 comments.
Suppressed comments (3)
services/libs/tinybird/pipes/repos_to_channels.pipe:28
- Grouping every repository by
lower(url)can collapse distinct repositories on providers whose path casing is significant; this pipe explicitly supports Gerrit. The repository's existing normalization guard is deliberately GitHub-only for this reason (backend/src/osspckgs/migrations/V1784718694__repos_github_lower_url_guard.sql:8-12). Restrict case-folding to integrations known to have case-insensitive repository paths rather than applying it globally.
services/libs/tinybird/pipes/repos_channels_copy.pipe:24 - This case-insensitive grouping applies to all integrations and can merge two legitimate case-sensitive Gerrit/other-host repository paths into one channel mapping. Existing repository normalization is intentionally GitHub-only because casing may be significant elsewhere (
backend/src/osspckgs/migrations/V1784718694__repos_github_lower_url_guard.sql:8-12). Use a platform-aware canonical key and preserve the original path for other hosts.
services/libs/tinybird/pipes/repos_to_channels.pipe:28 - The soft-delete filter runs before choosing the canonical case-insensitive row. If a repository URL changed casing and the newest variant was later deleted, that row is removed first and an older live-looking variant becomes canonical, so deleted repositories remain selectable. Aggregate the full row across all variants first, then filter on the selected row's
deletedAt.
Signed-off-by: Gašper Grom <gasper.grom@gmail.com>
Signed-off-by: Gašper Grom <gasper.grom@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 40 out of 40 changed files in this pull request and generated 4 comments.
Suppressed comments (1)
services/libs/tinybird/pipes/health_score_v2_raw_inputs_snapshot.pipe:88
- The activity subqueries now aggregate under the canonical
ch.repoUrl, butallReposat line 76 still emits every case-only duplicate fromrepositories. Consequently, the monthly append writes one populated snapshot row plus an all-NULL activity row for the same logical repository, and both survive because the datasource key is case-sensitive. SourceallReposandexcludedfrom a deduplicated canonical repository set as the other updated v2 pipes do.
INNER JOIN repos_channels_ds ch ON lower(ar.channel) = lower(ch.channel)
argMax(url, updatedAt) AS url combined with GROUP BY lower(url) let ClickHouse substitute the SELECT alias into the GROUP BY expression, producing an illegal nested aggregation (ILLEGAL_AGGREGATION) that tb check didn't catch but a real push to production did. Renamed the alias to canonicalUrl, matching the existing safe pattern in repo_health_score_copy.pipe. Signed-off-by: Gašper Grom <gasper.grom@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 40 out of 40 changed files in this pull request and generated 1 comment.
Suppressed comments (5)
services/libs/tinybird/pipes/repos_channels_copy.pipe:27
- This groups URLs case-insensitively for every provider, although the shared normalizer only lowercases paths for GitHub/GitLab (
services/libs/data-access-layer/src/integrations/index.ts:20-23) and this pipe explicitly handles Gerrit. Distinct repositories on a case-sensitive host can therefore collapse into one mapping and have one repository's activities attributed to the other. Use a provider-aware canonical key and preserve exact path casing elsewhere.
services/libs/tinybird/pipes/health_score_v2_maintainer.pipe:204 - This comparison case-folds every integration, not just GitHub. For case-sensitive Gerrit or custom-host paths, two distinct channels differing only by case will match the same mapping and corrupt both repositories' aggregates. Restrict case-insensitive matching to providers whose paths are normalized as case-insensitive, and retain exact matching for the others.
INNER JOIN
repos_channels_ds ch ON lower(ar.channel) = lower(ch.channel)
services/libs/tinybird/pipes/health_score_v2_raw_inputs_snapshot.pipe:88
- These aggregates now emit only the canonical
ch.repoUrl, butallReposat line 76 still enumerates raw case variants. A duplicate pair therefore produces one populated snapshot row and one all-NULL row, violating the datasource's logical one-row-per-repository snapshot invariant. Source the outer base from the already deduplicated channel mapping too.
INNER JOIN repos_channels_ds ch ON lower(ar.channel) = lower(ch.channel)
services/libs/tinybird/pipes/repo_health_score_stars.pipe:20
- The PR description says the change is limited to four Health Score v2 COPY pipes, but this modifies 40 pipes, including legacy health-score and general activity-filtering endpoints. That materially broadens both behavior and regression risk beyond the documented validation. Update the description and validation scope, or split/remove the unrelated endpoint changes.
services/libs/tinybird/pipes/repo_health_score_copy.pipe:12 - The canonical base channel is still joined with exact
USING (channel)comparisons below, while eachrepo_health_score_*node groups and emits the original activity channel casing. For the reported lowercase repository/mixed-case activity scenario, this deduplication can remove the mixed-case base row and make all legacy repo metrics miss. Normalize the downstream join keys or map each metric through the canonical channel before deduplicating the base.
…d velocity pipes (IN-1307) Signed-off-by: Gašper Grom <gasper.grom@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 40 out of 40 changed files in this pull request and generated 2 comments.
Suppressed comments (6)
Previously missed (1) — in code that hasn't changed since the last review.
services/libs/tinybird/pipes/activities_filtered.pipe:55
- The PR description states that only four Health Score v2 COPY pipes are changed, but this modifies the generic activity filtering API and the diff also changes legacy health-score and repository-selection pipes. Either narrow the patch to the stated scope or update the description and validation evidence to cover these user-facing query changes.
This issue also appears on line 54 of the same file.
services/libs/tinybird/pipes/repos_to_channels.pipe:31
- This generic repository expansion now collapses URLs case-insensitively for every platform. Because Gerrit project paths can be case-sensitive,
/Fooand/foomay represent different repositories but only the most recently updated one will remain selectable. Use a platform-aware canonical key rather than unconditionallower(url).
services/libs/tinybird/pipes/health_score_v2_raw_inputs_snapshot.pipe:88 - This join maps all case variants to the single canonical
ch.repoUrl, but the outerallReposbase still contains every case-distinct repository row and joins these aggregates by exactrepoUrl. The monthly snapshot will therefore persist one populated canonical row plus empty/partial rows for the duplicate casings, and its case-sensitive sorting key will retain all of them. Deduplicate the base with the same canonical mapping before appending the snapshot.
INNER JOIN repos_channels_ds ch ON lower(ar.channel) = lower(ch.channel)
services/libs/tinybird/pipes/activities_filtered.pipe:54
- This makes repository selection case-insensitive for every integration, not just the GitHub case mismatch addressed by IN-1307. On case-sensitive hosts such as Gerrit, selecting one repository can now also return activities for a distinct repository whose path differs only by case. Preserve exact matching for case-sensitive platforms or compare against a platform-aware canonical channel key.
AND lower(a.channel) IN (SELECT lower(channel) FROM repos_to_channels)
services/libs/tinybird/pipes/project_insights_health_breakdown_copy.pipe:157
- This repeats the unconditional case-insensitive deduplication for project rollups. It can merge distinct repositories on case-sensitive hosts and move the surviving repository to whichever project's row has the latest
updatedAt, dropping the other project association. Use the same platform-aware canonical key as the repository-channel mapping.
GROUP BY lower(url)
services/libs/tinybird/pipes/repo_health_score_copy.pipe:18
- This copy independently applies the same global
lower(url)deduplication even though repository path case semantics vary by platform. A case-sensitive repository can be removed from the V1 health-score population, and the latest duplicate's project association wins. Reuse a platform-aware canonical repository set instead of grouping every URL by lowercase.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 44 out of 44 changed files in this pull request and generated 2 comments.
Suppressed comments (4)
services/libs/tinybird/pipes/repos_channels_copy.pipe:27
- This deduplication case-folds URLs for every provider, but repository identity is only canonicalized case-insensitively for GitHub; non-GitHub lookups remain exact (
services/libs/data-access-layer/src/repositories/index.ts:205-229,448-477). Because this pipe explicitly supports Gerrit, two valid case-sensitive repository paths can collapse into one row and the newlower(channel)joins will attribute both repositories' activity to whichever row wins. Use a provider-aware canonical key (case-fold GitHub only) consistently across this deduplication and the downstream comparisons.
services/libs/tinybird/pipes/repo_health_score_copy.pipe:8 - The repo metric nodes modified in this PR now emit
lower(channel), but this new base still emitscanonicalUrlwith its original casing and lines 57-67 join them with exactUSING (channel). If the most recently updated repository row has mixed-case casing, every activity metric misses and is coalesced to zero—the same symptom this PR is fixing. Preserve the canonical output URL, but join the metric nodes through a consistently normalized key.
services/libs/tinybird/pipes/project_insights_health_breakdown_copy.pipe:157 - This independent
lower(url)grouping can also collapse distinct non-GitHub repositories whose paths differ only by case, assigning the retained row's score breakdown to one project and dropping the other. Keep this rollup aligned with a provider-aware repository identity rather than globally lowercasing URL paths.
FROM repositories FINAL
WHERE deletedAt IS NULL
GROUP BY lower(url)
services/libs/tinybird/pipes/health_score_v2_raw_inputs_snapshot.pipe:88
- The activity join now emits only the canonical
ch.repoUrl, butallReposstill enumerates case-sensitive raw repository URLs. A case-only duplicate therefore produces one populated snapshot row plus a second mostly empty row, contradicting this append-only snapshot's one-row-per-repo contract and skewing audit analyses. Source the base from the already deduplicated channel mapping.
INNER JOIN repos_channels_ds ch ON lower(ar.channel) = lower(ch.channel)
Health score v2 category pipes (development/lifecycle/maintainer/raw_inputs_snapshot) and repo_health_score_copy/repos_channels_copy/repos_to_channels(_excluded) built their base repository lists from raw, undeduped repositories rows, letting a case-only-duplicate row leak through with missing signals or a stale excluded/archived/deletedAt flag win over the canonical row. Dedupe via argMax(..., updatedAt) GROUP BY lower(url), filtering deletedAt/archived/excluded AFTER aggregation instead of before, and lowercase repo_health_score_copy_repos.channel to match the lower(channel) every sibling metric pipe emits (project_insights_copy's join to it lowers base.repoUrl to match). Signed-off-by: Gašper Grom <gasper.grom@gmail.com>
Signed-off-by: Gašper Grom <gasper.grom@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 46 out of 46 changed files in this pull request and generated 10 comments.
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
services/libs/tinybird/pipes/project_insights_health_breakdown_copy.pipe:158
- Filtering deleted rows before grouping resurrects an older active case variant when the newest canonical row is soft-deleted. This differs from the canonicalization rule used by the other updated pipes and can keep a deleted repository in the project rollup; aggregate
deletedAtfirst, then filter outside.
services/libs/tinybird/pipes/repos_to_channels.pipe:65 - The deduplicated URL can differ from a caller-supplied
reposvalue only by case, butgerrit_reposstill checks them with exact equality at line 73. In that case the/q/project:Gerrit channel is never generated, so Gerrit activity remains missing despite the case-insensitive downstream filters.
services/libs/tinybird/pipes/health_score_v2_raw_inputs_snapshot.pipe:100
- This second dedup has the same nullable-
argMaxproblem: active rows (deletedAt = NULL) are skipped when any older deleted case variant exists, sorfcan vanish whileallReposshould remain active. Preserve the newest row's nullable value via a tuple.
GROUP BY lower(url)
epipav
left a comment
epipav
left a comment
There was a problem hiding this comment.
Suggesting fixing the source data instead of the queries
I measured the scope in Tinybird first:
| Repos in the health-score population | 31,590 |
|---|---|
| Match only case-insensitively (the bug) | 18 |
Case-only duplicate rows in repositories |
5 |
Adding lower() across 44 pipes to repair 18 repos is expensive:
- Per-row
lower()on every request and nightly job over ~750M activity rows and 511M contribution rows. channelis a sorting-key column onpull_requests_analyzedandissues_analyzed.lower(channel)defeats index pruning there.- Blocking bug:
repo_health_score_*pipes now outputlower(channel), butrepo_health_score_copy.pipejoins themUSING (channel)against the original-casedargMax(url). 5,810 repos have mixed-case URLs and would get all-NULL scores on the first nightly run.
Proposal
Use Tinybird only to find the discrepancies, then fix them in Postgres, the source: normalize the affected repositories.url rows to the casing the activities carry, and merge the 5 case-only duplicates. Once the data is consistent, the existing exact-match joins are correct with no query changes.
Discrepancy query:
SELECT repoUrl, arrayDistinct(groupArray(p.channel)) AS activityChannels
FROM
(
SELECT
argMax(url, updatedAt) AS repoUrl,
argMax(enabled, updatedAt) AS en,
argMax(excluded, updatedAt) AS ex,
argMax(insightsProjectId, updatedAt) AS ip
FROM repositories FINAL
WHERE isNull(deletedAt)
GROUP BY lower(url)
) r
INNER JOIN (SELECT DISTINCT channel FROM pull_requests_analyzed) p
ON lower(p.channel) = lower(r.repoUrl)
WHERE en = true AND ex = false AND ip != ''
AND repoUrl NOT IN (SELECT DISTINCT channel FROM pull_requests_analyzed)
GROUP BY repoUrlThere was a problem hiding this comment.
Pull request overview
Copilot reviewed 46 out of 46 changed files in this pull request and generated 2 comments.
Suppressed comments (10)
Previously missed (1) — in code that hasn't changed since the last review.
services/libs/tinybird/pipes/project_insights_health_breakdown_copy.pipe:158
- Filtering
deletedAt IS NULLbefore grouping resurrects an older active casing when the newest duplicate is soft-deleted, so the deleted repository still contributes to the project rollup. Select the complete latest row first, includingdeletedAt, then filter the aggregated result.
services/libs/tinybird/pipes/repos_channels_copy.pipe:27
deletedAtis nullable, and ClickHouseargMaxskips rows where its argument isNULL. Therefore this does not necessarily read deletion state from the row that suppliedcanonicalUrl: if an older case variant is deleted and a newer variant is active, the stale deletion timestamp wins and the active repository is omitted. Select the complete latest row as a tuple so nullable state and the other fields remain aligned.
services/libs/tinybird/pipes/repos_to_channels.pipe:31- Because
deletedAtis nullable, ClickHouseargMax(deletedAt, updatedAt)ignores active rows whose value isNULL. A newer active case variant can consequently be discarded in favor of an older deleted variant, contrary to this node's latest-row contract. Use one tuple-valuedargMaxand unpack every field from that same row.
services/libs/tinybird/pipes/repos_to_channels_excluded.pipe:24 argMaxskips nullable arguments, so an active latest row (deletedAt = NULL) is ignored whenever the group also contains an older soft-deleted row. That makes the exclusion list omit the active canonical repository. Select the complete latest row as a tuple before testing its deletion and exclusion state.
services/libs/tinybird/pipes/repo_health_score_copy.pipe:19- ClickHouse ignores rows with a
NULLargument inargMax, so this can takedeletedAtfrom an older deleted duplicate while the other values come from a newer active row. The active repository is then removed from the v1 health-score COPY. Use a tuple-valuedargMaxto preserve the complete latest row, including a nullable deletion value.
services/libs/tinybird/pipes/health_score_v2_maintainer.pipe:199 argMax(deletedAt, updatedAt)skips active rows because their nullable argument isNULL. Thus an older deleted case variant can cause a newer active repository to be filtered from maintainer scoring, while the other independently aggregated fields come from another row. Select all fields in one tuple-valuedargMax.
argMax(deletedAt, updatedAt) AS deletedAt
services/libs/tinybird/pipes/health_score_v2_lifecycle.pipe:120
- The latest active row has
deletedAt = NULL, which ClickHouse skips in thisargMax; an older soft-deleted case variant can therefore make lifecycle scoring omit the active repository. Select the whole latest row as a tuple and filter on the tuple's nullable deletion field.
argMax(deletedAt, updatedAt) AS deletedAt
services/libs/tinybird/pipes/health_score_v2_development.pipe:165
- This nullable
argMaxcan return an older deletion timestamp because rows withdeletedAt = NULLare skipped. A newer active case variant is then wrongly excluded from development scoring. Use one tuple-valuedargMaxso all state comes from the actual latest row.
argMax(deletedAt, updatedAt) AS deletedAt
services/libs/tinybird/pipes/project_insights_copy.pipe:402
- Only the health-score join is normalized here.
project_insights_copy_repo_period_metricsstill groups by the original activitychannel, and the next join remains exact, so the described lowercase repository/mixed-case activity case still yields zero 365-day stars, forks, contributors, and organizations in this snapshot. Aggregaterm.channelby the canonical lowercase key and join on that key as well.
LEFT JOIN repo_health_score_copy_ds AS hs ON lower(base.repoUrl) = hs.channel
services/libs/tinybird/pipes/repo_health_score_copy.pipe:11
- This lowercases the base join key, but
repo_health_score_security.pipe:43-45still emits and groups the originalrepocasing, and line 71 joins it with exactUSING (channel). Existing mixed-case security evaluations therefore stop matching andsecurityPercentagebecomes NULL. Normalize the security pipe's output/filter to the same key, or make that join case-insensitive without creating duplicate matches.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 46 out of 46 changed files in this pull request and generated 1 comment.
Suppressed comments (10)
services/libs/tinybird/pipes/repos_channels_copy.pipe:27
argMaxskips rows where its argument is NULL. Because active rows havedeletedAt = NULL, an older deleted casing can supplydeletedAtwhile the newer active casing supplies the other fields, causing this filter to drop the active repository. Preserve the NULL inside a tuple so all fields reflect the row with the maximumupdatedAt.
services/libs/tinybird/pipes/repos_to_channels.pipe:31argMaxskips rows where its argument is NULL. Because active rows havedeletedAt = NULL, an older deleted casing can supplydeletedAtwhile the newer active casing supplies the other fields, causing this filter to drop the active repository. Preserve the NULL inside a tuple so all fields reflect the row with the maximumupdatedAt.
services/libs/tinybird/pipes/repos_to_channels_excluded.pipe:24argMaxskips rows where its argument is NULL. If a stale casing is soft-deleted and the newer canonical row is active, this aggregate takes the stale non-NULL deletion timestamp and excludes the active repository. Wrap the nullable value in a tuple so the latest row's NULL is retained.
services/libs/tinybird/pipes/health_score_v2_raw_inputs_snapshot.pipe:84argMax(deletedAt, updatedAt)ignores active rows because their argument is NULL. A group with an older deleted casing and a newer active casing is therefore removed fromallRepos, contrary to the intended latest-row selection. Preserve the NULL via a tuple.
SELECT argMax(url, updatedAt) AS repoUrl, argMax(deletedAt, updatedAt) AS deletedAt
services/libs/tinybird/pipes/health_score_v2_raw_inputs_snapshot.pipe:98
- This nullable
argMaxskips active rows, so an older soft-deleted casing can makerfappear deleted even when the newest canonical row is active. Preserve the winning row's NULL by aggregating a tuple.
argMax(deletedAt, updatedAt) AS deletedAt
services/libs/tinybird/pipes/health_score_v2_development.pipe:165
argMaxskips NULL arguments, so this does not actually select the latest row's deletion state. When an older casing is deleted and a newer casing is active, the stale non-NULLdeletedAtwins and the active repository is removed from development scoring. Preserve NULL inside a tuple.
argMax(deletedAt, updatedAt) AS deletedAt
services/libs/tinybird/pipes/health_score_v2_lifecycle.pipe:120
argMaxskips NULL arguments, so an active latest row cannot win this aggregate when an older case-duplicate has a non-NULLdeletedAt. The subsequent filter then removes the active repository from lifecycle scoring. Preserve the latest row's NULL via a tuple.
argMax(deletedAt, updatedAt) AS deletedAt
services/libs/tinybird/pipes/health_score_v2_maintainer.pipe:199
- Because
argMaxskips a NULL argument, this can combine the newest active row's URL/state with an older deleted casing's non-NULLdeletedAt, after which the repository is filtered out. Aggregate a tuple to retain NULL from the row with maximumupdatedAt.
argMax(deletedAt, updatedAt) AS deletedAt
services/libs/tinybird/pipes/repo_health_score_copy.pipe:19
argMax(deletedAt, updatedAt)skips active rows becausedeletedAtis NULL. If an older case-duplicate is deleted and the newest canonical row is active, this returns the stale deletion timestamp and excludes the repository from the v1 health-score COPY. Preserve NULL inside a tuple.
services/libs/tinybird/pipes/repo_health_score_copy.pipe:11- The lowercased base key does not match every sibling metric:
repo_health_score_security.pipe:43-45still emits and groups by the originalrepocasing. Mixed-case security evaluations therefore no longer join, makingsecurityPercentageNULL and changing the overall-score divisor from four categories to three. Normalize the security pipe's output and single-repo filter to the same key.
…g joins (IN-1307) Signed-off-by: Gašper Grom <gasper.grom@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 47 out of 47 changed files in this pull request and generated 3 comments.
Suppressed comments (1)
services/libs/tinybird/pipes/project_insights_health_breakdown_copy.pipe:156
- Filtering
deletedAtbeforeargMaxresurrects an older case variant when the newest canonical row is soft-deleted, so that stale repository still contributes to the project rollup. Select the complete latest tuple first and apply the deletion filter afterward, as the other deduplication nodes in this PR do.
WHERE deletedAt IS NULL
|
Re: @epipav's review — thanks for measuring the actual scope, that's useful data. On the blocking bug: I checked this against the current code and it isn't there. On the broader proposal — fixing the 5 duplicate/mismatched |
…N-1307) Same nullable-argMax tuple pattern applied to project_insights_health_breakdown_copy's repo dedup; health_score_v2_security's repoUrl is now case-deduped to match the canonical base used by health_score_v2/signal_detail; project_insights_copy's period-metrics activity aggregation now groups by lower(channel) to avoid fanning out duplicate rows when both casing variants of a channel exist. Signed-off-by: Gašper Grom <gasper.grom@gmail.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 228b843. Configure here.
| FROM repositories FINAL | ||
| GROUP BY lower(url) | ||
| ) | ||
| WHERE isNull (deletedAt) AND archived = false AND excluded = false |
There was a problem hiding this comment.
Security joins miss mixed-case URLs
High Severity
allRepos now emits the mixed-case canonical repositories.url, but the LEFT JOINs onto repos (scorecard, security practices, packages) still use exact url equality. repos.url is always lowercase-canonicalized for GitHub/GitLab, so those joins miss for mixed-case repos like hermes-agent and the security category stays empty. Sibling development/lifecycle pipes already join repos on lower(url).
Reviewed by Cursor Bugbot for commit 228b843. Configure here.
| FROM activityRelations_deduplicated_cleaned_bucket_union | ||
| WHERE timestamp >= now() - INTERVAL 730 DAY AND timestamp <= now() | ||
| GROUP BY channel | ||
| GROUP BY lower(channel) |
There was a problem hiding this comment.
Aliased GROUP BY will fail at runtime
High Severity
project_insights_copy_repo_period_metrics selects lower(channel) AS channel and then GROUP BY lower(channel). ClickHouse treats channel in the GROUP BY expression as that alias, so the grouping key no longer matches the select list and the COPY pipe fails with NOT_AN_AGGREGATE. The repo-scoped health-score pipes in this PR already group on the alias (GROUP BY channel) for that reason.
Reviewed by Cursor Bugbot for commit 228b843. Configure here.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 48 out of 48 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
services/libs/tinybird/pipes/health_score_v2.pipe:97
- This base now retains one canonical casing, but
health_score_v2_impact.pipestill enumerates exact-case repository rows and joinsreposwithr.url = base.url. If package data is attached to the discarded casing, the impact producer emits that casing only and the exact downstream join dropsimpactScoreRaw. Canonicalize and aggregate the impact producer with the same repository key before reducing this base to one row.
services/libs/tinybird/pipes/repo_health_score_security.pipe:43 - The single-repository filter earlier in this pipe still compares
repoandrepoUrlcase-sensitively. Lowercasing only the final output means a mixed-case security row is filtered out before this aggregation, so directrepoUrlcalls still return no score. Normalize the predicate as well.
| FROM activityRelations_deduplicated_cleaned_bucket_union | ||
| WHERE timestamp >= now() - INTERVAL 730 DAY AND timestamp <= now() | ||
| GROUP BY channel | ||
| GROUP BY lower(channel) |
| latest .3 AS excluded, | ||
| latest .4 AS deletedAt | ||
| FROM repositories FINAL | ||
| GROUP BY lower(url) |
Summary
hermes-agent (and other repos where GitHub's live org-name casing differs from
repositories.url's lowercase-normalized form — e.g.NousResearch/hermes-agentvsnousresearch/hermes-agent) was missing most Health Score v2 signals and showing zero rows for PRs/issues/contributors despite having real activity. Root cause was a family of case-sensitivity and duplicate-row bugs across the Tinybird channel-based join/filter chain, found and fixed in stages as each fix surfaced the next:health_score_v2_maintainer,_development,_lifecycle,_raw_inputs_snapshot,_signal_detail,health_score_v2.pipe): joins againstrepos_channels_ds.channelused exact-string equality, silently dropping every PR/issue/commit row under the mixed-case channel. Wrapped both sides of the join inlower().activities_filtered*,pull_requests_filtered,activityTypes_by_project,activity_heatmap_by_weekday_and_2hours_blocks): same case-sensitive channel filter, same fix.repositoriescase-only duplicates: some repos have tworepositoriesrows differing only by URL casing.repos_channels_copy.pipeandrepos_to_channels.pipeenumerated both, fanning out duplicate channel rows into everything downstream;health_score_v2.pipe,health_score_v2_signal_detail.pipe,repo_health_score_copy.pipe,project_insights_health_breakdown_copy.pipededuped their base repo enumeration to match.repo_health_score_*.pipefiles) and the two*_average_resolve_velocity.pipefiles had the identical case-sensitivity bug, unfixed until this pass — normalized filter/GROUP BY/output tolower(channel), and array-membership checks tolower(channel) IN (arrayMap(x -> lower(x), ...)).NOT_AN_AGGREGATEerror:GROUP BY lower(channel)fails onceSELECT lower(channel) AS channelalready aliases the same value. Fixed by grouping on the alias (GROUP BY channel) instead of re-deriving it.No schema change, no ingestion change — all changes are inside
services/libs/tinybird/pipes/.Also fixed on the same investigation, but not part of this diff:
project_insights_copy.pipe'sCOPY_SCHEDULEcron had gone stale and hadn't re-run since the v2 join fix landed — recovered with a manual copy-job re-run rather than a code change.Validation
Already deployed directly to production via
crowd-tinybird-manager(staging deploy blocked for some files by a pre-existing, unrelated staging drift —lfx_insights_stg.issues_analyzedmissing achannelcolumn, andrepos_to_channels_excludednever synced to staging). Validated against live prod Tinybird data:There's no automated test framework for
.pipeSQL files in this repo (confirmed — only fixtures exist), so this validation against live data is the extent of verification possible here.JIRA
IN-1307 — Investigate missing health signals for Hermes project on LFX Insights