From f1dd661213acdefdd7ee52cadb20bd0ddad9524b Mon Sep 17 00:00:00 2001 From: pat-s Date: Tue, 14 Jul 2026 17:11:54 +0000 Subject: [PATCH] fix(local): route multi-signature packages to human triage in the proposer (#119) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem A live `propose-patches.R --open-issue` run aborted with: ``` Patch registry validation FAILED: - ambiguous duplicate entries: hmmTMB|ubuntu-2604|*, imt|ubuntu-2604|*, refundBayes|ubuntu-2604|* Error: Candidate registry failed validation; not writing or proposing. ``` Root cause: a package whose failing builds match **more than one** auto-proposable signature (e.g. some logs hit `tbb-stddef-removed`, others `rcppparallel-bundled-tbb`) landed in two separate signature groups, so the proposer emitted two candidate entries with the same `package|platforms|versions` key. The validator's ambiguous-duplicate check then failed the whole candidate set, aborting the run — so even the unambiguous candidates (e.g. `vacalibration`) never got proposed. ## Fix Add a pure, tested `dedupe_candidates()` that: - keeps a package's candidate only when it maps to a **single** signature (collapsing an accidental repeat under the same signature), and - routes a package that maps to **multiple** signatures (conflicting fix tiers -- makevars vs source patch) to **human triage** instead of guessing between them. The proposer prints the skipped ambiguous packages and proceeds with the clean candidates, so one ambiguous package no longer blocks the rest. This matches the issue #115 guardrail: don't auto-pick when the fix is ambiguous. ## Verification - New unit tests in `test-proposal-tracking-lib.R` cover the single-signature, multi-signature (conflict), duplicate-under-one-signature, and empty cases. - Reproduced the original failure end-to-end with a stubbed DB (`hmmTMB` ambiguous + `vacalibration` clean): `hmmTMB` is now listed under "Ambiguous -> human triage", `vacalibration` is proposed, and the candidate registry validates (`Patch registry OK`). - Full suite: 78 tests pass; all pre-commit hooks pass. Reviewed-on: https://git.devxy.io/devxy/build-cran-binaries/pulls/119 --- 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")