fix: deterministic package repo tie-break - #4263
Conversation
Signed-off-by: anilb <epipav@gmail.com>
|
|
|
Your PR title doesn't contain a Jira issue key. Consider adding it for better traceability. Example:
Projects:
Please add a Jira issue key to your PR title. |
Signed-off-by: anilb <epipav@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR aims to make “winner repo” selection deterministic when a package maps to multiple repositories with tied confidence, so Postgres-derived API responses and Tinybird-enriched health/scorecard data don’t fluctuate run-to-run.
Changes:
- Postgres: adds
repo_id DESCas a deterministic tie-breaker to severalLIMIT 1repo-selection laterals inservices/libs/data-access-layer/src/osspckgs/api.ts. - Tinybird: adds
repoIdas a final component in theargMaxtuple for per-package repo selection inossPackages_enriched.pipe.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| services/libs/data-access-layer/src/osspckgs/api.ts | Adds deterministic ordering for selecting a single repo per package in multiple Postgres queries. |
| services/libs/tinybird/pipes/ossPackages_enriched.pipe | Adds a final tie-break key to Tinybird argMax selection to make repo choice deterministic under ties. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@epipav if this is already in production can we merge it? Some Tinybird PRs are becoming stale, and might lead to confusion on what's in production or not. Thanks! |
…reak Signed-off-by: anilb <epipav@gmail.com> # Conflicts: # services/libs/data-access-layer/src/osspckgs/api.ts
PR SummaryLow Risk Overview Postgres: Tinybird: Reviewed by Cursor Bugbot for commit dc22458. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (3)
services/libs/data-access-layer/src/osspckgs/api.ts:524
- The equivalent scorecard lateral in
stewardships.ts:626-634still orders only by confidence.listMyPackagesuses that selected scorecard for its health value, filter, and sort, so equal-confidence mappings can still flip between runs despite the PR's stated scope. Please add the samerepo_id DESCtie-break there.
ORDER BY pr.confidence DESC, pr.repo_id DESC
services/libs/data-access-layer/src/osspckgs/api.ts:343
- No DAL test currently calls these scorecard-based status/list/scatter functions with equal-confidence repo mappings, so the regression this change targets is unverified. Please add coverage that creates two tied mappings and asserts the higher
repo_idsupplies the selected scorecard.
This issue also appears on line 524 of the same file.
ORDER BY pr.confidence DESC, pr.repo_id DESC
services/libs/data-access-layer/src/osspckgs/sqlFragments.ts:27
- The existing
getPackageDetailsByPurlsintegration test inserts only one repo mapping, so it does not exercise either tie-break added here. Please add tied mappings that verify a declared source wins first and the higherrepo_idwins when source and confidence are equal.
ORDER BY pr2.confidence DESC, (pr2.source = 'declared') DESC, pr2.repo_id DESC
|
@cursor review |
Signed-off-by: anilb <epipav@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (4)
services/libs/data-access-layer/src/osspckgs/api.ts:524
- This package-list scorecard selector now prefers a declared link over a higher
repo_idon equal confidence, contrary to the statedconfidence DESC, repo_id DESCrule. That is a behavioral change beyond resolving nondeterminism and can alter displayed health and filtering.
ORDER BY pr.confidence DESC, (pr.source = 'declared') DESC, pr.repo_id DESC
services/libs/data-access-layer/src/osspckgs/api.ts:979
- The scatter scorecard lookup should add only the deterministic
repo_idtie-break described by the PR. The inserted declared-source preference can select a lower-ID repository and therefore changes scatter health data rather than merely stabilizing it.
ORDER BY pr.confidence DESC, (pr.source = 'declared') DESC, pr.repo_id DESC
services/libs/data-access-layer/src/osspckgs/api.ts:343
- The PR says scorecard selectors should keep
confidenceas the only preference and userepo_idto break ties. Placingsource = 'declared'first changes status counts whenever equally confident links have different sources, instead of only making the existing choice deterministic.
This issue also appears in the following locations of the same file:
- line 524
- line 979
ORDER BY pr.confidence DESC, (pr.source = 'declared') DESC, pr.repo_id DESC
services/libs/data-access-layer/src/osspckgs/stewardships.ts:632
- This scorecard lookup previously ranked links only by confidence, so inserting a declared-source preference changes which repository supplies health data when confidence ties. Keep the existing semantics and add
repo_iddirectly as the deterministic tie-break.
ORDER BY pr.confidence DESC, (pr.source = 'declared') DESC, pr.repo_id DESC
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit dc22458. Configure here.
Note
Low Risk
Narrow query-ordering change; may shift which repo is shown when ties exist, but does not alter auth, writes, or broader business logic.
Overview
When a package maps to multiple repos with the same confidence, which repo wins was undefined, so scorecard-driven health, filters, and detail views could flip between runs.
Postgres (
osspckgs/api.ts): Every lateral that picks onepackage_reposrow for scorecard (status counts, package list, scatter) now sorts byconfidence DESCthenrepo_id DESC. Package detail keeps its existingdeclaredpreference and addsrepo_id DESCafter that.Tinybird (
ossPackages_enriched.pipe): The per-packageargMax(repoId, …)tie-break tuple now includesrepoIdafter confidence andverifiedAt, matching the same deterministic rule for enriched health scores.Reviewed by Cursor Bugbot for commit ecab2d3. Bugbot is set up for automated code reviews on this repo. Configure here.