Gate pkgType = 'source' on ref, own it in val_build() only (fixes #181)

.gitignore

@@ -13,3 +13,9 @@ dev/nfs_smb_latency_probe.txt
 # run under devtools::test(). Ignore any *-tests/ dir so a stray test
 # run doesn't accidentally commit hundreds of files.
 *-tests/
+
+# Sensitive per-run artifacts (do not commit)
+dev/low_covr.rds
+dev/low_covr_rerun_compare_*.rds
+dev/covr_diff_*.rds
+dev/covr_diff_*.html

DESCRIPTION

@@ -1,6 +1,6 @@
 Package: val.pipeline
 Title: Validation Pipeline Example
-Version: 0.1.59
+Version: 0.1.60
 Authors@R: 
   c(
     person(

NEWS.md

@@ -1,3 +1,34 @@
+# val.pipeline 0.1.60
+
+- **Stop `val_pipeline()` from silently regressing covr_coverage on
+  some source-tier packages.** Two related fixes for #181:
+  1. The outer seam in `val_pipeline()` pushed
+     `options(pkgType = "source")` into the parent session before
+     dispatching to `val_build()`, unconditionally and outside the
+     `ref` gate. That set is now removed; `pkgType` is owned in
+     exactly one place -- `val_build()`, via the new
+     `apply_val_build_options()` helper, gated on `ref == "source"`.
+  2. For source-tier assessment, `val_build()` now sets
+     `pkgType = "both"` (NOT `"source"`). Entering `val_build()`
+     under `getOption('pkgType') == 'source'` was reproducibly
+     regressing logrx from >90% to ~62% covr_coverage. Likely
+     mechanism: `configure_riskmetric_offline_if_requested()`
+     (which `val_build()` calls near entry) uses
+     `utils::available.packages()`, whose default `type` filter
+     narrows the offline cache to source-only under
+     `pkgType == "source"` -- any dependency present only as a
+     binary in a PPM-style mirror drops out and downstream
+     test-time installs fail silently. `"both"` keeps the cache
+     complete (binary-preferred, source fallback) while
+     `ref = "source"` still drives val.pipeline's own
+     source-tarball assessment path in `val_pkg()`.
+
+  Also fixes a stale `on.exit(function() options(old))` in
+  `val_build()` that constructed a function value and threw it away
+  (never restored the option snapshot) AND silently wiped the
+  earlier `on.exit(options(old_cfg), add = TRUE)` because it lacked
+  `add = TRUE`. (#181)
+
 # val.pipeline 0.1.59
 
 - **Fix `<U+2014>` fallback in the summary report subtitle and a few

R/utils.R

@@ -185,6 +185,64 @@ apply_config_path <- function(config_path) {
 }
 
 
+#' Set `repos` and (conditionally) `pkgType` for a val_build() scope
+#'
+#' Sets `options(repos = opt_repos)` and, when `ref == "source"`, also
+#' `options(pkgType = "both")`. Returns the previous values in the
+#' form returned by [base::options()] so the caller can restore them
+#' via `on.exit(options(old), add = TRUE)`.
+#'
+#' # Why `pkgType = "both"` for source-tier assessment (not `"source"`)
+#'
+#' Intuition would say "source-tier assessment => `pkgType = 'source'`",
+#' but that combination is exactly what regressed `covr_coverage` on
+#' logrx from >90% to ~62% (see #181). Reproduced empirically:
+#' entering `val_build(ref = "source", ...)` under
+#' `getOption('pkgType') == 'source'` produces the ~62% number;
+#' entering under `"both"` or `"binary"` produces >90%. The likely
+#' mechanism: `configure_riskmetric_offline_if_requested()` (which
+#' runs near the top of val_build's body) calls
+#' `utils::available.packages()`, whose default `type = getOption('pkgType')`
+#' filters the resolved cache. Under `"source"` the cache is
+#' source-only -- packages available only as binaries in a PPM-style
+#' mirror drop out of the offline cache, and downstream install
+#' probes for a source-tier package's test-time dependencies fail
+#' silently (test suite short-circuits, covr reports the resulting
+#' partial coverage). `"both"` returns the fullest cache while still
+#' allowing source installs where source is what's actually needed.
+#'
+#' `ref` still governs val.pipeline's OWN source-vs-remote assessment
+#' path (see `val_pkg()`'s tarball fetch) -- `pkgType` here only
+#' controls what `install.packages()` / `available.packages()` do
+#' for the dependency install probe, which we want as permissive
+#' as possible.
+#'
+#' Extracted from `val_build()` so the option-management contract is
+#' testable in isolation.
+#'
+#' @param ref One of `"source"` or `"remote"`. `pkgType = "both"` is
+#'   set only when `ref == "source"`; `"remote"` leaves the caller's
+#'   `pkgType` untouched.
+#' @param opt_repos Named character vector of repository URLs, as
+#'   accepted by `options(repos = ...)`.
+#'
+#' @return An `options()`-shaped list holding the previous values of
+#'   the slots we mutated (always includes `repos`; includes `pkgType`
+#'   only when `ref == "source"`). Suitable for
+#'   `on.exit(options(old), add = TRUE)`.
+#'
+#' @keywords internal
+apply_val_build_options <- function(ref, opt_repos) {
+  ref <- match.arg(ref, c("source", "remote"))
+  if (identical(ref, "source")) {
+    old <- options(repos = opt_repos, pkgType = "both")
+  } else {
+    old <- options(repos = opt_repos)
+  }
+  old
+}
+
+
 
 #' 
 #' Helper function to determine which repo a package came from

R/val_build.R

@@ -232,16 +232,31 @@ val_build <- function(
   decisions <- pull_config(val = "decisions_lst", rule_type = "default")
   remote_pkgs <- pull_config(val = "remote_only", rule_type = "default")
   # opt_repos <- pull_config(val = "opt_repos", rule_type = "default") |> unlist()
-
-  old <- options()
-  on.exit(function() options(old))
-  if(ref == 'source') {
-    options(repos = opt_repos, pkgType = "source") # , rlang_interactive = FALSE
-  } else {
-    options(repos = opt_repos) # , rlang_interactive = FALSE
-  }
-  # options("repos")
-
+
+  # For `ref == "source"` runs, force `options(pkgType = "both")` --
+  # NOT `"source"`. See `apply_val_build_options()` in `R/utils.R`
+  # for the full mechanism; short version: entering val_build under
+  # `getOption('pkgType') == 'source'` regressed covr_coverage on
+  # some source-tier packages (>90% on logrx became ~62%),
+  # reproduced empirically by Aaron. The likely offender is
+  # `configure_riskmetric_offline_if_requested()` above -- it calls
+  # `available.packages()`, whose default `type` filter narrows the
+  # offline cache to source-only when `pkgType == "source"`, so any
+  # dep only present as a binary in a PPM-style mirror drops out
+  # and downstream test-time installs fail silently.
+  #
+  # `"both"` keeps the cache complete (binary-preferred, source
+  # fallback) while `ref = "source"` still governs val.pipeline's
+  # own source-tarball assessment path in `val_pkg()`. See #181.
+  #
+  # The `on.exit(options(old), add = TRUE)` guard restores whichever
+  # slots we mutated -- fixes the previous
+  # `on.exit(function() options(old))` which constructed a function
+  # value and threw it away (never restored options) and lacked
+  # `add = TRUE` (silently wiped the earlier config-path on.exit).
+  old <- apply_val_build_options(ref = ref, opt_repos = opt_repos)
+  on.exit(options(old), add = TRUE)
+
   #
   # ---- Which pkgs, ordered ----
   #

R/val_pipeline.R

@@ -281,9 +281,19 @@ val_pipeline <- function(
     pull_config(val = "decisions_lst", rule_type = "default")
 
   # Keep options aligned with the prep run when picking up from disk.
+  # Note: we intentionally do NOT set `pkgType = "source"` here. That
+  # was previously set unconditionally at this seam, which meant every
+  # `val_pipeline() -> val_build()` handoff entered val_build with
+  # `pkgType = "source"` already live in the parent session -- even
+  # for `ref = "remote"` runs -- and pushed the state divergence that
+  # regressed covr_coverage on some source-tier packages (see #181).
+  # `val_build()` sets `pkgType = "source"` itself, gated on
+  # `ref == "source"`, at the top of its own body. That single owner
+  # keeps the option contract consistent whether callers use
+  # `val_pipeline()` or `val_build()` directly.
   old <- options()
   on.exit(options(old), add = TRUE)
-  options(repos = prep$opt_repos, pkgType = "source", scipen = 999)
+  options(repos = prep$opt_repos, scipen = 999)
 
   #
   # ---- val_build() ----