diff --git a/backend/src/osspckgs/migrations/V1788307300__package_repo_signal.sql b/backend/src/osspckgs/migrations/V1788307300__package_repo_signal.sql new file mode 100644 index 0000000000..2405155289 --- /dev/null +++ b/backend/src/osspckgs/migrations/V1788307300__package_repo_signal.sql @@ -0,0 +1,178 @@ +-- Secondary manifest repository signal (CM-1393). +-- +-- Extends the confidence scoring introduced in V1788307200 with the manifest field +-- that produced a declared link. A repo URL read from a fallback field (homepage, +-- bug_tracker) is weaker evidence than one read from the dedicated repository field, +-- so it lands one tier lower. +-- +-- The column defaults to 'primary', so existing rows keep their current score until +-- the next enrichment pass writes a real signal or a rescore sweep runs. + +ALTER TABLE package_repos + ADD COLUMN IF NOT EXISTS signal text NOT NULL DEFAULT 'primary' + CHECK (signal IN ('primary', 'secondary')); + +-- Adding a parameter changes the signature, so the V1788307200 function is dropped +-- rather than replaced — CREATE OR REPLACE would leave both overloads callable. +DROP FUNCTION IF EXISTS package_repo_confidence( + text, text, text, bool, bool, bool, text, bool, bigint +); + +CREATE OR REPLACE FUNCTION package_repo_confidence( + p_source text, + p_ecosystem text, + p_signal text, + p_provenance text, + p_archived bool, + p_is_fork bool, + p_disabled bool, + p_host text, + p_competing_github bool, + p_repo_id bigint +) +RETURNS numeric(12, 9) +LANGUAGE plpgsql IMMUTABLE AS $$ +DECLARE + base numeric; + source_priority int; + offset_units bigint; +BEGIN + base := CASE p_source + WHEN 'manual' THEN 0.99 + WHEN 'heuristic' THEN 0.30 + WHEN 'deps_dev' THEN CASE p_provenance + WHEN 'SLSA_ATTESTATION' THEN 0.99 + WHEN 'RUBYGEMS_PUBLISH_ATTESTATION' THEN 0.95 + WHEN 'PYPI_PUBLISH_ATTESTATION' THEN 0.95 + WHEN 'GO_ORIGIN' THEN 0.90 + ELSE 0.50 + END + -- maven splits off npm/cargo/the rest: POM blocks are notoriously + -- stale (legacy SVN URLs, org renames, dead mirrors). + WHEN 'declared' THEN CASE WHEN p_ecosystem = 'maven' THEN 0.80 ELSE 0.85 END + ELSE 0.30 + END; + + -- Signal adjusts the declared tier only. A deps.dev publish attestation already + -- proves the publisher–repo relationship, and manual links are operator-pinned. + IF p_source = 'declared' AND p_signal = 'secondary' THEN + base := base - 0.10; + END IF; + + source_priority := CASE p_source + WHEN 'manual' THEN 3 + WHEN 'deps_dev' THEN 2 + WHEN 'declared' THEN 1 + ELSE 0 + END; + + IF p_disabled IS TRUE THEN + -- Scale proportionally so pre-disabled claim ordering is preserved across sources. + -- The offset uses a tighter modulo so max contribution (3*1000+999)*1e-9 ≈ 4e-6 + -- stays below the 0.00016 minimum scaled tier gap and cannot invert source ordering. + base := 0.05 + LEAST(base, 0.99) * 0.004; + offset_units := source_priority::bigint * 1000 + COALESCE(p_repo_id, 0) % 1000; + ELSE + IF p_archived IS TRUE THEN + base := base - 0.20; + END IF; + + IF p_is_fork IS TRUE THEN + base := base - 0.10; + END IF; + + IF p_competing_github IS TRUE AND p_host IS NOT NULL AND p_host <> 'github' THEN + base := base - 0.05; + END IF; + + base := GREATEST(base, 0.05); + + -- Tie-breaker: reduces same-source collisions to the rare case where two repo IDs for + -- the same package are congruent mod 1,000,000. BEST_REPO_LINK_JOIN uses a secondary + -- ORDER BY repo_id DESC as the canonical deterministic pick when confidence ties. + offset_units := source_priority::bigint * 1000000 + COALESCE(p_repo_id, 0) % 1000000; + END IF; + + RETURN LEAST(base + offset_units * 0.000000001, 0.999999999); +END; +$$; + +-- Replaced only to pass cur.signal through to the widened scoring function; the +-- chunking, locking and keyset paging are unchanged from V1788307200. +CREATE OR REPLACE PROCEDURE rescore_package_repo_confidence( + p_repo_ids bigint[] DEFAULT NULL, + chunk_size int DEFAULT 25000, + INOUT applied_rows int DEFAULT 0 +) +LANGUAGE plpgsql AS $$ +DECLARE + batch_rows int; + updated_rows int; + cursor_id bigint := 0; +BEGIN + IF chunk_size IS NULL OR chunk_size <= 0 THEN + RAISE EXCEPTION 'rescore_package_repo_confidence: chunk_size must be positive, got %', chunk_size; + END IF; + + -- Session-level: survives the internal COMMITs below. + IF NOT pg_try_advisory_lock(hashtextextended('rescore_package_repo_confidence', 0)) THEN + RAISE EXCEPTION 'rescore_package_repo_confidence: another execution is already in progress'; + END IF; + + applied_rows := 0; + + LOOP + WITH batch AS ( + SELECT pr.id + FROM package_repos pr + WHERE pr.id > cursor_id + AND (p_repo_ids IS NULL OR pr.repo_id = ANY(p_repo_ids)) + -- deps_dev rows with NULL provenance were ingested before this column existed; + -- skip them so the backfill does not downgrade SLSA/attestation links to 0.50. + -- They will be rescored correctly once the next ingest populates provenance. + AND NOT (pr.source = 'deps_dev' AND pr.provenance IS NULL) + ORDER BY pr.id + LIMIT chunk_size + FOR UPDATE + ), + updated AS ( + UPDATE package_repos pr + SET confidence = s.confidence, verified_at = NOW() + FROM batch b + JOIN package_repos cur ON cur.id = b.id + JOIN packages p ON p.id = cur.package_id + JOIN repos r ON r.id = cur.repo_id, + LATERAL ( + SELECT package_repo_confidence( + cur.source, p.ecosystem, cur.signal, cur.provenance, + r.archived, r.is_fork, r.disabled, r.host, + EXISTS ( + SELECT 1 + FROM package_repos c + JOIN repos cr ON cr.id = c.repo_id + WHERE c.package_id = cur.package_id + AND c.repo_id <> cur.repo_id + AND cr.host = 'github' + ), + cur.repo_id + ) AS confidence + ) s + WHERE pr.id = b.id + AND s.confidence IS DISTINCT FROM cur.confidence + RETURNING pr.id + ) + SELECT COUNT(*), COALESCE(MAX(b.id), cursor_id), + (SELECT COUNT(*) FROM updated) + INTO batch_rows, cursor_id, updated_rows + FROM batch b; + + applied_rows := applied_rows + updated_rows; + + COMMIT; + + EXIT WHEN batch_rows < chunk_size; + END LOOP; + + PERFORM pg_advisory_unlock(hashtextextended('rescore_package_repo_confidence', 0)); +END; +$$; diff --git a/docs/adr/0021-secondary-manifest-repository-signal.md b/docs/adr/0021-secondary-manifest-repository-signal.md new file mode 100644 index 0000000000..bd74346851 --- /dev/null +++ b/docs/adr/0021-secondary-manifest-repository-signal.md @@ -0,0 +1,132 @@ +# ADR-0021: Secondary manifest repository signal + +**Date**: 2026-09-01 +**Status**: accepted +**Deciders**: Joana Maia + +## Context + +Every registry writer only created a `package_repos` row when the ecosystem's +canonical repository field parsed — npm `repository`, cargo `repository`, +rubygems `source_code_uri`, NuGet ``, POM ``. A large +share of packages leave that field empty while publishing the same repo URL in +`homepage`, `bugs.url`, `projectUrl`, `bug_tracker_uri`, or the POM ``, and +those packages ended up with no repo link at all — invisible to criticality, +blast radius, and Insights. + +Simply widening each writer to accept any of those fields would trade +under-coverage for wrong links: fallback fields are free-form, so +`https://example.com/docs/getting-started` canonicalizes into a plausible +`owner/repo` shape without being a repository. Fallback links also should not +rank equally with a declared one. +[ADR-0020](./0020-package-repo-confidence-scoring.md) already reserved the +`signal` column and its −0.10 penalty for exactly this. + +## Decision + +One shared helper, `resolveManifestRepo(candidates)` +(`packages_worker/src/utils/resolveManifestRepo.ts`), resolves a package's repo +from an ordered candidate list. The first candidate is the ecosystem's canonical +field and resolves as `primary`; every later candidate resolves as `secondary`. +The result carries `{ repo, signal }`, and each writer persists the +returned `signal` on the link. No writer computes a confidence value. + +### Chains + +| Ecosystem | Chain | +| --- | --- | +| npm | `repository` → `homepage` → `bugs.url` | +| pypi | Source/Code project URL → `Homepage` → bug tracker URL | +| cargo | `repository` → `homepage` | +| rubygems | `source_code_uri` → `homepage_uri` → `bug_tracker_uri` | +| packagist | `support.source` → `homepage` | +| nuget | `` → `projectUrl` | +| maven | POM `` → POM `` | + +### Host gate + +Candidates go through the shared `canonicalizeRepoUrl`. A `secondary` candidate +is rejected when canonicalization yields `host === 'other'` — recognized VCS +hosts only. The `primary` candidate keeps its historical behaviour and still +accepts `other`, so existing links to self-hosted Gitea, cgit, and SVN are +unaffected. Packagist already applied this gate locally; it is now the shared +rule. + +Cargo is the exception on mechanics, not on policy: its pipeline is set-based +SQL over a dump, so `normalizeRepos` stages both `declared_repository_url` and +`homepage` into `repo_norm`, and a new `repo_choice` table applies the same +first-wins-with-host-gate rule in SQL. `documentation` is not staged — it is +almost always docs.rs, which the host gate rejects anyway. + +## Alternatives Considered + +### Alternative 1: Widen each writer's existing extractor in place + +- **Pros**: no new module; smallest diff per ecosystem. +- **Cons**: seven copies of the fallback order and the host gate, which is how + the current per-ecosystem divergence arose in the first place; the `signal` + value would be derived independently in each writer. +- **Why not**: the whole point is one rule; nine implementations of "which + field won" is the defect, not the fix. + +### Alternative 2: Accept fallback URLs on any host, like the primary field does + +- **Pros**: maximum coverage; no URL is discarded. +- **Cons**: a documentation site or a marketing page with two path segments + becomes a repo link, creating a `repos` row and an incorrect + `packages_published` attribution — the exact failure this epic exists to fix. +- **Why not**: coverage gained by inventing repos is negative value; the + primary field at least carries the publisher's explicit claim. + +### Alternative 3: Score fallback links lower directly in the writers instead of adding `signal` + +- **Pros**: no schema column; visible in one place. +- **Cons**: reintroduces per-writer confidence literals, and the penalty could + not be retuned or audited afterwards — nothing records *why* a row scored + lower. +- **Why not**: ADR-0020 makes the stored score derivable from stored evidence; + `signal` is that evidence. + +### Alternative 4: Backfill a separate pass that mines fallback fields for packages with no link + +- **Pros**: zero risk to the existing write paths; can be re-run at will. +- **Cons**: a second code path that has to re-fetch or re-read every manifest, + and it goes stale the moment a package is re-ingested. +- **Why not**: the data is already in hand at write time; the write path is + the cheapest place to fix coverage. + +## Consequences + +### Positive + +- Packages that only publish their repo in a secondary field now get a link, + and the link is honestly labelled as weaker. +- The fallback order and the host gate exist once, so adding an ecosystem means + declaring a candidate list. +- Per-run counters (`primary_field_hit`, `fallback_hit_by_field`, `no_signal`) + make the coverage uplift measurable against the pre-merge baseline. + +### Negative + +- Secondary links are, by construction, less certain than declared ones; some + will be wrong even with the host gate. +- Maven and cargo needed local restructuring (maven's POM-specific + `normalizeScmUrl` feeds the same `package_repos` write path; cargo's + `repo_choice` applies the host-gate rule in SQL) to reach the same behaviour. +- Row counts in `package_repos` grow, and the dedup/keep-highest path now sees + more competing links per package. + +### Risks + +- **A secondary link can outrank a genuine one when the declared field is + missing on the true repo but present on a fork.** Mitigation: ADR-0020's + fork and archived penalties, plus ADR-0022's ownership evidence, which + penalises the fork's owner mismatch far more heavily than the secondary + penalty. +- **Recognized-host gating rejects legitimate self-hosted repos found in a + fallback field.** Accepted deliberately: an unrecognized host in a free-form + field carries no signal that it is a repository at all. Revisit if the + `no_signal` counters show a material self-hosted tail. +- **Coverage growth is hard to attribute after the fact.** Mitigation: record + per-ecosystem `package_repos` row counts before merge, and compare against + the `fallback_hit_by_field` counters afterwards. diff --git a/docs/adr/README.md b/docs/adr/README.md index 8d2aba9ec2..23ac0a51e3 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -27,6 +27,7 @@ Use the `/adr` skill in Claude Code to record new ADRs or query past decisions. | [ADR-0018](./0018-per-client-rate-limiting-members-resolve.md) | Per-client rate limiting for `POST /members/resolve` using in-memory store | accepted | 2026-08-12 | | [ADR-0019](./0019-docker-builder-runner-libc.md) | Same libc in Docker builder and runner | accepted | 2026-08-27 | | [ADR-0020](./0020-package-repo-confidence-scoring.md) | Deterministic package→repo confidence scoring | accepted | 2026-09-01 | +| [ADR-0021](./0021-secondary-manifest-repository-signal.md) | Secondary manifest repository signal | accepted | 2026-09-01 | ## Why ADRs? diff --git a/services/apps/packages_worker/src/cargo/enrich.ts b/services/apps/packages_worker/src/cargo/enrich.ts index 9da1d9ad34..990d952c23 100644 --- a/services/apps/packages_worker/src/cargo/enrich.ts +++ b/services/apps/packages_worker/src/cargo/enrich.ts @@ -22,6 +22,7 @@ const INGESTION_SOURCE = 'cargo-registry' const REPO_LINK_SOURCE = 'declared' // same convention as npm/maven for manifest-declared repo URLs const CARGO_CONFIDENCE = packageRepoConfidenceCall('p', 'r', { source: '$(source)', + signal: 'rc.signal', provenance: 'NULL', }) @@ -69,8 +70,11 @@ export async function enrichPackages(qx: QueryExecutor): Promise { return withTunedSession(qx, 'repos', async (tx) => { const repoRow = await tx.selectOne( `WITH new_repos AS ( INSERT INTO repos (url, host, updated_at) - SELECT DISTINCT rn.repository_url, rn.host, NOW() - FROM ${STAGING_SCHEMA}.enrich_packages e - JOIN ${STAGING_SCHEMA}.repo_norm rn ON rn.declared = e.declared_repository_url + SELECT DISTINCT rc.repository_url, rc.host, NOW() + FROM ${STAGING_SCHEMA}.repo_choice rc + WHERE rc.repository_url IS NOT NULL ON CONFLICT (url) DO NOTHING RETURNING url ), ins_audit AS ( INSERT INTO ${STAGING_SCHEMA}.audit_changes (package_id, field) - SELECT e.package_id, f.field - FROM ${STAGING_SCHEMA}.enrich_packages e - JOIN ${STAGING_SCHEMA}.repo_norm rn ON rn.declared = e.declared_repository_url - JOIN new_repos nr ON nr.url = rn.repository_url + SELECT rc.package_id, f.field + FROM ${STAGING_SCHEMA}.repo_choice rc + JOIN new_repos nr ON nr.url = rc.repository_url CROSS JOIN LATERAL (VALUES ('repos.url'), ('repos.host')) AS f(field) RETURNING 1 ) SELECT (SELECT COUNT(*) FROM new_repos)::int AS repos`, ) - // Prunes stale 'declared' links before relinking — covers junk/unparseable declared - // values, rewrites (declared URL now maps elsewhere), and removals (this dump's - // declared_repository_url is NULL, meaning the crate no longer declares a repo at - // all — loadDump.ts stages every matched crate every run, so NULL here is - // authoritative, not "no data this run"). Safe to always prune on that signal because - // the DELETE is scoped to source = 'declared' — cargo only ever removes links it owns. - // Without this, package_repos would accumulate a link to a repo no crate declares - // anymore, and consumers such as security-contacts (which join through - // repos ⋈ package_repos, not packages.repository_url) would keep reading it. + // Prunes cargo-owned declared links whose target changed since the last run: removals + // (NULL in this dump is authoritative — loadDump stages every crate every run), URL + // rewrites, and junk/unparseable values. An unchanged link is left alone so the upsert + // below's ON CONFLICT ... KEEP_HIGHEST_CONFLICT_UPDATE handles same-repo signal/confidence + // changes (e.g. a primary→secondary downgrade) without a delete+reinsert. Scoped to + // source = 'declared' so only cargo-owned rows are touched. const pruneRow = await tx.selectOne( `WITH targets AS ( - SELECT e.package_id, r.id AS repo_id - FROM ${STAGING_SCHEMA}.enrich_packages e - LEFT JOIN ${STAGING_SCHEMA}.repo_norm rn ON rn.declared = e.declared_repository_url - LEFT JOIN repos r ON r.url = rn.repository_url + SELECT rc.package_id, r.id AS repo_id + FROM ${STAGING_SCHEMA}.repo_choice rc + LEFT JOIN repos r ON r.url = rc.repository_url ), del AS ( DELETE FROM package_repos pr USING targets t WHERE pr.package_id = t.package_id AND pr.source = $(source) - AND (t.repo_id IS NULL OR pr.repo_id IS DISTINCT FROM t.repo_id) + AND pr.repo_id IS DISTINCT FROM t.repo_id RETURNING pr.package_id ), ins_audit AS ( @@ -240,19 +238,18 @@ export async function enrichRepos(qx: QueryExecutor): Promise SELECT pr.package_id, pr.repo_id, pr.source, pr.confidence FROM package_repos pr WHERE pr.package_id IN ( - SELECT package_id FROM ${STAGING_SCHEMA}.enrich_packages WHERE declared_repository_url IS NOT NULL + SELECT package_id FROM ${STAGING_SCHEMA}.repo_choice WHERE repository_url IS NOT NULL ) ), ins AS ( INSERT INTO package_repos ( - package_id, repo_id, source, provenance, confidence, created_at, verified_at + package_id, repo_id, source, signal, provenance, confidence, created_at, verified_at ) - SELECT e.package_id, r.id, $(source), NULL, + SELECT rc.package_id, r.id, $(source), rc.signal, NULL, s.confidence, NOW(), NOW() - FROM ${STAGING_SCHEMA}.enrich_packages e - JOIN ${STAGING_SCHEMA}.repo_norm rn ON rn.declared = e.declared_repository_url - JOIN repos r ON r.url = rn.repository_url - JOIN packages p ON p.id = e.package_id + FROM ${STAGING_SCHEMA}.repo_choice rc + JOIN repos r ON r.url = rc.repository_url + JOIN packages p ON p.id = rc.package_id CROSS JOIN LATERAL (SELECT ${CARGO_CONFIDENCE} AS confidence) s ON CONFLICT (package_id, repo_id) DO UPDATE SET ${KEEP_HIGHEST_CONFLICT_UPDATE} diff --git a/services/apps/packages_worker/src/cargo/normalizeRepos.ts b/services/apps/packages_worker/src/cargo/normalizeRepos.ts index f32b7aff4d..fd4e8e3f17 100644 --- a/services/apps/packages_worker/src/cargo/normalizeRepos.ts +++ b/services/apps/packages_worker/src/cargo/normalizeRepos.ts @@ -11,29 +11,15 @@ const log = getServiceChildLogger('cargo-normalize') // Batch size for the mapping upsert — keeps parameter arrays well under Postgres limits. const INSERT_BATCH = 5000 -/** - * Builds `cargo_sync.repo_norm(declared, repository_url, host)`, mapping every - * distinct `declared_repository_url` staged in `enrich_packages` to its canonical - * `https:////` form via the shared `canonicalizeRepoUrl` - * (same normalizer npm/pypi use — no per-ecosystem fork). - * - * Only inputs `canonicalizeRepoUrl` can reduce to an owner/name pair are stored; - * unparseable URLs or those with fewer than two path segments are omitted, so a - * LEFT JOIN from `enrich_packages` yields NULL — that both recovers derivable - * URLs (Gap A) and clears that class of junk from `packages.repository_url` - * (Gap C). Note this does not validate that the result is an actual repository - * host — a URL-shaped homepage with ≥2 path segments (e.g. - * `https://example.com/owner/repo`) still canonicalizes and is stored. - * - * Normalization runs in TypeScript because the bulk set-based cargo pipeline - * cannot call the parser per row; the resulting mapping table lets the SQL - * enrich phases join back to it. Idempotent: the table is rebuilt each run. - */ export async function normalizeRepos(qx: QueryExecutor): Promise { - const rows: Array<{ declared_repository_url: string }> = await qx.select( - `SELECT DISTINCT declared_repository_url + const rows: Array<{ url: string }> = await qx.select( + `SELECT DISTINCT declared_repository_url AS url FROM ${STAGING_SCHEMA}.enrich_packages - WHERE declared_repository_url IS NOT NULL`, + WHERE declared_repository_url IS NOT NULL + UNION + SELECT DISTINCT homepage AS url + FROM ${STAGING_SCHEMA}.enrich_packages + WHERE homepage IS NOT NULL`, ) await qx.result( @@ -46,9 +32,9 @@ export async function normalizeRepos(qx: QueryExecutor): Promise ({ - declared: declared_repository_url, - canonical: canonicalizeRepoUrl(declared_repository_url), + .map(({ url }) => ({ + declared: url, + canonical: canonicalizeRepoUrl(url), })) .filter((r): r is { declared: string; canonical: CanonicalRepo } => r.canonical !== null) @@ -66,7 +52,29 @@ export async function normalizeRepos(qx: QueryExecutor): Promise 'other'; + CREATE INDEX ON ${STAGING_SCHEMA}.repo_choice (package_id)`, + ) + + const choiceRow = await qx.selectOne( + `SELECT COUNT(*) FILTER (WHERE repository_url IS NOT NULL AND signal = 'secondary')::int AS fallbacks + FROM ${STAGING_SCHEMA}.repo_choice`, + ) + + const result: NormalizeReposResult = { + scanned: rows.length, + normalized: mapped.length, + homepageFallbacks: choiceRow.fallbacks, + } log.info(result, 'cargo repo normalization complete') return result } diff --git a/services/apps/packages_worker/src/cargo/types.ts b/services/apps/packages_worker/src/cargo/types.ts index aafa402340..510ab5b08a 100644 --- a/services/apps/packages_worker/src/cargo/types.ts +++ b/services/apps/packages_worker/src/cargo/types.ts @@ -15,6 +15,7 @@ export interface LoadResult { export interface NormalizeReposResult { scanned: number normalized: number + homepageFallbacks: number } export interface EnrichPackagesResult { diff --git a/services/apps/packages_worker/src/deps-dev/workflows/ingestRepos.ts b/services/apps/packages_worker/src/deps-dev/workflows/ingestRepos.ts index 113d1dd998..cd74904c50 100644 --- a/services/apps/packages_worker/src/deps-dev/workflows/ingestRepos.ts +++ b/services/apps/packages_worker/src/deps-dev/workflows/ingestRepos.ts @@ -111,10 +111,10 @@ WITH github_staged AS MATERIALIZED ( WHERE r2.host = 'github' ) INSERT INTO package_repos ( - package_id, repo_id, source, provenance, confidence, verified_at, created_at + package_id, repo_id, source, signal, provenance, confidence, verified_at, created_at ) SELECT DISTINCT ON (p.id, r.id) - p.id, r.id, 'deps_dev', s.provenance, + p.id, r.id, 'deps_dev', 'primary', s.provenance, c.confidence, NOW(), NOW() FROM staging.osspckgs_package_repos_raw s JOIN packages p ON p.purl = REGEXP_REPLACE(s.purl, '@[^@]+$', '') @@ -125,6 +125,7 @@ CROSS JOIN LATERAL ( 'r', { source: `'deps_dev'`, + signal: `'primary'`, provenance: 's.provenance', }, `((r.host <> 'github' AND EXISTS (SELECT 1 FROM github_staged gs WHERE gs.package_id = p.id)) OR ${competingGithubRepoExpr('p.id', 'r.id')})`, diff --git a/services/apps/packages_worker/src/maven/runMavenEnrichmentLoop.ts b/services/apps/packages_worker/src/maven/runMavenEnrichmentLoop.ts index 621a0174f5..a1f12242d2 100644 --- a/services/apps/packages_worker/src/maven/runMavenEnrichmentLoop.ts +++ b/services/apps/packages_worker/src/maven/runMavenEnrichmentLoop.ts @@ -4,7 +4,9 @@ import { listMavenCriticalPackagesById, listMavenPackagesToSync, logAuditFieldChange, + removeDeclaredPackageRepo, replacePackageMaintainers, + setPackageRepositoryUrl, touchPackageSyncedAt, upsertMaintainer, upsertPackage, @@ -12,9 +14,11 @@ import { upsertRepo, upsertVersionsBatch, } from '@crowd/data-access-layer' +import type { PackageRepoSignal } from '@crowd/data-access-layer/src/packages/repoConfidence' import { getServiceChildLogger } from '@crowd/logging' import { getMavenConfig } from '../config' +import { resolveManifestRepo } from '../utils/resolveManifestRepo' import { extractArtifact, getPomCacheStats, normalizeScmUrl } from './extract' import { isMavenFetchError, resolveVersionsList } from './metadata' @@ -54,13 +58,27 @@ type PackageRow = MavenPackageToSync // ─── Helpers ────────────────────────────────────────────────────────────────── // prettier-ignore -export async function writeRepoLink(qx: QueryExecutor, packageId: number, repositoryUrl: string | null, changed?: Set): Promise { - if (!repositoryUrl) return +export async function writeRepoLink(qx: QueryExecutor, packageId: number, repositoryUrl: string | null, changed?: Set, signal: PackageRepoSignal = 'primary'): Promise { + if (!repositoryUrl) { + const removedFields = await removeDeclaredPackageRepo(qx, String(packageId)) + removedFields.forEach((f) => changed?.add(f)) + const clearedFields = await setPackageRepositoryUrl(qx, String(packageId), null) + clearedFields.forEach((f) => changed?.add(f)) + return + } const parsed = parseRepoUrl(repositoryUrl) - if (!parsed) return + if (!parsed) { + const removedFields = await removeDeclaredPackageRepo(qx, String(packageId)) + removedFields.forEach((f) => changed?.add(f)) + const clearedFields = await setPackageRepositoryUrl(qx, String(packageId), null) + clearedFields.forEach((f) => changed?.add(f)) + return + } const repoId = await upsertRepo(qx, { url: repositoryUrl, ...parsed }) - const repoChanged = await upsertPackageRepo(qx, String(packageId), String(repoId), { source: 'declared' }) + const repoChanged = await upsertPackageRepo(qx, String(packageId), String(repoId), { source: 'declared', signal }) repoChanged.forEach((f) => changed?.add(f)) + const removedFields = await removeDeclaredPackageRepo(qx, String(packageId), String(repoId)) + removedFields.forEach((f) => changed?.add(f)) } // Postgres deadlock (40P01) is transient: concurrent transactions upserting the same shared @@ -261,7 +279,11 @@ async function processCriticalPackage(qx: QueryExecutor, pkg: PackageRow, forceF return { status: 'error' } } - const repositoryUrl = normalizeScmUrl(result.scmUrl) + const scmRepositoryUrl = normalizeScmUrl(result.scmUrl) + const fallbackRepo = scmRepositoryUrl + ? null + : resolveManifestRepo([{ field: 'url', url: result.homepageUrl, signal: 'secondary' }]) + const repositoryUrl = scmRepositoryUrl ?? fallbackRepo?.repo.url ?? null await withDeadlockRetry(() => qx.tx(async (t: QueryExecutor) => { @@ -335,7 +357,7 @@ async function processCriticalPackage(qx: QueryExecutor, pkg: PackageRow, forceF pmChanged.forEach((f) => changed.add(f)) } - await writeRepoLink(t, packageId, repositoryUrl, changed) + await writeRepoLink(t, packageId, repositoryUrl, changed, fallbackRepo ? 'secondary' : 'primary') await logAuditFieldChange(t, 'maven', pkg.purl, Array.from(changed)) diff --git a/services/apps/packages_worker/src/npm/normalize.ts b/services/apps/packages_worker/src/npm/normalize.ts index 4be34c8710..f66b234019 100644 --- a/services/apps/packages_worker/src/npm/normalize.ts +++ b/services/apps/packages_worker/src/npm/normalize.ts @@ -1,5 +1,4 @@ -import { canonicalizeRepoUrl } from '../utils/canonicalizeRepoUrl' -import type { CanonicalRepo } from '../utils/canonicalizeRepoUrl' +import { ResolvedManifestRepo, resolveManifestRepo } from '../utils/resolveManifestRepo' import type { Packument } from './types' @@ -70,12 +69,24 @@ function dedup(arr: string[]): string[] { return [...new Set(arr)] } -export function extractRepo(packument: Packument): CanonicalRepo | null { +export function npmRepositoryField(packument: Packument): string | null { const repo = packument.repository if (!repo) return null - const raw = typeof repo === 'string' ? repo : repo.url - if (!raw) return null - return canonicalizeRepoUrl(raw) + return (typeof repo === 'string' ? repo : repo.url) || null +} + +function npmBugsUrl(packument: Packument): string | null { + const bugs = packument.bugs + if (!bugs) return null + return (typeof bugs === 'string' ? bugs : bugs.url) || null +} + +export function resolveNpmRepo(packument: Packument): ResolvedManifestRepo | null { + return resolveManifestRepo([ + { field: 'repository', url: npmRepositoryField(packument) }, + { field: 'homepage', url: packument.homepage }, + { field: 'bugs.url', url: npmBugsUrl(packument) }, + ]) } export function collectMaintainers(packument: Packument): Array<{ diff --git a/services/apps/packages_worker/src/npm/types.ts b/services/apps/packages_worker/src/npm/types.ts index 4c4de10694..8956333411 100644 --- a/services/apps/packages_worker/src/npm/types.ts +++ b/services/apps/packages_worker/src/npm/types.ts @@ -16,6 +16,7 @@ export interface Packument { license?: string | { type: string; url?: string } licenses?: Array<{ type: string; url?: string }> repository?: string | { type?: string; url: string; directory?: string } + bugs?: string | { url?: string; email?: string } author?: string | { name: string; email?: string; url?: string } maintainers?: Array<{ name: string; email?: string }> 'dist-tags': Record diff --git a/services/apps/packages_worker/src/npm/upsertPackage.ts b/services/apps/packages_worker/src/npm/upsertPackage.ts index 70dce6fa8a..fdae4158b9 100644 --- a/services/apps/packages_worker/src/npm/upsertPackage.ts +++ b/services/apps/packages_worker/src/npm/upsertPackage.ts @@ -1,5 +1,6 @@ import { getOrCreateRepoByUrl, + removeDeclaredPackageRepo, upsertNpmFundingLinks, upsertNpmPackage, upsertNpmVersions, @@ -12,10 +13,11 @@ import { stripNullBytesDeep } from '../utils/stripNullBytesDeep' import { collectMaintainers, - extractRepo, isPrerelease, normalizeLicenses, + npmRepositoryField, parseNpmName, + resolveNpmRepo, versionLicense, } from './normalize' import type { FundingEntry, Packument } from './types' @@ -36,9 +38,9 @@ export async function upsertPackage( const { namespace, name } = parseNpmName(raw) const licenses = normalizeLicenses(packument) const licensesRaw = typeof packument.license === 'string' ? packument.license : null - const declaredRepositoryUrl = rawRepoUrl(packument) - const repo = extractRepo(packument) - const repositoryUrl = repo?.url ?? null + const declaredRepositoryUrl = npmRepositoryField(packument) + const resolvedRepo = resolveNpmRepo(packument) + const repositoryUrl = resolvedRepo?.repo.url ?? null const versionEntries = Object.entries(packument.versions) const time = packument.time ?? {} const latestVersion = packument['dist-tags']?.latest ?? null @@ -80,16 +82,25 @@ export async function upsertPackage( }) pkgChanged.forEach((f) => changed.add(f)) - if (repo) { + if (resolvedRepo) { const { id: repoId, changedFields: repoChanged } = await getOrCreateRepoByUrl( t, - repo.url, - repo.host, + resolvedRepo.repo.url, + resolvedRepo.repo.host, ) repoChanged.forEach((f) => changed.add(f)) - const linkChanged = await upsertPackageRepo(t, pkgId, repoId, { source: 'declared' }) + const linkChanged = await upsertPackageRepo(t, pkgId, repoId, { + source: 'declared', + signal: resolvedRepo.signal, + }) linkChanged.forEach((f) => changed.add(f)) + + const removedFields = await removeDeclaredPackageRepo(t, pkgId, repoId) + removedFields.forEach((f) => changed.add(f)) + } else { + const removedFields = await removeDeclaredPackageRepo(t, pkgId) + removedFields.forEach((f) => changed.add(f)) } const verChanged = await upsertNpmVersions( @@ -125,12 +136,6 @@ function cleanKeywords(raw: unknown): string[] | null { return cleaned.length > 0 ? cleaned : null } -function rawRepoUrl(packument: Packument): string | null { - const repo = packument.repository - if (!repo) return null - return typeof repo === 'string' ? repo || null : repo.url || null -} - function extractFundingLinks( funding: FundingEntry | FundingEntry[] | undefined, ): Array<{ type?: string; url: string }> { diff --git a/services/apps/packages_worker/src/nuget/normalize.ts b/services/apps/packages_worker/src/nuget/normalize.ts index 4eb0548637..18bc94b5ec 100644 --- a/services/apps/packages_worker/src/nuget/normalize.ts +++ b/services/apps/packages_worker/src/nuget/normalize.ts @@ -1,6 +1,6 @@ import { XMLParser } from 'fast-xml-parser' -import { canonicalizeRepoUrl } from '../utils/canonicalizeRepoUrl' +import { resolveManifestRepo } from '../utils/resolveManifestRepo' import { NormalizedNuGetPackage, @@ -42,17 +42,6 @@ export function parseNuspecRepositoryUrl(nuspecXml: string): string | null { } } -const SCM_HOSTS = ['github.com', 'gitlab.com', 'bitbucket.org'] - -function isScmUrl(url: string | undefined): boolean { - if (!url) return false - try { - return SCM_HOSTS.some((h) => new URL(url).hostname.endsWith(h)) - } catch { - return false - } -} - function parseLicense( licenseExpression: string | undefined, licenseUrl: string | undefined, @@ -95,7 +84,6 @@ export function normalizeNuGetPackage( const homepage = searchResult?.projectUrl || latestListedEntry?.projectUrl || null // Scan all entries (prefer latest listed, then any) for a nuspec url. - // Fall back to a SCM-shaped projectUrl/homepage when no nuspec repository is present. const entriesForRepo = latestListedEntry ? [ latestListedEntry, @@ -107,9 +95,10 @@ export function normalizeNuGetPackage( const fetchedNuspecRepoUrl = nuspecXml ? parseNuspecRepositoryUrl(nuspecXml) : null const nuspecRepoUrl = fetchedNuspecRepoUrl ?? catalogRepoUrl const declaredRepositoryUrl = nuspecRepoUrl ?? null - const repo = - (nuspecRepoUrl ? canonicalizeRepoUrl(nuspecRepoUrl) : null) ?? - (isScmUrl(homepage) ? canonicalizeRepoUrl(homepage) : null) + const resolvedRepo = resolveManifestRepo([ + { field: 'repository', url: nuspecRepoUrl }, + { field: 'projectUrl', url: homepage }, + ]) const keywords = searchResult?.tags && searchResult.tags.length > 0 ? searchResult.tags : null @@ -164,7 +153,7 @@ export function normalizeNuGetPackage( description, homepage: homepage || null, declaredRepositoryUrl, - repo, + resolvedRepo, licenses, licensesRaw, keywords, diff --git a/services/apps/packages_worker/src/nuget/runNuGetEnrichmentLoop.ts b/services/apps/packages_worker/src/nuget/runNuGetEnrichmentLoop.ts index ad3687391a..9378491f7d 100644 --- a/services/apps/packages_worker/src/nuget/runNuGetEnrichmentLoop.ts +++ b/services/apps/packages_worker/src/nuget/runNuGetEnrichmentLoop.ts @@ -7,7 +7,9 @@ import { logAuditFieldChange, markNuGetPackageError, recordNuGetDownloadSnapshot, + removeDeclaredPackageRepo, replacePackageMaintainers, + setPackageRepositoryUrl, upsertMaintainer, upsertNuGetPackage, upsertNuGetVersionsBatch, @@ -118,7 +120,7 @@ async function processPackage( description: normalized.description, homepage: normalized.homepage, declaredRepositoryUrl: normalized.declaredRepositoryUrl, - repositoryUrl: normalized.repo?.url ?? null, + repositoryUrl: normalized.resolvedRepo?.repo.url ?? null, licenses: normalized.licenses, licensesRaw: normalized.licensesRaw, keywords: normalized.keywords, @@ -132,18 +134,27 @@ async function processPackage( }) pkgChanged.forEach((f) => changed.add(f)) - if (normalized.repo) { + if (normalized.resolvedRepo) { const { id: repoId, changedFields: repoChanged } = await getOrCreateRepoByUrl( t, - normalized.repo.url, - normalized.repo.host, + normalized.resolvedRepo.repo.url, + normalized.resolvedRepo.repo.host, ) repoChanged.forEach((f) => changed.add(f)) const linkChanged = await upsertPackageRepo(t, packageDbId.toString(), repoId, { source: 'declared', + signal: normalized.resolvedRepo.signal, }) linkChanged.forEach((f) => changed.add(f)) + + const removedFields = await removeDeclaredPackageRepo(t, packageDbId.toString(), repoId) + removedFields.forEach((f) => changed.add(f)) + } else { + const removedFields = await removeDeclaredPackageRepo(t, packageDbId.toString()) + removedFields.forEach((f) => changed.add(f)) + const clearedFields = await setPackageRepositoryUrl(t, packageDbId.toString(), null) + clearedFields.forEach((f) => changed.add(f)) } if (normalized.versions.length > 0) { diff --git a/services/apps/packages_worker/src/nuget/types.ts b/services/apps/packages_worker/src/nuget/types.ts index 2af096a43f..cb279b816b 100644 --- a/services/apps/packages_worker/src/nuget/types.ts +++ b/services/apps/packages_worker/src/nuget/types.ts @@ -1,4 +1,4 @@ -import { CanonicalRepo } from '../utils/canonicalizeRepoUrl' +import { ResolvedManifestRepo } from '../utils/resolveManifestRepo' export interface NuGetConfig { batchSize: number @@ -75,7 +75,7 @@ export interface NormalizedNuGetPackage { description: string | null homepage: string | null declaredRepositoryUrl: string | null - repo: CanonicalRepo | null + resolvedRepo: ResolvedManifestRepo | null licenses: string[] | null licensesRaw: string | null keywords: string[] | null diff --git a/services/apps/packages_worker/src/packagist/__tests__/ingest.test.ts b/services/apps/packages_worker/src/packagist/__tests__/ingest.test.ts index cb79a500b5..4ba423a371 100644 --- a/services/apps/packages_worker/src/packagist/__tests__/ingest.test.ts +++ b/services/apps/packages_worker/src/packagist/__tests__/ingest.test.ts @@ -25,7 +25,7 @@ import { fetchPackagistP2, fetchPackagistStats } from '../fetchPackage' import { fetchPackagistPackageList, parsePackagistPackageList } from '../listPackages' import { INGEST_MAX_ATTEMPTS } from '../retryPolicy' import { persistPackagistMetadata } from '../upsertMetadata' -import { persistPackagistPackageInfo } from '../upsertPackageInfo' +import { persistPackagistPackageInfo, reconcilePackagistHomepageRepo } from '../upsertPackageInfo' // Mock the heavy collaborators so we exercise the classification/retry/give-up logic. vi.mock('../fetchPackage', () => ({ @@ -33,7 +33,10 @@ vi.mock('../fetchPackage', () => ({ fetchPackagistP2: vi.fn(), buildPackagistUserAgent: vi.fn(() => 'ua'), })) -vi.mock('../upsertPackageInfo', () => ({ persistPackagistPackageInfo: vi.fn() })) +vi.mock('../upsertPackageInfo', () => ({ + persistPackagistPackageInfo: vi.fn(), + reconcilePackagistHomepageRepo: vi.fn().mockResolvedValue([]), +})) vi.mock('../upsertMetadata', () => ({ persistPackagistMetadata: vi.fn() })) vi.mock('../downloads', () => ({ monthlyWindowFor: vi.fn(), @@ -61,6 +64,7 @@ const mockFetchStats = vi.mocked(fetchPackagistStats) const mockFetchP2 = vi.mocked(fetchPackagistP2) const mockExpand = vi.mocked(expandComposerMetadata) const mockPersistInfo = vi.mocked(persistPackagistPackageInfo) +const mockReconcileHomepageRepo = vi.mocked(reconcilePackagistHomepageRepo) const mockPersistMetadata = vi.mocked(persistPackagistMetadata) const mockPersist30d = vi.mocked(persistPackagist30dWindow) const mockDaily = vi.mocked(insertDailyDownloads) @@ -100,7 +104,12 @@ describe('ingestOnePackagistMetadata', () => { function happyMocks() { mockFetchStats.mockResolvedValue(statsJson as never) - mockPersistInfo.mockResolvedValue({ found: true, changedFields: ['packages.description'] }) + mockPersistInfo.mockResolvedValue({ + found: true, + changedFields: ['packages.description'], + packageId: '1', + hasPrimaryRepo: true, + }) mockFetchP2.mockResolvedValue({ minifiedVersions: minified, lastModified: 'Wed, 01 Jul 2026 00:00:00 GMT', @@ -110,6 +119,7 @@ describe('ingestOnePackagistMetadata', () => { found: true, changedFields: ['versions.number'], unresolvedDependencyTargets: 0, + homepage: null, }) } @@ -133,6 +143,39 @@ describe('ingestOnePackagistMetadata', () => { ) }) + it('reconciles the homepage-fallback repo from the fresh p2 homepage when phase 1 had no primary repo', async () => { + happyMocks() + mockPersistInfo.mockResolvedValue({ + found: true, + changedFields: ['packages.description'], + packageId: '1', + hasPrimaryRepo: false, + }) + mockPersistMetadata.mockResolvedValue({ + found: true, + changedFields: ['versions.number'], + unresolvedDependencyTargets: 0, + homepage: 'https://github.com/monolog/monolog', + }) + + await ingestOnePackagistMetadata(qx, candidate, SCHEDULED_AT) + + expect(mockReconcileHomepageRepo).toHaveBeenCalledWith( + qx, + PURL, + '1', + 'https://github.com/monolog/monolog', + ) + }) + + it('skips homepage reconciliation when phase 1 already resolved a primary repo', async () => { + happyMocks() + + await ingestOnePackagistMetadata(qx, candidate, SCHEDULED_AT) + + expect(mockReconcileHomepageRepo).not.toHaveBeenCalled() + }) + it('records a p2 NOT_MODIFIED as success: info persisted, versions skipped', async () => { happyMocks() mockFetchP2.mockResolvedValue({ kind: 'NOT_MODIFIED' } as never) @@ -188,7 +231,12 @@ describe('ingestOnePackagistMetadata', () => { it('gives up on a persistent p2 404 after fast retries and marks it scanned(error)', async () => { vi.useFakeTimers() mockFetchStats.mockResolvedValue(statsJson as never) - mockPersistInfo.mockResolvedValue({ found: true, changedFields: [] }) + mockPersistInfo.mockResolvedValue({ + found: true, + changedFields: [], + packageId: '1', + hasPrimaryRepo: true, + }) mockFetchP2.mockResolvedValue({ kind: 'NOT_FOUND', statusCode: 404, @@ -215,7 +263,12 @@ describe('ingestOnePackagistMetadata', () => { // itself before the p2 fetch (which can throw) ever runs — see persistPackageInfo.test.ts vi.useFakeTimers() mockFetchStats.mockResolvedValue(statsJson as never) - mockPersistInfo.mockResolvedValue({ found: true, changedFields: ['packages.description'] }) + mockPersistInfo.mockResolvedValue({ + found: true, + changedFields: ['packages.description'], + packageId: '1', + hasPrimaryRepo: true, + }) mockFetchP2.mockResolvedValue({ kind: 'NOT_FOUND', statusCode: 404, @@ -237,7 +290,12 @@ describe('ingestOnePackagistMetadata', () => { it('throws on a transient p2 result without marking scanned, but phase 1 already persisted', async () => { mockFetchStats.mockResolvedValue(statsJson as never) - mockPersistInfo.mockResolvedValue({ found: true, changedFields: ['packages.description'] }) + mockPersistInfo.mockResolvedValue({ + found: true, + changedFields: ['packages.description'], + packageId: '1', + hasPrimaryRepo: true, + }) mockFetchP2.mockResolvedValue({ kind: 'TRANSIENT', message: 'HTTP 502' } as never) await expect(ingestOnePackagistMetadata(qx, candidate, SCHEDULED_AT)).rejects.toThrow() diff --git a/services/apps/packages_worker/src/packagist/__tests__/persistMetadata.test.ts b/services/apps/packages_worker/src/packagist/__tests__/persistMetadata.test.ts index 1665157b83..7c28187fb4 100644 --- a/services/apps/packages_worker/src/packagist/__tests__/persistMetadata.test.ts +++ b/services/apps/packages_worker/src/packagist/__tests__/persistMetadata.test.ts @@ -176,7 +176,12 @@ describe('persistPackagistMetadata', () => { const result = await persistPackagistMetadata(qx, PURL, expanded) - expect(result).toEqual({ found: false, changedFields: [], unresolvedDependencyTargets: 0 }) + expect(result).toEqual({ + found: false, + changedFields: [], + unresolvedDependencyTargets: 0, + homepage: 'https://monolog.example.org', + }) expect(mockVersions).not.toHaveBeenCalled() expect(mockIds).not.toHaveBeenCalled() expect(mockDeps).not.toHaveBeenCalled() diff --git a/services/apps/packages_worker/src/packagist/__tests__/persistPackageInfo.test.ts b/services/apps/packages_worker/src/packagist/__tests__/persistPackageInfo.test.ts index 61b50d8ed0..ab639dafcb 100644 --- a/services/apps/packages_worker/src/packagist/__tests__/persistPackageInfo.test.ts +++ b/services/apps/packages_worker/src/packagist/__tests__/persistPackageInfo.test.ts @@ -2,8 +2,10 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' import { getOrCreateRepoByUrl, + getPackageHomepage, logAuditFieldChanges, removeDeclaredPackageRepo, + setPackageRepositoryUrl, updatePackagistPackageStats, upsertPackageMaintainers, upsertPackageRepo, @@ -11,7 +13,7 @@ import { import type { QueryExecutor } from '@crowd/data-access-layer/src/queryExecutor' import type { NormalizedPackagistStats } from '../types' -import { persistPackagistPackageInfo } from '../upsertPackageInfo' +import { persistPackagistPackageInfo, reconcilePackagistHomepageRepo } from '../upsertPackageInfo' vi.mock('@crowd/data-access-layer/src/packages', () => ({ updatePackagistPackageStats: vi.fn(), @@ -19,6 +21,8 @@ vi.mock('@crowd/data-access-layer/src/packages', () => ({ getOrCreateRepoByUrl: vi.fn(), upsertPackageRepo: vi.fn().mockResolvedValue([]), removeDeclaredPackageRepo: vi.fn().mockResolvedValue([]), + getPackageHomepage: vi.fn().mockResolvedValue(null), + setPackageRepositoryUrl: vi.fn().mockResolvedValue([]), logAuditFieldChanges: vi.fn(), })) @@ -27,6 +31,8 @@ const mockMaintainers = vi.mocked(upsertPackageMaintainers) const mockRepoGet = vi.mocked(getOrCreateRepoByUrl) const mockRepoLink = vi.mocked(upsertPackageRepo) const mockRepoRemove = vi.mocked(removeDeclaredPackageRepo) +const mockGetHomepage = vi.mocked(getPackageHomepage) +const mockSetRepositoryUrl = vi.mocked(setPackageRepositoryUrl) const mockAudit = vi.mocked(logAuditFieldChanges) const qx = { @@ -78,7 +84,10 @@ describe('persistPackagistPackageInfo', () => { expect(mockUpdate.mock.calls[0][1]).not.toHaveProperty('downloadsLast30d') // canonicalized (lowercased) url + coarse host, linked with the manifest-declared convention expect(mockRepoGet).toHaveBeenCalledWith(qx, 'https://github.com/seldaek/monolog', 'github') - expect(mockRepoLink).toHaveBeenCalledWith(qx, '7', '55', { source: 'declared' }) + expect(mockRepoLink).toHaveBeenCalledWith(qx, '7', '55', { + source: 'declared', + signal: 'primary', + }) // any stale 'declared' link pointing at a different repo is pruned in the same pass expect(mockRepoRemove).toHaveBeenCalledWith(qx, '7', '55') expect(mockMaintainers).toHaveBeenCalledWith(qx, '7', stats.maintainers, 'packagist') @@ -102,7 +111,10 @@ describe('persistPackagistPackageInfo', () => { expect(mockMaintainers).not.toHaveBeenCalled() expect(mockRepoGet).toHaveBeenCalledWith(qx, 'https://github.com/seldaek/monolog', 'github') - expect(mockRepoLink).toHaveBeenCalledWith(qx, '8', '55', { source: 'declared' }) + expect(mockRepoLink).toHaveBeenCalledWith(qx, '8', '55', { + source: 'declared', + signal: 'primary', + }) }) it('prunes a stale declared link when the repository URL switches to a different repo', async () => { @@ -112,7 +124,10 @@ describe('persistPackagistPackageInfo', () => { const result = await persistPackagistPackageInfo(qx, PURL, stats) - expect(mockRepoLink).toHaveBeenCalledWith(qx, '7', '99', { source: 'declared' }) + expect(mockRepoLink).toHaveBeenCalledWith(qx, '7', '99', { + source: 'declared', + signal: 'primary', + }) // old link (some other repo_id) removed, new one (99) kept expect(mockRepoRemove).toHaveBeenCalledWith(qx, '7', '99') expect(result.changedFields).toContain('package_repos.repo_id') @@ -143,6 +158,42 @@ describe('persistPackagistPackageInfo', () => { expect(result.changedFields).toContain('package_repos.repo_id') }) + it('falls back to the stored homepage with a secondary signal when no repository URL is declared', async () => { + mockGetHomepage.mockResolvedValue('https://github.com/Seldaek/monolog') + mockUpdate.mockResolvedValue({ + id: '7', + isCritical: true, + changedFields: [], + }) + + await persistPackagistPackageInfo(qx, PURL, { ...stats, repositoryUrl: null }) + + expect(mockRepoGet).toHaveBeenCalledWith(qx, 'https://github.com/seldaek/monolog', 'github') + expect(mockRepoLink).toHaveBeenCalledWith(qx, '7', '55', { + source: 'declared', + signal: 'secondary', + }) + expect(mockUpdate).toHaveBeenCalledWith( + qx, + expect.objectContaining({ repositoryUrl: 'https://github.com/seldaek/monolog' }), + ) + }) + + it('rejects a homepage fallback that is not on a recognized VCS host', async () => { + mockGetHomepage.mockResolvedValue('https://monolog.example.com/docs/intro') + mockUpdate.mockResolvedValue({ + id: '7', + isCritical: true, + changedFields: [], + }) + + await persistPackagistPackageInfo(qx, PURL, { ...stats, repositoryUrl: null }) + + expect(mockRepoGet).not.toHaveBeenCalled() + expect(mockRepoLink).not.toHaveBeenCalled() + expect(mockRepoRemove).toHaveBeenCalledWith(qx, '7') + }) + it('skips the repo link and clears the stale one when the repository URL cannot be canonicalized', async () => { mockUpdate.mockResolvedValue({ id: '7', isCritical: true, changedFields: [] }) @@ -195,7 +246,12 @@ describe('persistPackagistPackageInfo', () => { const result = await persistPackagistPackageInfo(qx, PURL, stats) - expect(result).toEqual({ found: false, changedFields: [] }) + expect(result).toEqual({ + found: false, + changedFields: [], + packageId: null, + hasPrimaryRepo: true, + }) expect(mockRepoGet).not.toHaveBeenCalled() expect(mockMaintainers).not.toHaveBeenCalled() expect(mockAudit).not.toHaveBeenCalled() @@ -215,3 +271,52 @@ describe('persistPackagistPackageInfo', () => { ) }) }) + +// Phase 1 links a homepage-fallback repo from whatever homepage is already stored; +// this reconciles it once phase 2 has persisted a fresh homepage for a package that +// had none yet (a new package, or one whose homepage just changed). +describe('reconcilePackagistHomepageRepo', () => { + beforeEach(() => { + mockRepoGet.mockResolvedValue({ id: '55', changedFields: [] }) + }) + + it('links the fresh homepage with a secondary signal', async () => { + mockSetRepositoryUrl.mockResolvedValue(['packages.repository_url']) + mockRepoLink.mockResolvedValue(['package_repos.repo_id']) + + const changedFields = await reconcilePackagistHomepageRepo( + qx, + PURL, + '7', + 'https://github.com/Seldaek/monolog', + ) + + expect(mockSetRepositoryUrl).toHaveBeenCalledWith(qx, '7', 'https://github.com/seldaek/monolog') + expect(mockRepoGet).toHaveBeenCalledWith(qx, 'https://github.com/seldaek/monolog', 'github') + expect(mockRepoLink).toHaveBeenCalledWith(qx, '7', '55', { + source: 'declared', + signal: 'secondary', + }) + expect(mockRepoRemove).toHaveBeenCalledWith(qx, '7', '55') + expect(changedFields).toContain('packages.repository_url') + }) + + it('clears the link when there is no homepage to fall back to', async () => { + mockRepoRemove.mockResolvedValue(['package_repos.repo_id']) + + await reconcilePackagistHomepageRepo(qx, PURL, '7', null) + + expect(mockSetRepositoryUrl).toHaveBeenCalledWith(qx, '7', null) + expect(mockRepoGet).not.toHaveBeenCalled() + expect(mockRepoLink).not.toHaveBeenCalled() + expect(mockRepoRemove).toHaveBeenCalledWith(qx, '7') + }) + + it('clears the link when the homepage is not on a recognized VCS host', async () => { + await reconcilePackagistHomepageRepo(qx, PURL, '7', 'https://monolog.example.com/docs') + + expect(mockSetRepositoryUrl).toHaveBeenCalledWith(qx, '7', null) + expect(mockRepoGet).not.toHaveBeenCalled() + expect(mockRepoLink).not.toHaveBeenCalled() + }) +}) diff --git a/services/apps/packages_worker/src/packagist/activities.ts b/services/apps/packages_worker/src/packagist/activities.ts index 030b57beaa..a9eb288e8f 100644 --- a/services/apps/packages_worker/src/packagist/activities.ts +++ b/services/apps/packages_worker/src/packagist/activities.ts @@ -49,7 +49,7 @@ import { normalizePackagistStats, packagistNameFromPurl } from './normalize' import { INGEST_MAX_ATTEMPTS, TRANSITIVE_PREPARE_MAX_ATTEMPTS } from './retryPolicy' import { FetchError, isFetchError, isP2NotModified } from './types' import { persistPackagistMetadata } from './upsertMetadata' -import { persistPackagistPackageInfo } from './upsertPackageInfo' +import { persistPackagistPackageInfo, reconcilePackagistHomepageRepo } from './upsertPackageInfo' const log = getServiceChildLogger('packagist') @@ -175,7 +175,7 @@ export async function ingestOnePackagistMetadata( // persistPackagistPackageInfo audits its own writes atomically, inside the same // transaction — phase 1 is committed-and-audited before the p2 fetch (which can // throw) ever runs. - await persistPackagistPackageInfo(qx, candidate.purl, stats) + const phase1 = await persistPackagistPackageInfo(qx, candidate.purl, stats) // Phase 2: p2 endpoint const p2 = await fetchWithFastRetry( @@ -209,6 +209,17 @@ export async function ingestOnePackagistMetadata( 'packagist dependency targets not found in packages — edges skipped', ) } + // Phase 1's homepage-fallback repo link used whatever homepage was already stored — + // for a new package, or one whose homepage just changed, that's stale/absent until + // this p2 write lands it. Reconcile now so the link doesn't wait for the next run. + if (!phase1.hasPrimaryRepo && phase1.packageId) { + await reconcilePackagistHomepageRepo( + qx, + candidate.purl, + phase1.packageId, + persistResult.homepage, + ) + } lastModified = p2.value.lastModified } diff --git a/services/apps/packages_worker/src/packagist/upsertMetadata.ts b/services/apps/packages_worker/src/packagist/upsertMetadata.ts index bdd594c8f9..714a702bad 100644 --- a/services/apps/packages_worker/src/packagist/upsertMetadata.ts +++ b/services/apps/packages_worker/src/packagist/upsertMetadata.ts @@ -23,7 +23,12 @@ export async function persistPackagistMetadata( qx: QueryExecutor, purl: string, expanded: PackagistExpandedVersion[], -): Promise<{ found: boolean; changedFields: string[]; unresolvedDependencyTargets: number }> { +): Promise<{ + found: boolean + changedFields: string[] + unresolvedDependencyTargets: number + homepage: string | null +}> { // Registry data can contain NUL bytes (e.g. mojibake descriptions/licenses) that // Postgres text columns reject; strip them before any field is persisted. stripNullBytesDeep(expanded) @@ -114,5 +119,5 @@ export async function persistPackagistMetadata( await logAuditFieldChanges(t, WORKER, purl, changedFields) }) - return { found, changedFields, unresolvedDependencyTargets } + return { found, changedFields, unresolvedDependencyTargets, homepage } } diff --git a/services/apps/packages_worker/src/packagist/upsertPackageInfo.ts b/services/apps/packages_worker/src/packagist/upsertPackageInfo.ts index 962ec76bb8..51a6cb100b 100644 --- a/services/apps/packages_worker/src/packagist/upsertPackageInfo.ts +++ b/services/apps/packages_worker/src/packagist/upsertPackageInfo.ts @@ -1,7 +1,9 @@ import { getOrCreateRepoByUrl, + getPackageHomepage, logAuditFieldChanges, removeDeclaredPackageRepo, + setPackageRepositoryUrl, updatePackagistPackageStats, upsertPackageMaintainers, upsertPackageRepo, @@ -9,6 +11,7 @@ import { import type { QueryExecutor } from '@crowd/data-access-layer/src/queryExecutor' import { canonicalizeRepoUrl } from '../utils/canonicalizeRepoUrl' +import { resolveManifestRepo } from '../utils/resolveManifestRepo' import { stripNullBytesDeep } from '../utils/stripNullBytesDeep' import type { NormalizedPackagistStats } from './types' @@ -26,29 +29,43 @@ export async function persistPackagistPackageInfo( qx: QueryExecutor, purl: string, stats: NormalizedPackagistStats, -): Promise<{ found: boolean; changedFields: string[] }> { +): Promise<{ + found: boolean + changedFields: string[] + packageId: string | null + hasPrimaryRepo: boolean +}> { // Registry data can contain NUL bytes (e.g. mojibake descriptions) that Postgres // text columns reject; strip them before any field is persisted. stripNullBytesDeep(stats) - const canonical = stats.repositoryUrl ? canonicalizeRepoUrl(stats.repositoryUrl) : null // Packagist's repository field is free-form/author-supplied. canonicalizeRepoUrl's // 'other' bucket also matches non-repo URLs (wikis, issue trackers, registry pages) // that happen to have 2+ path segments, so only trust the verified SCM hosts here — // github.com/gitlab.com/bitbucket.org — rather than the shared utility's default, // which other callers (npm/maven/cargo) rely on staying permissive. - const trustedRepo = canonical && canonical.host !== 'other' ? canonical : null + const declared = stats.repositoryUrl ? canonicalizeRepoUrl(stats.repositoryUrl) : null + const primaryRepo = declared && declared.host !== 'other' ? declared : null let found = false + let packageId: string | null = null const changedFields: string[] = [] await qx.tx(async (t) => { + // The version manifests carry the homepage, not this endpoint — peek at the currently + // stored homepage so a package that only declares a homepage still gets a link, without + // a second write once the stats row is updated below. + const storedHomepage = await getPackageHomepage(t, purl) + const resolvedRepo = primaryRepo + ? { repo: primaryRepo, signal: 'primary' as const } + : resolveManifestRepo([{ field: 'homepage', url: storedHomepage, signal: 'secondary' }]) + // Step 1: Update packages row const result = await updatePackagistPackageStats(t, { purl, description: stats.description, declaredRepositoryUrl: stats.repositoryUrl, - repositoryUrl: trustedRepo?.url ?? null, + repositoryUrl: resolvedRepo?.repo.url ?? null, status: stats.status, totalDownloads: stats.downloadsTotal, dependentCount: stats.dependents, @@ -58,6 +75,7 @@ export async function persistPackagistPackageInfo( found = true const { id, isCritical } = result + packageId = id changedFields.push(...result.changedFields) // When there's no trusted repo (removed from the manifest, or no longer @@ -65,9 +83,12 @@ export async function persistPackagistPackageInfo( // previously-declared link that no longer applies — package_repos' unique key is // (package_id, repo_id), not (package_id, source), so upserting the new link alone // would leave a stale one dangling. - if (trustedRepo) { - const repo = await getOrCreateRepoByUrl(t, trustedRepo.url, trustedRepo.host) - const linkChanged = await upsertPackageRepo(t, id, repo.id, { source: 'declared' }) + if (resolvedRepo) { + const repo = await getOrCreateRepoByUrl(t, resolvedRepo.repo.url, resolvedRepo.repo.host) + const linkChanged = await upsertPackageRepo(t, id, repo.id, { + source: 'declared', + signal: resolvedRepo.signal, + }) const removedFields = await removeDeclaredPackageRepo(t, id, repo.id) changedFields.push(...repo.changedFields, ...linkChanged, ...removedFields) } else { @@ -92,5 +113,39 @@ export async function persistPackagistPackageInfo( await logAuditFieldChanges(t, WORKER, purl, changedFields) }) - return { found, changedFields } + return { found, changedFields, packageId, hasPrimaryRepo: !!primaryRepo } +} + +// Phase 1 (dynamic endpoint) resolves the homepage-fallback repo from whatever homepage +// is already stored, but the p2 endpoint (phase 2) is what actually carries a new/changed +// homepage — see ingestOnePackagistMetadata. Called after phase 2 persists, so a package +// with no declared repository field still gets linked to its homepage in the same run it's +// first seen, instead of waiting for the next scheduled ingestion. +export async function reconcilePackagistHomepageRepo( + qx: QueryExecutor, + purl: string, + packageId: string, + homepage: string | null, +): Promise { + const resolved = resolveManifestRepo([{ field: 'homepage', url: homepage, signal: 'secondary' }]) + const changedFields: string[] = [] + + await qx.tx(async (t) => { + changedFields.push(...(await setPackageRepositoryUrl(t, packageId, resolved?.repo.url ?? null))) + if (resolved) { + const repo = await getOrCreateRepoByUrl(t, resolved.repo.url, resolved.repo.host) + const linkChanged = await upsertPackageRepo(t, packageId, repo.id, { + source: 'declared', + signal: 'secondary', + }) + const removedFields = await removeDeclaredPackageRepo(t, packageId, repo.id) + changedFields.push(...repo.changedFields, ...linkChanged, ...removedFields) + } else { + const removedFields = await removeDeclaredPackageRepo(t, packageId) + changedFields.push(...removedFields) + } + await logAuditFieldChanges(t, WORKER, purl, changedFields) + }) + + return changedFields } diff --git a/services/apps/packages_worker/src/pypi/__tests__/normalize.test.ts b/services/apps/packages_worker/src/pypi/__tests__/normalize.test.ts index 18c0783af0..0c46e42621 100644 --- a/services/apps/packages_worker/src/pypi/__tests__/normalize.test.ts +++ b/services/apps/packages_worker/src/pypi/__tests__/normalize.test.ts @@ -246,7 +246,9 @@ describe('classifyProjectUrls', () => { null, ) expect(r.homepage).toBe('https://flask.palletsprojects.com/') - expect(r.declaredRepositoryUrl).toBe('https://github.com/pallets/flask/') + expect(r.repositoryCandidates).toEqual([ + { field: 'source', url: 'https://github.com/pallets/flask/' }, + ]) expect(r.fundingLinks).toEqual([{ type: 'other', url: 'https://palletsprojects.com/donate' }]) }) @@ -257,7 +259,48 @@ describe('classifyProjectUrls', () => { it('falls back to a repo-looking homepage when no explicit repo key', () => { const r = classifyProjectUrls({ Homepage: 'https://github.com/psf/requests' }, null) - expect(r.declaredRepositoryUrl).toBe('https://github.com/psf/requests') + expect(r.repositoryCandidates).toEqual([ + { field: 'homepage', url: 'https://github.com/psf/requests' }, + ]) + }) + + it('falls back to bug tracker URL when no explicit repo or homepage repo', () => { + const r = classifyProjectUrls({ 'Bug Tracker': 'https://github.com/foo/bar/issues' }, null) + expect(r.repositoryCandidates).toEqual([ + { field: 'bug_tracker', url: 'https://github.com/foo/bar/issues' }, + ]) + }) + + it('does not classify GitHub Issues URL as source via the git heuristic', () => { + const r = classifyProjectUrls({ 'GitHub Issues': 'https://github.com/foo/bar/issues' }, null) + expect(r.repositoryCandidates).toEqual([ + { field: 'bug_tracker', url: 'https://github.com/foo/bar/issues' }, + ]) + }) + + it('does not use bug tracker when an explicit repo key is present', () => { + const r = classifyProjectUrls( + { + Source: 'https://github.com/foo/bar', + 'Bug Tracker': 'https://github.com/foo/bar/issues', + }, + null, + ) + expect(r.repositoryCandidates).toEqual([{ field: 'source', url: 'https://github.com/foo/bar' }]) + }) + + it('keeps the source candidate and a repo-looking homepage as a fallback', () => { + const r = classifyProjectUrls( + { + Source: 'https://github.com/foo/bar', + Homepage: 'https://gitlab.com/foo/bar', + }, + null, + ) + expect(r.repositoryCandidates).toEqual([ + { field: 'source', url: 'https://github.com/foo/bar' }, + { field: 'homepage', url: 'https://gitlab.com/foo/bar' }, + ]) }) it('infers funding type from the host', () => { @@ -276,7 +319,11 @@ describe('classifyProjectUrls', () => { it('returns nulls/empties when there are no urls', () => { const r = classifyProjectUrls(null, null) - expect(r).toEqual({ homepage: null, declaredRepositoryUrl: null, fundingLinks: [] }) + expect(r).toEqual({ + homepage: null, + repositoryCandidates: [], + fundingLinks: [], + }) }) }) diff --git a/services/apps/packages_worker/src/pypi/normalize.ts b/services/apps/packages_worker/src/pypi/normalize.ts index 2ae91e29ba..76fe869bda 100644 --- a/services/apps/packages_worker/src/pypi/normalize.ts +++ b/services/apps/packages_worker/src/pypi/normalize.ts @@ -184,6 +184,8 @@ export function collectPypiMaintainers(info: PyPiInfo): PypiPerson[] { return [...map.values()] } +export type PypiRepositoryField = 'source' | 'homepage' | 'bug_tracker' + export interface PypiFundingLink { type: string url: string @@ -198,12 +200,17 @@ function inferFundingType(url: string): string { return 'other' } +export interface PypiRepoCandidate { + field: PypiRepositoryField + url: string +} + export function classifyProjectUrls( projectUrls: Record | null | undefined, homePage: string | null | undefined, ): { homepage: string | null - declaredRepositoryUrl: string | null + repositoryCandidates: PypiRepoCandidate[] fundingLinks: PypiFundingLink[] } { const entries = Object.entries(projectUrls ?? {}).map( @@ -218,18 +225,30 @@ export function classifyProjectUrls( findByKey(/^home[\s-]*page$/i) ?? findByKey(/^home$/i) + // Candidates are ordered by trust, most trusted first — a project can declare a Source + // field AND a Homepage/Bug Tracker that also happen to point at a repo host. Keeping all + // of them (rather than picking one before validation) lets the caller fall through to the + // next candidate when the top pick fails canonicalization (malformed URL, unsupported path). const REPO_HOST = /github\.com|gitlab\.com|bitbucket\.org/i - let declaredRepositoryUrl = + const sourceUrl = findByKey(/^source(\s*code)?$/i) ?? findByKey(/^repository$/i) ?? findByKey(/^repo$/i) ?? findByKey(/^code$/i) ?? - entries.find(([k, v]) => /source|repo|code|git/i.test(k) && REPO_HOST.test(v))?.[1] ?? + entries.find( + ([k, v]) => + /source|repo|code|git/i.test(k) && REPO_HOST.test(v) && !/bug|issue|tracker/i.test(k), + )?.[1] ?? null - // Many projects only declare a Homepage that is itself the repo. - if (!declaredRepositoryUrl && homepage && REPO_HOST.test(homepage)) { - declaredRepositoryUrl = homepage + const trackerUrl = + entries.find(([k, v]) => /bug|issue|tracker/i.test(k) && REPO_HOST.test(v))?.[1] ?? null + + const repositoryCandidates: PypiRepoCandidate[] = [] + if (sourceUrl) repositoryCandidates.push({ field: 'source', url: sourceUrl }) + if (homepage && REPO_HOST.test(homepage)) { + repositoryCandidates.push({ field: 'homepage', url: homepage }) } + if (trackerUrl && !sourceUrl) repositoryCandidates.push({ field: 'bug_tracker', url: trackerUrl }) const seen = new Set() const fundingLinks: PypiFundingLink[] = [] @@ -239,7 +258,7 @@ export function classifyProjectUrls( fundingLinks.push({ type: inferFundingType(v), url: v }) } - return { homepage, declaredRepositoryUrl, fundingLinks } + return { homepage, repositoryCandidates, fundingLinks } } export function parseKeywords(raw: string | null | undefined): string[] { diff --git a/services/apps/packages_worker/src/pypi/upsertProject.ts b/services/apps/packages_worker/src/pypi/upsertProject.ts index c62cbde6d6..04006655ea 100644 --- a/services/apps/packages_worker/src/pypi/upsertProject.ts +++ b/services/apps/packages_worker/src/pypi/upsertProject.ts @@ -1,14 +1,16 @@ import { getOrCreateRepoByUrl, + removeDeclaredPackageRepo, upsertNpmFundingLinks, upsertPackageMaintainers, upsertPackageRepo, upsertPypiPackage, upsertPypiVersions, } from '@crowd/data-access-layer/src/packages' +import type { PackageRepoSignal } from '@crowd/data-access-layer/src/packages/repoConfidence' import type { QueryExecutor } from '@crowd/data-access-layer/src/queryExecutor' -import { canonicalizeRepoUrl } from '../utils/canonicalizeRepoUrl' +import { resolveManifestRepo } from '../utils/resolveManifestRepo' import { stripNullBytesDeep } from '../utils/stripNullBytesDeep' import { @@ -37,11 +39,20 @@ export async function upsertProject( `https://pypi.org/project/${pypiName}/` const description = info.summary?.trim() ? info.summary.trim() : null - const { homepage, declaredRepositoryUrl, fundingLinks } = classifyProjectUrls( + const { homepage, repositoryCandidates, fundingLinks } = classifyProjectUrls( info.project_urls, info.home_page, ) - const repo = declaredRepositoryUrl ? canonicalizeRepoUrl(declaredRepositoryUrl) : null + const declaredRepositoryUrl = repositoryCandidates[0]?.url ?? null + const resolvedRepo = resolveManifestRepo( + repositoryCandidates.map((candidate) => ({ + field: candidate.field, + url: candidate.url, + signal: candidate.field === 'source' ? 'primary' : 'secondary', + })), + ) + const repo = resolvedRepo?.repo ?? null + const repoSignal: PackageRepoSignal = resolvedRepo?.signal ?? 'primary' const { licenses, licensesRaw } = resolvePypiLicenses(info) const keywords = parseKeywords(info.keywords) const maintainers = collectPypiMaintainers(info) @@ -86,8 +97,17 @@ export async function upsertProject( repo.host, ) repoChanged.forEach((f) => changed.add(f)) - const linkChanged = await upsertPackageRepo(t, pkgId, repoId, { source: 'declared' }) + const linkChanged = await upsertPackageRepo(t, pkgId, repoId, { + source: 'declared', + signal: repoSignal, + }) linkChanged.forEach((f) => changed.add(f)) + + const removedFields = await removeDeclaredPackageRepo(t, pkgId, repoId) + removedFields.forEach((f) => changed.add(f)) + } else { + const removedFields = await removeDeclaredPackageRepo(t, pkgId) + removedFields.forEach((f) => changed.add(f)) } if (versionRows.length > 0) { diff --git a/services/apps/packages_worker/src/rubygems/normalize.ts b/services/apps/packages_worker/src/rubygems/normalize.ts index b002bc8995..41ac62b3b3 100644 --- a/services/apps/packages_worker/src/rubygems/normalize.ts +++ b/services/apps/packages_worker/src/rubygems/normalize.ts @@ -1,4 +1,4 @@ -import { canonicalizeRepoUrl } from '../utils/canonicalizeRepoUrl' +import { resolveManifestRepo } from '../utils/resolveManifestRepo' import { NormalizedRubyGemsOwner, @@ -27,7 +27,11 @@ export function normalizeRubyGemsPackage(doc: RubyGemsGemResponse): NormalizedRu description: nonEmpty(doc.info), homepage: nonEmpty(doc.homepage_uri), declaredRepositoryUrl, - repo: declaredRepositoryUrl ? canonicalizeRepoUrl(declaredRepositoryUrl) : null, + resolvedRepo: resolveManifestRepo([ + { field: 'source_code_uri', url: declaredRepositoryUrl }, + { field: 'homepage_uri', url: doc.homepage_uri }, + { field: 'bug_tracker_uri', url: doc.bug_tracker_uri }, + ]), licenses, licensesRaw: licenses ? licenses.join(', ') : null, latestVersion: nonEmpty(doc.version), diff --git a/services/apps/packages_worker/src/rubygems/runRubyGemsCoreLoop.ts b/services/apps/packages_worker/src/rubygems/runRubyGemsCoreLoop.ts index 914fd039b4..2fe0258c20 100644 --- a/services/apps/packages_worker/src/rubygems/runRubyGemsCoreLoop.ts +++ b/services/apps/packages_worker/src/rubygems/runRubyGemsCoreLoop.ts @@ -5,6 +5,8 @@ import { listRubyGemsPackagesToSync, logAuditFieldChange, recordDownloadSnapshot, + removeDeclaredPackageRepo, + setPackageRepositoryUrl, upsertPackage, upsertPackageRepo, } from '@crowd/data-access-layer' @@ -109,7 +111,7 @@ async function processPackage( description: normalized.description, homepage: normalized.homepage, declaredRepositoryUrl: normalized.declaredRepositoryUrl, - repositoryUrl: normalized.repo?.url ?? null, + repositoryUrl: normalized.resolvedRepo?.repo.url ?? null, licenses: normalized.licenses, licensesRaw: normalized.licensesRaw, latestVersion: normalized.latestVersion, @@ -118,18 +120,27 @@ async function processPackage( }) pkgChanged.forEach((f) => changed.add(f)) - if (normalized.repo) { + if (normalized.resolvedRepo) { const { id: repoId, changedFields: repoChanged } = await getOrCreateRepoByUrl( t, - normalized.repo.url, - normalized.repo.host, + normalized.resolvedRepo.repo.url, + normalized.resolvedRepo.repo.host, ) repoChanged.forEach((f) => changed.add(f)) const linkChanged = await upsertPackageRepo(t, packageDbId.toString(), repoId, { source: 'declared', + signal: normalized.resolvedRepo.signal, }) linkChanged.forEach((f) => changed.add(f)) + + const removedFields = await removeDeclaredPackageRepo(t, packageDbId.toString(), repoId) + removedFields.forEach((f) => changed.add(f)) + } else { + const removedFields = await removeDeclaredPackageRepo(t, packageDbId.toString()) + removedFields.forEach((f) => changed.add(f)) + const clearedFields = await setPackageRepositoryUrl(t, packageDbId.toString(), null) + clearedFields.forEach((f) => changed.add(f)) } if (normalized.totalDownloads > 0) { diff --git a/services/apps/packages_worker/src/rubygems/types.ts b/services/apps/packages_worker/src/rubygems/types.ts index 1c46632591..1bfeffc549 100644 --- a/services/apps/packages_worker/src/rubygems/types.ts +++ b/services/apps/packages_worker/src/rubygems/types.ts @@ -1,4 +1,4 @@ -import { CanonicalRepo } from '../utils/canonicalizeRepoUrl' +import { ResolvedManifestRepo } from '../utils/resolveManifestRepo' export interface BatchResult { processed: number @@ -25,6 +25,7 @@ export interface RubyGemsGemResponse { info?: string | null homepage_uri?: string | null source_code_uri?: string | null + bug_tracker_uri?: string | null licenses?: string[] | null downloads?: number } @@ -46,7 +47,7 @@ export interface NormalizedRubyGemsPackage { description: string | null homepage: string | null declaredRepositoryUrl: string | null - repo: CanonicalRepo | null + resolvedRepo: ResolvedManifestRepo | null licenses: string[] | null licensesRaw: string | null latestVersion: string | null diff --git a/services/apps/packages_worker/src/scripts/rescorePackageRepos.ts b/services/apps/packages_worker/src/scripts/rescorePackageRepos.ts index 793f4fcbb8..9cf8d4aaae 100644 --- a/services/apps/packages_worker/src/scripts/rescorePackageRepos.ts +++ b/services/apps/packages_worker/src/scripts/rescorePackageRepos.ts @@ -2,7 +2,7 @@ /** * Recompute package_repos.confidence with package_repo_confidence() - * (see V1788307200). Used for the initial backfill; the recurring sweep runs as the + * (see V1788307300). Used for the initial backfill; the recurring sweep runs as the * package-repo-confidence-sweep-daily Temporal schedule. * * Usage: diff --git a/services/apps/packages_worker/src/utils/__tests__/resolveManifestRepo.test.ts b/services/apps/packages_worker/src/utils/__tests__/resolveManifestRepo.test.ts new file mode 100644 index 0000000000..7b9afd1620 --- /dev/null +++ b/services/apps/packages_worker/src/utils/__tests__/resolveManifestRepo.test.ts @@ -0,0 +1,67 @@ +import { describe, expect, it } from 'vitest' + +import { resolveManifestRepo } from '../resolveManifestRepo' + +describe('resolveManifestRepo', () => { + it('resolves the first candidate as primary', () => { + expect( + resolveManifestRepo([ + { field: 'repository', url: 'git+https://github.com/babel/babel.git' }, + { field: 'homepage', url: 'https://github.com/other/repo' }, + ]), + ).toEqual({ + repo: { url: 'https://github.com/babel/babel', host: 'github' }, + signal: 'primary', + }) + }) + + it('falls through to the next field when the primary one is missing', () => { + expect( + resolveManifestRepo([ + { field: 'repository', url: null }, + { field: 'homepage', url: 'https://github.com/foo/bar' }, + ]), + ).toEqual({ + repo: { url: 'https://github.com/foo/bar', host: 'github' }, + signal: 'secondary', + }) + }) + + it('falls through when the primary field cannot be canonicalized', () => { + const resolved = resolveManifestRepo([ + { field: 'repository', url: 'not a url' }, + { field: 'bugs.url', url: 'https://gitlab.com/group/sub/project/-/issues' }, + ]) + expect(resolved?.repo.url).toBe('https://gitlab.com/group/sub/project') + expect(resolved?.signal).toBe('secondary') + }) + + it('rejects a secondary candidate that is not on a recognized VCS host', () => { + expect( + resolveManifestRepo([ + { field: 'repository', url: null }, + { field: 'homepage', url: 'https://example.com/docs/getting-started' }, + ]), + ).toBeNull() + }) + + it('keeps a primary candidate on an unrecognized host', () => { + const resolved = resolveManifestRepo([ + { field: 'repository', url: 'https://git.sr.ht/~sircmpwn/aerc' }, + ]) + expect(resolved?.repo.host).toBe('other') + expect(resolved?.signal).toBe('primary') + }) + + it('honours an explicit signal override', () => { + expect( + resolveManifestRepo([ + { field: 'homepage', url: 'https://github.com/foo/bar', signal: 'secondary' }, + ])?.signal, + ).toBe('secondary') + }) + + it('returns null when no candidate resolves', () => { + expect(resolveManifestRepo([{ field: 'repository', url: ' ' }])).toBeNull() + }) +}) diff --git a/services/apps/packages_worker/src/utils/resolveManifestRepo.ts b/services/apps/packages_worker/src/utils/resolveManifestRepo.ts new file mode 100644 index 0000000000..2bc82f7365 --- /dev/null +++ b/services/apps/packages_worker/src/utils/resolveManifestRepo.ts @@ -0,0 +1,34 @@ +import type { PackageRepoSignal } from '@crowd/data-access-layer/src/packages/repoConfidence' + +import { CanonicalRepo, canonicalizeRepoUrl } from './canonicalizeRepoUrl' + +export interface ManifestRepoCandidate { + field: string + url: string | null | undefined + // Defaults to `primary` for the first candidate and `secondary` for the rest; set it + // when the primary field was already resolved elsewhere. + signal?: PackageRepoSignal +} + +export interface ResolvedManifestRepo { + repo: CanonicalRepo + signal: PackageRepoSignal +} + +export function resolveManifestRepo( + candidates: ManifestRepoCandidate[], +): ResolvedManifestRepo | null { + for (const [index, candidate] of candidates.entries()) { + const raw = candidate.url?.trim() + if (!raw) continue + + const repo = canonicalizeRepoUrl(raw) + if (!repo) continue + + const signal: PackageRepoSignal = candidate.signal ?? (index === 0 ? 'primary' : 'secondary') + if (signal === 'secondary' && repo.host === 'other') continue + + return { repo, signal } + } + return null +} diff --git a/services/libs/data-access-layer/src/packages/packages.ts b/services/libs/data-access-layer/src/packages/packages.ts index ea987f9b51..0e3d257150 100644 --- a/services/libs/data-access-layer/src/packages/packages.ts +++ b/services/libs/data-access-layer/src/packages/packages.ts @@ -270,7 +270,11 @@ export interface PackagistStatsUpdateInput { export async function updatePackagistPackageStats( qx: QueryExecutor, input: PackagistStatsUpdateInput, -): Promise<{ id: string; isCritical: boolean; changedFields: string[] } | null> { +): Promise<{ + id: string + isCritical: boolean + changedFields: string[] +} | null> { const row: { id: string; is_critical: boolean; changed_fields: string[] } | undefined = await qx.selectOneOrNone( `WITH old AS ( @@ -288,8 +292,8 @@ export async function updatePackagistPackageStats( dependent_count = COALESCE($(dependentCount), dependent_count), last_synced_at = NOW() WHERE purl = $(purl) AND ecosystem = 'packagist' - RETURNING id, is_critical, description, declared_repository_url, repository_url, status, - total_downloads, dependent_count, ingestion_source + RETURNING id, is_critical, description, declared_repository_url, repository_url, + status, total_downloads, dependent_count, ingestion_source ) SELECT ins.id::text AS id, ins.is_critical, array_remove(ARRAY[ @@ -306,7 +310,31 @@ export async function updatePackagistPackageStats( ) if (!row) return null - return { id: row.id, isCritical: row.is_critical, changedFields: row.changed_fields } + return { + id: row.id, + isCritical: row.is_critical, + changedFields: row.changed_fields, + } +} + +export async function setPackageRepositoryUrl( + qx: QueryExecutor, + packageId: string, + url: string | null, +): Promise { + const affected = await qx.result( + `UPDATE packages SET repository_url = $(url) + WHERE id = $(packageId)::bigint AND repository_url IS DISTINCT FROM $(url)`, + { url, packageId }, + ) + return affected > 0 ? ['packages.repository_url'] : [] +} + +export async function getPackageHomepage(qx: QueryExecutor, purl: string): Promise { + const row = await qx.selectOneOrNone(`SELECT homepage FROM packages WHERE purl = $(purl)`, { + purl, + }) + return row?.homepage ?? null } export interface PackagistVersionAggregates { diff --git a/services/libs/data-access-layer/src/packages/repoConfidence.test.ts b/services/libs/data-access-layer/src/packages/repoConfidence.test.ts index 2ac185a784..95fea9e0d7 100644 --- a/services/libs/data-access-layer/src/packages/repoConfidence.test.ts +++ b/services/libs/data-access-layer/src/packages/repoConfidence.test.ts @@ -24,9 +24,10 @@ describe('packageRepoConfidenceLabel', () => { }) describe('packageRepoLinkClaimParams', () => { - it('defaults provenance for sources that carry none', () => { + it('defaults provenance and signal for claims that carry neither', () => { expect(packageRepoLinkClaimParams({ source: 'declared' })).toEqual({ source: 'declared', + signal: 'primary', provenance: null, }) }) @@ -35,10 +36,12 @@ describe('packageRepoLinkClaimParams', () => { expect( packageRepoLinkClaimParams({ source: 'deps_dev', + signal: 'secondary', provenance: 'SLSA_ATTESTATION', }), ).toEqual({ source: 'deps_dev', + signal: 'secondary', provenance: 'SLSA_ATTESTATION', }) }) @@ -48,6 +51,7 @@ describe('packageRepoConfidenceCall', () => { it('binds a new claim to parameters by default', () => { const sql = packageRepoConfidenceCall('p', 'r') expect(sql).toContain('$(source)') + expect(sql).toContain('$(signal)') expect(sql).toContain('$(provenance)') expect(sql).toContain('p.ecosystem') expect(sql).toContain('r.archived') @@ -56,9 +60,11 @@ describe('packageRepoConfidenceCall', () => { it('reads a rescored claim off the stored row', () => { const sql = packageRepoConfidenceCall('p', 'r', { source: 'pr.source', + signal: 'pr.signal', provenance: 'pr.provenance', }) expect(sql).not.toContain('$(') + expect(sql).toContain('pr.signal') expect(sql).toContain('pr.provenance') }) }) diff --git a/services/libs/data-access-layer/src/packages/repoConfidence.ts b/services/libs/data-access-layer/src/packages/repoConfidence.ts index 3280c986ad..4151c483a8 100644 --- a/services/libs/data-access-layer/src/packages/repoConfidence.ts +++ b/services/libs/data-access-layer/src/packages/repoConfidence.ts @@ -1,4 +1,5 @@ export type PackageRepoSource = 'declared' | 'deps_dev' | 'heuristic' | 'manual' +export type PackageRepoSignal = 'primary' | 'secondary' export type PackageRepoConfidenceLabel = 'high' | 'medium' | 'low' export const CONFIDENCE_HIGH_THRESHOLD = 0.8 @@ -12,15 +13,18 @@ export function packageRepoConfidenceLabel(confidence: number): PackageRepoConfi export type PackageRepoLinkClaim = { source: PackageRepoSource + signal?: PackageRepoSignal provenance?: string | null } export function packageRepoLinkClaimParams(claim: PackageRepoLinkClaim): { source: PackageRepoSource + signal: PackageRepoSignal provenance: string | null } { return { source: claim.source, + signal: claim.signal ?? 'primary', provenance: claim.provenance ?? null, } } @@ -40,6 +44,7 @@ export function competingGithubRepoExpr(packageIdExpr: string, repoIdExpr: strin export type PackageRepoClaimExprs = { source: string + signal: string provenance: string } @@ -47,12 +52,14 @@ export type PackageRepoClaimExprs = { // back off the stored row instead. export const CLAIM_FROM_PARAMS: PackageRepoClaimExprs = { source: '$(source)', + signal: '$(signal)', provenance: '$(provenance)', } export function claimFromRow(alias: string): PackageRepoClaimExprs { return { source: `${alias}.source`, + signal: `${alias}.signal`, provenance: `${alias}.provenance`, } } @@ -61,6 +68,9 @@ export function claimFromRow(alias: string): PackageRepoClaimExprs { // own row and always replaces, downgrades included (ADR-0020). export const KEEP_HIGHEST_CONFLICT_UPDATE = `source = CASE WHEN EXCLUDED.confidence > package_repos.confidence THEN EXCLUDED.source ELSE package_repos.source END, + signal = CASE WHEN EXCLUDED.source = package_repos.source + OR EXCLUDED.confidence > package_repos.confidence + THEN EXCLUDED.signal ELSE package_repos.signal END, provenance = CASE WHEN EXCLUDED.source = package_repos.source OR EXCLUDED.confidence > package_repos.confidence THEN EXCLUDED.provenance ELSE package_repos.provenance END, @@ -69,7 +79,7 @@ export const KEEP_HIGHEST_CONFLICT_UPDATE = `source = CASE WHEN EXCLUD ELSE GREATEST(EXCLUDED.confidence, package_repos.confidence) END, verified_at = NOW()` -// The only path that may produce a package_repos confidence value (V1788307200). The caller +// The only path that may produce a package_repos confidence value (V1788307300). The caller // must have the package and repo rows joined — ecosystem and repo state are read off them. export function packageRepoConfidenceCall( packageAlias: string, @@ -80,7 +90,7 @@ export function packageRepoConfidenceCall( const competing = competingGithubExpr ?? competingGithubRepoExpr(`${packageAlias}.id`, `${repoAlias}.id`) return `package_repo_confidence( - ${claim.source}, ${packageAlias}.ecosystem, ${claim.provenance}, + ${claim.source}, ${packageAlias}.ecosystem, ${claim.signal}, ${claim.provenance}, ${repoAlias}.archived, ${repoAlias}.is_fork, ${repoAlias}.disabled, ${repoAlias}.host, ${competing}, ${repoAlias}.id diff --git a/services/libs/data-access-layer/src/packages/repoConfidenceScoring.integration.test.ts b/services/libs/data-access-layer/src/packages/repoConfidenceScoring.integration.test.ts index cec62be5e0..ef146d7c6d 100644 --- a/services/libs/data-access-layer/src/packages/repoConfidenceScoring.integration.test.ts +++ b/services/libs/data-access-layer/src/packages/repoConfidenceScoring.integration.test.ts @@ -5,7 +5,7 @@ import { getDbConnection } from '@crowd/database' import type { QueryExecutor } from '../queryExecutor' import { pgpQx } from '../queryExecutor' -// Integration test: hits the running packages-db, where V1788307200 defines +// Integration test: hits the running packages-db, where V1788307300 defines // package_repo_confidence. Skipped when the DB env vars are missing so unit-test runs // in CI stay green. const HAVE_DB = @@ -18,6 +18,7 @@ const HAVE_DB = type ScoreInput = { source: string ecosystem?: string + signal?: string provenance?: string | null archived?: boolean | null isFork?: boolean | null @@ -44,11 +45,12 @@ describe.skipIf(!HAVE_DB)('package_repo_confidence', () => { async function score(input: ScoreInput): Promise { const row = await qx.selectOne( `SELECT package_repo_confidence( - $(source), $(ecosystem), $(provenance), + $(source), $(ecosystem), $(signal), $(provenance), $(archived), $(isFork), $(disabled), $(host), $(competingGithub), $(repoId) )::float8 AS score`, { ecosystem: 'npm', + signal: 'primary', provenance: null, archived: null, isFork: null, @@ -75,6 +77,17 @@ describe.skipIf(!HAVE_DB)('package_repo_confidence', () => { expect(await score({ source: 'heuristic' })).toBeCloseTo(0.3, 2) }) + it('penalises a secondary signal on declared links only', async () => { + expect(await score({ source: 'declared', signal: 'secondary' })).toBeCloseTo(0.75, 2) + expect( + await score({ source: 'declared', ecosystem: 'maven', signal: 'secondary' }), + ).toBeCloseTo(0.7, 2) + expect(await score({ source: 'manual', signal: 'secondary' })).toBeCloseTo(0.99, 2) + expect( + await score({ source: 'deps_dev', provenance: 'GO_ORIGIN', signal: 'secondary' }), + ).toBeCloseTo(0.9, 2) + }) + it('stacks repo-state penalties and floors at 0.05', async () => { expect(await score({ source: 'declared', archived: true })).toBeCloseTo(0.65, 2) expect(await score({ source: 'declared', isFork: true })).toBeCloseTo(0.75, 2) diff --git a/services/libs/data-access-layer/src/packages/repoConfidenceWrites.integration.test.ts b/services/libs/data-access-layer/src/packages/repoConfidenceWrites.integration.test.ts index 2f6770071e..1472d3a8b9 100644 --- a/services/libs/data-access-layer/src/packages/repoConfidenceWrites.integration.test.ts +++ b/services/libs/data-access-layer/src/packages/repoConfidenceWrites.integration.test.ts @@ -8,7 +8,7 @@ import { pgpQx } from '../queryExecutor' import { upsertPackageRepo } from './repos' -// Integration test: hits the running packages-db, where V1788307200 defines +// Integration test: hits the running packages-db, where V1788307300 defines // package_repo_confidence and rescore_package_repo_confidence. Skipped when the DB env // vars are missing so unit-test runs in CI stay green. const HAVE_DB = @@ -152,7 +152,7 @@ describe.skipIf(!HAVE_DB)('package_repos write and rescore policy', () => { const link = await storedLink(githubRepoId) const expected: { confidence: number } = await qx.selectOne( `SELECT package_repo_confidence( - pr.source, p.ecosystem, pr.provenance, + pr.source, p.ecosystem, pr.signal, pr.provenance, r.archived, r.is_fork, r.disabled, r.host, false, pr.repo_id )::float8 AS confidence FROM package_repos pr diff --git a/services/libs/data-access-layer/src/packages/repos.ts b/services/libs/data-access-layer/src/packages/repos.ts index 771db09bac..6d692b3f45 100644 --- a/services/libs/data-access-layer/src/packages/repos.ts +++ b/services/libs/data-access-layer/src/packages/repos.ts @@ -100,9 +100,9 @@ export async function upsertPackageRepo( ), ins AS ( INSERT INTO package_repos ( - package_id, repo_id, source, provenance, confidence, created_at + package_id, repo_id, source, signal, provenance, confidence, created_at ) - SELECT $(packageId)::bigint, $(repoId)::bigint, $(source), $(provenance), + SELECT $(packageId)::bigint, $(repoId)::bigint, $(source), $(signal), $(provenance), scored.confidence, NOW() FROM scored ON CONFLICT (package_id, repo_id) DO UPDATE SET