fix(manifest): actionable errors when USCOGDATA_URL is unset or returns non-JSON

`cog_gov_search()` (and every other verb) used to fail with a cryptic
`jsonlite` lexical error when the package's placeholder default URL was
hit and the server returned an HTML welcome page that got cached as
`manifest.json`. Three guards added:

1. `.check_url_configured()` aborts with class `uscogdata_url_not_configured`
   when the resolved URL is empty or still contains the
   `REPLACE_WITH_SHARE_TOKEN` sentinel. Message names both
   `Sys.setenv(USCOGDATA_URL = ...)` and `options(uscogdata.url = ...)`
   remediations and points at the bundled fixture.
2. `.fetch_or_cache_manifest()` parses the response body before persisting
   it. Non-JSON payloads raise class `uscogdata_invalid_manifest` (URL,
   Content-Type, parse error) and never touch the on-disk cache.
3. Cache writes are atomic via a sibling tempfile + `file.rename`, and
   existing caches with non-JSON content are silently refetched instead
   of returning a parse error to the caller.

Local-path manifests that aren't valid JSON now surface the same
`uscogdata_invalid_manifest` class with file context.

NEWS.md

@@ -1,5 +1,27 @@
 # uscogdata 0.1.0 (development)
 
+## Clearer errors when `USCOGDATA_URL` is unconfigured or returns non-JSON
+
+* `cog_open()` now aborts with the `uscogdata_url_not_configured` error
+  class when the resolved corpus URL still contains the placeholder
+  `REPLACE_WITH_SHARE_TOKEN` sentinel (or is empty). The message lists both
+  remediation paths (`Sys.setenv(USCOGDATA_URL = ...)` and
+  `options(uscogdata.url = ...)`) and points at the bundled fixture for
+  offline testing. Previously the package proceeded to fetch the placeholder
+  URL, cached the resulting HTML welcome page, and failed downstream with a
+  cryptic `jsonlite` lexical-error.
+* `.fetch_or_cache_manifest()` now parses the HTTP response body before
+  persisting it. Non-JSON responses (login pages, 404 HTML) raise
+  `uscogdata_invalid_manifest` with the URL, Content-Type, and underlying
+  parse error — and never write to the on-disk cache.
+* Manifest cache writes are now atomic (write to `manifest.json.tmp.<pid>`
+  in `cache_dir`, then `file.rename` over the target), so an interrupted
+  fetch cannot replace a previously-good cache.
+* Existing caches with non-JSON content (poisoned by the prior code path)
+  are silently refetched instead of returning a parse error to the caller.
+* Local `USCOGDATA_URL` paths whose `manifest.json` is not valid JSON now
+  surface the same `uscogdata_invalid_manifest` class with file context.
+
 ## Per-capita denominators now use per-year Census F-33 population
 
 * `cog_spending()` and `cog_revenue()` previously divided all years' amounts

R/manifest.R

@@ -1,5 +1,66 @@
 # R/manifest.R
 
+# Sentinel substring baked into the placeholder default URL. If we see this
+# in the resolved URL, the user hasn't configured USCOGDATA_URL yet.
+.PLACEHOLDER_TOKEN <- "REPLACE_WITH_SHARE_TOKEN"
+
+#' Abort with actionable guidance when the resolved corpus URL is still the
+#' placeholder shipped with the package (or any URL containing the sentinel).
+#' Called from `cog_open()` before any I/O so users see a clear message
+#' instead of a downstream JSON parse error.
+#' @noRd
+.check_url_configured <- function(url) {
+  if (!is.character(url) || length(url) != 1L || !nzchar(url)) {
+    cli::cli_abort(c(
+      "USCOGDATA_URL is not configured.",
+      i = "Set the corpus location via one of:",
+      "*" = "{.code Sys.setenv(USCOGDATA_URL = \"<url-or-local-path>/\")}",
+      "*" = "{.code options(uscogdata.url = \"<url-or-local-path>/\")}",
+      i = "For an offline smoke test, use the bundled fixture: {.code system.file(\"extdata/fixture_corpus\", package = \"uscogdata\")}."
+    ), class = "uscogdata_url_not_configured")
+  }
+  if (grepl(.PLACEHOLDER_TOKEN, url, fixed = TRUE)) {
+    sentinel <- .PLACEHOLDER_TOKEN
+    cli::cli_abort(c(
+      "USCOGDATA_URL is not configured (placeholder URL detected).",
+      x = "Current value contains the sentinel {.val {sentinel}}: {.url {url}}",
+      i = "Set the corpus location via one of:",
+      "*" = "{.code Sys.setenv(USCOGDATA_URL = \"<url-or-local-path>/\")}",
+      "*" = "{.code options(uscogdata.url = \"<url-or-local-path>/\")}",
+      i = "For an offline smoke test, use the bundled fixture: {.code system.file(\"extdata/fixture_corpus\", package = \"uscogdata\")}.",
+      i = "For the live Civilytics corpus, request the Nextcloud share URL from the package maintainer."
+    ), class = "uscogdata_url_not_configured")
+  }
+  invisible(url)
+}
+
+#' Try to parse a JSON file. Returns parsed object on success, NULL on
+#' any parse failure (so callers can decide whether to refetch).
+#' @noRd
+.try_parse_manifest_file <- function(path) {
+  tryCatch(
+    jsonlite::fromJSON(path, simplifyVector = FALSE),
+    error = function(e) NULL
+  )
+}
+
+#' Abort with a clear, classified error when a manifest payload (string or
+#' file) cannot be parsed as JSON. Surfaces the URL, content-type if known,
+#' and the underlying parse error.
+#' @noRd
+.abort_invalid_manifest <- function(source, content_type = NA_character_, parse_error = NULL) {
+  ct <- if (is.na(content_type) || !nzchar(content_type)) "<unknown>" else content_type
+  pmsg <- if (is.null(parse_error)) "" else conditionMessage(parse_error)
+  cli::cli_abort(c(
+    "Corpus manifest is not valid JSON.",
+    x = "Source: {source}",
+    i = "Content-Type: {ct}",
+    i = "Likely causes: USCOGDATA_URL points at a login page, a 404 HTML page, or the wrong share; or the corpus has not been published yet.",
+    i = "Set USCOGDATA_URL to a directory (local path or HTTPS) that serves manifest.json directly.",
+    if (nzchar(pmsg)) c(">" = "Parse error: {pmsg}") else NULL
+  ), class = "uscogdata_invalid_manifest")
+}
+
 #' Fetch manifest.json from URL (or read from a local fixture path),
 #' cache locally, validate TTL.
 #' @noRd
