Graduate BioC blocklist helper into R/; expand write_qualified_pkg_lists() by default (#183) by aclark02-arcus · Pull Request #184 · pharmaR/val.pipeline

aclark02-arcus · 2026-09-16T15:07:43Z

Closes #183.

Scope 3 (per Aaron's ask): graduate the dev/ helper into R/, wire it into write_qualified_pkg_lists() as the new default, remove the dev/ script. Breaking change → 0.2.0.

What changed

New exported API

build_bioc_blocklist(qual_metadata, opt_repos = NULL, config_path = NULL, qualified_decision = "Low", min_universe = 100L) — returns the blocklist as a data frame with package, version, matched_repo, reason columns. Use this directly for auditing/reporting.
is_bioc_repo(repos) — case-insensitive substring match on "bioc" against both alias names and URLs. Covers:
- The OSS BioC: config alias.
- Non-OSS forks that consolidate the four BioC sub-repos into a single entry (both alias and URL contain "bioc").
- URL-only aliases (e.g. an internal mirror named sci whose URL is a Bioconductor path).

Breaking change to existing exported function

write_qualified_pkg_lists() gains three new args:

use_full_universe = TRUE (default). For any BioC-detected source in blocklist_sources, the blocklist is expanded against the full available.packages() universe via build_bioc_blocklist(). Pre-0.2.0 behaviour (assessed-only inverse) is available via use_full_universe = FALSE.
opt_repos = NULL, config_path = NULL — passthrough to build_bioc_blocklist().

Failure handling: if available.packages() throws (offline runner, config error, etc.), the function falls back to the assessed-only blocklist for that source with a warning — it does NOT abort. Surrounding qualified-CRAN.txt writes continue.

Housekeeping

dev/dev_build_bioc_blocklist.R deleted (redundant).
.gitignore drops the CSV output pattern (no longer written to dev/).

Test coverage

New file tests/testthat/test-build_bioc_blocklist.R — 10 tests, all mocking utils::available.packages() via testthat::local_mocked_bindings(.package = "utils") (no new dep):

Test	Covers
`is_bioc_repo() matches alias, URL, both, and neither`	Full dispatch matrix incl. non-OSS consolidated case
`build_bioc_blocklist() expands universe and tags reasons`	Happy path, reason column
`... URL-only aliases in qm mask`	Copilot review fix #2 from prior round
`... riskmetric-derived labels (BioCsoft)`	Substring-fallback leg of the mask
`... errors on no BioC repos`	`is_bioc_repo()` returns all-FALSE
`... enforces min_universe floor`	Guardrail against silent empty universe
`... errors on empty available.packages()`	Hard zero-row case
`... warns when blocklist is empty`	Every-pkg-allowlisted edge
`... supports 'package' key col too`	qual_assessments-shaped frames

tests/testthat/test-write_qualified_pkg_lists.R — added 2 tests:

Expansion path end-to-end: fake universe with 120 filler pkgs verifies the "not_assessed" leak is actually closed in output.
Network-failure fallback: available.packages mocked to stop(); expect warning, expect CRAN allowlist still written.

5 existing tests updated to use_full_universe = FALSE where they were exercising the pre-0.2.0 assessed-only inverse behaviour explicitly (documents that intent).

Full suite: 1105 pass, 0 fail, 1 skip (pre-existing).

Why 0.2.0 (breaking bump)

write_qualified_pkg_lists() is an exported function whose default behaviour changed. Any downstream caller running on an offline runner that gets blocklist-BioC.txt written will now see either:

A much larger blocklist (correct behaviour), or
A warning + assessed-only fallback (correct behaviour under failure).

Neither breaks correctness, but the file contents differ from prior versions on the happy path. Per the branch/version policy, that's a minor bump, not a patch.

Follow-ups (deferred)

PR Log val.pipeline version + install path at the top of every val_build() run (#179) #180 (v0.1.60) and PR Gate pkgType = 'source' on ref, own it in val_build() only (fixes #181) #182 (v0.1.60) — whichever merges first, the loser needs a rebase-bump. This PR's 0.2.0 already dominates both.
Downstream provisioning docs (probably out-of-tree) should mention the new use_full_universe arg and the fallback semantics.

…verse The existing blocklist-BioC.txt from write_qualified_pkg_lists() is the inverse of the *assessed* BioC set only. BioC packages dropped at remote_reduce (or otherwise never seen) are absent from both the allowlist and the blocklist, so PPM would silently serve them. dev/dev_build_bioc_blocklist.R pulls the full BioC universe via available.packages() against the config's BioC repo(s) and emits blocklist = universe − Low-risk allowlist as a CSV with a 'reason' column (not_assessed vs assessed_<risk>). Repo detection uses a case-insensitive substring match on 'bioc' against both the repo alias and the URL, so non-OSS forks that consolidate the four BioC sub-repos into a single entry are covered (both alias and URL contain 'bioc'). Smoke test against R_4.5.2/20260730/qual_metadata.rds: universe 2300 assessed by pipeline 150 Low-risk allowlist 13 full blocklist 2287 (137 assessed_High/Medium + 2150 not_assessed) Fixes #183 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

🟡 Changes recommended

Historical config snapshots and URL-only BioC repository aliases are handled incorrectly, potentially producing an inaccurate blocklist.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds a development helper to generate a complete BioC provisioning blocklist from the full repository universe.

Changes:

Detects BioC repositories and queries available.packages().
Generates a reasoned CSV blocklist excluding Low-risk packages.
Bumps the package version and documents the change.

File summaries

File	Description
`dev/dev_build_bioc_blocklist.R`	Adds the blocklist-generation helper.
`NEWS.md`	Documents the BioC blocklist fix.
`DESCRIPTION`	Bumps the version to 0.1.60.

Review details

Files reviewed: 3/3 changed files
Comments generated: 2
Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Two correctness fixes from the PR #184 review: 1. When val_dir is provided and the caller didn't pin config_path, default to <val_dir>/config.yml if it exists. Historical runs have a snapshotted config next to their qual_metadata.rds; without this, the BioC universe would be resolved against the session/installed config and compared with decisions from a different BioC release (config drift). 2. The qm-side BioC mask previously used only grepl('bioc', qm$repo_name, ignore.case = TRUE). That misses the URL-only-alias case (e.g. alias 'sci' -> bioconductor URL) — qm$repo_name stores the alias 'sci' with no 'bioc' substring, so those Low-risk pkgs would be excluded from the allowlist and wrongly blocklisted. Fix is a union: match against names(bioc_repos) OR the case-insensitive substring fallback. The fallback is still needed because riskmetric sometimes emits labels like 'BioCsoft' that don't literally equal the config alias 'BioC'. Smoke tests validate both mask paths end-to-end and confirm the 2,287-pkg blocklist result is unchanged for the 20260730 prod run (both mask paths agree there). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

aclark02-arcus · 2026-09-16T15:22:43Z

Adopted both Copilot suggestions in f05ad6f.

1. Snapshotted config (line 132) — legit. When val_dir is provided and the caller didn't pin config_path, default to <val_dir>/config.yml if it exists. Without this a historical run's decisions would be compared against a newer BioC universe.

2. URL-only-alias mask (line 181) — legit and subtle. Fixed as a union of two checks: qm$repo_name %in% names(bioc_repos) (catches URL-only aliases like sci → bioc URL) OR the case-insensitive substring fallback (still needed for riskmetric-derived labels like BioCsoft that don't literally equal the config alias BioC — confirmed by the smoke test's output on the 20260730 run).

Verification:

Both mask paths tested end-to-end with fake qm frames (URL-only alias + BioCsoft label). All invariants hold.
Smoke test on the real 20260730 run still produces the same 2,287-pkg blocklist (both paths agree there).
Using snapshotted config: .../config.yml log line confirms the val_dir path picks 3.22 correctly.

Housekeeping in the same push: extended .gitignore to cover dev/low_covr.rds and the diff-run/blocklist output patterns, so git add -A no longer sweeps them in.

…sts by default Fixes #183 (scope: full integration, breaking). Motivation ---------- write_qualified_pkg_lists() previously wrote blocklist-BioC.txt as the inverse of the *assessed* BioC set. BioC pkgs dropped at remote_reduce (or otherwise never seen by the pipeline) were absent from both the allowlist and the blocklist, so PPM would silently serve them. The 2026-07-30 prod run showed 2,150 BioC packages leaking this way. Changes ------- 1. **NEW exported** `build_bioc_blocklist(qual_metadata, opt_repos = NULL, config_path = NULL, qualified_decision = 'Low', min_universe = 100L)` returns the blocklist as a data frame with columns `package`, `version`, `matched_repo`, `reason` (`not_assessed` vs `assessed_<risk>`), sorted by reason then package. 2. **NEW exported** `is_bioc_repo(repos)` — case-insensitive substring match on 'bioc' against BOTH alias names and URLs. Covers non-OSS forks that consolidate the four BioC sub-repos into a single entry, plus URL-only aliases (e.g. mirror named 'sci' whose URL is a Bioconductor path). 3. **BREAKING**: `write_qualified_pkg_lists()` gains `use_full_universe = TRUE` (default). For any BioC-detected source in blocklist_sources, the blocklist is expanded against the full available.packages() universe via build_bioc_blocklist(). Set FALSE to restore pre-0.2.0 behaviour. Also gains `opt_repos` and `config_path` passthrough args for that call. Network failure at runtime is non-fatal: falls back to assessed-only inverse with a warning so surrounding qualified-<other>.txt writes still succeed. Test coverage ------------- - 10 tests in test-build_bioc_blocklist.R: is_bioc_repo dispatch (alias/URL/both/neither/unnamed); expansion + reason tagging; URL-only alias mask; riskmetric-derived BioCsoft label; no-BioC-repo error; min_universe floor; empty available.packages error; empty-blocklist warning; 'package' key col support. - 2 new tests in test-write_qualified_pkg_lists.R: expansion path end-to-end (120 filler pkgs verify the 'not_assessed' leak is actually closed); network-failure fallback. - 5 existing tests updated to `use_full_universe = FALSE` where they were exercising the pre-0.2.0 assessed-only inverse behaviour. Mocking uses testthat 3's local_mocked_bindings() with `.package = 'utils'` — no new test-time dep. Full suite: 1105 pass, 0 fail, 1 skip (pre-existing). Housekeeping ------------ - dev/dev_build_bioc_blocklist.R removed (redundant). - .gitignore drops the CSV output pattern (no longer written to dev/). - DESCRIPTION: 0.1.60 -> 0.2.0 (breaking on exported function). - NEWS.md: rewritten for 0.2.0. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

aclark02-arcus · 2026-09-16T15:48:49Z

Big rework in 7b4c3c4: graduated the helper into R/, wired into write_qualified_pkg_lists() as the default, removed the dev/ script. Version bumped to 0.2.0 (breaking on default behaviour of an exported function).

New API:

build_bioc_blocklist(qm, opt_repos, config_path, qualified_decision, min_universe) — data-frame return with reason column
is_bioc_repo(repos) — case-insensitive alias-OR-URL match

Wire-up: write_qualified_pkg_lists(use_full_universe = TRUE) calls build_bioc_blocklist() for BioC-detected sources in blocklist_sources. Network failure -> assessed-only fallback with warning (does not abort surrounding writes).

Test coverage: 12 new tests (10 in test-build_bioc_blocklist.R, 2 in test-write_qualified_pkg_lists.R) using testthat::local_mocked_bindings(.package = 'utils'). 5 existing tests updated with use_full_universe = FALSE where they were pinning the pre-0.2.0 behaviour explicitly.

Full suite: 1105 pass, 0 fail, 1 skip (pre-existing riskmetric-not-installed check).

PR title/body updated.

Copilot

🟡 Changes recommended

URL-only BioC sources and empty expansions can produce incorrect blocklists, and custom configuration defaults are not consistently honored.

Get a fresh assessment by requesting another Copilot review.

Review details

Files not reviewed (3)

man/build_bioc_blocklist.Rd: Generated file
man/is_bioc_repo.Rd: Generated file
man/write_qualified_pkg_lists.Rd: Generated file

Files reviewed: 7/11 changed files
Comments generated: 3
Review effort level: Balanced

+    qualified_decision = pull_config(val = "decisions_lst",
+                                     rule_type = "default")[1],


+      # Network failures are non-fatal here: fall back to the
+      # assessed-only inverse with a warning so the surrounding
+      # allowlist writes still succeed.
+      if (isTRUE(use_full_universe) && any(is_bioc_repo(stats::setNames("", src)))) {


+        if (!is.null(expanded) && nrow(expanded) > 0L) {
+          pkgs <- sort(unique(expanded$package))
+        }


…y-alias wire-up, non-null expansion result Three legit correctness fixes from the PR #184 review round 2: 1. build_bioc_blocklist() qualified_decision default previously evaluated pull_config() in the signature, which reads the session/ installed config regardless of the caller's config_path. Passing a historical config with a different first decision was silently ignored. Fix: resolve in the body when NULL, forwarding config_path. 2. write_qualified_pkg_lists() BioC gating used is_bioc_repo(setNames('', src)) — classifying by ALIAS only. That missed the URL-only-alias case: blocklist_sources = 'sci' with opt_repos = list(sci = '.../bioconductor/...') would silently fall back to the pre-0.2.0 assessed-only blocklist. Fix: resolve the effective repo map once outside the loop, then classify each src by BOTH its alias and the URL mapped from that map. 3. write_qualified_pkg_lists() previously kept the assessed-only 'pkgs' when the expansion returned zero rows, but 0 rows is a legitimate result from build_bioc_blocklist() (every universe pkg is on the allowlist). A previously-assessed_High pkg absent from the current universe would stay wrongly blocklisted. Fix: only NULL (== error caught) preserves the fallback; empty df replaces. Test coverage ------------- Two new tests in test-write_qualified_pkg_lists.R: - URL-only-alias BioC repo (alias 'sci' -> bioc URL): 120 filler pkgs on blocklist proves expansion actually triggered. - Empty expansion result: universe contains only Low-risk pkgs + fillers all allowlisted, expansion returns 0 rows; verify the written blocklist is empty (NOT the assessed-only inverse that would contain GhostHighPkg). Full targeted suite: 94/94 pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

aclark02-arcus · 2026-09-16T19:01:42Z

Adopted all three Copilot suggestions in 2644482. All legit:

1. build_bioc_blocklist() qualified_decision default (R/build_bioc_blocklist.R:72)
Signature-default pull_config(...) reads the session/installed config regardless of the passed config_path. Fix: resolve in the body when NULL, forwarding config_path. Historical/custom configs with a different first decision now work correctly.

2. URL-only-alias wire-up (R/utils.R:3113)
The gate was is_bioc_repo(setNames('', src)) — classifying by alias only. blocklist_sources = 'sci' with opt_repos = list(sci = '.../bioconductor/...') would silently fall back to pre-0.2.0 behaviour. Fix: resolve the effective repo map once outside the loop, classify each src by BOTH its alias AND the URL mapped from that map.

3. Empty expansion result (R/utils.R:3133)
Previously if (!is.null(expanded) && nrow(expanded) > 0L) kept the assessed-only pkgs when expansion returned 0 rows. But 0 rows is a legitimate result — every universe pkg is on the allowlist. A previously-assessed_High pkg absent from the current universe would stay wrongly blocklisted. Fix: only NULL (error caught) preserves fallback; empty df replaces.

New tests:

URL-only-alias BioC repo end-to-end (120 filler pkgs on blocklist proves expansion actually triggered, not the pre-0.2.0 fallback).
Empty expansion result (GhostHighPkg absent from current universe — assessed-only would have included it, expansion correctly emits empty blocklist).

Full targeted suite: 94/94 pass (23 build_bioc_blocklist + 71 write_qualified_pkg_lists).

aclark02-arcus requested a balanced review from Copilot September 16, 2026 15:09

Copilot started reviewing on behalf of aclark02-arcus September 16, 2026 15:10 View session

Copilot AI reviewed Sep 16, 2026

View reviewed changes

Comment thread dev/dev_build_bioc_blocklist.R Outdated

Comment thread dev/dev_build_bioc_blocklist.R Outdated

aclark02-arcus force-pushed the ac-183-bioc-blocklist-universe branch from 78a2eb8 to f05ad6f Compare September 16, 2026 15:22

aclark02-arcus changed the title ~~Add dev/ helper: BioC blocklist against full available.packages() universe (#183)~~ Graduate BioC blocklist helper into R/; expand write_qualified_pkg_lists() by default (#183) Sep 16, 2026

aclark02-arcus requested a balanced review from Copilot September 16, 2026 18:37

Copilot started reviewing on behalf of aclark02-arcus September 16, 2026 18:38 View session

Copilot AI reviewed Sep 16, 2026

View reviewed changes

aclark02-arcus merged commit 9db3cb0 into main Sep 16, 2026
1 of 2 checks passed

Navigation Menu

Graduate BioC blocklist helper into R/; expand write_qualified_pkg_lists() by default (#183) - #184

Graduate BioC blocklist helper into R/; expand write_qualified_pkg_lists() by default (#183)#184
aclark02-arcus merged 4 commits into
mainfrom
ac-183-bioc-blocklist-universe

aclark02-arcus commented Sep 16, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

aclark02-arcus commented Sep 16, 2026

Uh oh!

aclark02-arcus commented Sep 16, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

aclark02-arcus commented Sep 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		qualified_decision = pull_config(val = "decisions_lst",
		rule_type = "default")[1],

Conversation

aclark02-arcus commented Sep 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changed

New exported API

Breaking change to existing exported function

Housekeeping

Test coverage

Why 0.2.0 (breaking bump)

Follow-ups (deferred)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

aclark02-arcus commented Sep 16, 2026

Uh oh!

aclark02-arcus commented Sep 16, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Files not reviewed (3)

Uh oh!

aclark02-arcus commented Sep 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aclark02-arcus commented Sep 16, 2026 •

edited

Loading