From aae4cce99f1e194c64d01f0b46d208bc3e09134b Mon Sep 17 00:00:00 2001 From: jgabry Date: Thu, 10 Sep 2026 10:06:57 -0600 Subject: [PATCH 1/8] Ignore build products beside the test models by pattern The test model directory listed fifteen executables by name and was already missing four, and a build record beside a test model would not have matched any of them. Ignore everything in that directory except the Stan sources instead. R CMD build keeps records out of the tarball by the same rule. Part of #1258. --- .Rbuildignore | 1 + tests/testthat/resources/stan/.gitignore | 22 ++++++---------------- 2 files changed, 7 insertions(+), 16 deletions(-) diff --git a/.Rbuildignore b/.Rbuildignore index cf04ce6ac..ab37a2a16 100644 --- a/.Rbuildignore +++ b/.Rbuildignore @@ -14,3 +14,4 @@ ^release-prep\.R$ ^\.vscode$ ^dev-notes$ +^tests/testthat/resources/stan/\..*\.cmdstanr\.json$ diff --git a/tests/testthat/resources/stan/.gitignore b/tests/testthat/resources/stan/.gitignore index c55f933eb..5ac071410 100644 --- a/tests/testthat/resources/stan/.gitignore +++ b/tests/testthat/resources/stan/.gitignore @@ -1,16 +1,6 @@ -/bernoulli -/bernoulli_external -/bernoulli_fp -/bernoulli_include -/bernoulli_log_lik -/bernoulli_ppc -/bernoulli_threads -/chain_fails -/divide_real_by_two -/fail -/info_message -/init_warnings -/logistic -/logistic_profiling -/schools - +# Everything compiled beside a test model is a build product: the executable, +# which has no extension, the C++ it was generated from, and the build record +# cmdstanr writes next to it. Ignore all of it and keep only the sources. +* +!.gitignore +!*.stan From 2c1e17d97828454d6669842eece59a705b377e6e Mon Sep 17 00:00:00 2001 From: jgabry Date: Thu, 10 Sep 2026 10:16:22 -0600 Subject: [PATCH 2/8] Add the build record schema, writer and reader A build record is a JSON file beside an executable, named from the executable's file name with a leading dot and .cmdstanr.json appended, describing how the executable was built. This adds the format version 1 schema, one validator that the constructor and the reader share, a writer that stages and renames, and a reader that returns why a record cannot be used rather than erroring. The version is checked first and on its own, so a record in a format this cmdstanr does not read is reported as unsupported_format with its version and nothing else, while a record failing any field check is unreadable whole. A record whose artifact hash does not match the executable comes back with nothing it contains. reported_features is encoded by key presence so that enabled, disabled and unknown survive a trip through the file, and is checked for shape but never for membership. Files are hashed with rlang::hash_file(), which raises the rlang floor to 1.0.0. Nothing writes a record beside a user's model yet; that is Stage 3. Part of #1258. --- DESCRIPTION | 2 +- R/build_record.R | 355 +++++++++++++++++++++++++++++ tests/testthat/test-build-record.R | 253 ++++++++++++++++++++ 3 files changed, 609 insertions(+), 1 deletion(-) create mode 100644 R/build_record.R create mode 100644 tests/testthat/test-build-record.R diff --git a/DESCRIPTION b/DESCRIPTION index af0bdf9f3..8b59d605b 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -47,7 +47,7 @@ Imports: R6 (>= 2.4.0), vctrs, withr (>= 2.5.0), - rlang (>= 0.4.7) + rlang (>= 1.0.0) Suggests: bayesplot, fs, diff --git a/R/build_record.R b/R/build_record.R new file mode 100644 index 000000000..533c9ef49 --- /dev/null +++ b/R/build_record.R @@ -0,0 +1,355 @@ +# The build record: a JSON file beside an executable describing how it was +# built. The schema, the writer and the reader all live here. + +# The only format version this cmdstanr reads or writes. +build_record_format_version <- 1L + +#' Where an executable's build record lives +#' +#' Beside the executable and named from its file name, never from +#' `$model_name()`, which substitutes underscores for spaces while the +#' executable path does not. Two executables in one directory therefore cannot +#' share a record. +#' +#' @noRd +build_record_path <- function(exe_file) { + checkmate::assert_string(exe_file) + file.path( + dirname(exe_file), + paste0(".", basename(exe_file), ".cmdstanr.json") + ) +} + +#' Hash a file's contents +#' +#' Every hash in a record comes from here, so the algorithm can change in one +#' place. +#' +#' @noRd +hash_file <- function(path) { + unname(rlang::hash_file(path)) +} + + +# schema ------------------------------------------------------------------ + +# Shapes are as jsonlite::fromJSON(simplifyVector = FALSE) returns them. An +# object is a named list, an array is an unnamed list, and a scalar is an atomic +# vector of length one that is not NA. + +#' Reject a build record, naming the field that failed +#' +#' @noRd +stop_build_record_field <- function(field, requirement) { + stop("build record field `", field, "` ", requirement, ".", call. = FALSE) +} + +#' Fetch a member the schema requires +#' +#' @noRd +record_member <- function(x, name, field) { + if (!name %in% names(x)) { + stop_build_record_field(field, "is missing") + } + x[[name]] +} + +#' @noRd +require_record_object <- function(value, field) { + if (!is.list(value) || is.null(names(value)) || !all(nzchar(names(value)))) { + stop_build_record_field(field, "must be a JSON object") + } +} + +#' @noRd +require_record_array <- function(value, field) { + if (!is.list(value) || !is.null(names(value))) { + stop_build_record_field(field, "must be a JSON array") + } +} + +#' @noRd +require_record_string <- function(value, field) { + if (!checkmate::test_string(value)) { + stop_build_record_field(field, "must be a string") + } +} + +#' @noRd +require_record_flag <- function(value, field) { + if (!checkmate::test_flag(value)) { + stop_build_record_field(field, "must be true or false") + } +} + +#' @noRd +require_record_string_array <- function(value, field) { + require_record_array(value, field) + for (i in seq_along(value)) { + require_record_string(value[[i]], paste0(field, "[[", i, "]]")) + } +} + +#' A file the build consumed, identified by content and by where it then was +#' +#' @noRd +require_dependency_entry <- function(value, field) { + require_record_object(value, field) + require_record_string( + record_member(value, "hash", paste0(field, ".hash")), + paste0(field, ".hash") + ) + require_record_string( + record_member(value, "built_from", paste0(field, ".built_from")), + paste0(field, ".built_from") + ) +} + +#' Check a build record against the format version 1 schema +#' +#' This function is the schema. The constructor and the reader both call it and +#' nothing else checks a record's fields, so a record one caller can use is a +#' record every caller can use. Fields are checked in the order the record +#' holds them and the first failure names its field. Members the schema +#' does not name are ignored rather than rejected. `reported_features` is +#' checked for shape and never for membership, because an absent feature means +#' unknown and the set CmdStan reports is the binary's to decide. +#' +#' @noRd +validate_build_record <- function(record) { + checkmate::assert_list(record, .var.name = "record") + + format_version <- record_member(record, "format_version", "format_version") + if (!checkmate::test_int(format_version) || + format_version != build_record_format_version) { + stop_build_record_field( + "format_version", + paste0("must be ", build_record_format_version) + ) + } + + request <- record_member(record, "request", "request") + require_record_object(request, "request") + + cpp_options <- record_member( + request, "cpp_options_supplied", "request.cpp_options_supplied" + ) + require_record_object(cpp_options, "request.cpp_options_supplied") + for (i in seq_along(cpp_options)) { + option_name <- names(cpp_options)[[i]] + field <- paste0("request.cpp_options_supplied.", option_name) + if (!grepl(paste0("^", make_variable_name_pattern, "$"), option_name)) { + stop_build_record_field(field, "must be named for a Make variable") + } + require_record_string(cpp_options[[i]], field) + } + + require_record_string_array( + record_member( + request, "stanc_options_supplied", "request.stanc_options_supplied" + ), + "request.stanc_options_supplied" + ) + require_record_string_array( + record_member( + request, "stanc_options_injected", "request.stanc_options_injected" + ), + "request.stanc_options_injected" + ) + + stanc_name <- record_member(request, "stanc_name", "request.stanc_name") + require_record_string(stanc_name, "request.stanc_name") + if (!nzchar(stanc_name)) { + stop_build_record_field("request.stanc_name", "must not be empty") + } + + require_record_string_array( + record_member(request, "include_paths", "request.include_paths"), + "request.include_paths" + ) + + reported_features <- record_member( + record, "reported_features", "reported_features" + ) + require_record_object(reported_features, "reported_features") + for (i in seq_along(reported_features)) { + feature_name <- names(reported_features)[[i]] + field <- paste0("reported_features.", feature_name) + if (feature_name == "stan_version") { + require_record_string(reported_features[[i]], field) + } else { + require_record_flag(reported_features[[i]], field) + } + } + + dependencies <- record_member(record, "dependencies", "dependencies") + require_record_object(dependencies, "dependencies") + require_dependency_entry( + record_member(dependencies, "stan_file", "dependencies.stan_file"), + "dependencies.stan_file" + ) + # An absent user header or make/local means there was none. + for (optional in c("user_header", "make_local")) { + if (optional %in% names(dependencies)) { + require_dependency_entry( + dependencies[[optional]], paste0("dependencies.", optional) + ) + } + } + included_files <- record_member( + dependencies, "included_files", "dependencies.included_files" + ) + require_record_array(included_files, "dependencies.included_files") + for (i in seq_along(included_files)) { + require_dependency_entry( + included_files[[i]], paste0("dependencies.included_files[[", i, "]]") + ) + } + + require_record_string( + record_member(record, "artifact", "artifact"), "artifact" + ) + + builder <- record_member(record, "builder", "builder") + require_record_object(builder, "builder") + require_record_string( + record_member(builder, "path", "builder.path"), "builder.path" + ) + builder_version <- record_member(builder, "version", "builder.version") + require_record_string(builder_version, "builder.version") + # A string that is not a CmdStan version is the wrong shape, not an odd value. + if (!grepl("^[0-9]+\\.[0-9]+\\.[0-9]+(-rc[0-9]+)?$", builder_version)) { + stop_build_record_field( + "builder.version", "must be a CmdStan version such as \"2.39.0\"" + ) + } + + require_record_string(record_member(record, "tbb_dir", "tbb_dir"), "tbb_dir") + + untracked <- record_member( + record, "known_untracked_dependencies", "known_untracked_dependencies" + ) + require_record_array(untracked, "known_untracked_dependencies") + for (i in seq_along(untracked)) { + field <- paste0("known_untracked_dependencies[[", i, "]]") + require_record_object(untracked[[i]], field) + kind <- record_member(untracked[[i]], "kind", paste0(field, ".kind")) + require_record_string(kind, paste0(field, ".kind")) + if (!kind %in% c("make_local_include", "user_header_include")) { + stop_build_record_field( + paste0(field, ".kind"), + "must be \"make_local_include\" or \"user_header_include\"" + ) + } + require_record_string( + record_member( + untracked[[i]], "detected_in", paste0(field, ".detected_in") + ), + paste0(field, ".detected_in") + ) + } + + invisible(record) +} + +#' Assemble a build record +#' +#' The one place a record is built. `format_version` comes first and the rest +#' follow the schema's order, so the written JSON reads in that order too. +#' +#' @noRd +new_build_record <- function(request, reported_features, dependencies, artifact, + builder, tbb_dir, + known_untracked_dependencies = list()) { + record <- list( + format_version = build_record_format_version, + request = request, + reported_features = reported_features, + dependencies = dependencies, + artifact = artifact, + builder = builder, + tbb_dir = tbb_dir, + known_untracked_dependencies = known_untracked_dependencies + ) + validate_build_record(record) + record +} + + +# writing and reading ----------------------------------------------------- + +#' Write a build record beside its executable +#' +#' Staged in the same directory and renamed into place so a reader never meets +#' a half-written record. A failed rename warns, and the warning is suppressed +#' so that `warn = 2` cannot pre-empt the error below. `auto_unbox` writes a length-one vector as a JSON +#' scalar, which is why the schema holds every array as a list and every scalar +#' as a length-one vector: a one-element `included_files` still writes as an +#' array. Nothing is ever `NULL` or `NA`, since an unknown state is an absent +#' key. +#' +#' @noRd +write_build_record <- function(record, exe_file) { + validate_build_record(record) + path <- build_record_path(exe_file) + staged <- tempfile(pattern = basename(path), tmpdir = dirname(path)) + jsonlite::write_json( + record, staged, auto_unbox = TRUE, pretty = TRUE, digits = NA + ) + if (!isTRUE(suppressWarnings(file.rename(staged, path)))) { + unlink(staged) + stop("Could not write the build record to ", path, ".", call. = FALSE) + } + invisible(path) +} + +#' Read the build record beside an executable +#' +#' Returns the reason a record cannot be used instead of signalling it, because +#' every one of those reasons is an ordinary outcome. The version is checked +#' first and on its own, so a record written in a format we do not read is +#' never measured against the current schema. Anything failing a field check is +#' unreadable whole and comes back with no `format_version`, and a record whose +#' hash does not match the executable comes back with nothing it contains. +#' +#' @noRd +read_build_record <- function(exe_file) { + checkmate::assert_file_exists(exe_file) + path <- build_record_path(exe_file) + if (!file.exists(path)) { + return(list(status = "unavailable", reason = "missing")) + } + unreadable <- list(status = "unavailable", reason = "unreadable") + + record <- tryCatch( + jsonlite::fromJSON(path, simplifyVector = FALSE), + error = function(e) NULL + ) + if (!is.list(record) || !"format_version" %in% names(record)) { + return(unreadable) + } + format_version <- record[["format_version"]] + if (!checkmate::test_int(format_version)) { + return(unreadable) + } + if (format_version != build_record_format_version) { + return(list( + status = "unavailable", + reason = "unsupported_format", + format_version = format_version + )) + } + + accepted <- tryCatch({ + validate_build_record(record) + TRUE + }, error = function(e) FALSE) + if (!accepted) { + return(unreadable) + } + if (!identical(hash_file(exe_file), record[["artifact"]])) { + return(list(status = "unavailable", reason = "artifact_mismatch")) + } + + list(status = "available", record = record) +} diff --git a/tests/testthat/test-build-record.R b/tests/testthat/test-build-record.R new file mode 100644 index 000000000..831e4d6c4 --- /dev/null +++ b/tests/testthat/test-build-record.R @@ -0,0 +1,253 @@ +local_fake_exe <- function(name = "bernoulli") { + path <- file.path( + withr::local_tempdir(.local_envir = parent.frame()), + name + ) + writeBin(as.raw(c(0x7f, 0x45, 0x4c, 0x46)), path) + path +} + +example_record <- function(exe_file) { + new_build_record( + request = list( + cpp_options_supplied = list(STAN_THREADS = "true"), + stanc_options_supplied = list("--O1"), + stanc_options_injected = list("--name=bernoulli_model"), + stanc_name = "bernoulli", + include_paths = list(dirname(exe_file)) + ), + reported_features = list( + stan_threads = TRUE, + stan_opencl = FALSE, + stan_version = "2.39.0" + ), + dependencies = list( + stan_file = list(hash = "0f1e", built_from = "bernoulli.stan"), + included_files = list( + list(hash = "2d3c", built_from = "helpers.stan") + ), + make_local = list(hash = "4b5a", built_from = "make/local") + ), + artifact = hash_file(exe_file), + builder = list(path = "/opt/cmdstan-2.39.0", version = "2.39.0"), + tbb_dir = "/opt/cmdstan-2.39.0/stan/lib/stan_math/lib/tbb", + known_untracked_dependencies = list( + list(kind = "make_local_include", detected_in = "make/local") + ) + ) +} + +test_that("build_record_path names the record after the executable file", { + exe <- local_fake_exe() + expect_equal( + build_record_path(exe), + file.path(dirname(exe), ".bernoulli.cmdstanr.json") + ) + + windows_exe <- local_fake_exe("bernoulli.exe") + expect_equal( + build_record_path(windows_exe), + file.path(dirname(windows_exe), ".bernoulli.exe.cmdstanr.json") + ) + + spaced <- local_fake_exe("my model") + expect_equal( + build_record_path(spaced), + file.path(dirname(spaced), ".my model.cmdstanr.json") + ) +}) + +test_that("a written record reads back unchanged", { + exe <- local_fake_exe() + record <- example_record(exe) + path <- write_build_record(record, exe) + + result <- read_build_record(exe) + expect_equal(result$status, "available") + expect_false("reason" %in% names(result)) + expect_equal(result$record, record) + + text <- paste(readLines(path, warn = FALSE), collapse = "\n") + expect_false(grepl("null", text, fixed = TRUE)) + expect_match(text, '"included_files"\\s*:\\s*\\[') +}) + +test_that("an empty object writes as {} and an empty array as []", { + exe <- local_fake_exe() + record <- example_record(exe) + record$request$cpp_options_supplied <- structure(list(), names = character()) + record$dependencies$included_files <- list() + path <- write_build_record(record, exe) + + text <- paste(readLines(path, warn = FALSE), collapse = "\n") + expect_match(text, '"cpp_options_supplied"\\s*:\\s*\\{\\s*\\}') + expect_match(text, '"included_files"\\s*:\\s*\\[\\s*\\]') + + result <- read_build_record(exe) + expect_equal( + result$record$request$cpp_options_supplied, + structure(list(), names = character()) + ) + expect_equal(result$record$dependencies$included_files, list()) +}) + +test_that("an executable with no record beside it is missing", { + exe <- local_fake_exe() + expect_equal( + read_build_record(exe), + list(status = "unavailable", reason = "missing") + ) +}) + +test_that("a record that is not JSON is unreadable", { + exe <- local_fake_exe() + writeLines("{not json", build_record_path(exe)) + + result <- read_build_record(exe) + expect_equal(result$reason, "unreadable") + expect_false("record" %in% names(result)) + expect_false("format_version" %in% names(result)) +}) + +test_that("a record with a field of the wrong type is unreadable", { + exe <- local_fake_exe() + record <- example_record(exe) + record$builder$version <- 42 + jsonlite::write_json( + record, build_record_path(exe), + auto_unbox = TRUE, pretty = TRUE, digits = NA + ) + + result <- read_build_record(exe) + expect_equal(result$reason, "unreadable") + expect_false("record" %in% names(result)) + expect_false("format_version" %in% names(result)) +}) + +test_that("a record in a format we do not read is checked on its version alone", { + exe <- local_fake_exe() + record <- example_record(exe) + record$format_version <- 99L + record$builder <- "garbage" + jsonlite::write_json( + record, build_record_path(exe), + auto_unbox = TRUE, pretty = TRUE, digits = NA + ) + + result <- read_build_record(exe) + expect_equal(result$reason, "unsupported_format") + expect_equal(result$format_version, 99) + expect_false("record" %in% names(result)) +}) + +test_that("a record whose hash does not match the executable is a mismatch", { + exe <- local_fake_exe() + write_build_record(example_record(exe), exe) + writeBin(as.raw(c(0x7f, 0x45, 0x4c, 0x46, 0x00)), exe) + + result <- read_build_record(exe) + expect_equal(result$reason, "artifact_mismatch") + expect_false("record" %in% names(result)) +}) + +test_that("reported features keep enabled, disabled and unknown apart", { + write_features <- function(exe, features) { + record <- example_record(exe) + record$reported_features <- features + write_build_record(record, exe) + read_build_record(exe) + } + + on_exe <- local_fake_exe("threads_on") + off_exe <- local_fake_exe("threads_off") + silent_exe <- local_fake_exe("threads_unreported") + + on <- write_features( + on_exe, list(stan_threads = TRUE, stan_version = "2.39.0") + ) + off <- write_features( + off_exe, list(stan_threads = FALSE, stan_version = "2.39.0") + ) + silent <- write_features(silent_exe, list(stan_version = "2.39.0")) + + expect_equal(on$status, "available") + expect_equal(off$status, "available") + expect_equal(silent$status, "available") + + expect_true(on$record$reported_features$stan_threads) + expect_false(off$record$reported_features$stan_threads) + expect_null(silent$record$reported_features$stan_threads) + + expect_true("stan_threads" %in% names(on$record$reported_features)) + expect_true("stan_threads" %in% names(off$record$reported_features)) + expect_false("stan_threads" %in% names(silent$record$reported_features)) +}) + +test_that("the validator names the field that fails", { + exe <- local_fake_exe() + base <- example_record(exe) + rebuild <- function(record) do.call(new_build_record, record[-1]) + + bad_version <- base + bad_version$builder$version <- "2.39" + expect_error(rebuild(bad_version), "`builder.version`", fixed = TRUE) + + no_name <- base + no_name$request$stanc_name <- "" + expect_error(rebuild(no_name), "`request.stanc_name`", fixed = TRUE) + + logical_option <- base + logical_option$request$cpp_options_supplied <- list(STAN_THREADS = TRUE) + expect_error( + rebuild(logical_option), + "`request.cpp_options_supplied.STAN_THREADS`", + fixed = TRUE + ) + + unknown_kind <- base + unknown_kind$known_untracked_dependencies <- list( + list(kind = "mystery", detected_in = "make/local") + ) + expect_error( + rebuild(unknown_kind), + "`known_untracked_dependencies[[1]].kind`", + fixed = TRUE + ) + + odd_feature <- base + odd_feature$reported_features$stan_threads <- "yes" + expect_error( + rebuild(odd_feature), "`reported_features.stan_threads`", fixed = TRUE + ) + + no_source <- base + no_source$dependencies$stan_file <- NULL + expect_error(rebuild(no_source), "`dependencies.stan_file`", fixed = TRUE) +}) + +test_that("a record missing a required field is unreadable whole", { + exe <- local_fake_exe() + path <- write_build_record(example_record(exe), exe) + edited <- jsonlite::fromJSON(path, simplifyVector = FALSE) + edited$tbb_dir <- NULL + jsonlite::write_json( + edited, path, auto_unbox = TRUE, pretty = TRUE, digits = NA + ) + + result <- read_build_record(exe) + expect_equal(result$reason, "unreadable") + expect_false("format_version" %in% names(result)) + expect_false("record" %in% names(result)) +}) + +test_that("a record carrying a member the schema does not name still reads", { + exe <- local_fake_exe() + path <- write_build_record(example_record(exe), exe) + edited <- jsonlite::fromJSON(path, simplifyVector = FALSE) + edited$extra <- "written by a later cmdstanr" + jsonlite::write_json( + edited, path, auto_unbox = TRUE, pretty = TRUE, digits = NA + ) + + expect_equal(read_build_record(exe)$status, "available") +}) From 4bbcc6923fa01f4bfcfb18de0892634f55406f28 Mon Sep 17 00:00:00 2001 From: jgabry Date: Thu, 10 Sep 2026 10:17:58 -0600 Subject: [PATCH 3/8] Describe the test ignore patterns as in place The design note asked for pattern-based ignore rules in this repository before Stage 3 writes a record; they landed with Stage 2, so say so. The Stage 2 paragraph in the order of work now points at Stage 3 for the user-facing lifecycle wording, matching where #1258 lists it. Part of #1258. --- dev-notes/compilation-state.md | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/dev-notes/compilation-state.md b/dev-notes/compilation-state.md index 6580e5912..debcff9b5 100644 --- a/dev-notes/compilation-state.md +++ b/dev-notes/compilation-state.md @@ -1078,11 +1078,11 @@ all three. **cmdstanr writes neither ignore file itself.** Compiling a model should not modify a user's repository configuration; the recommendation belongs in documentation. -**This repository needs the patterns too, before Stage 3 writes anything.** -`tests/testthat/resources/stan/.gitignore` enumerates fifteen compiled binaries by -hand and is already behind; four models there have no entry. Every compiled test -model will add a record, so replace the enumeration with patterns rather than -extending it. +**This repository carries the patterns itself.** +`tests/testthat/resources/stan/.gitignore` ignores everything in that directory +but the Stan sources, since every compiled test model adds a record beside its +executable, and `.Rbuildignore` keeps those records out of a tarball built from +the source tree. Stage 2 put both in place before anything wrote a record. ### Binding the record to its executable @@ -3356,8 +3356,9 @@ fixtures. record beside a user's Stan program until Stage 3. Name, format and the version-control story are decided (ยง4), settled enough to build against and revisable until the release. What this stage owes is the groundwork that has to be -in place before Stage 3 creates a file: ignore patterns in this repository, and the -user-facing wording to ship with the writer. +in place before Stage 3 creates a file: ignore patterns in this repository. The +user-facing wording about the record's lifecycle ships with the writer in Stage 3, +where #1258 lists it. ### Stage 3: transactional record writing From 0bc23557e9bfce59ab9e868d296c92fc125d25d1 Mon Sep 17 00:00:00 2001 From: jgabry Date: Thu, 10 Sep 2026 10:20:37 -0600 Subject: [PATCH 4/8] Compare two build records field by field compare_build_records() names every compared field that differs between a record read from disk and one assembled for the current call, in the order the design's table lists them, rather than stopping at the first. Only the fields the table marks compared are consulted: cpp_options after sorting by name, stanc options as an ordered argument vector, the stanc name, the content hashes of the Stan program, its includes in order and make/local, the user header by hash and by path, and the CmdStan installation. Paths are compared as the records hold them; normalising them is the recorder's job. Part of #1258. --- R/build_record.R | 73 ++++++++++++++ tests/testthat/test-build-record.R | 155 +++++++++++++++++++++++++++++ 2 files changed, 228 insertions(+) diff --git a/R/build_record.R b/R/build_record.R index 533c9ef49..26335d639 100644 --- a/R/build_record.R +++ b/R/build_record.R @@ -353,3 +353,76 @@ read_build_record <- function(exe_file) { list(status = "available", record = record) } + +#' Compare a recorded build against the current one +#' +#' Returns the name of every compared field whose value differs between the +#' two records, in the order the schema's comparison table lists them. Every +#' field is checked, and nothing stops at the first difference, so a caller +#' who changed more than one thing is told about all of them. +#' +#' @noRd +compare_build_records <- function(recorded, current) { + differences <- character() + sort_by_name <- function(x) x[order(names(x))] + + if (!identical( + sort_by_name(recorded$request$cpp_options_supplied), + sort_by_name(current$request$cpp_options_supplied) + )) { + differences <- c(differences, "cpp_options") + } + + if (!identical( + recorded$request$stanc_options_supplied, + current$request$stanc_options_supplied + )) { + differences <- c(differences, "stanc_options") + } + + if (!identical(recorded$request$stanc_name, current$request$stanc_name)) { + differences <- c(differences, "stanc_name") + } + + if (!identical( + recorded$dependencies$stan_file$hash, + current$dependencies$stan_file$hash + )) { + differences <- c(differences, "stan_file") + } + + included_hashes <- function(record) { + vapply(record$dependencies$included_files, `[[`, character(1), "hash") + } + if (!identical(included_hashes(recorded), included_hashes(current))) { + differences <- c(differences, "included_files") + } + + recorded_header <- recorded$dependencies$user_header + current_header <- current$dependencies$user_header + header_differs <- is.null(recorded_header) != is.null(current_header) || ( + !is.null(recorded_header) && ( + !identical(recorded_header$hash, current_header$hash) || + !identical(recorded_header$built_from, current_header$built_from) + ) + ) + if (header_differs) { + differences <- c(differences, "user_header") + } + + recorded_local <- recorded$dependencies$make_local + current_local <- current$dependencies$make_local + local_differs <- is.null(recorded_local) != is.null(current_local) || ( + !is.null(recorded_local) && !identical(recorded_local$hash, current_local$hash) + ) + if (local_differs) { + differences <- c(differences, "make_local") + } + + if (!identical(recorded$builder$path, current$builder$path) || + !identical(recorded$builder$version, current$builder$version)) { + differences <- c(differences, "builder") + } + + differences +} diff --git a/tests/testthat/test-build-record.R b/tests/testthat/test-build-record.R index 831e4d6c4..f900914fb 100644 --- a/tests/testthat/test-build-record.R +++ b/tests/testthat/test-build-record.R @@ -251,3 +251,158 @@ test_that("a record carrying a member the schema does not name still reads", { expect_equal(read_build_record(exe)$status, "available") }) + +test_that("two identical build records compare with no differences", { + exe <- local_fake_exe() + recorded <- example_record(exe) + current <- example_record(exe) + expect_equal(compare_build_records(recorded, current), character(0)) +}) + +test_that("a changed cpp option value differs as cpp_options", { + exe <- local_fake_exe() + recorded <- example_record(exe) + current <- example_record(exe) + current$request$cpp_options_supplied <- list(STAN_THREADS = "false") + expect_equal(compare_build_records(recorded, current), "cpp_options") +}) + +test_that("a reordered stanc_options_supplied differs as stanc_options", { + exe <- local_fake_exe() + recorded <- example_record(exe) + current <- example_record(exe) + recorded$request$stanc_options_supplied <- list("--O0", "--O1") + current$request$stanc_options_supplied <- list("--O1", "--O0") + expect_equal(compare_build_records(recorded, current), "stanc_options") +}) + +test_that("a changed stanc_name differs as stanc_name", { + exe <- local_fake_exe() + recorded <- example_record(exe) + current <- example_record(exe) + current$request$stanc_name <- "other_model" + expect_equal(compare_build_records(recorded, current), "stanc_name") +}) + +test_that("a changed stan_file hash differs as stan_file", { + exe <- local_fake_exe() + recorded <- example_record(exe) + current <- example_record(exe) + current$dependencies$stan_file$hash <- "ffff" + expect_equal(compare_build_records(recorded, current), "stan_file") +}) + +test_that("a reordered included_files differs as included_files", { + exe <- local_fake_exe() + recorded <- example_record(exe) + current <- example_record(exe) + recorded$dependencies$included_files <- list( + list(hash = "aaaa", built_from = "one.stan"), + list(hash = "bbbb", built_from = "two.stan") + ) + current$dependencies$included_files <- list( + list(hash = "bbbb", built_from = "two.stan"), + list(hash = "aaaa", built_from = "one.stan") + ) + expect_equal(compare_build_records(recorded, current), "included_files") +}) + +test_that("a user_header present on only one side differs as user_header", { + exe <- local_fake_exe() + recorded <- example_record(exe) + current <- example_record(exe) + current$dependencies$user_header <- list(hash = "cccc", built_from = "header.hpp") + expect_equal(compare_build_records(recorded, current), "user_header") +}) + +test_that("a user_header with the same hash but a different built_from differs as user_header", { + exe <- local_fake_exe() + recorded <- example_record(exe) + current <- example_record(exe) + recorded$dependencies$user_header <- list(hash = "cccc", built_from = "header.hpp") + current$dependencies$user_header <- list(hash = "cccc", built_from = "other/header.hpp") + expect_equal(compare_build_records(recorded, current), "user_header") +}) + +test_that("a make_local present on only one side differs as make_local", { + exe <- local_fake_exe() + recorded <- example_record(exe) + current <- example_record(exe) + current$dependencies$make_local <- NULL + expect_equal(compare_build_records(recorded, current), "make_local") +}) + +test_that("a changed builder version differs as builder", { + exe <- local_fake_exe() + recorded <- example_record(exe) + current <- example_record(exe) + current$builder$version <- "2.40.0" + expect_equal(compare_build_records(recorded, current), "builder") +}) + +test_that("a changed builder path differs as builder", { + exe <- local_fake_exe() + recorded <- example_record(exe) + current <- example_record(exe) + current$builder$path <- "/opt/cmdstan-2.40.0" + expect_equal(compare_build_records(recorded, current), "builder") +}) + +test_that("two differences are both reported, in table order", { + exe <- local_fake_exe() + recorded <- example_record(exe) + current <- example_record(exe) + current$dependencies$stan_file$hash <- "ffff" + current$builder$version <- "2.40.0" + expect_equal(compare_build_records(recorded, current), c("stan_file", "builder")) +}) + +test_that("the same cpp options in a different assignment order do not differ", { + exe <- local_fake_exe() + recorded <- example_record(exe) + current <- example_record(exe) + recorded$request$cpp_options_supplied <- list( + STAN_THREADS = "true", STAN_NO_RANGE_CHECKS = "true" + ) + current$request$cpp_options_supplied <- list( + STAN_NO_RANGE_CHECKS = "true", STAN_THREADS = "true" + ) + expect_equal(compare_build_records(recorded, current), character(0)) +}) + +test_that("an included file with the same hash and a different built_from does not differ", { + exe <- local_fake_exe() + recorded <- example_record(exe) + current <- example_record(exe) + recorded$dependencies$included_files <- list( + list(hash = "aaaa", built_from = "one.stan") + ) + current$dependencies$included_files <- list( + list(hash = "aaaa", built_from = "elsewhere/one.stan") + ) + expect_equal(compare_build_records(recorded, current), character(0)) +}) + +test_that("a make_local with the same hash and a different built_from does not differ", { + exe <- local_fake_exe() + recorded <- example_record(exe) + current <- example_record(exe) + recorded$dependencies$make_local <- list(hash = "4b5a", built_from = "make/local") + current$dependencies$make_local <- list(hash = "4b5a", built_from = "other/make/local") + expect_equal(compare_build_records(recorded, current), character(0)) +}) + +test_that("differences outside the comparison table never count", { + exe <- local_fake_exe() + recorded <- example_record(exe) + current <- example_record(exe) + current$request$stanc_options_injected <- list("--name=other_model") + current$request$include_paths <- list("/some/other/path") + current$reported_features$stan_opencl <- TRUE + current$tbb_dir <- "/opt/other/tbb" + current$known_untracked_dependencies <- list( + list(kind = "user_header_include", detected_in = "other.hpp") + ) + current$artifact <- "deadbeef" + expect_equal(compare_build_records(recorded, current), character(0)) +}) From 07e8fd53b34dac41453158e5c525d326d2dd2ccc Mon Sep 17 00:00:00 2001 From: jgabry Date: Thu, 10 Sep 2026 10:41:49 -0600 Subject: [PATCH 5/8] Write the validator and the comparison as tables The schema helpers collapse to one shape check and one member fetch that takes the shape by name, so the validator reads as the list of fields and their shapes. A missing member and a JSON null both arrive as NULL and are reported as missing. compare_build_records() is now a named list of extractors, one per compared row, applied to both records, so the table in the design note and the code have the same rows in the same order. No check, message or result changes. Part of #1258. --- R/build_record.R | 291 +++++++++++++++++------------------------------ 1 file changed, 102 insertions(+), 189 deletions(-) diff --git a/R/build_record.R b/R/build_record.R index 26335d639..034e1b618 100644 --- a/R/build_record.R +++ b/R/build_record.R @@ -36,6 +36,26 @@ hash_file <- function(path) { # Shapes are as jsonlite::fromJSON(simplifyVector = FALSE) returns them. An # object is a named list, an array is an unnamed list, and a scalar is an atomic # vector of length one that is not NA. +is_json_object <- function(x) { + is.list(x) && !is.null(names(x)) && all(nzchar(names(x))) +} + +is_json_array <- function(x) { + is.list(x) && is.null(names(x)) +} + +record_shapes <- list( + string = list( + test = checkmate::test_string, requirement = "must be a string" + ), + flag = list( + test = checkmate::test_flag, requirement = "must be true or false" + ), + object = list(test = is_json_object, requirement = "must be a JSON object"), + array = list(test = is_json_array, requirement = "must be a JSON array") +) + +cmdstan_version_pattern <- "^[0-9]+\\.[0-9]+\\.[0-9]+(-rc[0-9]+)?$" #' Reject a build record, naming the field that failed #' @@ -44,65 +64,37 @@ stop_build_record_field <- function(field, requirement) { stop("build record field `", field, "` ", requirement, ".", call. = FALSE) } -#' Fetch a member the schema requires -#' -#' @noRd -record_member <- function(x, name, field) { - if (!name %in% names(x)) { +# `field` is the path the error names, such as "dependencies.stan_file.hash". +# A missing member and an explicit JSON null both arrive as NULL. +record_shape <- function(value, shape, field) { + if (is.null(value)) { stop_build_record_field(field, "is missing") } - x[[name]] -} - -#' @noRd -require_record_object <- function(value, field) { - if (!is.list(value) || is.null(names(value)) || !all(nzchar(names(value)))) { - stop_build_record_field(field, "must be a JSON object") - } -} - -#' @noRd -require_record_array <- function(value, field) { - if (!is.list(value) || !is.null(names(value))) { - stop_build_record_field(field, "must be a JSON array") - } -} - -#' @noRd -require_record_string <- function(value, field) { - if (!checkmate::test_string(value)) { - stop_build_record_field(field, "must be a string") + if (!record_shapes[[shape]]$test(value)) { + stop_build_record_field(field, record_shapes[[shape]]$requirement) } + invisible(value) } -#' @noRd -require_record_flag <- function(value, field) { - if (!checkmate::test_flag(value)) { - stop_build_record_field(field, "must be true or false") - } +record_member <- function(x, name, shape, field = name) { + record_shape(x[[name]], shape, field) } -#' @noRd -require_record_string_array <- function(value, field) { - require_record_array(value, field) +record_string_array <- function(x, name, field) { + value <- record_member(x, name, "array", field) for (i in seq_along(value)) { - require_record_string(value[[i]], paste0(field, "[[", i, "]]")) + record_shape(value[[i]], "string", paste0(field, "[[", i, "]]")) } + invisible(value) } #' A file the build consumed, identified by content and by where it then was #' #' @noRd -require_dependency_entry <- function(value, field) { - require_record_object(value, field) - require_record_string( - record_member(value, "hash", paste0(field, ".hash")), - paste0(field, ".hash") - ) - require_record_string( - record_member(value, "built_from", paste0(field, ".built_from")), - paste0(field, ".built_from") - ) +record_dependency_entry <- function(value, field) { + record_shape(value, "object", field) + record_member(value, "hash", "string", paste0(field, ".hash")) + record_member(value, "built_from", "string", paste0(field, ".built_from")) } #' Check a build record against the format version 1 schema @@ -119,133 +111,91 @@ require_dependency_entry <- function(value, field) { validate_build_record <- function(record) { checkmate::assert_list(record, .var.name = "record") - format_version <- record_member(record, "format_version", "format_version") + format_version <- record[["format_version"]] if (!checkmate::test_int(format_version) || format_version != build_record_format_version) { stop_build_record_field( - "format_version", - paste0("must be ", build_record_format_version) + "format_version", paste0("must be ", build_record_format_version) ) } - request <- record_member(record, "request", "request") - require_record_object(request, "request") - + request <- record_member(record, "request", "object") cpp_options <- record_member( - request, "cpp_options_supplied", "request.cpp_options_supplied" + request, "cpp_options_supplied", "object", "request.cpp_options_supplied" ) - require_record_object(cpp_options, "request.cpp_options_supplied") for (i in seq_along(cpp_options)) { option_name <- names(cpp_options)[[i]] field <- paste0("request.cpp_options_supplied.", option_name) if (!grepl(paste0("^", make_variable_name_pattern, "$"), option_name)) { stop_build_record_field(field, "must be named for a Make variable") } - require_record_string(cpp_options[[i]], field) + record_shape(cpp_options[[i]], "string", field) } - - require_record_string_array( - record_member( - request, "stanc_options_supplied", "request.stanc_options_supplied" - ), - "request.stanc_options_supplied" + record_string_array( + request, "stanc_options_supplied", "request.stanc_options_supplied" ) - require_record_string_array( - record_member( - request, "stanc_options_injected", "request.stanc_options_injected" - ), - "request.stanc_options_injected" + record_string_array( + request, "stanc_options_injected", "request.stanc_options_injected" + ) + stanc_name <- record_member( + request, "stanc_name", "string", "request.stanc_name" ) - - stanc_name <- record_member(request, "stanc_name", "request.stanc_name") - require_record_string(stanc_name, "request.stanc_name") if (!nzchar(stanc_name)) { stop_build_record_field("request.stanc_name", "must not be empty") } + record_string_array(request, "include_paths", "request.include_paths") - require_record_string_array( - record_member(request, "include_paths", "request.include_paths"), - "request.include_paths" - ) - - reported_features <- record_member( - record, "reported_features", "reported_features" - ) - require_record_object(reported_features, "reported_features") + reported_features <- record_member(record, "reported_features", "object") for (i in seq_along(reported_features)) { feature_name <- names(reported_features)[[i]] - field <- paste0("reported_features.", feature_name) - if (feature_name == "stan_version") { - require_record_string(reported_features[[i]], field) - } else { - require_record_flag(reported_features[[i]], field) - } + shape <- if (feature_name == "stan_version") "string" else "flag" + record_shape( + reported_features[[i]], shape, paste0("reported_features.", feature_name) + ) } - dependencies <- record_member(record, "dependencies", "dependencies") - require_record_object(dependencies, "dependencies") - require_dependency_entry( - record_member(dependencies, "stan_file", "dependencies.stan_file"), - "dependencies.stan_file" - ) + dependencies <- record_member(record, "dependencies", "object") # An absent user header or make/local means there was none. - for (optional in c("user_header", "make_local")) { - if (optional %in% names(dependencies)) { - require_dependency_entry( - dependencies[[optional]], paste0("dependencies.", optional) - ) - } + optional <- intersect(c("user_header", "make_local"), names(dependencies)) + for (name in c("stan_file", optional)) { + record_dependency_entry(dependencies[[name]], paste0("dependencies.", name)) } included_files <- record_member( - dependencies, "included_files", "dependencies.included_files" + dependencies, "included_files", "array", "dependencies.included_files" ) - require_record_array(included_files, "dependencies.included_files") for (i in seq_along(included_files)) { - require_dependency_entry( + record_dependency_entry( included_files[[i]], paste0("dependencies.included_files[[", i, "]]") ) } - require_record_string( - record_member(record, "artifact", "artifact"), "artifact" - ) + record_member(record, "artifact", "string") - builder <- record_member(record, "builder", "builder") - require_record_object(builder, "builder") - require_record_string( - record_member(builder, "path", "builder.path"), "builder.path" - ) - builder_version <- record_member(builder, "version", "builder.version") - require_record_string(builder_version, "builder.version") + builder <- record_member(record, "builder", "object") + record_member(builder, "path", "string", "builder.path") + version <- record_member(builder, "version", "string", "builder.version") # A string that is not a CmdStan version is the wrong shape, not an odd value. - if (!grepl("^[0-9]+\\.[0-9]+\\.[0-9]+(-rc[0-9]+)?$", builder_version)) { + if (!grepl(cmdstan_version_pattern, version)) { stop_build_record_field( "builder.version", "must be a CmdStan version such as \"2.39.0\"" ) } - require_record_string(record_member(record, "tbb_dir", "tbb_dir"), "tbb_dir") + record_member(record, "tbb_dir", "string") - untracked <- record_member( - record, "known_untracked_dependencies", "known_untracked_dependencies" - ) - require_record_array(untracked, "known_untracked_dependencies") + untracked <- record_member(record, "known_untracked_dependencies", "array") for (i in seq_along(untracked)) { field <- paste0("known_untracked_dependencies[[", i, "]]") - require_record_object(untracked[[i]], field) - kind <- record_member(untracked[[i]], "kind", paste0(field, ".kind")) - require_record_string(kind, paste0(field, ".kind")) + entry <- record_shape(untracked[[i]], "object", field) + kind <- record_member(entry, "kind", "string", paste0(field, ".kind")) if (!kind %in% c("make_local_include", "user_header_include")) { stop_build_record_field( paste0(field, ".kind"), "must be \"make_local_include\" or \"user_header_include\"" ) } - require_record_string( - record_member( - untracked[[i]], "detected_in", paste0(field, ".detected_in") - ), - paste0(field, ".detected_in") + record_member( + entry, "detected_in", "string", paste0(field, ".detected_in") ) } @@ -282,11 +232,11 @@ new_build_record <- function(request, reported_features, dependencies, artifact, #' #' Staged in the same directory and renamed into place so a reader never meets #' a half-written record. A failed rename warns, and the warning is suppressed -#' so that `warn = 2` cannot pre-empt the error below. `auto_unbox` writes a length-one vector as a JSON -#' scalar, which is why the schema holds every array as a list and every scalar -#' as a length-one vector: a one-element `included_files` still writes as an -#' array. Nothing is ever `NULL` or `NA`, since an unknown state is an absent -#' key. +#' so that `warn = 2` cannot pre-empt the error below. `auto_unbox` writes a +#' length-one vector as a JSON scalar, which is why the schema holds every +#' array as a list and every scalar as a length-one vector: a one-element +#' `included_files` still writes as an array. Nothing is ever `NULL` or `NA`, +#' since an unknown state is an absent key. #' #' @noRd write_build_record <- function(record, exe_file) { @@ -357,72 +307,35 @@ read_build_record <- function(exe_file) { #' Compare a recorded build against the current one #' #' Returns the name of every compared field whose value differs between the -#' two records, in the order the schema's comparison table lists them. Every -#' field is checked, and nothing stops at the first difference, so a caller -#' who changed more than one thing is told about all of them. +#' two records, in the order the design's table lists them. Every field is +#' checked, and nothing stops at the first difference, so a caller who changed +#' more than one thing is told about all of them. Each entry below extracts +#' the value a row compares, so the list is the table. #' #' @noRd compare_build_records <- function(recorded, current) { - differences <- character() - sort_by_name <- function(x) x[order(names(x))] - - if (!identical( - sort_by_name(recorded$request$cpp_options_supplied), - sort_by_name(current$request$cpp_options_supplied) - )) { - differences <- c(differences, "cpp_options") + sorted <- function(x) x[order(names(x))] + optional_dependency <- function(record, name, fields) { + record$dependencies[[name]][fields] } - - if (!identical( - recorded$request$stanc_options_supplied, - current$request$stanc_options_supplied - )) { - differences <- c(differences, "stanc_options") - } - - if (!identical(recorded$request$stanc_name, current$request$stanc_name)) { - differences <- c(differences, "stanc_name") - } - - if (!identical( - recorded$dependencies$stan_file$hash, - current$dependencies$stan_file$hash - )) { - differences <- c(differences, "stan_file") - } - - included_hashes <- function(record) { - vapply(record$dependencies$included_files, `[[`, character(1), "hash") - } - if (!identical(included_hashes(recorded), included_hashes(current))) { - differences <- c(differences, "included_files") - } - - recorded_header <- recorded$dependencies$user_header - current_header <- current$dependencies$user_header - header_differs <- is.null(recorded_header) != is.null(current_header) || ( - !is.null(recorded_header) && ( - !identical(recorded_header$hash, current_header$hash) || - !identical(recorded_header$built_from, current_header$built_from) - ) + compared <- list( + cpp_options = function(x) sorted(x$request$cpp_options_supplied), + stanc_options = function(x) x$request$stanc_options_supplied, + stanc_name = function(x) x$request$stanc_name, + stan_file = function(x) x$dependencies$stan_file$hash, + included_files = function(x) { + lapply(x$dependencies$included_files, `[[`, "hash") + }, + user_header = function(x) { + optional_dependency(x, "user_header", c("hash", "built_from")) + }, + make_local = function(x) optional_dependency(x, "make_local", "hash"), + builder = function(x) x$builder[c("path", "version")] ) - if (header_differs) { - differences <- c(differences, "user_header") - } - - recorded_local <- recorded$dependencies$make_local - current_local <- current$dependencies$make_local - local_differs <- is.null(recorded_local) != is.null(current_local) || ( - !is.null(recorded_local) && !identical(recorded_local$hash, current_local$hash) + differs <- vapply( + compared, + function(value) !identical(value(recorded), value(current)), + logical(1) ) - if (local_differs) { - differences <- c(differences, "make_local") - } - - if (!identical(recorded$builder$path, current$builder$path) || - !identical(recorded$builder$version, current$builder$version)) { - differences <- c(differences, "builder") - } - - differences + names(compared)[differs] } From 1d3ed2fce40a63877e8127d2badea378c78c2ddb Mon Sep 17 00:00:00 2001 From: jgabry Date: Thu, 10 Sep 2026 11:25:43 -0600 Subject: [PATCH 6/8] Compare the artifact hash and reject repeated names and fractional versions The comparison now has the artifact row the design's table gives it. The reader's hash check proves a record describes the executable beside it; only comparing the snapshot's hash to the on-disk record's catches another process replacing the executable with an equivalent build, which section 5 says to report as a replaced executable. A JSON object must have unique names. jsonlite writes a repeated name with a numeric suffix, so the writer accepted a record the reader then rejected. The integer check on format_version uses tol = 0, since checkmate's default tolerance let a fractional version through to the exact comparison and reported it as an unsupported format. Found in the Codex review of Stage 2. Part of #1258. --- R/build_record.R | 19 +++++++++++------- tests/testthat/test-build-record.R | 32 +++++++++++++++++++++++++++++- 2 files changed, 43 insertions(+), 8 deletions(-) diff --git a/R/build_record.R b/R/build_record.R index 034e1b618..c8e409682 100644 --- a/R/build_record.R +++ b/R/build_record.R @@ -34,10 +34,11 @@ hash_file <- function(path) { # schema ------------------------------------------------------------------ # Shapes are as jsonlite::fromJSON(simplifyVector = FALSE) returns them. An -# object is a named list, an array is an unnamed list, and a scalar is an atomic -# vector of length one that is not NA. +# object is a named list with no name repeated, an array is an unnamed list, and +# a scalar is an atomic vector of length one that is not NA. is_json_object <- function(x) { - is.list(x) && !is.null(names(x)) && all(nzchar(names(x))) + is.list(x) && !is.null(names(x)) && all(nzchar(names(x))) && + !anyDuplicated(names(x)) } is_json_array <- function(x) { @@ -112,7 +113,7 @@ validate_build_record <- function(record) { checkmate::assert_list(record, .var.name = "record") format_version <- record[["format_version"]] - if (!checkmate::test_int(format_version) || + if (!checkmate::test_int(format_version, tol = 0) || format_version != build_record_format_version) { stop_build_record_field( "format_version", paste0("must be ", build_record_format_version) @@ -206,6 +207,9 @@ validate_build_record <- function(record) { #' #' The one place a record is built. `format_version` comes first and the rest #' follow the schema's order, so the written JSON reads in that order too. +#' `request` arrives in the forms the record compares: `cpp_options_supplied` +#' as canonical Make assignments, one per name, and the two stanc option lists +#' as the argument vectors stanc receives. #' #' @noRd new_build_record <- function(request, reported_features, dependencies, artifact, @@ -235,8 +239,8 @@ new_build_record <- function(request, reported_features, dependencies, artifact, #' so that `warn = 2` cannot pre-empt the error below. `auto_unbox` writes a #' length-one vector as a JSON scalar, which is why the schema holds every #' array as a list and every scalar as a length-one vector: a one-element -#' `included_files` still writes as an array. Nothing is ever `NULL` or `NA`, -#' since an unknown state is an absent key. +#' `included_files` still writes as an array. No field the schema names is ever +#' `NULL` or `NA`, since an unknown state is an absent key. #' #' @noRd write_build_record <- function(record, exe_file) { @@ -279,7 +283,7 @@ read_build_record <- function(exe_file) { return(unreadable) } format_version <- record[["format_version"]] - if (!checkmate::test_int(format_version)) { + if (!checkmate::test_int(format_version, tol = 0)) { return(unreadable) } if (format_version != build_record_format_version) { @@ -330,6 +334,7 @@ compare_build_records <- function(recorded, current) { optional_dependency(x, "user_header", c("hash", "built_from")) }, make_local = function(x) optional_dependency(x, "make_local", "hash"), + artifact = function(x) x$artifact, builder = function(x) x$builder[c("path", "version")] ) differs <- vapply( diff --git a/tests/testthat/test-build-record.R b/tests/testthat/test-build-record.R index f900914fb..918133efa 100644 --- a/tests/testthat/test-build-record.R +++ b/tests/testthat/test-build-record.R @@ -140,6 +140,21 @@ test_that("a record in a format we do not read is checked on its version alone", expect_false("record" %in% names(result)) }) +test_that("a record whose format_version is not an integer is unreadable", { + exe <- local_fake_exe() + record <- example_record(exe) + record$format_version <- 1.5 + jsonlite::write_json( + record, build_record_path(exe), + auto_unbox = TRUE, pretty = TRUE, digits = NA + ) + + result <- read_build_record(exe) + expect_equal(result$reason, "unreadable") + expect_false("record" %in% names(result)) + expect_false("format_version" %in% names(result)) +}) + test_that("a record whose hash does not match the executable is a mismatch", { exe <- local_fake_exe() write_build_record(example_record(exe), exe) @@ -204,6 +219,14 @@ test_that("the validator names the field that fails", { fixed = TRUE ) + repeated_option <- base + repeated_option$request$cpp_options_supplied <- list( + STAN_THREADS = "false", STAN_THREADS = "true" + ) + expect_error( + rebuild(repeated_option), "`request.cpp_options_supplied`", fixed = TRUE + ) + unknown_kind <- base unknown_kind$known_untracked_dependencies <- list( list(kind = "mystery", detected_in = "make/local") @@ -332,6 +355,14 @@ test_that("a make_local present on only one side differs as make_local", { expect_equal(compare_build_records(recorded, current), "make_local") }) +test_that("a changed artifact differs as artifact", { + exe <- local_fake_exe() + recorded <- example_record(exe) + current <- example_record(exe) + current$artifact <- "deadbeef" + expect_equal(compare_build_records(recorded, current), "artifact") +}) + test_that("a changed builder version differs as builder", { exe <- local_fake_exe() recorded <- example_record(exe) @@ -403,6 +434,5 @@ test_that("differences outside the comparison table never count", { current$known_untracked_dependencies <- list( list(kind = "user_header_include", detected_in = "other.hpp") ) - current$artifact <- "deadbeef" expect_equal(compare_build_records(recorded, current), character(0)) }) From 1ff52e8c55c269e08ee4b371cbbae8509a32fe01 Mon Sep 17 00:00:00 2001 From: jgabry Date: Thu, 10 Sep 2026 11:25:43 -0600 Subject: [PATCH 7/8] Pin the dependency hash checks and the null feature rule The review deleted the hash check in record_dependency_entry() and the user header and make/local hash comparisons and every test still passed. One test each now fails when those go: a dependency missing its hash is unreadable, a feature written as null is unreadable, a changed header hash at an unchanged path and a changed make/local hash each name their field, and an artifact mismatch reports no format_version. Part of #1258. --- tests/testthat/test-build-record.R | 48 ++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/tests/testthat/test-build-record.R b/tests/testthat/test-build-record.R index 918133efa..ea5172b6b 100644 --- a/tests/testthat/test-build-record.R +++ b/tests/testthat/test-build-record.R @@ -124,6 +124,36 @@ test_that("a record with a field of the wrong type is unreadable", { expect_false("format_version" %in% names(result)) }) +test_that("a dependency missing its hash is unreadable", { + exe <- local_fake_exe() + record <- example_record(exe) + record$dependencies$stan_file$hash <- NULL + jsonlite::write_json( + record, build_record_path(exe), + auto_unbox = TRUE, pretty = TRUE, digits = NA + ) + + result <- read_build_record(exe) + expect_equal(result$reason, "unreadable") + expect_false("record" %in% names(result)) + expect_false("format_version" %in% names(result)) +}) + +test_that("a feature written as null is unreadable", { + exe <- local_fake_exe() + record <- example_record(exe) + record$reported_features$stan_threads <- NA + jsonlite::write_json( + record, build_record_path(exe), + auto_unbox = TRUE, pretty = TRUE, digits = NA + ) + + result <- read_build_record(exe) + expect_equal(result$reason, "unreadable") + expect_false("record" %in% names(result)) + expect_false("format_version" %in% names(result)) +}) + test_that("a record in a format we do not read is checked on its version alone", { exe <- local_fake_exe() record <- example_record(exe) @@ -163,6 +193,7 @@ test_that("a record whose hash does not match the executable is a mismatch", { result <- read_build_record(exe) expect_equal(result$reason, "artifact_mismatch") expect_false("record" %in% names(result)) + expect_false("format_version" %in% names(result)) }) test_that("reported features keep enabled, disabled and unknown apart", { @@ -347,6 +378,15 @@ test_that("a user_header with the same hash but a different built_from differs a expect_equal(compare_build_records(recorded, current), "user_header") }) +test_that("a changed user_header hash at the same built_from differs as user_header", { + exe <- local_fake_exe() + recorded <- example_record(exe) + current <- example_record(exe) + recorded$dependencies$user_header <- list(hash = "cccc", built_from = "header.hpp") + current$dependencies$user_header <- list(hash = "dddd", built_from = "header.hpp") + expect_equal(compare_build_records(recorded, current), "user_header") +}) + test_that("a make_local present on only one side differs as make_local", { exe <- local_fake_exe() recorded <- example_record(exe) @@ -355,6 +395,14 @@ test_that("a make_local present on only one side differs as make_local", { expect_equal(compare_build_records(recorded, current), "make_local") }) +test_that("a changed make_local hash differs as make_local", { + exe <- local_fake_exe() + recorded <- example_record(exe) + current <- example_record(exe) + current$dependencies$make_local$hash <- "ffff" + expect_equal(compare_build_records(recorded, current), "make_local") +}) + test_that("a changed artifact differs as artifact", { exe <- local_fake_exe() recorded <- example_record(exe) From a37ecd83f70d12a0c07b7f48ff8f3c5889d0516c Mon Sep 17 00:00:00 2001 From: jgabry Date: Thu, 10 Sep 2026 11:35:16 -0600 Subject: [PATCH 8/8] Check the record's own shape and pin the version tolerance The validator now runs the object shape test on the record itself, so a repeated top-level member is unreadable like a repeated member anywhere else. Before, the root only had to be a list. The test for a fractional format_version now uses 1.000000001, which the default integer tolerance accepts and the reader must still reject. Part of #1258. --- R/build_record.R | 2 +- tests/testthat/test-build-record.R | 16 +++++++++++++++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/R/build_record.R b/R/build_record.R index c8e409682..9e6497917 100644 --- a/R/build_record.R +++ b/R/build_record.R @@ -110,7 +110,7 @@ record_dependency_entry <- function(value, field) { #' #' @noRd validate_build_record <- function(record) { - checkmate::assert_list(record, .var.name = "record") + record_shape(record, "object", "record") format_version <- record[["format_version"]] if (!checkmate::test_int(format_version, tol = 0) || diff --git a/tests/testthat/test-build-record.R b/tests/testthat/test-build-record.R index ea5172b6b..982379eb2 100644 --- a/tests/testthat/test-build-record.R +++ b/tests/testthat/test-build-record.R @@ -124,6 +124,20 @@ test_that("a record with a field of the wrong type is unreadable", { expect_false("format_version" %in% names(result)) }) +test_that("a record repeating a member at its top level is unreadable", { + exe <- local_fake_exe() + json <- jsonlite::toJSON(example_record(exe), auto_unbox = TRUE, digits = NA) + writeLines( + sub("}$", ",\"artifact\":\"different\"}", json), + build_record_path(exe) + ) + + result <- read_build_record(exe) + expect_equal(result$reason, "unreadable") + expect_false("record" %in% names(result)) + expect_false("format_version" %in% names(result)) +}) + test_that("a dependency missing its hash is unreadable", { exe <- local_fake_exe() record <- example_record(exe) @@ -173,7 +187,7 @@ test_that("a record in a format we do not read is checked on its version alone", test_that("a record whose format_version is not an integer is unreadable", { exe <- local_fake_exe() record <- example_record(exe) - record$format_version <- 1.5 + record$format_version <- 1.000000001 jsonlite::write_json( record, build_record_path(exe), auto_unbox = TRUE, pretty = TRUE, digits = NA