@@ -11,23 +72,54 @@
     if (!file.exists(local_manifest)) {
       cli::cli_abort("Local fixture has no manifest.json at {local_manifest}")
     }
-    return(jsonlite::fromJSON(local_manifest, simplifyVector = FALSE))
+    return(tryCatch(
+      jsonlite::fromJSON(local_manifest, simplifyVector = FALSE),
+      error = function(e) .abort_invalid_manifest(source = local_manifest, parse_error = e)
+    ))
   }
 
   cache_path <- file.path(cache_dir, "manifest.json")
   ttl <- as.integer(.cfg("manifest_ttl_secs"))
 
-  needs_fetch <- !file.exists(cache_path) ||
-                 difftime(Sys.time(), file.info(cache_path)$mtime, units = "secs") > ttl
+  cache_fresh <- file.exists(cache_path) &&
+    difftime(Sys.time(), file.info(cache_path)$mtime, units = "secs") <= ttl
+
+  # Honor a fresh cache only if its contents still parse as JSON. A previous
+  # version of this package could write HTML directly into the cache; treat
+  # such poisoned caches as if they were missing so the next call recovers.
+  if (cache_fresh) {
+    parsed <- .try_parse_manifest_file(cache_path)
+    if (!is.null(parsed)) return(parsed)
+  }
 
-  if (needs_fetch) {
   resp <- httr2::request(paste0(url, "manifest.json")) |>
     httr2::req_error(is_error = function(r) httr2::resp_status(r) >= 400) |>
     httr2::req_perform()
-    writeLines(httr2::resp_body_string(resp), cache_path)
-  }
+  body <- httr2::resp_body_string(resp)
 
-  jsonlite::fromJSON(cache_path, simplifyVector = FALSE)
+  # Parse BEFORE persisting. If the server returned HTML / a login page /
+  # any non-JSON body with a 2xx status, we must not write it to the cache.
+  parsed <- tryCatch(
+    jsonlite::fromJSON(body, simplifyVector = FALSE),
+    error = function(e) {
+      ct <- tryCatch(httr2::resp_content_type(resp), error = function(e2) NA_character_)
+      .abort_invalid_manifest(
+        source = paste0(url, "manifest.json"),
+        content_type = ct,
+        parse_error = e
+      )
+    }
+  )
+
+  # Atomic write: tmp file alongside cache_path (same filesystem -> no EXDEV)
+  # then rename. Ensures a partial write or interrupted process never
+  # replaces a previously-good cache.
+  if (!dir.exists(cache_dir)) dir.create(cache_dir, recursive = TRUE)
+  tmp <- paste0(cache_path, ".tmp.", Sys.getpid())
+  on.exit(if (file.exists(tmp)) unlink(tmp), add = TRUE)
+  writeLines(body, tmp)
+  file.rename(tmp, cache_path)
+  parsed
 }
 
 #' @noRd

R/session.R

