diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 9ec282b..e3ce504 100644 --- a/.claude-plugin/marketplace.json +++ b/.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": [ diff --git a/.claude-plugin/plugin.json b/.claude-plugin/plugin.json index 4c76681..9f025dc 100644 --- a/.claude-plugin/plugin.json +++ b/.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." } diff --git a/.github/ISSUE_TEMPLATE/proposal.md b/.github/ISSUE_TEMPLATE/proposal.md index 597ef3f..109de0b 100644 --- a/.github/ISSUE_TEMPLATE/proposal.md +++ b/.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 diff --git a/CLAUDE.md b/CLAUDE.md index 619caec..d80e2f8 100644 --- a/CLAUDE.md +++ b/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 diff --git a/NEWS.md b/NEWS.md index fdf5308..379258d 100644 --- a/NEWS.md +++ b/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. diff --git a/fixtures/SCORECARD.md b/fixtures/SCORECARD.md index 68ab86c..b18b6df 100644 --- a/fixtures/SCORECARD.md +++ b/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. diff --git a/fixtures/cases/invalid-utf8/expected.txt b/fixtures/cases/invalid-utf8/expected.txt new file mode 100644 index 0000000..0335af3 --- /dev/null +++ b/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 diff --git a/fixtures/cases/invalid-utf8/plant.R b/fixtures/cases/invalid-utf8/plant.R new file mode 100644 index 0000000..4c229ea --- /dev/null +++ b/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) diff --git a/fixtures/cases/invalid-utf8/setup.sh b/fixtures/cases/invalid-utf8/setup.sh new file mode 100644 index 0000000..9eb652f --- /dev/null +++ b/fixtures/cases/invalid-utf8/setup.sh @@ -0,0 +1 @@ +Rscript "$(dirname "$0")/plant.R" diff --git a/fixtures/expected/pkgreviewtest.md b/fixtures/expected/pkgreviewtest.md index c786066..ed0cbfb 100644 --- a/fixtures/expected/pkgreviewtest.md +++ b/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 diff --git a/skills/pkgreview-core/VERSION b/skills/pkgreview-core/VERSION index dc1e644..9c6d629 100644 --- a/skills/pkgreview-core/VERSION +++ b/skills/pkgreview-core/VERSION @@ -1 +1 @@ -1.6.0 +1.6.1 diff --git a/skills/pkgreview-core/check/pkgreview-check.R b/skills/pkgreview-core/check/pkgreview-check.R index f2ac705..dafe213 100644 --- a/skills/pkgreview-core/check/pkgreview-check.R +++ b/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))]