Release v1.6.1: check script finishes on invalid UTF-8

.claude-plugin/marketplace.json

@@ -5,13 +5,13 @@
   },
   "metadata": {
     "description": "The openwashdata package review skills: review-package, review-issue, create-next-issue, review-status, review-complete, review-upgrade, create-release, add-doi.",
-    "version": "1.6.0"
+    "version": "1.6.1"
   },
   "plugins": [
     {
       "name": "pkgreview",
       "description": "Review workflow for R data packages in registered organizations: issue-per-area review on dev, release and DOI skills, standard upgrades. The skills read shared references from skills/pkgreview-core.",
-      "version": "1.6.0",
+      "version": "1.6.1",
       "source": "./",
       "strict": false,
       "skills": [

.claude-plugin/plugin.json

@@ -1,5 +1,5 @@
 {
   "name": "pkgreview",
-  "version": "1.6.0",
+  "version": "1.6.1",
   "description": "Review workflow for R data packages in registered organizations (openwashdata, Global Health Engineering): issue-per-area review on dev, release and DOI skills, standard upgrades."
 }

.github/ISSUE_TEMPLATE/proposal.md

@@ -11,7 +11,15 @@ What is wrong or missing today, with the evidence (a release, a review, a file).
 
 ## Proposal
 
-The change, concretely: which files, which skills, which check lines. Add an Options section only when the choice is genuinely open; otherwise this section is the decision unless an objection arrives.
+The change, concretely: which skills, which check lines, which rules. Add an Options section only when the choice is genuinely open; otherwise this section is the decision unless an objection arrives.
+
+## Files affected
+
+One line per file or directory the change touches, so a reviewer sees the blast radius before any code exists.
+
+## Open questions
+
+What the proposal assumes or leaves undecided. Empty is a valid answer; a question here that turns into a deviation during implementation is answered in a comment on this issue.
 
 ## Tasks
 

CLAUDE.md

@@ -94,6 +94,11 @@ ships as Claude Code skills.
 - The release PR body lists `Closes #N` for every issue it lands (PRs
   here target `main`, so the keyword fires); no per-issue status or
   closing comments
+- When the implementation deviates from what the issue says (a
+  different mechanism, a value the issue did not name, a task left
+  out), record the deviation and its reason in one comment on that
+  issue before the PR opens; the issue stays the record of what was
+  decided, the comment of what was done differently
 - After the merge: `scripts/release.sh [version]` from `main`
 - No emojis and no em dashes in anything committed to this repo;
   `scripts/lint.sh` enforces this and the skill path references

NEWS.md

@@ -2,6 +2,15 @@
 
 One section per release. Skill, script, template, fixture, and process changes are recorded here; checklist wording changes are recorded in `docs/checklist-reconciliation.md` (repo rule 3), one row per item. Releases before 1.5.0 are described by their reconciliation sections and tag messages.
 
+# pkgreview 1.6.1
+
+Patch release for the check script (issue #86, found in the openwashdata sweep of 2026-09-23 and 2026-09-24). No checklist item changed.
+
+- Check script: a dataset, dictionary, or text file with invalid UTF-8 bytes no longer stops the script before the report is written. Strings whose bytes are not valid UTF-8 and that carry no latin1 declaration (Latin-1 text read without its encoding, as in gdho, wsabrazil, and washinvestments) are counted from the data as loaded, then replaced byte by byte with `?` in the copy the scans read (#86).
+- Check script: new advisory line per dataset, "No invalid UTF-8 strings in text data", with the affected columns and the number of rows in each. It maps to the existing data item "All text data is encoded in UTF-8; no encoding errors" and sits next to the existing UTF-8 line, which keeps naming every non-UTF-8 column, declared latin1 or not (#86).
+- Check script: text files (`README.md`, `CITATION.cff`, `_pkgdown.yml`, the workflow file, `data_processing.R`) are read through the same replacement, so a line with an invalid byte is matched like any other instead of being skipped with a warning; the header of a historical data file is read the same way in the git-history scan (#86).
+- Fixtures: case `fixtures/cases/invalid-utf8/` plants undeclared Latin-1 bytes in a copy of `pkgreviewtest` through `plant.R`, so no file with invalid bytes is tracked. The expected report for `pkgreviewtest` gains the one PASS line; the 19 FAIL + 1 FLAG mapping to D1 to D17 is unchanged (#86).
+
 # pkgreview 1.6.0
 
 Development workflow release (issues #72 to #80, filed 2026-09-06). No checklist item changed.

fixtures/SCORECARD.md

@@ -52,6 +52,8 @@ Note on D18 and the fixture layout (issue #52): `fixtures/pkgreviewtest/` cannot
 
 Note on the v1.5.0 lines (washr 1.1.0 and owdata reconciliation, issues #56 to #68): the fixture DESCRIPTION carries `X-schema.org-keywords` (the same four keywords as CITATION.cff), `X-schema.org-spatialCoverage`, and `X-schema.org-temporalCoverage`, so the keywords, coverage, and dictionary-schema advisory lines PASS and the README export-link line PASSes on the existing `## Download` links. Three lines report NOT RUN against `pkgreviewtest/`, and each is a not-applicable report, not a finding: the git-history scan (subdirectory, see D18), the `docs/`-tracked line (no pkgdown workflow; the missing workflow is the reviewer's required Website item, not a script finding), and the R-CMD-check dev-trigger line (no workflow file; that gap is D5's presence finding). The 17-finding count and the D1 to D17 mapping are unchanged.
 
+Note on the v1.6.1 line (issue #86): the check script reports invalid UTF-8 strings (bytes that are not valid UTF-8 and carry no latin1 declaration) on a line of their own, next to the existing UTF-8 line. Against `pkgreviewtest/` that line PASSes, because the `region` strings of D3 are declared latin1 and translate cleanly, so D3 stays a one-line finding and the count of 19 FAIL + 1 FLAG lines is unchanged. The FAIL is exercised by the case `fixtures/cases/invalid-utf8/`, which plants undeclared Latin-1 bytes in a copy of the fixture (dataset, dictionary, processing script) and expects a complete report.
+
 ## How to use this scorecard
 
 Run the full review workflow against `fixtures/pkgreviewtest` after every significant change to the checklists in `skills/pkgreview-core/references/checklists/` or to the review skills and commands.

fixtures/cases/invalid-utf8/expected.txt

@@ -0,0 +1,6 @@
+- [FAIL] (advisory) No invalid UTF-8 strings in text data [pkgreviewtest]: region (3 row(s)), waterSource (2 row(s)); the other lines of this report read a copy with the invalid bytes replaced by "?"
+- [FAIL] (advisory) All text data encoded in UTF-8 [pkgreviewtest]: non-UTF-8: region, waterSource
+- [FAIL] (advisory) Dictionary schema: five washr columns, UTF-8 without BOM, single-class variable_type: non-UTF-8 bytes in the file
+- [FAIL] (required) Dictionary descriptions present (no empty or placeholder): defective: status, users_count
+- [FLAG] (required) PII signal scan (never auto-certified; human sign-off required): suspicious columns: owner_phone
+- [FAIL] (advisory) No commented-out code in data_processing.R: 6 line(s): 27, 28, 29, 35, 36, 54

fixtures/cases/invalid-utf8/plant.R

@@ -0,0 +1,34 @@
+# Plants invalid UTF-8 in a copy of pkgreviewtest (openwashdata/pkgreview#86).
+# Run by setup.sh inside the copy. The bytes are Latin-1 text that was read
+# without declaring its encoding, the pattern behind the three crashes named
+# in the issue. They are built from raw values here so that no file with
+# invalid bytes is tracked in this repository.
+
+latin1_bytes <- function(...) rawToChar(as.raw(c(...)))
+geneve <- latin1_bytes(0x47, 0x65, 0x6e, 0xe8, 0x76, 0x65)        # "Geneve" with e grave
+zurich <- latin1_bytes(0x5a, 0xfc, 0x72, 0x69, 0x63, 0x68)        # "Zurich" with u umlaut
+protegee <- latin1_bytes(0x70, 0x72, 0x6f, 0x74, 0xe9, 0x67, 0xe9, 0x65)
+
+# 1. Dataset: three values in region, two in waterSource. Encoding() is
+#    "unknown" for all five, unlike the latin1-declared strings of defect D3.
+load("data/pkgreviewtest.rda")
+pkgreviewtest$region[c(2, 7, 11)] <- geneve
+pkgreviewtest$waterSource[c(3, 5)] <- paste("source", protegee)
+stopifnot(sum(!validUTF8(pkgreviewtest$waterSource)) == 2L,
+          all(Encoding(pkgreviewtest$region[c(2, 7, 11)]) == "unknown"))
+save(pkgreviewtest, file = "data/pkgreviewtest.rda")
+
+# 2. Dictionary: one description with the same kind of bytes.
+dict <- readLines("data-raw/dictionary.csv", warn = FALSE)
+i <- grep("Type of water source", dict, fixed = TRUE, useBytes = TRUE)
+stopifnot(length(i) == 1L)
+dict[i] <- paste0(dict[i], " (source ", protegee, ")")
+writeLines(dict, "data-raw/dictionary.csv", useBytes = TRUE)
+
+# 3. Processing script: one commented-out assignment, appended so that the
+#    line numbers of the existing block (defect D12) stay where they are.
+#    Before the fix the line was skipped with a warning, so the
+#    commented-out-code count missed it.
+con <- file("data-raw/data_processing.R", open = "ab")
+writeBin(charToRaw(paste0("# office <- \"", zurich, "\"\n")), con)
+close(con)

fixtures/cases/invalid-utf8/setup.sh

@@ -0,0 +1 @@
+Rscript "$(dirname "$0")/plant.R"

fixtures/expected/pkgreviewtest.md

@@ -3,7 +3,7 @@
 Package: `(normalized)`  
 Standard: mechanical subset of the pkgreview checklists  
 Organization profile: --org=openwashdata (openwashdata.md)  
-Result: 17 PASS, 19 FAIL (4 required-tier), 1 FLAG, 3 NOT RUN
+Result: 18 PASS, 19 FAIL (4 required-tier), 1 FLAG, 3 NOT RUN
 
 ## metadata
 
@@ -28,6 +28,7 @@ Result: 17 PASS, 19 FAIL (4 required-tier), 1 FLAG, 3 NOT RUN
 - [NOT RUN] (required) Git-history PII signal scan (text data files): package is a subdirectory of a larger git repository; the visible history is the enclosing repo's, not the package's. Run the scan against the package's own repository
 - [FAIL] (advisory) Missing values coded as NA, no sentinels [pkgreviewtest]: sentinel values in: users_count (4)
 - [FAIL] (advisory) All text data encoded in UTF-8 [pkgreviewtest]: non-UTF-8: region
+- [PASS] (advisory) No invalid UTF-8 strings in text data [pkgreviewtest]
 - [FAIL] (advisory) Date variables stored as Date class [pkgreviewtest]: not Date class: installation_date
 - [FAIL] (advisory) Categorical values consistent (no case-only variants) [pkgreviewtest]: status (5 variants, 2 after case-folding)
 - [FAIL] (advisory) Unique identifier `id` is unique [pkgreviewtest]: 1 duplicated value(s): WP-005

skills/pkgreview-core/VERSION

@@ -1 +1 @@
-1.6.0
+1.6.1

skills/pkgreview-core/check/pkgreview-check.R

@@ -105,7 +105,25 @@ add <- function(area, tier, status, check, detail = "") {
 
 path <- function(...) file.path(pkg, ...)
 has <- function(...) file.exists(path(...))
-read_lines_if <- function(...) if (has(...)) readLines(path(...), warn = FALSE) else character()
+
+# Invalid UTF-8 (openwashdata/pkgreview#86): a string whose bytes are not
+# valid UTF-8 and that is not declared latin1 stops the regex and case
+# functions below ("input string N is invalid"), so no report was written.
+# utf8_safe() replaces each invalid byte with "?" in the working copy the
+# scans read; the defect itself is counted from the original values and
+# reported in the data section. Strings declared latin1 (fixture defect
+# D3) translate cleanly and stay as they are.
+utf8_invalid <- function(x) !is.na(x) & Encoding(x) != "latin1" & !validUTF8(x)
+utf8_safe <- function(x) {
+  bad <- utf8_invalid(x)
+  if (any(bad)) {
+    fixed <- iconv(x[bad], "UTF-8", "UTF-8", sub = "?")
+    fixed[is.na(fixed)] <- "?"
+    x[bad] <- fixed
+  }
+  x
+}
+read_lines_if <- function(...) if (has(...)) utf8_safe(readLines(path(...), warn = FALSE)) else character()
 
 # ---------------------------------------------------------------------------
 # Load package data
@@ -122,6 +140,24 @@ for (f in rda_files) {
   if (ok) for (nm in ls(e)) datasets[[nm]] <- get(nm, envir = e)
 }
 
+# Count the invalid UTF-8 values per character column, then hand the scans
+# a copy with the invalid bytes replaced (#86).
+invalid_utf8 <- list()
+for (nm in names(datasets)) {
+  df <- datasets[[nm]]
+  n_bad <- integer()
+  if (is.data.frame(df)) {
+    for (col in names(df)) {
+      x <- df[[col]]
+      if (!is.character(x)) next
+      k <- sum(utf8_invalid(x))
+      if (k > 0) { n_bad[col] <- k; df[[col]] <- utf8_safe(x) }
+    }
+    datasets[[nm]] <- df
+  }
+  invalid_utf8[[nm]] <- n_bad
+}
+
 # ---------------------------------------------------------------------------
 # Area 1: metadata
 # ---------------------------------------------------------------------------
@@ -227,6 +263,9 @@ if (!file.exists(dict_path)) {
   add("data", "required", "FAIL", "data-raw/dictionary.csv present", "file missing")
 } else {
   dict <- read.csv(dict_path, stringsAsFactors = FALSE)
+  # Invalid bytes in the dictionary are reported by the schema line below
+  # from the raw file; the description and type checks read a safe copy (#86).
+  dict[] <- lapply(dict, function(x) if (is.character(x)) utf8_safe(x) else x)
   all_vars <- unique(unlist(lapply(datasets, names)))
   missing_vars <- setdiff(all_vars, dict$variable_name)
   add("data", "required",
@@ -248,7 +287,7 @@ if (!file.exists(dict_path)) {
   has_bom <- length(head_bytes) == 3L && identical(as.integer(head_bytes), c(0xEFL, 0xBBL, 0xBFL))
   dict_lines <- readLines(dict_path, warn = FALSE)
   bad_utf8 <- !all(validUTF8(dict_lines))
-  header <- if (length(dict_lines)) sub("^\ufeff", "", dict_lines[1]) else ""
+  header <- if (length(dict_lines)) sub("^\ufeff", "", utf8_safe(dict_lines[1])) else ""
   cols <- tryCatch(scan(text = header, what = "", sep = ",", quiet = TRUE, strip.white = FALSE),
                    error = function(e) character())
   types <- if ("variable_type" %in% names(dict)) as.character(dict$variable_type) else character()
@@ -356,10 +395,13 @@ if (!is_repo) {
         blob <- system2("git", c("-C", shQuote(pkg), "show",
                                  paste0(rev, ":", p)),
                         stdout = TRUE, stderr = FALSE)
+        # A header with invalid UTF-8 bytes would raise a warning here and
+        # end the scan of this file with no hit; read a safe copy (#86).
+        head1 <- if (length(blob)) utf8_safe(blob[1]) else ""
         if (length(blob) &&
-            (grepl(pii_name_re, blob[1], ignore.case = TRUE) ||
+            (grepl(pii_name_re, head1, ignore.case = TRUE) ||
              any(grepl("(^|,)(lat|latitude|lon|long|longitude|gps)($|,)",
-                       blob[1], ignore.case = TRUE))))
+                       head1, ignore.case = TRUE))))
           { hit <- TRUE; break }
       }
       hit
@@ -399,13 +441,26 @@ for (nm in names(datasets)) {
   enc_cols <- character()
   for (col in names(df)) {
     x <- df[[col]]
-    if (is.character(x) && (any(Encoding(x) == "latin1") || !all(validUTF8(x[!is.na(x)]))))
+    if (is.character(x) && (any(Encoding(x) == "latin1") || !all(validUTF8(x[!is.na(x)])) ||
+                            col %in% names(invalid_utf8[[nm]])))
       enc_cols <- c(enc_cols, col)
   }
   add("data", "advisory", if (length(enc_cols) == 0) "PASS" else "FAIL",
       lab("All text data encoded in UTF-8"),
       if (length(enc_cols)) paste("non-UTF-8:", paste(enc_cols, collapse = ", ")) else "")
 
+  # Invalid UTF-8 (#86): values whose bytes are not valid UTF-8 and that
+  # carry no latin1 declaration, counted from the data as loaded. This is
+  # the case that used to stop the script; the other lines of this report
+  # read a copy with the invalid bytes replaced by "?".
+  inv <- invalid_utf8[[nm]]
+  add("data", "advisory", if (length(inv) == 0) "PASS" else "FAIL",
+      lab("No invalid UTF-8 strings in text data"),
+      if (length(inv))
+        sprintf("%s; the other lines of this report read a copy with the invalid bytes replaced by \"?\"",
+                paste(sprintf("%s (%d row(s))", names(inv), inv), collapse = ", "))
+      else "")
+
   # Date columns stored as Date class
   date_cols <- names(df)[grepl("date", names(df), ignore.case = TRUE)]
   bad_dates <- date_cols[!vapply(df[date_cols], function(x) inherits(x, "Date"), logical(1))]