@@ -5,6 +5,7 @@
 #' @noRd
 cog_open <- function(url = .resolve_url(),
                      cache_dir = .resolve_cache_dir()) {
+  .check_url_configured(url)
   if (!dir.exists(cache_dir)) dir.create(cache_dir, recursive = TRUE)
 
   con <- DBI::dbConnect(duckdb::duckdb())

tests/testthat/test-manifest.R

@@ -0,0 +1,101 @@
+# tests/testthat/test-manifest.R
+#
+# Tests for the guards on .fetch_or_cache_manifest() and cog_open() that
+# protect users from silent failures when USCOGDATA_URL is misconfigured
+# or returns non-JSON content.
+
+test_that("cog_open aborts with actionable error when URL is the placeholder default", {
+  uscogdata:::cog_close()
+  on.exit(uscogdata:::cog_close(), add = TRUE)
+
+  placeholder <- "https://cloud.civilytics.org/s/REPLACE_WITH_SHARE_TOKEN/download/"
+  withr::with_envvar(c(USCOGDATA_URL = placeholder), {
+    expect_error(
+      uscogdata:::cog_open(),
+      class = "uscogdata_url_not_configured"
+    )
+  })
+})
+
+test_that("placeholder guard fires for any URL containing the sentinel token", {
+  uscogdata:::cog_close()
+  on.exit(uscogdata:::cog_close(), add = TRUE)
+
+  # Sentinel detection should be substring-based — covers any host that still
+  # has REPLACE_WITH_SHARE_TOKEN baked in (default or partial user edit).
+  withr::with_envvar(c(USCOGDATA_URL = "https://other.example/s/REPLACE_WITH_SHARE_TOKEN/x/"), {
+    expect_error(
+      uscogdata:::cog_open(),
+      class = "uscogdata_url_not_configured"
+    )
+  })
+})
+
+test_that("placeholder guard error names both env var and option as remediation", {
+  uscogdata:::cog_close()
+  on.exit(uscogdata:::cog_close(), add = TRUE)
+
+  placeholder <- "https://cloud.civilytics.org/s/REPLACE_WITH_SHARE_TOKEN/download/"
+  withr::with_envvar(c(USCOGDATA_URL = placeholder), {
+    msg <- tryCatch(uscogdata:::cog_open(), error = conditionMessage)
+    expect_match(msg, "USCOGDATA_URL", fixed = TRUE)
+    expect_match(msg, "uscogdata.url", fixed = TRUE)
+  })
+})
+
+test_that("local manifest containing HTML produces uscogdata_invalid_manifest, not raw parse error", {
+  uscogdata:::cog_close()
+  on.exit(uscogdata:::cog_close(), add = TRUE)
+
+  tmp <- withr::local_tempdir()
+  writeLines(
+    c("<html>", "  <head><title>Welcome to our server</title></head>", "</html>"),
+    file.path(tmp, "manifest.json")
+  )
+
+  withr::with_envvar(c(USCOGDATA_URL = paste0(tmp, "/")), {
+    err <- expect_error(
+      uscogdata:::cog_open(),
+      class = "uscogdata_invalid_manifest"
+    )
+    expect_match(conditionMessage(err), "manifest", ignore.case = TRUE)
+  })
+})
+
+test_that("remote manifest fetch does not poison cache when response is HTML", {
+  uscogdata:::cog_close()
+  on.exit(uscogdata:::cog_close(), add = TRUE)
+
+  tmp_cache <- withr::local_tempdir()
+  cache_path <- file.path(tmp_cache, "manifest.json")
+
+  # Pretend the cache already exists with stale-but-fresh-by-mtime HTML
+  # (simulating a previous poisoned write from the old behavior). When the
+  # fetcher sees invalid JSON in the cache, it must refetch rather than
+  # silently returning a parse error to the caller.
+  writeLines("<html>poisoned</html>", cache_path)
+  Sys.setFileTime(cache_path, Sys.time())  # ensure within TTL
+
+  # We don't have a live HTTP fixture here, so the refetch will fail at the
+  # network layer — but the failure should NOT be a jsonlite parse error on
+  # the cached HTML; it should be a network-level httr2 error. The cache
+  # file itself must remain untouched (no atomic-write half-states).
+  withr::with_envvar(
+    c(
+      USCOGDATA_URL = "https://invalid.localhost.uscogdata.test/",
+      USCOGDATA_CACHE_DIR = tmp_cache
+    ),
+    {
+      err <- tryCatch(uscogdata:::cog_open(), error = identity)
+      expect_s3_class(err, "error")
+      # Must not be a JSON lexical error on HTML.
+      expect_false(grepl("lexical error", conditionMessage(err), fixed = TRUE))
+    }
+  )
+
+  # Atomic write contract: no stray tmp files left behind in cache_dir.
+  expect_length(
+    list.files(tmp_cache, pattern = "manifest\\.json\\.tmp"),
+    0L
+  )
+})