Page dft_stac_fetch() to exhaustion (#51)

[NewGraphEnvironment]

Problem

dft_stac_fetch() called rstac::get_request() with no items_fetch(), so it read one page of STAC items and handed the partial collection straight to gdalcubes::stac_image_collection(). A wide AOI therefore built a raster with missing tiles — no error, no warning, plausible output. Because it scales with AOI size it would have surfaced first on the largest, most-published area rather than in a test. dft_stac_cube() has always paged correctly; fetch was simply never brought across.

Closes #51.

Measurement came before the design, and killed the design the issue proposed

The issue suggested "a hard error when a next link survives". Measured against Planetary Computer (io-lulc-annual-v02, packaged AOI, 2017–2023, ground truth 14 items):

limit=NULL raw_n=14 raw_next=FALSE | fetched_n=14 fetched_next=FALSE | matched=NULL
limit=1    raw_n=1  raw_next=TRUE  | fetched_n=14 fetched_next=TRUE  | matched=NULL
limit=3    raw_n=3  raw_next=TRUE  | fetched_n=14 fetched_next=TRUE  | matched=NULL
limit=500  raw_n=14 raw_next=FALSE | fetched_n=14 fetched_next=FALSE | matched=NULL

The next link survives a successful items_fetch(). Confirmed at source: rstac:::items_fetch.doc_items mutates only items$features and never items$links, so what returns is page 1's document with a concatenated feature list. That guard would have aborted every correctly-paged fetch — fail-toward-abort.

So the link is stripped rather than errored on. That is not cosmetic: left attached to attr(, "stac_items"), a caller re-running items_fetch() on it re-fetches pages 2..N into an already-complete feature list and silently duplicates them. The fix creates that hazard, so the fix closes it.

What landed

• stac_items_paged() — pages to exhaustion, then signs. Order is load-bearing: signing first leaves every item from page 2 onward unsigned. Extracted rather than inlined because the paging path is otherwise untestable offline (the next statement in dft_stac_fetch() is stac_image_collection() followed by real cube reads).
• Two completeness checks of deliberately different reach. Duplicate item ids — never skipped, and the only signal available on PC, which matters because stac_image_collection() drops duplicates behind a .pkgenv$debug-gated message so nothing downstream would ever report them. And items_matched() vs the count — PC sends no numberMatched, so it never executes there and is fixtured in tests rather than left as dead code.
• Keeps GET. The cube posts because ext_filter/CQL2 requires it, not for paging. Commented, with the trigger to switch if fetch ever gains intersects or a filter.
• attr(, "cache_key") (the issue's secondary ask), documented as per-call not per-year.

⚠️ Deliberate cache-format break — fetch caches rebuild once

No cache-key parameter changes with this fix. Without a break, a raster written from a truncated item set keeps being served by the file.exists() short-circuit — so the wide-AOI users the bug hit hardest would get no fix at all on upgrade, silently and permanently under force = FALSE. stac_cache_key() gains a salt; the frozen guardian moves 79f67b7b9dae → 2264b5dbef6e, re-frozen with the reason recorded, as its own comment demands.

Cost is a one-time re-fetch of small annual land-cover rasters — not the cube's multi-hour Sentinel-2 stream, which is why the cube chose a read-path check for its analogous problem and fetch can afford a key break. dft_stac_cube() caches are unaffected; stac_cube_cache_key() is a separate function and is untouched.

Verification

Every guard was checked by restoring the defect and confirming the suite goes red — baseline FAIL=0:

restored defect	FAIL
no paging (the reported bug)	2
sign before page	1
stale next not stripped	2
no duplicate-id check	1
no items_matched guard	1
cache salt removed	9
is.null instead of length guard	1
dft_stac_fetch() bypasses the helper	1
exact-case next strip	2

One test measured FAIL=0 and was rewritten. The wiring test originally asserted only that a stubbed stac_image_collection() was reached — which the old inline pipeline satisfies just as well, so it could not fail. Reading it would never have shown that. It now asserts on the item ids the stub received, with rstac::get_request() booby-trapped so a bypass errors differently and offline.

Also: interop against the real consumer (the links-rewritten doc_items still satisfies rstac::items_length() and builds a 4-image gdalcubes collection); lintr 1 lint against a 2-lint baseline, the remainder a confirmed false positive.

• Offline suite 476 pass
• DRIFT_TEST_NETWORK=true → 92 pass / 0 fail on this file against live Planetary Computer

What the code review changed

/code-check's three reviewers ran long but did deliver, and they are the reason this PR is worth reading. Between them they found fourteen mutations that passed a suite I had already verified with a nine-defect restoration harness. Two changed shipped behaviour:

• A change made earlier on this branch was reversed. I had widened the next-link strip to be case-insensitive, reasoning it was strictly safer. It is not: rstac:::items_next.doc_items selects with links(items, rel == "next"), which is case-sensitive (measured: next 1, NEXT 0, Next 0). So a NEXT link is inert to items_fetch() and cannot cause the duplicate the strip guards against. What its presence actually means is that rstac could not follow it and stopped after page one — the very truncation this PR fixes — which on PC nothing else can detect. The "safer" strip was deleting the only local evidence of a truncated fetch. It now warns and keeps.
• Deleting attr(, "cache_key") left the suite green. A documented v0.10.0 return element whose only assertion sat behind the network skip, i.e. absent from CI. Now covered offline.

Other blind spots closed, each re-measured in a worktree: the wiring mock discarded its arguments (so dft_stac_fetch() hardcoding sign_fn — making a documented argument a silent no-op — was invisible); every items_sign mock ignored sign_fn; my own "pin the default limit" test pinned the test helper's default rather than the function's; expect_error(paged(), "99") passed with the two user-facing numbers swapped; and expect_lt(truncated, big), labelled "the two answers must differ", could not fail.

Also from review: the items_matched() guard is now asymmetric — abort on fewer items than reported (truncation), warn on more, since pgstac/stac-fastapi can return an estimated numberMatched and aborting there would fail toward abort on a complete fetch.

⚠️ One commit in this branch's history does not parse

fc2e861 landed a R/dft_stac_fetch.R missing a comma and the cache salt, under a message claiming "458 pass". Cause: a review agent ran defect-restoration cycles in the shared checkout while the implementation session committed. bbb679a swept the repair in, so the tip has always been healthy and every commit from 019707a onward parses — but a git bisect through fc2e861 hits an unloadable package. I rebuilt the history to remove it and the force-push was declined, so it remains. Squash-merge and it never reaches main; if you merge-commit, it does.

The two guards that follow are worth keeping: a mutation harness runs in git worktree add --detach, never the live checkout, and a commit on a branch where one has run is verified by parsing the committed blob, not by git status being clean.

Two pre-existing defects found, deliberately not fixed here

Both out of scope, both worth issues, and they are the same finding:

• test-dft_stac_cube.R:62 — the cube's frozen cache-key guardian is red on main. Bisected to 90f9d93, the commit that introduced it: it has never been green. Not re-frozen here, because re-freezing an unexplained drift is exactly what its own comment forbids.
• drift has no CI workflow that runs the test suite. .github/workflows/ holds only pkgdown.yaml and update-citation-cff.yaml. That is why the above survived a release cycle — every CI run is green because nothing runs the tests.

This means acc3243 cuts v0.10.0 over a suite with one known-red test. Nothing is tagged yet, so that call is still open at merge time.

Relates to NewGraphEnvironment/sred-2025-2026#16

🤖 Generated with Claude Code

https://claude.ai/code/session_01W1utBhUeke24nQo5M97LUr