From 1f05eefcaa6306236cbeeed2c844e007e705dba8 Mon Sep 17 00:00:00 2001 From: "Jonathan D.A. Jewell" <6759885+hyperpolymath@users.noreply.github.com> Date: Tue, 22 Sep 2026 08:44:23 +0000 Subject: [PATCH] fix(test): probe R once so the guard answers what the consumer asks The live-R guard asked a different question from the record it stood in front of: it probed an empty package list, its consumer probed the four in R_PACKAGES, so on any checkout where R answers but the packages are absent -- every bare checkout -- the guard reported ready and the consumer threw outside any try, erroring the testset before its first @test (#43). The @info branch could not fire for the case it was written for. provenance.jl now answers the question once. RProbeStatus carries a status (:ok, :packages_missing, :r_unreachable, :no_lockfile) beside the record, and r_probe_status performs the single probe whose record the suite then asserts on, so guard and consumer cannot diverge. The failure's component names travel as a ProbeFailure field rather than as prose, so classification is a field read, and probe_r collects every absent package instead of stopping at the first: four missing packages report four. The skip is loud and named: the testset is titled with the status and the reason -- "r - not probed (packages_missing): ... dada2, Biostrings, ShortRead, vegan" -- and recorded with @test_skip, so it lands in the summary's own Broken column rather than in an @info that scrolls past. On CI, where ci.yml installs R and restores all 79 packages from renv.lock, the same block is a hard assertion: a skip there is a provisioning regression, not a bare checkout. The negative control is pure and runs everywhere the live probe skips: :r_unreachable, :packages_missing and :no_lockfile are asserted to be distinguishable in both symbol and text. Verified against the real module on a box with R 4.5.0 and none of the four packages: the :packages_missing path is exercised live, names all four, and records Broken rather than passing or erroring. Closes #43 --- src/core/provenance.jl | 102 ++++++++++++++++++++++++++++++- test/unit/test_provenance.jl | 115 ++++++++++++++++++++++++++++------- 2 files changed, 192 insertions(+), 25 deletions(-) diff --git a/src/core/provenance.jl b/src/core/provenance.jl index 6e8add60..84601ae5 100644 --- a/src/core/provenance.jl +++ b/src/core/provenance.jl @@ -33,6 +33,7 @@ using ..RRuntime: with_r_lock export ProbeFailure, DatabaseReleaseMismatch, ToolProbe, TOOL_PROBES, ToolRecord, JuliaRecord, RRecord, + RProbeStatus, r_probe_ok, r_probe_status, classify_r_probe_failure, DatabaseFormatSpec, DatabaseFormatRecord, DatabaseRecord, CapturedEnvironment, Attestation, probe_tool, probe_julia, probe_r, probe_database, probe_host, probe_metamanifold, @@ -50,8 +51,18 @@ const SCHEMA_VERSION = 1 struct ProbeFailure <: Exception component :: String reason :: String + # Names carried separately from `reason` so a caller can act on WHICH packages + # were absent instead of matching text. Empty for every probe that does not ask + # about packages at all, and for failures that are not about packages: a + # component can be unreachable for reasons that have nothing to do with what is + # installed. `missing_packages` is what makes "R is not installed" and "R is + # installed, dada2 is not" two different answers (#43) rather than one message. + missing_packages :: Vector{String} end +ProbeFailure(component::AbstractString, reason::AbstractString) = + ProbeFailure(String(component), String(reason), String[]) + Base.showerror(io::IO, e::ProbeFailure) = print(io, "provenance: could not prove '$(e.component)': $(e.reason)") @@ -331,22 +342,107 @@ function probe_r(; packages::AbstractVector{<:AbstractString} = R_PACKAGES, with_r_lock(; timeout) do r_version = RCall.rcopy(String, RCall.reval("R.version.string")) found = OrderedDict{String,String}() + missing = String[] for pkg in packages raw = RCall.rcopy(RCall.reval(r_version_query(pkg))) - raw isa AbstractString || throw(ProbeFailure("r", - "package '$pkg' is not installed in the R library in use")) - found[String(pkg)] = String(raw) + # `NA_character_` rather than an exception is how r_version_query + # reports a package DESCRIPTION cannot be read. Collect and carry on: + # a caller told that one package is absent fixes one package, and a + # checkout missing four reports four (#43). + if raw isa AbstractString + found[String(pkg)] = String(raw) + else + push!(missing, String(pkg)) + end end + isempty(missing) || throw(ProbeFailure("r", + "R is reachable but these required packages are not installed: " * + join(missing, ", "), missing)) (r_version, found) end catch err err isa ProbeFailure && rethrow() + # R itself is the component that could not be proved here, so nothing is + # claimed about packages: the interpreter never answered the question. throw(ProbeFailure("r", sprint(showerror, err))) end return RRecord(version, file_sha256(renv_lock), versions) end +""" + RProbeStatus + +What an R probe found, as a status rather than as a value or an exception. + +The question a provenance guard has to answer is not "is R installed" but "may I +read an R record". Those differ whenever the interpreter answers and the packages +do not, which is every bare checkout: the guard used to probe with an empty +package list, its consumer probed `R_PACKAGES`, and so the guard reported ready +exactly where the consumer could only throw (#43). One probe, one answer, one +place to read it from. + +`status` is `:ok` only when `record` may be used. The other three are distinct +because they need different fixes: `:no_lockfile` is a broken checkout, +`:r_unreachable` is a missing toolchain, `:packages_missing` is a half-restored +renv library. `missing` names the packages in the third case and is empty +otherwise. +""" +struct RProbeStatus + status :: Symbol + record :: Union{RRecord,Nothing} + missing :: Vector{String} + reason :: String +end + +""" + r_probe_ok(s::RProbeStatus) -> Bool + +Whether `s` holds an R record that may be read. False means no R record exists, so +neither the version nor any package version may be presented as a fact. +""" +r_probe_ok(s::RProbeStatus) = s.status === :ok + +""" + classify_r_probe_failure(err::ProbeFailure) -> RProbeStatus + +The status a failed R probe deserves. Pure: no R, no filesystem, no I/O. + +Kept separate from `r_probe_status` so that "R is unreachable" and "R is reachable +but the packages are absent" can be told apart by a test on a machine that has no +R at all (#43, where the whole point is that these carry different messages to +whoever is reading a skip). +""" +classify_r_probe_failure(err::ProbeFailure) = + isempty(err.missing_packages) ? + RProbeStatus(:r_unreachable, nothing, String[], err.reason) : + RProbeStatus(:packages_missing, nothing, copy(err.missing_packages), err.reason) + +""" + r_probe_status(; packages=R_PACKAGES, renv_lock, timeout=60) -> RProbeStatus + +Probe R once and report the outcome as a status. + +`packages` defaults to `R_PACKAGES` and callers should leave it alone: a readiness +check that narrows the list asks a different question from the record it is +standing in front of, which is the defect #43 records. +""" +function r_probe_status(; packages::AbstractVector{<:AbstractString} = R_PACKAGES, + renv_lock::AbstractString = joinpath(repo_root(), "renv.lock"), + timeout::Union{Real,Nothing} = 60) + # Mirrors probe_r's own guard rather than string-matching its message: a missing + # lockfile is this checkout's problem, and reporting it as an unreachable R would + # send whoever reads it to install a toolchain they already have. + isfile(renv_lock) || + return RProbeStatus(:no_lockfile, nothing, String[], "no renv.lock at $renv_lock") + try + return RProbeStatus(:ok, probe_r(; packages, renv_lock, timeout), String[], "") + catch err + err isa ProbeFailure || rethrow() + return classify_r_probe_failure(err) + end +end + ## Reference databases struct DatabaseFormatSpec format :: String diff --git a/test/unit/test_provenance.jl b/test/unit/test_provenance.jl index 42e66205..80a88a34 100644 --- a/test/unit/test_provenance.jl +++ b/test/unit/test_provenance.jl @@ -457,31 +457,102 @@ canned(out::String, err::String = "", code::Int = 0) = end end - r_ready = try - PV.probe_r(; packages = String[], timeout = 30) - true - catch err - @info "Provenance: skipping live R probe" reason=sprint(showerror, err) - false - end - - if r_ready - record = PV.probe_r(; timeout = 120) - @test occursin("R version", record.version) - - ## The bug this exists to prevent. renv.lock is the pin and DESCRIPTION is - # what is loaded; they agree, and both keep R's major.minor-patch - # convention that packageVersion() would flatten. vegan is the witness: - # 2.7-3, never 2.7.3. - lock = read(joinpath(@__DIR__, "..", "..", "renv.lock"), String) - for (pkg, version) in record.packages - @test occursin("\"$pkg\"", lock) - @test occursin("\"Version\": \"$version\"", lock) + ## The R probe (#43). ONE probe answers both questions -- is R reachable, and are + # the packages the pipeline requires installed -- and its record is reused + # rather than asked for twice. Asking twice was the defect: the guard probed an + # empty package list, its consumer probed the four in R_PACKAGES, so on a + # checkout with R and no packages the guard said "ready" and the consumer threw + # outside any try, erroring the testset before its first @test. + r_status = PV.r_probe_status(; timeout = 120) + + if PV.r_probe_ok(r_status) + @testset "r" begin + record = r_status.record + + # The invariant the split guard could not state: the record the suite + # asserts on covers every package the pipeline requires. A narrowed + # probe cannot produce a record that satisfies this. + @test collect(keys(record.packages)) == PV.R_PACKAGES + + @test occursin("R version", record.version) + + ## The bug this exists to prevent. renv.lock is the pin and DESCRIPTION + # is what is loaded; they agree, and both keep R's major.minor-patch + # convention that packageVersion() would flatten. vegan is the witness: + # 2.7-3, never 2.7.3. + lock = read(joinpath(@__DIR__, "..", "..", "renv.lock"), String) + for (pkg, version) in record.packages + @test occursin("\"$pkg\"", lock) + @test occursin("\"Version\": \"$version\"", lock) + end + if haskey(record.packages, "vegan") + @test occursin('-', record.packages["vegan"]) + end end - if haskey(record.packages, "vegan") - @test occursin('-', record.packages["vegan"]) + else + ## Loud, named, and broken rather than passed. The testset title carries + # which world this is and what was missing, so the summary reads + # "packages_missing: dada2, Biostrings, ShortRead, vegan" instead of + # airing an @info that scrolls past (#43 criteria 2 and 3). + @testset "r — not probed ($(r_status.status)): $(r_status.reason)" begin + # ci.yml installs R and restores all 79 packages from renv.lock, so on + # CI a skip here is a provisioning regression, not a bare checkout. The + # failure names the missing packages via this testset's title. + if get(ENV, "CI", "false") == "true" + @test r_status.status === :ok + end + @test_skip PV.probe_r() end end end + ## One probe, not two that can drift (#43, criterion 1). This is a source check + # because the machine it has to hold on is the machine with no R: a skipped + # assertion asserts nothing, and the regression it guards against is a second + # call site, which a runtime check can only observe on a box that has R. + @testset "the R guard and the record it guards are one probe" begin + source = read(joinpath(@__DIR__, "test_provenance.jl"), String) + # Built by concatenation on purpose: written whole, this assertion's own + # source would contain the pattern it forbids and fail on itself. + narrowed = "packages = " * "String[]" + @test !occursin(narrowed, source) + @test occursin("PV.r_probe_status(", source) + end + + ## The negative control (#43, criterion 4): "R is not reachable" and "R is + # reachable but the packages are absent" are different worlds with different + # fixes, and the suite must say which one it is looking at. Pure, so it runs + # wherever the suite runs -- including everywhere the live probe above skips. + @testset "an unreachable R is told apart from a missing package" begin + unreachable = PV.ProbeFailure("r", "RCall: could not find R") + absent_pkgs = PV.ProbeFailure("r", + "R is reachable but these required packages are not installed: dada2, vegan", + ["dada2", "vegan"]) + + unreachable_status = PV.classify_r_probe_failure(unreachable) + @test unreachable_status.status === :r_unreachable + @test isempty(unreachable_status.missing) + @test !PV.r_probe_ok(unreachable_status) + @test occursin("could not find R", unreachable_status.reason) + + absent_status = PV.classify_r_probe_failure(absent_pkgs) + @test absent_status.status === :packages_missing + @test absent_status.missing == ["dada2", "vegan"] + @test !PV.r_probe_ok(absent_status) + # Distinguishable means distinguishable in the text a human reads, not only + # in a symbol a programmer compares. + @test !occursin("could not find R", absent_status.reason) + @test occursin("dada2", absent_status.reason) + @test absent_status.reason != unreachable_status.reason + + # And the third world, which needs no R at all: a probe is answered about the + # checkout before it is answered about the interpreter, so a missing pin file + # is reported as a missing pin file rather than as an unreachable R. + no_lock = PV.r_probe_status(renv_lock = "/nonexistent/renv.lock") + @test no_lock.status === :no_lockfile + @test !PV.r_probe_ok(no_lock) + @test isempty(no_lock.missing) + @test occursin("no renv.lock", no_lock.reason) + end + end