From ec64fb6db8d30d06d4d8630c14604b803eb4ba18 Mon Sep 17 00:00:00 2001 From: pat-s Date: Tue, 14 Jul 2026 17:11:02 +0000 Subject: [PATCH] fix(local): route multi-signature packages to human triage in the proposer A package whose failing builds match more than one auto-proposable signature (e.g. some logs hit `tbb-stddef-removed` and others `rcppparallel-bundled-tbb`) produced two candidate registry entries with the same package/platforms/versions key, so the candidate set failed `validate-patches.R` with "ambiguous duplicate entries" and the whole proposer run aborted. Add a pure `dedupe_candidates()` that keeps a package only when it maps to a single signature and routes genuinely ambiguous packages (conflicting fix tiers) to human triage instead of guessing between them. The proposer prints the skipped packages and proceeds with the unambiguous candidates. --- local/proposal-tracking-lib.R | 31 +++++++++++++++++++++++ local/propose-patches.R | 18 ++++++++++++- local/tests/test-proposal-tracking-lib.R | 32 ++++++++++++++++++++++++ 3 files changed, 80 insertions(+), 1 deletion(-) diff --git a/local/proposal-tracking-lib.R b/local/proposal-tracking-lib.R index 4e8a468..2edd9c8 100644 --- a/local/proposal-tracking-lib.R +++ b/local/proposal-tracking-lib.R @@ -6,6 +6,37 @@ # Kept free of DB/HTTP/clock so it can be sourced by the proposer, the tracker # entrypoint, and the unit tests. Timestamps are passed in by callers. +# Split proposal candidates into the ones safe to emit and the ambiguous ones. +# A candidate is a list with at least `package` and `signature`. A package that +# maps to more than one distinct auto-proposable signature is genuinely +# ambiguous (two conflicting fix tiers, e.g. makevars vs source patch): emitting +# both would create colliding registry entries, so those are routed to human +# triage instead of guessed at. Returns list(keep = ..., ambiguous = ...), where +# `ambiguous` is a named list of package -> the distinct signatures seen. +dedupe_candidates <- function(candidates) { + if (length(candidates) == 0L) { + return(list(keep = list(), ambiguous = list())) + } + pkgs <- vapply(candidates, function(c) as.character(c$package), character(1L)) + by_pkg <- split(candidates, pkgs) + keep <- list() + ambiguous <- list() + for (pkg in names(by_pkg)) { + cs <- by_pkg[[pkg]] + sigs <- unique(vapply( + cs, + function(c) as.character(c$signature), + character(1L) + )) + if (length(sigs) == 1L) { + keep[[length(keep) + 1L]] <- cs[[1L]] # one signature -> take the first + } else { + ambiguous[[pkg]] <- sigs + } + } + list(keep = keep, ambiguous = ambiguous) +} + # Stable identity of a ledger record: one proposal per (package, signature). ledger_key <- function(record) { paste0( diff --git a/local/propose-patches.R b/local/propose-patches.R index 20583ba..d43668a 100644 --- a/local/propose-patches.R +++ b/local/propose-patches.R @@ -135,9 +135,25 @@ for (r in report) { } } +# A package that maps to more than one auto-proposable signature is ambiguous +# (conflicting fix tiers) and would collide on the same registry key; route it +# to human triage instead of emitting both. +split_candidates <- dedupe_candidates(candidates) +candidates <- split_candidates$keep +if (length(split_candidates$ambiguous) > 0L) { + cat("\nAmbiguous (multiple signatures) -> human triage, not proposed:\n") + for (pkg in names(split_candidates$ambiguous)) { + cat(sprintf( + " %s: %s\n", + pkg, + toString(split_candidates$ambiguous[[pkg]]) + )) + } +} + if (length(candidates) == 0L) { cat( - "No auto-proposable candidates (nothing classified, safe, and unregistered).\n" + "\nNo auto-proposable candidates (nothing classified, safe, unregistered, and unambiguous).\n" ) q(status = 0) } diff --git a/local/tests/test-proposal-tracking-lib.R b/local/tests/test-proposal-tracking-lib.R index 55da4b6..a2fbe05 100644 --- a/local/tests/test-proposal-tracking-lib.R +++ b/local/tests/test-proposal-tracking-lib.R @@ -46,6 +46,38 @@ test_that("merge_ledger handles an empty/NULL starting ledger", { expect_length(merge_ledger(list(), new), 1L) }) +test_that("dedupe_candidates keeps single-signature pkgs, routes conflicts to triage", { + candidates <- list( + list(package = "StanHeaders", signature = "tbb-stddef-removed"), + list(package = "hmmTMB", signature = "tbb-stddef-removed"), + list(package = "hmmTMB", signature = "rcppparallel-bundled-tbb") # conflict + ) + out <- dedupe_candidates(candidates) + kept <- vapply(out$keep, function(c) c$package, character(1L)) + expect_identical(sort(kept), "StanHeaders") # hmmTMB dropped as ambiguous + expect_true("hmmTMB" %in% names(out$ambiguous)) + expect_setequal( + out$ambiguous$hmmTMB, + c("tbb-stddef-removed", "rcppparallel-bundled-tbb") + ) +}) + +test_that("dedupe_candidates collapses a package repeated under one signature", { + candidates <- list( + list(package = "rstan", signature = "tbb-stddef-removed"), + list(package = "rstan", signature = "tbb-stddef-removed") + ) + out <- dedupe_candidates(candidates) + expect_length(out$keep, 1L) + expect_length(out$ambiguous, 0L) +}) + +test_that("dedupe_candidates handles the empty list", { + out <- dedupe_candidates(list()) + expect_length(out$keep, 0L) + expect_length(out$ambiguous, 0L) +}) + test_that("signature_hit_rate splits addressed vs open per signature", { report <- build_triage_report(mk_failures(), registered_pkgs = "RcppParallel") hit <- signature_hit_rate(report, registered_pkgs = "RcppParallel") -- 2.54.0