From 5bf7e45cfb3317fdcd87b4acad8798e16c652478 Mon Sep 17 00:00:00 2001 From: almac2022 Date: Thu, 27 Aug 2026 16:09:34 -0700 Subject: [PATCH 1/6] Update CLAUDE.md with soul conventions Sync conventions from soul/conventions/ for LLM-assisted development. Adds cartography and pkgdown-publishing; repairs the corrupted soul conventions marker. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01U9SAqmvFeENADk4rYcbtHS --- CLAUDE.md | 1590 ++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 1578 insertions(+), 12 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index aee3a25..6eefd81 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -118,7 +118,137 @@ Filed 2026-05-12: fish_passage_peace_2025_reporting#14 (Parsnip pilot from flood When porting a vignette to an appendix in fp_* reports, walk through the three stages and ask: is there an fp_template issue yet (stage 2), or are we going direct to a region report (stage 3)? Direct works for a pilot but means future regions re-prep from scratch. -<\!-- BEGIN SOUL CONVENTIONS — DO NOT EDIT BELOW THIS LINE --> + + + +# Cartography + +## Style Registry + +Use the `gq` package for all shared layer symbology. Never hardcode hex color values when a registry style exists. + +```r +library(gq) +reg <- gq_reg_main() # load once per script — 51+ layers +``` + +**Core pattern:** `reg$layers$lake`, `reg$layers$road`, `reg$layers$bec_zone`, etc. + +### Translators + +| Target | Simple layer | Classified layer | +|--------|-------------|-----------------| +| tmap | `gq_tmap_style(layer)` → `do.call(tm_polygons, ...)` | `gq_tmap_classes(layer)` → field, values, labels | +| mapgl | `gq_mapgl_style(layer)` → paint properties | `gq_mapgl_classes(layer)` → match expression | + +### Custom styles + +For project-specific layers not in the main registry, use a hand-curated CSV and merge: + +```r +reg <- gq_reg_merge(gq_reg_main(), gq_reg_custom("path/to/custom.csv")) +``` + +Install: `pak::pak("NewGraphEnvironment/gq")` + +## Map Targets + +| Output | Tool | When | +|--------|------|------| +| PDF / print figures | `tmap` v4 | Bookdown PDF, static reports | +| Interactive HTML | `mapgl` (MapLibre GL) | Bookdown gitbook, memos, web pages | +| QGIS project | Native QML | Field work, Mergin Maps | + +## Key Rules + +- **`sf_use_s2(FALSE)`** at top of every mapping script +- **Compute area BEFORE simplify** in SQL +- **No map title** — title belongs in the report caption +- **Legend over least-important terrain** — swap legend and logo sides when it reduces AOI occlusion. No fixed convention for which side. +- **Four-corner rule** — legend, logo, scale bar, keymap each get their own corner. Never stack two in the same quadrant. +- **Bbox must match canvas aspect ratio** — compute the ratio from geographic extents and page dimensions. Mismatch causes white space bands. +- **Consistent element-to-frame spacing** — all inset elements should have visually equal margins from the frame edge +- **Map fills to frame** — basemap extends edge-to-edge, no dead bands. Use near-zero `inner.margins` and `outer.margins`. +- **Suppress auto-legends** — build manual ones from registry values +- **ALL CAPS labels appear larger** — use title case for legend labels (gq `gq_tmap_classes()` handles this automatically via `to_title()` fallback) + +## Self-Review (after every render) + +Read the PNG and check before showing anyone. + +### Placement + +1. Correct polygon/study area shown? (verify source data, not just the bbox) +2. Map fills the page? (no white/black bands) +3. Keymap inside frame with spacing from edge? +4. No element overlap? (each in its own corner) +5. Legend over least-important terrain? +6. Consistent spacing across all elements? +7. Scale bar breaks appropriate for extent? + +### Does it communicate? + +Every check above is about **where elements sit**. A map can satisfy all seven +and still fail to say what it is about — so these are not optional extras, they +are the half of the review that the placement list structurally cannot reach. + +8. **Is every prominent feature in the legend?** Work the other direction from + the usual one: rank what draws the eye *in the rendered image*, then confirm + each of the top few appears in the legend. Building the legend from the layer + list instead answers "did I list my layers", which is a different question and + always says yes. +9. **Is the subject obvious to someone who has never seen this area?** An AOI + that renders identically to its surroundings is not delineated by a thin + boundary line — the reader has to be told where to look. Containment (a fill, + a dimmed exterior, a mask) is what does it. +10. **Does the symbology have a hierarchy, or is it flat?** If one class holds + the great majority of the features, it will dominate regardless of how + correct its size is. Ask what the map is *for* and de-emphasise or filter + accordingly — and say in the caption or prose that you did. +11. **Does the basemap earn its contrast cost?** A basemap that adds no readable + terrain is not neutral: it lowers the contrast of everything drawn over it. + Blend parameters that mute it into a flat field are worse than no basemap. +12. **Is the type sized for the width it is published at, not rendered at?** A + 7 in figure squeezed into a ~700 px column loses roughly 40% — text set at + `size = 0.5` for the render lands at a few pixels on the page. Check the + figure at its delivered width. + +### Why this half exists + +Added 2026-08-26 after gq's flagship vignette map was reported as passing all +seven placement checks and was, on being looked at, unreadable: 89% of its point +symbols were one modelled class, the basemap was a featureless grey field, the +AOI was indistinguishable from its surroundings, and the single most prominent +feature on the map — a bright red 397-feature habitat network — **was not in the +legend at all**, while the prose beneath the figure described its styling in +detail (gq#61). + +The seven checks had returned green, accurately. They were simply not asking. + +See the `cartography` skill for full reference: basemap blending, BC spatial data queries, label hierarchy, mapgl gotchas, and worked examples. + +## Land Cover Change + +Use [drift](https://github.com/NewGraphEnvironment/drift) and [flooded](https://github.com/NewGraphEnvironment/flooded) together for riparian land cover change analysis. flooded delineates floodplain extents from DEMs and stream networks; drift tracks what's changing inside them over time. + +**Pipeline:** + +```r +# 1. Delineate floodplain AOI (flooded) +valleys <- flooded::fl_valley_confine(dem, streams) + +# 2. Fetch, classify, summarize (drift) +rasters <- drift::dft_stac_fetch(aoi, source = "io-lulc", years = c(2017, 2020, 2023)) +classified <- drift::dft_rast_classify(rasters, source = "io-lulc") +summary <- drift::dft_rast_summarize(classified, unit = "ha") + +# 3. Interactive map with layer toggle +drift::dft_map_interactive(classified, aoi = aoi) +``` + +- Class colors come from drift's shipped class tables (IO LULC, ESA WorldCover) +- For production COGs on S3, `dft_map_interactive()` serves tiles via titiler — set `options(drift.titiler_url = "...")` +- See the [drift vignette](https://www.newgraphenvironment.com/drift/articles/neexdzii-kwa.html) for a worked example (Neexdzii Kwa floodplain, 2017-2023) # CI Monitoring @@ -153,6 +283,45 @@ Without this scan, post-merge workflow failures linger until someone (often the The skill watches workflows triggered by a fresh merge in real time — that's the targeted catch. This convention is the backstop for failures that landed when no one was watching (merges via web UI, scheduled triggers, manually-triggered workflows). +## A green run does not mean the site is current + +CI conclusion and published content are two different facts. Check the second one +directly when it matters — the deploy commit, not the run status: + +```bash +git fetch -q origin gh-pages && git log -1 --format='%s' FETCH_HEAD +# "Deploying to gh-pages from @ owner/repo@ 🚀" <- is your HEAD? +``` + +GitHub can create a workflow run minutes after the push that triggered it, and +out of order with a later push. Observed 2026-08-26 in `fly`: `7a7700c` built and +deployed at 17:21, then its own *parent* `be77eca` had its run created at 17:22:52 +— twelve minutes after that push — and deployed over it. Both runs green, `gh run +list` all success, published site one commit stale. + +Things that do **not** fix this, so don't reach for them: + +- `cancel-in-progress: true` — cancels an *overlapping* run. Here the runs never + overlapped (`created == started` on both, second created after first finished), + so there was nothing to cancel. +- A `concurrency:` group — the r-lib pkgdown template already sets one at the job + level (`group: pkgdown-${{ github.event_name != 'pull_request' || github.run_id }}`). + Grepping for a top-level `concurrency:` key misses it and invites a redundant + "fix". Serializing runs doesn't order events that arrive late. + +There is no workflow-side fix, because the reordering happens before the workflow +exists. The remedy is detection: check the deploy provenance, and re-dispatch +(`gh workflow run --ref main`) if it's behind. Harmless when the stale +commit changed nothing the site publishes — confirm via `.Rbuildignore` / `_pkgdown.yml` +rather than assuming. + +## Don't use `gh run watch` to wait + +It polls hard enough to trip GitHub's *secondary* rate limit, which `gh api +/rate_limit` does not report — every primary bucket reads full while calls return +403. Retrying extends it. Poll sparsely with `gh run view --json status,conclusion`, +and prefer `git fetch` over the REST API for anything git can answer. + # Code Check Conventions @@ -161,12 +330,141 @@ Add new checks here when a bug class is discovered — they compound over time. ## Shell Scripts +### A guard must not fail toward "skip" +- When a check decides whether to do something consequential (cut a tag, send a + mail, run a migration), work out which way it fails when the command inside it + errors. If the error path and the "nothing to do" path look the same, the + guard is indistinguishable from a working one right up until it silently eats + the action. +- `IF=$(some-cmd ...)` inside `[ -z "$IF" ]` is the usual shape: the command + aborts, stdout is empty, and empty reads as "nothing changed". **Assign first, + test the exit status, then test the value.** + ```bash + if OUT=$(git diff --name-only "$A".."$B" -- . "${EXCL[@]}" 2>/dev/null); then + [ -z "$OUT" ] && NOTHING_CHANGED=1 # only trust emptiness on success + fi + ``` +- Caught 2026-08-12 in soul's `gh-pr-merge` release gate: the diff aborted, the + empty output read as "nothing shipped", and a branch of five commits of real + package changes was classified as needing no release. +- **Test a guard against both known answers before shipping it.** One case that + should fire and one that should not. The draft above returned the same value + for both, which reading the code did not reveal. + +### An empty result set is not a pass — a loop over nothing exits 0 +- The same class one level up, and pointed the worse direction. Iterating a + result set makes "there was nothing to check" and "everything checked out" + produce **identical** output: the body never runs, nothing prints, exit 0. + Where a mis-fired guard silently skips an action, this silently makes an + affirmative claim of success. + ```bash + RUN_IDS=$(gh run list ... | jq '... | .databaseId') # empty when nothing dispatched + for RUN_ID in $RUN_IDS; do gh run watch "$RUN_ID" --exit-status; done + # -> zero iterations, exit 0, caller reports "all green" + ``` +- **Poll for the expected results to exist, then branch on empty explicitly.** + Absence of evidence has to be reported as absence, not as evidence. +- Caught 2026-08-26 in gq: GitHub never dispatched PR #56's workflows — + `gh pr checks` said "no checks reported" and the check-runs API returned + `total_count: 0`. The watch loop exited 0 having watched nothing. The same + workflows had fired correctly for PR #54 an hour earlier, so this is a + GitHub-side dispatch miss that can hit any repo at any time. Fixed in + `gh-pr-merge` step 10; verified against both a SHA with runs and a SHA without. +- Generalizes past CI: any "verify N things" loop where the list is *computed* + — files matched by a glob, rows returned by a query, hosts resolved from an + inventory. If zero is a possible answer, zero needs its own branch. +- **The mirror mistake: a boolean exit status collapses several distinct + outcomes into "not success".** `gh run watch --exit-status` is non-zero for + cancelled and skipped as well as failed, so a run GitHub *cancelled* gets + reported as a failure and sends someone to read a log that does not exist. + This is the safe direction — a false alarm rather than a false pass — but it + is still wrong, and crying wolf is how a guard stops being read. + ```bash + gh run watch "$RUN_ID" --interval 30 >/dev/null 2>&1 + case "$(gh run view "$RUN_ID" --json conclusion -q .conclusion)" in + success) ;; + cancelled|skipped) echo "⊘ superseded, not a failure" ;; + ""|null) echo "⚠ could not read conclusion" ;; # gh failed + *) echo "✗ failed" ;; + esac + ``` + Prefer branching on the **reported outcome** over a pass/fail exit code + wherever the tool exposes one. Caught 2026-08-26 immediately after shipping + the rule above: `r-lib`'s check workflow sets `cancel-in-progress: true`, so a + second push to main minutes after a merge legitimately cancels the first run. + +### git pathspec excludes: use the long form +- `:!path` is short-form magic, and git keeps parsing magic characters after the + `!`. A path starting with one aborts the whole command: + `:!_pkgdown.yml` → `fatal: Unimplemented pathspec magic '_'`. +- Use `:(exclude)path`. `:!./path` also works, but the long form says what it means. +- Anything building pathspecs from a file (`.Rbuildignore`, `.gitignore`) will + eventually meet a leading `_`, `(`, or `^`. + +### Reading a file line-by-line drops the last line without a trailing newline +- `while IFS= read -r line; do ...; done < file` skips a final line that has no + newline after it. Use `while IFS= read -r line || [ -n "$line" ]`. + +### Empty arrays under `set -u` on bash 3.2 +- macOS still ships bash **3.2**, where `"${ARR[@]}"` on an empty array is an + unbound-variable error under `set -u`. Guard with `[ ${#ARR[@]} -gt 0 ]` + before expanding. Scripts written and tested on Linux bash 5 hit this only on + a Mac, and only when the array happens to be empty. + ### Quoting - Variables in double-quoted strings containing single quotes break if value has `'` - `"echo '${VAR}'"` — if VAR contains `'`, shell syntax breaks - Use `printf '%s\n' "$VAR" | command` to pipe values safely - Heredocs: unquoted `< out.md <<'EOF' # prose safe, placeholder left literal + Project: __NAME__ + EOF + sed -i '' "s|__NAME__|$NAME|" out.md + ``` + - Detection is cheap and worth doing whenever prose went through an unquoted + heredoc: `grep -n ', ,\|(( ))\| |' file` finds the empty spans a swallowed + code span leaves behind. - Pass-through-ssh args: `printf '%q'` escapes per-arg so workload paths with spaces / quotes / metacharacters survive the local-shell → ssh-argv → remote-shell round-trip. Without it, `ssh host 'cmd' "$path"` joins args with spaces on remote and re-parses, losing argument boundaries. +- `git commit -m "$(cat <<'EOF' ... EOF)"` chokes on apostrophes in prose bodies in some contexts — the bash parser surfaces an unmatched-quote error even though heredoc bodies should be quote-neutral. Resilient default for multi-line commit messages: write the body to `/tmp/msg.txt` and use `git commit -F /tmp/msg.txt`. +- **The same trap has a silent variant: `Rscript -e` / `python -c` carrying backslash escapes.** The heredoc case above fails loudly, which costs a retry. Passing a regex inline does not: `\\b` reaches the interpreter mangled, so `grepl()` returns 0 matches against text it matches perfectly from a file. Nothing errors. Seen 2026-07-31 in rfp#93 — the 0 read as "my regex is wrong" and nearly triggered a rewrite of working code; the identical regex scored 4 matches the moment it ran from `/tmp/x.R`. + - Rule: anything carrying a regex, nested quotes or backslashes gets written to a file and run (`Rscript /tmp/x.R`). Inline `-e` is for trivial one-liners only. + - Diagnostic: when an inline command returns a surprising *result* rather than an error, suspect the quoting layer before the code, and re-run from a file to find out which is wrong. That one step separates a real bug from a shell artifact. + +### Merging stderr into stdout corrupts the stdout you are parsing +- `system2(cmd, stdout = TRUE, stderr = TRUE)` (and `2>&1` generally) interleaves + the two streams **without respecting line boundaries**, so a write on stderr + can land in the middle of a stdout line. If you are parsing that line, it fails + — not with a missing value, but with trailing garbage: + ``` + RFPVALUEMAPS {...,"chain":["finder","surveyor's chain"]}QObject::killTimer: Ti + ^ parse error here + ``` +- **It only shows up on a long line**, which is what makes it a latent trap: the + probe worked for a year against a 20-field payload and broke the first time it + met a 145-field one. Nothing about the change looks related. +- Fix: send stderr to a **file**, keep stdout clean, and read the file back only + when reporting a failure — so diagnostics are not lost: + ```r + err <- tempfile(); on.exit(unlink(err), add = TRUE) + out <- system2(cmd, args, stdout = TRUE, stderr = err) + # ... on failure: paste(utils::tail(readLines(err, warn = FALSE), 30), collapse = "\n") + ``` +- Anything chatty on stderr does this — Qt, GDAL, JVM warnings, progress bars. + Suspect it whenever a subprocess parser fails on *content* rather than on + absence. ### Heredoc precedence in pipelines - `cmd1 | cmd2 <" && pwd)"` - After moving scripts, verify `../` depth still resolves correctly - Usage comments should match actual script location +### Diagnose env/PATH problems in the shell that actually runs, not the ambient one +- Get ground truth **before** forming any theory: + `env -i HOME=$HOME TERM=$TERM bash -lc 'echo $PATH | tr ":" "\n" | nl'` + (swap in `zsh` to check the other side). Numbering shows ordering and + duplication in one read. +- **Claude Code runs bash regardless of the user's login shell**, so a PATH + measured from an agent shell says nothing about the terminal the user sees. + Establish which shell is interactive (`echo $0`, or the prompt style) before + opening any rc file. +- **The mutation is usually one level down from the obvious file.** A + `for file in ~/.{path,exports,aliases,extra}; do source "$file"; done` loop in + `.bash_profile` hides real `PATH=` assignments in files you never opened. Grep + every sourced file, not just the rc files. +- Caught 2026-08-19: a 39-entry PATH with 12 duplicates took **three** wrong + diagnoses — `.zprofile` (which did run `brew shellenv` five times, but the + interactive shell was bash, so it was irrelevant), then `.bashrc` sourcing + `.bash_profile`, then tmux inheriting a stale env. The cause was `~/.path` + hand-prepending what `brew shellenv` already sets, plus three directories that + no longer existed. One `env -i` run ended it. +- The same mistake closed an infra issue prematurely: MacPorts was removed and + verified **in bash**, while `.zprofile` kept exporting `/opt/local/bin` on + every zsh login for months. Verified in one shell, broken in the one that runs. + ### Silent Failures - `|| true` hides real errors — is the failure actually safe to ignore? - Empty variable before destructive operation (rm, destroy) — add guard: `[ -n "$VAR" ] || exit 1` - `grep` returning empty silently — downstream commands get empty input +### `cmd > file` truncates before `cmd` runs — a failed command leaves a poisoned empty file +- The shell creates/truncates the redirect target **before** the command executes. If the command then fails (times out, wrong arg, no network), you're left with a **zero-byte file** — not the absence of a file. `set -euo pipefail` does not save you: the truncation already happened before the command's non-zero exit fires. +- The trap springs on the *next* run when an **existence-only guard** treats that empty file as valid: `[ -f "$f" ] || cmd > "$f"` sees the file, skips regeneration forever, and every downstream reader silently consumes an empty value. For a secret/credential cache this reads as a confusing auth failure (empty header → `403`) with no obvious cause. +- Caught 2026-08 in cyclops#10: `op read "op://..." > ~/.config/newgraph/zotero-api-key` guarded by `[ -f ]` — a timed-out 1Password approval would have written an empty file that the guard then blessed permanently. +- Fix — three parts: + 1. Guard on **non-empty**, not existence: `[ -s "$f" ]`. + 2. Write **atomically** so a partial/failed run lands nothing: `cmd --out-file "$tmp" && chmod … && mv "$tmp" "$f"` (or `cmd > "$tmp" && mv`), with `trap 'rm -f "$tmp"' EXIT`. + 3. Prefer a tool's own `--out-file`/`-o` over `>` where it exists — the value never transits stdout, so `set -x`/`tee`/a pipeline can't capture it. + +### Empty is not unset — `VAR=` passes a presence check that `unset` fails +- A command-scoped assignment built from fallbacks, `VAR="${A:-${B:-}}" cmd ...`, sets `VAR` to the **empty string** when neither source is set. That is not the same as leaving it unset, and for a tool that branches on *presence* rather than truthiness it is worse than both. +- Measured 2026-07-31 (rfp#93): rasterio tests `"PROJ_LIB" in os.environ` — a membership test an empty string passes — then calls `set_proj_data_search_path("")`, suppressing its own bundled `proj_data`: + ``` + PROJ_LIB= rio warp ... -> Error: Cannot find proj.db + (unset) rio warp ... -> EPSG resolves normally + ``` + It surfaced as a missing-dependency error, not a quoting bug, and only on installs whose PROJ layout the caller could not introspect — so the fallback chain looked like the culprit. +- Same shape wherever presence is the test rather than value: Python `os.environ`, bash `[ -v VAR ]`, R `Sys.getenv(x, unset = NA)`. +- Fix — build the command as an array, add the assignment only when there is a value: + ```bash + cmd=("$TOOL") + if [ -n "${MY_VAR:-}" ]; then cmd=(env "REAL_VAR=$MY_VAR" "${cmd[@]}"); fi + "${cmd[@]}" ... + ``` +- Do not write `[ -n "$X" ] && arr=(...)` as a bare top-level list: under `set -e` a false test makes the list return non-zero and aborts the script. Use an explicit `if`. + +### Parallel writers sharing one output file interleave mid-record +- `xargs -P N ... >> shared_file` (or any fan-out where N processes append to the same fd/path) is only safe while each record fits in a single `write()`. O_APPEND makes individual `write()` calls atomic, but a large record (anything beyond pipe/stdio buffer size, ~64 KB) spans multiple writes — concurrent jobs interleave mid-record and corrupt the file. +- The trap is latent: small records never trip it, so the pattern looks proven until the first large payload arrives. Caught 2026-07-11 in rtj's `stac_register-pypgstac.sh` — 20 parallel `curl | jq -c` jobs appending STAC items to one NDJSON worked for every prior collection (KB-scale items), then 9 MB floodplain items interleaved and produced an orjson decode error ~864 KB into line 1. +- Fix pattern: each parallel job writes its own temp file (unique name, e.g. md5 of the input), concatenate after the fan-out completes: + ```bash + cat urls.txt | xargs -P 20 -I {} fetch_one.sh {} "$OUT_DIR" # each writes $OUT_DIR/.json + cat "$OUT_DIR"/*.json > combined.ndjson + ``` +- Pair with a count guard — parallel `curl` failures under xargs are also silent: `[ "$(wc -l < combined.ndjson)" -eq "$EXPECTED" ] || exit 1` before any downstream load. + +### `mktemp` template needs enough X's, and a failed `mktemp` leaves an empty var +- BSD/macOS `mktemp -d -t ` requires the template to contain at least 3 `X`s (`XXXXXX` is the safe default). Without them, mktemp errors to stderr (`too few X's in template`) and **prints nothing to stdout**. +- Pattern: `SCRATCH=$(mktemp -d -t aider-smoke) && cd "$SCRATCH" && `. When mktemp fails, `$SCRATCH=""`. `cd ""` is a no-op that **leaves you in the caller's cwd**. The destructive command (`rm`, `git init`, `git add+commit`) then runs in cwd instead of a throwaway tmpdir. +- Caught the hard way 2026-05-13: a Claude smoke test inside the rtj checkout did exactly this, accidentally committed a `demo.R` to the active feature branch, which then rode the squash-merge into rtj/main and had to be cleaned up post-merge. +- Fix patterns: + - Always use `XXXXXX` (6 X's) in the template: `mktemp -d -t aider-smoke.XXXXXX`. + - Guard the result: `SCRATCH=$(mktemp -d ...) || exit 1; [ -n "$SCRATCH" ] || exit 1`. + - Use `set -euo pipefail` so the failed command-substitution kills the script. + +### `cmd dir/*` dies on ARG_MAX at scale — and only after the expensive work succeeded + +- A glob expands to argv. 98k filenames is roughly 6 MB against a ~2 MB limit, so + `cat "$DIR"/*.json` fails with `argument list too long` — **after** whatever + produced those files already succeeded. Silent-after-success: the costly stage + worked and the cheap one threw it away. +- Caught 2026-07 in rtj#196: it killed a STAC registration following a completed + 80-minute download. +- Safe form — `find` batches under the limit itself: + ```bash + find "$DIR" -maxdepth 1 -name '*.json' -exec cat {} + > combined.ndjson + ``` +- The trap is latent, and it rides in on the fix for a different one: + per-file fan-out (see "Parallel writers sharing one output file interleave + mid-record" above) is correct, and it is exactly what produces the file count + that later blows argv. Small sets look proven for as long as you test on them. + +### A `curl` in a parallel fan-out needs `--max-time` + +- Without it, one hung connection pins a worker slot indefinitely. Since a fan-out + usually prints nothing until it finishes, a wedged pool and a slow pool look + identical from outside — there is no signal to distinguish "still working" from + "will never finish". +- Set `--max-time` on every per-URL fetch, and pair any silent multi-minute stage + with a periodic progress line (a file count is enough). Same reasoning as + `statement_timeout` on long DB work: the point is to fail loud rather than hang + quiet. + +### BSD vs GNU sed/grep portability (macOS hits this constantly) +- macOS ships BSD `sed`/`grep`. Linux CI/cloud-init hosts ship GNU. Snippets that work on one silently misbehave on the other. +- **`\+` and `\|` are GNU BRE extensions.** On BSD they're treated as literal `+` and `|`, so the regex still "matches" but matches nothing useful — leaving raw input unchanged. + - Symptom seen 2026-05-28: `sed 's/[^a-z0-9]\+/-/g'` on macOS left spaces in an issue-title slug, producing an invalid git branch name. + - Fix: use `sed -E` (POSIX ERE) so `+`, `|`, `?`, `(...)` all work without escapes on both flavors. The same regex becomes `sed -E 's/[^a-z0-9]+/-/g'`. +- **`s|pat|repl|` delimiter conflicts with `|` in alternation/replacement on BSD.** Pick a delimiter that does not appear in pattern or replacement (`#`, `,`, `:` are common choices). Compound `s|x|y|; s|^| /||` chains where the trailing `||` looks like an empty delimiter break on BSD sed even when GNU accepts them. +- **Don't parse `ls`.** BSD `ls` emits ANSI colour codes when stdout is a TTY *or* when `CLICOLOR_FORCE` is set in env (often by shell rc files), and the codes leak through pipes. Downstream `grep`/`sed` chokes on the embedded escapes (`[01;31m...[0m`). + - Use `find -maxdepth 1 -mindepth 1 -type d -exec basename {} \;` for directory listings, or `printf '%s\n' /*/` for a glob, or `for d in /*/; do basename "$d"; done`. +- **When writing a snippet you expect to ship in a `skills/` SKILL.md or any cloud-init runcmd**: it must be POSIX-portable. Default to `sed -E`, avoid `\+`/`\|`, and don't pipe `ls`. + +### `gh` CLI +- **`gh pr create` resolves branch from CWD, not `--repo`**. Specifying `--repo NewGraphEnvironment/X` does NOT switch branch resolution — the command still reads the current working directory's checked-out branch. To open a PR in repo X, `cd` into X's checkout first, or pass `--head ` explicitly. +- **`gh issue create` with heredoc bodies fails on prose containing special shell characters** (apostrophes, dollar signs, backticks). Use `--body-file /tmp/issue.md` instead — every project's `newgraph.md` convention specifies this; codified here for the underlying class. +- **Before `gh pr merge`, verify the branch is fully pushed.** `gh pr merge` merges the REMOTE branch — commits made locally but never pushed are silently excluded, so the PR merges "successfully" while `main` is missing work you know you committed. Check `git status -sb` shows no `ahead N` before merging (or that `git rev-list --count @{u}..HEAD` is 0). Worse: if you then delete the local branch (`--delete-branch`, or a follow-up `git branch -D`), the unpushed commits become **dangling** — recoverable via `git reflog` / `git fsck --lost-found` then `git cherry-pick`, but only if you notice they're missing. Caught twice 2026-07 in `floodplains`: PR #6 merged 1 of 3 branch commits (the drift#34 `changes_only` fix + a CLAUDE.md update were unpushed → stranded as danglers → recovered and re-merged via a follow-up PR); a second branch sat 4-ahead-unpushed at compact time. The same check belongs in the `gh-pr-merge` skill's pre-merge step. + ### Process Visibility - Secrets passed as command-line args are visible in `ps aux` - Use env files, stdin pipes, or temp files with `chmod 600` instead @@ -236,6 +653,19 @@ Add new checks here when a bug class is discovered — they compound over time. ``` - Guard with `test -s /root/.ssh/authorized_keys` to fail loudly if `cc_ssh` hasn't run before runcmd (rare race). +## Spatial CLIs (bcdata, ogr, gdal) + +### Negative coordinates get parsed as CLI options — every BC bbox hits this +- BC longitudes are all negative, so `--bounds -124.73 49.485 -124.595 49.565` fails with `Error: No such option: -1`. The parser sees a leading `-` and reads it as a flag. Affects click/argparse-based tools generally, not just bcdata. +- Use the **bracketed single-argument form with `=`**: `--bounds="[-124.73, 49.485, -124.595, 49.565]"`. The `=` keeps the value attached to the option, and the brackets keep it one token. A bare comma-joined string (`--bounds "-124.73,49.485,..."`) is not equivalent — it threw an unrelated traceback. +- Same class: any CLI taking negative numbers (elevation offsets, `--nodata -9999`, buffer distances). Reach for `--opt=value` by default rather than discovering it per-tool. + +### bcdata: an empty result raises AttributeError, it does not return an empty collection +- A bbox query matching nothing exits non-zero with `AttributeError: You are calling a geospatial method on the GeoDataFrame, but the active geometry column to use has not been set.` — geopandas complaining about an empty frame, several layers below the query. +- The trap: that reads as a broken query, not as "zero features," so a real and meaningful **absence** looks like tooling failure. Don't conclude a layer is unavailable from this error. +- **Prove absence before acting on it.** Re-run the same query against a wider bbox known to contain features; if that returns rows, the empty result is real data. Caught 2026-08-22 establishing that BC's FTEN trail layers are genuinely empty over an entire island — the wider-box control returned 851 features, which is what turned "the query is broken" into "the province has no trails here." +- Wrap counts defensively: `try: json.load(...)` around the parse, and treat the failure as `0 features` only after the wider-box control passes. + ## OpenTofu / Terraform ### State @@ -243,11 +673,28 @@ Add new checks here when a bug class is discovered — they compound over time. - Missing outputs that scripts need — add them to main.tf - Snapshot/image IDs in tfvars after deleting the snapshot — stale reference +### Duplicate module blocks across envs double-track global resources +- A module instantiated in two env dirs (e.g. `module "iam"` in both `env/prod` and `env/dev`) means account-global resources (IAM users, roles) can be tracked in BOTH local states. Removing the module block from one env turns its state copies into pending DESTROYS — which would delete the real resource out from under the other env. +- Caught 2026-07-18 (rtj#185): `env/dev` state secretly held `role_terraform_awshak` — the role every `role-assume.sh` apply depends on — and a config cleanup turned it into a planned destroy. +- Fix: `tofu state rm ''` in the env relinquishing ownership (no cloud change; auto-backs-up state), leaving exactly one owning env. Verify the resource survives (`aws iam get-role ...`). +- Review check: any plan that destroys resources in a shared/global-resource module → first confirm which OTHER env states track the same addresses (`grep env/*/terraform.tfstate` or check the remote backend keys). + ### Destructive Operations - Validate resource IDs before destroy: `[ -n "$ID" ] || exit 1` - `tofu destroy` without `-target` destroys everything including reserved IPs - Snapshot ID extraction by name: use `awk -v n="$NAME" '$2 == n {print $1}'` (exact match on column 2). `grep -F "$NAME"` is substring-match and can grab a stale snapshot whose name contains the new name as a substring. +### "Has been deleted" in plan output is not authoritative — verify against the cloud API first +- The AWS provider (5.x and some 6.x) has a known class of bug where a transient read error (false 404, regional-endpoint hiccup) is interpreted as "resource deleted outside of OpenTofu." The plan will show the resource and any children scheduled for destroy + recreate (`forces replacement` cascades through children that interpolate the parent's id/arn). +- If you didn't delete the resource and the plan says it's gone, **verify against the cloud API before applying**: `aws s3 head-bucket --bucket X`, `aws iam get-role --role-name X`, etc. A `tofu plan -refresh=true` re-run a moment later often reports "No changes." +- Caught 2026-05-14 in rtj env/prod for stac-era5-land: bucket fully intact (60 objects, 307 MB) but plan said deleted with 5 child resources "must be replaced." Apply would have clobbered the policy + lifecycle configs against the still-existing bucket. Recovery via `-target` on the unrelated resource being added (rtj#157 then codifies `lifecycle { prevent_destroy = true }` on the bucket + load-bearing children). +- **Belt-and-suspenders defense:** add `lifecycle { prevent_destroy = true }` to high-value resources (S3 buckets, RDS instances, anything irreplaceable) in their module. Tofu will refuse to plan a destroy until the lifecycle line itself is removed in config — converts the failure mode from "apply silently clobbers" into "plan errors with `Instance cannot be destroyed`." Don't apply it to count-based resources where `count: 1 → 0` is a legitimate transition. + +### Check IaC ownership before CLI-mutating cloud config +- Before changing bucket policies, lifecycle rules, IAM policies, etc. with the aws CLI, grep the Terraform modules for the resource. If tofu owns it, a CLI change is not "drift" — it is **reverted on the next apply** (silent rule deletion). `put-bucket-lifecycle-configuration` additionally REPLACES the whole config, so a CLI "add one rule" can also clobber tofu-owned rules immediately. +- Caught 2026-07-18 (water-temp-bc#23): a NoncurrentVersionExpiration rule was one `aws s3api put` away from being applied — rtj `modules/s3` owns `aws_s3_bucket_lifecycle_configuration`, so it would have first clobbered the IA-transition rule, then been reverted. Correct path was a module variable + `tofu apply` (rtj#187). +- Corollary: when a pipeline's write pattern evolves (append-only → rewrite-in-place), **re-audit the IAM verbs its role actually needs**. water-temp-bc's GHA role lacked `s3:DeleteObject`; the first compaction run half-applied a `sync --delete` and left the store with duplicate keys until manually repaired (rtj#147 reopened). Check for an existing module toggle first — `allow_delete` already existed. + ## DigitalOcean ### Snapshot disk-size constraint @@ -266,6 +713,16 @@ Add new checks here when a bug class is discovered — they compound over time. 3. A retry fallback in the wrapping shell script (`up.sh` style) that detects the 422 in tofu output and uses `doctl compute reserved-ip-action assign ` to recover. Tofu doesn't retry; it leaves state half-applied (assignment recorded but DO didn't actually attach). - **Snapshot-based spins are MORE prone to the race** than first-boot from blank Ubuntu (more startup events compete for the droplet's event queue). - **Audit existing modules:** `grep -L 'time_sleep' env/do/*//main.tf` finds modules missing the gate. As of 2026-05-02, openclaw and geoserv have no `time_sleep` — they will race eventually. +- **`depends_on` alone does not re-create the gate on a replace.** A `time_sleep` with `depends_on` but no `triggers` stays untouched in state when the droplet is replaced (`tofu apply -replace=...`), so the settle delay silently doesn't run and the reassignment races anyway. Verified empirically on OpenTofu 1.12.0. A *targeted destroy* does sweep dependents, so `tofu destroy -target=module.droplet` + `apply` is safe while `-replace` is not. Add `triggers = { droplet_id = module.droplet.id }` to close it, and prefer targeted destroy in any documented rebuild recipe. + +### SSH keys apply at droplet creation only — guard the ForceNew edit +- DO injects `ssh_key_ids` into `/root/.ssh/authorized_keys` **once, at first boot** (cloud-init's `cc_ssh`) and never revisits the list. A key registered after a droplet was built therefore never reaches it, no matter what tfvars says. Symptom: a machine that reaches freshly-built hosts fine is denied by an older one. +- `ssh_keys` is **`ForceNew: true`** in the DO provider (a `TypeSet`, so reordering is safe). "Just add the key to tfvars" therefore plans a **destroy/recreate of the running host** — and doesn't even grant the new machine access to the host it destroyed. On a production database or tile server that is a catastrophe dressed as a one-line fix. +- **Guard the shared droplet module** with `lifecycle { ignore_changes = [ssh_keys] }`. It is safe precisely because DO cannot apply the change anyway: the only possible effect of that diff is an unwanted replace. `ignore_changes` governs updates only — creates and replaces recompute from config, so a deliberate rebuild still picks up the current list. +- Document the tradeoff where operators hit it: after the guard, editing `ssh_key_ids` produces **no plan diff at all**, and a typo'd key ID surfaces at create rather than at plan. +- To authorize a machine on a running droplet, append its pubkey — with `printf '\n%s\n'`, never `printf '%s\n'`. If the remote `authorized_keys` lacks a trailing newline (common once anyone has appended by hand), a bare append concatenates onto the last line and invalidates **both** keys — locking you out via the procedure meant to prevent lockout. `ssh-copy-id` handles this correctly. +- Check *which* file. DO's injection targets root only. A non-root SSH user has keys only if that env's cloud-init explicitly copied them at first boot — and that copy is one-time, so appending to root's file later grants the non-root user nothing. +- Caught in rtj#193: one machine had no path to a production STAC host for months because its key was registered after the droplet was built, and the obvious remediation would have destroyed the host. ## Docker / Postgres @@ -281,6 +738,38 @@ Add new checks here when a bug class is discovered — they compound over time. - `ALTER DATABASE SET search_path TO ...` is a database-level setting **stored in the postgres data dir**. Wiped with `docker compose down -v`. Must be re-applied on every restore. - Codify in your restore script, not in cloud-init or compose env (those don't apply to db-level settings). +### `pkill ` does NOT cancel its Postgres query +- Killing the client (R, Python, psql) closes its connection. The libpq backend on the server keeps running the in-flight query until it finishes — **server-side orphan**. The orphaned backend holds whatever locks it had (table, view, advisory). Every later `DROP VIEW` / `LOCK TABLE` / `ALTER` on the same object blocks behind it indefinitely — *silent hangs* indistinguishable from a slow query. +- Caught 2026-05-25 in link#205: a `pkill`'d `wsg_run_one.R` left a `frs_network_features` SELECT running 1h45m; subsequent recomputes wedged on `DROP VIEW barriers_bt_access` for 1h08m before someone noticed. +- **Always terminate the server-side backend**, not just the client: + ```sql + SELECT pid, pg_terminate_backend(pid) + FROM pg_stat_activity + WHERE datname='' AND state='active' AND now()-query_start > interval '3 minutes' + AND pid <> pg_backend_pid(); + ``` + Then kill the client. Order matters when you don't know which side will block. + +### Set `statement_timeout` + `lock_timeout` on long DB ops +- Any long-running DB op from an R/Python/etc. client should set both at session start, ideally via env (`PGOPTIONS='-c statement_timeout=600000 -c lock_timeout=60000'`) or on the connection itself (`DBI::dbExecute(conn, "SET statement_timeout = '600000'")`). A runaway query then cancels server-side (no orphan); a blocked `DROP VIEW` gives up rather than wedging behind a zombie lock. Without it, silent hangs become indistinguishable from "still working" and you wait hours. +- Pick a generous-but-bounded timeout (10× expected query time). The point isn't tight enforcement — it's "fail loud instead of fail silent." + +### Function-as-join-predicate: index visibility depends on inlineability +- `JOIN b ON some_function(a.cols, b.cols)` — Postgres can only use the underlying indexes if `some_function` is `LANGUAGE sql` (inlineable). `plpgsql` functions are opaque and force per-row evaluation → seq scan / nested loop without indexes. Verify with `\df+ ` (look at `Language`) and `EXPLAIN` (look for the function body expanded into Filter / Index Cond). +- Caught in link#205 with `whse_basemapping.fwa_downstream` — it IS `LANGUAGE sql` + the planner did inline it; the symptom was elsewhere (see below). But if a function-based join is slow and the function is plpgsql, that's the first thing to look at. + +### Joining on a per-tenant key (e.g. `id_segment` per-WSG) against a multi-tenant table is cartesian +- `id_segment` in link's persist schema is unique *within* a WSG, not globally (link#203). `WHERE id_segment IN (SELECT id_segment FROM streams WHERE wsg=aoi)` against persist matches access rows from *every* WSG sharing those id_segment values → N(WSGs)× duplicates → PK violations downstream and 50× memory. +- Fix: filter by the full tenant key (`watershed_group_code = aoi`) when the table has it. Pattern: introspect via `information_schema.columns` at runtime and branch — the same function can serve a working schema (single tenant, no WSG col) and persist (multi-tenant, with WSG col). + +### View vs. real table changes the planner's join direction +- A `CREATE VIEW v AS SELECT * FROM big_table WHERE … ` carries no row-count statistics. Used as a join input, the planner may pick the other side (big) as the outer driver, blowing nested-loop cost ~1000× — the symptom looks like "the indexes aren't being used" but it's actually a wrong-direction nested loop. +- Caught in link#205: AOI-scoping streams via a `VIEW` left Postgres thinking the 26k FINA segments were as big as the 800k persist barriers; it picked barriers as outer; 71M estimated result rows; >10 min wall. +- Fix when AOI-scoping into a smaller dataset: **materialise as a real `CREATE TABLE` with indexes + `ANALYZE`**. The planner then sees the small row count and picks it as outer. Drop the table on `on.exit` if it's transient. + +### Two-statement DELETE/INSERT into a persist table is not atomic +- A "DELETE WHERE wsg='X'; INSERT …" pair into a persist table from an orchestration script: if the INSERT fails (e.g. duplicate key from a subtle JOIN bug), the DELETE already ran → **data loss for that WSG**. Wrap in a single transaction (`BEGIN; … ; COMMIT`) when the persist table is the only source of truth, so a failed INSERT rolls back the DELETE. (link#205 lost FINA's `streams_mapping_code` to this; the surrounding cheap-recompute orchestration in `wsg_recompute_one.R` should wrap both statements in a tx.) + ## Tailscale ### ACL "users" semantics @@ -307,8 +796,184 @@ Add new checks here when a bug class is discovered — they compound over time. - `printf '%q'` escapes values for shell safety - Temp files for secrets: create with `chmod 600`, delete after use +### Gitleaks pre-commit hook +Configuration patterns and false-positive handling for the `gitleaks` pre-commit hook (kdot's Brewfile ships `gitleaks` + `pre-commit`; cyclops standardizes the hook): +- **`.gitleaks.toml` schema in v8.30+**: top-level table is `[[allowlists]]` (PLURAL, array of tables). Each entry MUST include at least one of `commits` / `paths` / `regexes` / `stopwords`. The singular `[allowlist]` and `fingerprints = [...]` forms shown in older docs fail to validate. Use `paths` + `regexes` together for targeted file-and-content allowlists. Example in `soul/.gitleaks.toml`. +- **PEM marker regex spans multi-line**: gitleaks's `private-key` rule is `(?i)-----BEGIN...PRIVATE KEY-----[\s\S]*-----END...-----`. It matches across comment prefixes, blank lines, and code-fence boundaries. **Commenting out the markers does NOT neutralize the match.** Only fix in content is to omit the literal `-----BEGIN/END...-----` strings entirely and replace with prose ("Paste your private key here, preserving headers" etc.). See the `rtj` cypher `tfvars.example` precedent. +- **`curl-auth-header` rule false-positives on non-auth headers**: matches any `-H "X: Y"` shape, not just credential-bearing headers. Trips on docs with custom CORS or app-specific headers (e.g. `Zotero-Allowed-Request: true`). Fix: targeted `[[allowlists]]` with `paths` + `regexes`. Don't path-allowlist the whole file unless content is entirely safe. +- **`pre-commit install` legacy-hook handling**: running `pre-commit install` on a repo with an existing `.git/hooks/pre-commit` renames it to `.legacy` and keeps invoking it after framework hooks. No breakage, but means hook surface is split between `.pre-commit-config.yaml` and `.git/hooks/pre-commit.legacy`. For full visibility, migrate the legacy check into `.pre-commit-config.yaml` as a `local` hook so the whole hook surface is declared in one place. +- **AWS canonical example keys are allowlisted by default** (`AKIAIOSFODNN7EXAMPLE` etc.) — don't use those in test fixtures expecting a block. Use `ghp_`-shape PAT lookalikes or other non-allowlisted patterns for hook-trigger tests. + +### "Public bucket" ≠ listable: GetObject vs ListBucket +- A bucket policy granting only `s3:GetObject` on `bucket/*` makes exact-key fetches public but NOT listing — and dataset discovery (`arrow::open_dataset()`, duckdb globs, STAC `/vsicurl/` directory reads) requires `s3:ListBucket` on the **bucket ARN** (no `/*`; it's a bucket-level action). +- The breakage hides: anyone with ANY ambient AWS credentials lists fine, so "anonymous access works" goes unverified for years. Caught 2026-07-18 (water-temp-bc#23 → rtj#187): anonymous `open_dataset()` had never worked on a bucket whose whole purpose was credential-less querying. +- Review checks: for an open-data bucket, the policy needs BOTH statements (GetObject on `bucket/*`, ListBucket on `bucket`); acceptance-test anonymous access from a credential-stripped environment (`env -u AWS_ACCESS_KEY_ID ... AWS_CONFIG_FILE=/dev/null`). Note ListBucket makes the full key listing publicly enumerable — intended for open data, wrong for mixed-content buckets. + +## Spreadsheets + +### A stored value is not wrong just because the raw number looks wrong + +Before reporting that a spreadsheet value is off by a factor, check the cell's +**number format**. A cell formatted `0.0%` multiplies by 100 for display: stored +`0.028` renders as `2.8%`. Reading raw values with `readxl` and comparing them against +what the column header implies will make correct data look 100x wrong. + +- `tidyxl::xlsx_formats(path)$local$numFmt[cell$local_format_id]` gives the format. +- The header text is not the signal. A column headed `(%)` may legitimately store a + proportion, because the format supplies the percent. + +**Why:** this cost a full wrong turn in the fish data submission work — a formula +`AVERAGE(...)/100` was reported as a provincial template defect, a correction notice to +the ministry was drafted, and the "fix" would have shipped `280.0%` where `2.8%` was +meant. Caught only because a human opened the file and looked at it. + +### Verify PDF links from the annotations, not the extracted text + +`pdftotext` returns anchor text, not the href. A link whose anchor reads "here" leaves +no URL in the text layer, so grepping the text proves nothing either way. Extract the +annotation instead: + +```bash +qpdf --qdf --object-streams=disable in.pdf - | strings | grep -oE 'https?://[^ )>]*' +``` + +`pdftotext` also splits ligatures — "fish" comes out as " sh" — so a grep for any term +containing `fi`, `fl` or `ffi` can report a false absence. + ## R / Package Installation +### Read-back shape must match write-back shape + +A script that reads a file, transforms it, and writes it **back to the same path** is +idempotent only if the reader accepts the shape the writer produces. If it reads with +`col_names = FALSE` expecting raw input but writes a parsed frame with headers, the +second run parses its own output as data. + +The damage is worst when the file carries a join key. In the fish data pipeline a +pit-tag merge re-derived `rowid` every run and wrote it back; a second run would have +appended the same 53 tags again and renumbered the key joining tags to individual fish, +silently shifting five prior years of records. A type error was the only thing that had +prevented it. + +- Guard the merge on a natural key (`anti_join` on the id), not on run count. +- Write back only when there is something new. +- Test by running twice and diffing the file — `cmp` should report no change. + +### Moving prose into a code chunk hides it from tools that scan the document + +- Tools that scan an R Markdown document for prose — citation detection, + cross-references, spell-check, word counts — skip code chunks. Making a section + conditional by moving it into a `results='asis'` chunk therefore removes it from + everything that was reading it as prose, with no error. +- Caught 2026-08 in `template_permit_fish`: the move hid the section's `[@key]` + citations from `rbbt::bbt_detect_citations()`, and the next `bbt_write_bib()` + **overwrote `references.bib` with zero entries** — breaking citations in every + document sharing that Rmd, not just the one changed. The symptom is `(key?)` in + the rendered output, far from the edit that caused it. +- Fix for rbbt specifically: pass keys used inside chunks explicitly — + `bbt_write_bib(path, keys = union(bbt_detect_citations(), "the_key"))`. +- General rule: before moving content into a chunk, name what else was reading it + as prose. + +### `glue()` trims common leading whitespace +- `glue::glue()` strips the common indentation of its input, so a template whose + output must preserve exact indentation (XML, YAML, Makefiles, Python) comes + out subtly wrong — valid-looking, wrongly indented. +- For those blocks use a raw string with a `gsub()` placeholder instead of a + glue template. Seen in rfp's QML form builder, where the photo widget's XML + indentation has to survive verbatim. +- Related, and the opposite mistake: glue does **not** re-parse interpolated + values, so literal `{...}` inside a *value* is safe. Don't rewrite a working + generator to escape braces that were never a problem — probe it first. + +### `on.exit()` at a script's top level never fires +- `on.exit()` registers a handler on the *current frame*. At the top level of a + file run with `Rscript`, that frame is the global environment, which never + exits — so the handler is registered and then simply never called. +- It looks correct, and it is correct inside a function. The failure is silent + and, when the thing being cleaned up lives outside the repo, invisible to + `git status`: rfp accumulated six staging directories in `$HOME` before anyone + noticed, from two different scripts that both looked right. +- Use `withr::defer(cleanup, envir = globalenv())`, which registers a finalizer + that runs at session end. It prints `Ran 1/1 deferred expressions` — that line + in script output is the confirmation it worked, not noise. +- Probe rather than assume when checking this: a cleanup target inside + `tempdir()` is removed by R's own session cleanup regardless, so testing there + reports success for both the working and broken versions. + +### A `data-raw/` script must load the source tree, not the installed package +- `requireNamespace("pkg")` succeeds whenever **any** version is installed, so a + guard shaped like `if (!requireNamespace("pkg")) pkgload::load_all()` silently + runs against the installed one. A generation script operates on the source + tree by definition; reading a different copy of the package to do it is the + bug. +- The gap is routinely enormous and nobody notices, because nothing errors. + Measured in rfp: the installed package was **sixteen releases behind** the + working branch, with a lookup table missing a whole row and an internal + constant missing three entries. +- It fails quietly in both directions. One script iterated the stale lookup and + **skipped an item entirely**, reporting 11 where the source had 12. Another + generated two committed artifacts through a stale scan; those artifacts turned + out byte-identical when regenerated correctly, but only because the input data + happened not to exercise the missing entries — the same accident that let the + original bug ship. +- Fix: `pkgload::load_all(quiet = TRUE)` **unconditionally**, and call functions + unqualified. `pkg::` and `pkg:::` in a `data-raw/` script reach the installed + namespace and defeat the point. +- Check for it by asserting a count the script should cover: + `nrow(registry)` against items processed. A silent skip is invisible otherwise. + +### `lintr` also resolves against the installed package, not the source tree +- The same installed-vs-source trap as the `data-raw` case above, in a tool + where it reads as a code defect rather than a stale dependency. + `object_usage_linter` resolves a package-level object through the installed + namespace, so **every internal constant added on the current branch** is + reported as `no visible binding for global variable`. +- It is convincing because the surrounding constants resolve fine — they are in + the installed copy. Confirm before "fixing" anything: + ```r + exists(".my_new_constant", asNamespace("pkg")) # FALSE -> lint artifact + exists(".an_old_constant", asNamespace("pkg")) # TRUE + ``` + If the new one is absent from the installed namespace and the old one is + present, the warning clears on reinstall and there is nothing to change. +- Corollary for reading a lint report at all: **compare against the baseline + before treating a count as signal.** Lint the file as it stands at `HEAD` + (`git show HEAD:R/f.R > /tmp/f.R`) and diff the counts by linter. A file that + already carried 26 lints in the repo's prevailing style is not a file your + change made worse. +- And check whether the repo has a `.lintr` at all. Without one, `lint_package()` + runs the strict defaults, which disagree with tidyverse continuation-indent + style on essentially every wrapped call — hundreds of hits that are house + style, not defects. + +### Regenerated binaries churn git even when nothing changed +- Formats that embed a creation timestamp or other run-varying metadata produce + a different file on every rebuild. An unconditional write then puts a binary + diff in every commit, and a real change becomes invisible among the noise. +- GeoPackage is the live case: `gpkg_contents.last_change` made a ~100 KB file + churn on each rebuild of an unchanged form. +- **Write to a temp file, compare the things that actually matter, replace only + on a real difference.** Choose the comparison deliberately — for a GPKG that + is `PRAGMA table_info` **plus** geometry type **plus** CRS, because CRS lives + outside the column list and comparing columns alone silently keeps a stale + projection. Then a file appearing in the diff means something genuinely changed. +- Text artifacts that are byte-stable can just be rewritten every time; the + guard is only worth it where the format is not. + +### A drift guard must cover every input it claims to +- Guards that assert "nothing has been added without a decision" are only worth + their maintenance if they walk **all** the inputs. One that checks a subset + gives the same green signal while the uncovered part drifts freely. +- Enumerate the source of truth programmatically rather than listing what you + remember: walk the registry / schema / directory, diff it against the declared + set, and fail on anything in neither "handled" nor "deliberately ignored". +- Require a **reason** on every ignored entry. An ignored item without one is a + backlog note pretending to be a decision, and it gets re-litigated at every + review. +- Then prove the alarm can fire: feed it a deliberately undeclared input and + assert it is reported. A guard nobody has seen fail is decoration. + ### pak Behavior - pak stops on first unresolvable package — all subsequent packages are skipped - Removed CRAN packages (like `leaflet.extras`) must move to GitHub source @@ -318,8 +983,184 @@ Add new checks here when a bug class is discovered — they compound over time. - Branch pins (`pkg@branch`) are not reproducible — document why used - Pinned download URLs (RStudio .deb) go stale — document where to update +### `R CMD build` ships every top-level directory not in `.Rbuildignore` +- Internal coordination directories — `comms/`, `research/`, `planning/`, `dev/` — land in the tarball and therefore in the library of anyone installing from GitHub. `R CMD check` only flags this as a NOTE ("Non-standard files/directories found at top level"), which is easy to scroll past among the notes you have decided to live with. +- `.gitignore` does **not** cover this. A locally-gitignored file (e.g. `.aider.chat.history.md`) is still picked up by `R CMD build`. +- The gap appears over time rather than at scaffold: found 2026-07-31 in rfp, where `planning`, `.claude`, `CLAUDE.md` and `dev` were all excluded but `comms` and `research` — added later — were not. 10 files of cross-repo coordination notes were shipping. +- This matters most for the three-layer repo split (see `newgraph.md`): `comms/` is internal-by-definition, so a public-flipped package that ships it leaks exactly what the flip was meant to purge. +- Audit every R repo at once: + ```bash + for d in ~/Projects/repo/*/; do + [ -f "$d/DESCRIPTION" ] || continue + for sub in comms research planning dev; do + if [ -d "$d/$sub" ] && ! grep -qE "^\^${sub}\\\$" "$d/.Rbuildignore" 2>/dev/null; then + echo "$(basename "$d") ships $sub/" + fi + done + done + ``` + Run 2026-07-31: 20 hits across 16 repos. `comms/` in `link`, `fish_passage_template_reporting`, `neexdzii_kwa_benthic_2025`; `research/` in `link`; the rest `planning/` or `dev/`. +- Verify a fix against the tarball, not the config — the `.Rbuildignore` regex is easy to get subtly wrong: + ```bash + R CMD build . >/dev/null && tar tzf pkg_*.tar.gz | grep -c '^pkg/comms/' # expect 0 + ``` + +### Base name shadowing in formal args +- Avoid `names`, `length`, `data`, `c`, `t`, `T`, `F`, etc. as formal argument names. R's function-lookup fallback often rescues `names(x)` calls inside a function whose arg is also called `names` — but it's a confusing read, breaks under refactors, and generates a real "could not find function" error when the lookup heuristic misses (e.g. inside lapply/vapply/match.fun chains). Prefer descriptive alternatives: `label_names`, `n`, `df`, etc. +- Caught in mc#33 round 1 — `mc_label_ensure(names)` worked by luck when calling `names(existing)` to read a named-vector's names; renamed to `label_names` for safety. + +### Cross-function consistency for label/string normalization +- When two functions in the same package both decide whether a string is a "system value" (or any normalized form), they MUST use the same comparison. Mismatches are silent bugs that surface only on edge cases. +- mc#33 example: `mc_label_ensure` used `toupper(nm) %in% sys` (case-insensitive system-label skip), but `resolve_label_names` used `nm %in% sys` (case-sensitive). Result: `add = "inbox"` with `create_missing = TRUE` was silently broken — ensure skipped creation, resolve couldn't match. Fix: both use `toupper(nm) %in% sys` and the resolver normalizes its return to the canonical case. +- Generalized check: when reviewing a diff that adds normalization (case, whitespace, prefix-trim) on one side of an interaction, grep for the other side and align them. + +### Cache keys must cover every output-affecting input +- A file cache keyed by fewer inputs than the write depends on returns silently wrong data — the worst failure class: no error, plausible-looking output. Enumerate every parameter that changes the written artifact and put each in the key (or its hash). The safe failure direction is over-keying (spurious refetch), never under-keying. +- drift#25 example: `dft_stac_fetch()` cached STAC rasters as `/.nc` — no AOI in the key. A second watershed silently received the first watershed's raster masked to its own extent (~3% overlap looked plausible enough to almost ship). Fix: filename gains a hash over AOI geometry + `res`/`crs`/`dt`/`aggregation`/`resampling`/`stac_url`/`collection`/`asset`. +- Hash *resolved* values, not raw args: defaults filled from config (`%||%`) must resolve before hashing, or `f(x)` and `f(x, url = )` key differently for identical output. +- R hashing gotchas (`rlang::hash()` serializes, so type and attributes matter): + - sf geometry: hash WKB (`sf::st_as_binary(sf::st_geometry(x), endian = "little")`), not the sfc object — sfc carries a PROJ-generated CRS WKT that drifts across PROJ versions (spurious cache misses), and hashing a whole sf data.frame leaks attribute columns into the key. Pass the CRS string as a separate key member. + - Coerce numeric types: `10L` and `10` hash differently — `as.numeric()` before hashing. +- Check the cache's `force`/refresh escape hatch actually overwrites: drift#25's `force = TRUE` errored on the existing file ("File already exists"), broken exactly when needed. Prefer the writer's explicit `overwrite = TRUE` arg over a bare `unlink()` — unlink fails silently on Windows under an open file handle. + +### terra: operator dispatch and edge cases in package code +- **SpatRaster `%in%` is not dispatched when terra is *imported* (only when *attached*).** Inside a package (terra in `Imports`, used via `::`), `some_raster %in% vec` falls through to base `match()` and errors with `'match' requires vector arguments`. A `library(terra)` smoke test passes (attaching installs the S4 method), so the bug hides until package context. Use `terra::subst(x, from, to, others = ...)` or `terra::classify()` for code-set membership/masking instead of the `%in%` operator. Same trap for any operator terra defines via S4 that base also defines as an ordinary function. (drift#34) +- **`terra::freq()` errors on an all-NA raster** (`replacement has length zero`) rather than returning a 0-row table. Any path that can yield an all-NA layer (an impossible filter, everything masked out) must guard: `f <- tryCatch(terra::freq(r), error = function(e) NULL)`, then treat `NULL`/0 rows as "no values". Don't assume the empty case gives `nrow(freq(r)) == 0`. (drift#34) +- **`terra::minmax()` reports *cached* statistics, not computed ones.** It defaults to `compute = FALSE` and returns `Inf`/`-Inf` for any raster whose min/max have never been calculated — which is every file-backed raster until something touches it. A guard written on top of it therefore fires on real data: + ```r + r <- terra::rast("a_richly_varied_image.png") + terra::hasMinMax(r) # FALSE FALSE FALSE FALSE + terra::minmax(r) # min Inf ... / max -Inf ... + terra::minmax(r, compute = TRUE) # min 0 0 0 0 / max 11 18 18 255 + ``` +- The trap is that it *appears* to work, because plenty of upstream operations compute min/max as a side effect — `terra::crop()` does, so anything arriving via `maptiles::get_tiles(crop = TRUE)` has them. Correct by accident, through an internal that is not a contract. Pass `compute = TRUE`, and test the guard against a **file-backed** fixture: one built by `rast(vals = ...)` is in memory, has statistics cached, and cannot reach this. (gq#57, 2026-08 — a flat-tile detector called every file-backed raster flat, and the whole fixture set shared the one property that hid it.) + +### sf: `st_join(largest = TRUE)` ignores the join predicate +- `sf::st_join(x, y, join = predicate, largest = TRUE)` does **not** use `predicate` to decide matches — with `largest = TRUE`, sf runs `st_intersection(x, y)` and keeps the feature of greatest overlap area, so matching is *always* intersection-based regardless of what `join =` is set to. A function that exposes a configurable predicate AND a largest-overlap mode therefore silently mis-attributes when both are combined: pass `st_within` expecting containment, get anything that merely *overlaps*. Verify against sf source, not the argument list — the `join` arg is accepted and ignored, not rejected. Fix: abort when a non-default predicate is combined with the largest-overlap mode, rather than honouring one and dropping the other. (drift#42) +- Corollary: `largest = TRUE` also drops zero-area geometries from consideration — so a predicate join against **point** or **line** overlays cannot use largest mode at all (no area to compare). Point/line attribution must go through the plain (`largest = FALSE`) predicate path. + +### sf: name validation must account for the geometry column +- The active geometry column is a named entry in `names(x)`, but its name is **not fixed** — `"geometry"` from `sf::st_read()` of some sources, `"geom"` from a GeoPackage/PostGIS layer, `"geometry"` or `"_ogr_geometry_"` elsewhere. Code that validates user-supplied column names with `cols %in% names(x)` will happily accept the geometry column, then break downstream (`st_join` drops `y`'s geometry, so a requested "attribute" column silently never appears; a 0-row short-circuit path may instead attach a stray empty sfc). A same-name collision check across two sf objects also misses this when the two layers name their geometry differently. Guard explicitly with `attr(x, "sf_column")` — reject it from the caller-supplied column set. (drift#42) + +### sf: reproject the polygon to get a lat/lon bbox, never transform the projected bbox corners +- To hand a geographic (EPSG:4326) bounding box to a bbox-filtered query (WFS/OGC features, `?bbox=`), reproject the whole AOI **geometry** then take its bbox: `sf::st_bbox(sf::st_transform(aoi, 4326))`. Do **not** compute the bbox in the projected CRS and transform its two corner points — a projected rectangle's edges bow under reprojection, so the corner-transformed box is skewed and generally too short on one axis. The pre-filter then silently under-covers the true extent: features inside the AOI but outside the shrunken box are never fetched, and a downstream clip can only *remove*, never recover them. Symptom: counts a few percent low near the north/south extremes of an area, with no error. A native-CRS bbox filter (e.g. ogr2ogr `-spat -spat_srs EPSG:3005`) is unaffected — only the reproject-the-corners step is the bug. (rfp#12) + +### arrow dplyr backend: no grouped slice — bridge to duckdb +- arrow's dplyr backend errors on grouped `slice_max`/`slice_min` (`arrow_not_supported("Slicing grouped data")`). The working pattern for any "latest per group" over parquet/S3: `arrow::open_dataset(...) |> dplyr::filter(...) |> arrow::to_duckdb() |> dplyr::group_by(...) |> dplyr::slice_max(...)`. +- The `to_duckdb()` bridge is also a return-type contract: helpers that return the lazy query should keep the bridge even when they no longer need it internally, or downstream callers using grouped verbs break. (water-temp-bc#17, #23) + +### as.POSIXct.Date silently ignores tz= +- `as.POSIXct(x, tz = "UTC")` on a `Date` ignores `tz` and converts in the system local zone — west of UTC this shifts date boundaries by the local offset and silently drops edge data. Force UTC via `as.POSIXct(format(x), tz = "UTC")` when accepting Date inputs; widen Date upper bounds to `< next-day-midnight` so the whole calendar day is included. (water-temp-bc#17) + +### as.POSIXct on character infers ONE format for the whole vector +- `as.POSIXct(x)` on a character vector picks a single format by finding the first candidate that parses **every** element — and `strptime` **ignores trailing characters**. So one coarse value silently truncates the entire column, and nothing warns: + ```r + as.POSIXct(c("2026-08-15 18:33:46", "2026-08-15 18:34:20", "2026-08-16")) + #> all three at 00:00:00 <- the times are gone + ``` + One minute-precision value does the same to its neighbours' seconds. Order-independent, and the values are not `NA` afterwards, so an `is.na()` guard on the result cannot see it. +- Same family as the `Date` case above, and worse: that one shifts by a known offset, this one destroys information. +- Fix: match each value's **shape** with an anchored regex, then parse it with the format that shape implies — per element, not per vector. Anchoring at both ends is what turns trailing junk into an error instead of a silent truncation. +- `tryCatch` around the whole call is not a fix either. `as.POSIXct.character` **throws** on an unrecognised string rather than returning `NA`, so a catch-all handler that blanks the vector then makes the "which value failed?" report name element one — usually a perfectly good timestamp. Compute the failing set per element inside the error path. +- Caught 2026-08-24 in crate#9. Three bugs in one parse (this, a dropped `+02` offset, and the misleading error), all silent, all with the suite green at 171 passing. + +### An offset regex must be anchored to a time, or a date looks like a zone +- Refusing or stripping a trailing UTC offset with something like `[+-][0-9]{2}(:?[0-9]{2})?$` also matches the end of a plain ISO date: `"2026-08-15"` ends in `-15`, which reads as a −15 hour zone. Require the offset to follow `HH:MM[:SS[.fff]]`. +- The mirror mistake is requiring four offset digits. `±hh` is valid ISO 8601 and is what Postgres emits for whole-hour zones; a two-digit-offset value then falls through the guard, gets stripped as trailing junk, and the instant moves by hours with nothing reported. + +### `paste0()` treats a zero-length argument as `""` +- `paste0(character(0), "x")` returns `"x"` — length **one**, not zero. So a composite key built from an empty data frame yields one phantom row rather than none: + ```r + paste0(df$a, "\x1f", df$b) # nrow(df) == 0 -> "\x1f" + ``` +- Downstream that reads as a real record. Caught 2026-08-24 in trap#14: an empty annotation table produced one key, which the join then reported as "an annotation matching no session". Guard with an explicit `if (!nrow(x)) return(character(0))`. +- Same shape for any vectorised builder fed a possibly-empty frame — `sprintf()`, `file.path()`, `interaction()`. + +### open_dataset(unify_schemas = TRUE) requires aligned types +- Cross-prefix/file schema unification only merges what types allow: `timestamp[us, tz=UTC]` will not merge with naked `timestamp[us]`, `Grade: string` not with `Grade: double`. Audit the schemas of every file group BEFORE promising unified reads over a mixed archive; plan a normalization pass otherwise. (water-temp-bc#17) + +### duckdb larger-than-memory dedup: shard the work — settings won't save you +- duckdb's **window operator** (QUALIFY row_number ...) does not spill enough to survive big partitions (OOM'd an 8 GB limit on a ~124M-row input). The **arg_max/struct-payload hash aggregate** cannot spill its state either (observed OOM with an empty temp dir). `preserve_insertion_order = false` and fewer threads help but do not fix it. +- **In-memory duckdb connections never offload to disk at all** — `SET temp_directory` on `dbConnect(duckdb())` is a no-op for operator spill. File-backed (`dbdir = `) is required for any spilling. +- The structure that works at any scale: **hash-shard by a column inside the group key** (e.g. `hash(STATION_NUMBER) % K = k`, K = `ceiling(input_rows / shard_rows)`), one aggregation pass per shard, each writing its own ordered output file. A key never crosses shards, so dedup stays exact; memory scales 1/K. Extra passes cost scan time only — per-pass aggregate state is what OOMs, so when in doubt shard smaller. (water-temp-bc#23) +- **Local runs at the same duckdb `memory_limit`/`threads` do NOT validate a constrained runner.** 10M-row shards passed a Mac at the exact 4 GB / 2-thread settings but OOM'd the real 7 GB GHA runner (partition 46 squeaked through in 94s, 47 died 15s in) — abundant physical RAM masks how tight duckdb's accounting runs at its internal limit. Only the real runner is the real test; size shards with margin (water-temp-bc ships 6M), and treat a near-timeout/near-limit pass as a failure to fix, not a pass. (water-temp-bc#23 run 29675228557, fixed in PR #25) + +### `nzchar(NA)` is TRUE — non-empty checks silently pass NA +- `nzchar(NA)` returns `TRUE`, so the natural "is this cell filled in" test — `all(nzchar(trimws(x)))` — waves through a column full of `NA`. `trimws(NA)` is `NA`, and `nzchar()` of that is `TRUE` unless you pass `keepNA = TRUE`. +- Use an explicit guard: `filled <- function(x) !is.na(x) & nzchar(trimws(x))`. Same trap in reverse for `read.csv()`, which yields `""` for an empty field but `NA` for a literal `NA` — so a file can fail one check and pass the other for the same visual blank. +- Bites hardest in validators, where the whole point is catching a half-authored row. (link#233, 2026-08: a dictionary contract test asserting every row carried a description would have passed on an entirely NA column.) + +### Test fixtures must mirror production column TYPES, not just shapes +- A fixture-green suite can hide type bugs that only real data exposes: water-temp-bc#23's fixtures had `Grade` as string when production has double, so a `coalesce(Grade, '')` sentinel inside the dedup ordering passed all 27 tests and broke on first contact with real data. +- When writing fixtures for a pipeline over an existing dataset, print the real schema (`arrow::open_dataset(...)$schema`) and copy the types verbatim. Any type-sensitive expression (coalesce sentinels, casts, comparisons) is only tested if the fixture types match. + +### CSV whitespace: `trim_ws` and `strip.white` do not do what the name suggests + +- `readr::read_csv()` defaults to **`trim_ws = TRUE`** and silently strips leading + and trailing whitespace. Where whitespace is *meaningful* — a QGIS layer name + deliberately prefixed with a space so it sorts first — a trimmed value binds to + nothing, with no error. Use base `utils::read.csv()`, or pass + `trim_ws = FALSE`. +- `read.csv(strip.white = TRUE)` applies **only to unquoted fields**, and + `write.csv()` quotes every character column. So a round-trip guard that + compares `read.csv()` against `read.csv(strip.white = TRUE)` is *structurally + incapable of failing* — both readers return the same thing, and the check + passes for nothing. +- The second point is the trap: the guard looks right, runs green, and proves + nothing. Probing for the real failure mode is what surfaces the `readr` one. + Caught 2026-08 in rfp#174, where five leading-space layer names were at stake. + +### `R CMD check` rejects a filename containing a space + +- "checking for portable file names" fails on any file in the built package + whose name has a space. It is an ERROR, not a NOTE, so CI goes red. +- Bites when shipped files are named after human-readable strings — layer names, + form labels, report titles. 40 of 50 in one case, one of which *began* with a + space. +- Fix: derive a slug for the filename and keep the real name in an index CSV + beside it. Resolve through the index, never by reconstructing a path from the + display string. + +### Do not edit files a long test run is reading + +- `devtools::test()` (and most runners) load each test file **when they reach + it**, not at launch. A 30-minute run therefore reads whatever is on disk at + that moment, so edits made while it runs are half-applied and the result + describes a tree that never existed. +- The tell is a **changing pass count** across runs of "the same" tree — + 3490, then 3496, then 3500. A moving denominator means the input was moving. +- Cost 2026-08 in rfp#178: two full Docker suites (~1 hour) both reported + `FAIL 1`, and the failure was a test written *during* the run, executing + against source from *before* the fix that made it pass. It was nearly reported + as a regression. +- **Commit before a long run.** While it runs, do work that touches nothing it + reads — issue bodies, PR text, planning. And when a long run fails, get the + `file:line` before forming any theory: a mid-flight edit and a real regression + look identical in a summary line. + ## General +### Two agent sessions must not share one git working tree — give each a worktree + +- A git working tree has exactly **one** checked-out branch. When two concurrent Claude sessions operate in the same directory, either can `git checkout` out from under the other **mid-edit**. The victim's uncommitted work stays on disk but is now sitting on the *other* session's branch — so a later `git add`/`commit` silently lands it on the wrong branch, and a `--delete-branch` merge can strand it entirely. +- Symptoms: an `Edit` fails with "File does not exist" for a file you just wrote (their branch doesn't have it); `git branch --show-current` returns a branch you never created; your new files show as untracked on someone else's feature branch; `planning/active/` suddenly empty. +- Caught three times in one session (2026-07, floodplains): twice mid-implementation, and once while running a `--public-clean` scrub — the scrub committed to a parallel session's feature branch instead of `main`, which would have flipped the repo public with an **un-scrubbed `main`**. That third one is the dangerous class: the safety work (`.claude/visibility`, stripped internal conventions) sat on a branch nobody was about to merge. +- **Prevention:** one worktree per session — `git worktree add ../- -b `. Each session gets its own directory and its own checked-out branch; no contention. +- **Detection (cheap; do it before any commit, merge, or visibility flip):** assert the branch is what you think it is, not just that the tree is clean. + ```bash + [ "$(git branch --show-current)" = "$EXPECTED_BRANCH" ] || { echo "WRONG BRANCH"; exit 1; } + ``` +- **Recovery:** back up the touched files first (`cp` to a scratch dir, `git diff > x.diff`), confirm the other branch's changes don't overlap yours (`git diff --name-only main..their-branch`), then `git checkout ` — uncommitted changes carry across cleanly when there is no overlap. Commit **and push** immediately; an unpushed branch is what gets stranded. If you already committed onto their branch, restore their pointer with `git branch -f ` (your commit stays reachable via reflog). +- **Recovery when their branch is already pushed, with an open PR:** do **not** rewrite it — `git branch -f` plus a force-push into a PR another session is working in trades your problem for theirs. Cherry-pick forward instead, through a throwaway worktree so their checkout is never disturbed: + ```bash + git worktree add -q /tmp/repo-main main + git -C /tmp/repo-main cherry-pick + git -C /tmp/repo-main push origin main + git worktree remove /tmp/repo-main + ``` + Their PR now carries a commit whose content is already on main. That is harmless — git sees identical changes on both sides and merges cleanly — and verifiable before you rely on it: `git diff origin/main -- ` on their branch should be empty. The cost is one duplicated commit message in the log, which is cheaper than a contested force-push. +- **The moment to use a worktree is when you are about to touch a second repo**, not after something goes wrong. Observed 2026-08-26 in gq#57: a fix in the primary repo needed a matching change in `soul`, and `soul`'s shared checkout had meanwhile been switched to a parallel session's feature branch. The commit landed in their open PR silently — `git push` reported success, because it was a perfectly valid push to a branch nobody had said was wrong. + ### Adopting Existing Config When importing config from one location into a canonical one (legacy `~/.bash_profile` → dotfiles repo, old script's env → repo, another project's `settings.json` → soul): @@ -329,6 +1170,276 @@ When importing config from one location into a canonical one (legacy `~/.bash_pr - **Ask before dropping a reference** — it may be something the user forgot to reinstall on this machine, not something to delete. - **Curated subset, not verbatim copy.** The diff should reflect what you verified, not the whole source. +### Test the cold/create path of idempotent code, not just the warm no-op +- Idempotent provisioning code (a resolver-file writer, a config installer, a "create unless present" block) has two paths: the **cold** path that actually creates/writes, and the **warm** path that detects "already present" and skips. They exercise almost-disjoint code. +- Testing only on a host where the artifact already exists hits **only the warm no-op** — which cannot catch any cold-path bug: missing-directory, a derivation that returns empty, a pipefail abort before the write, wrong permissions, a flush that never runs. The warm path's job is literally to do nothing, so a green warm test proves almost nothing about onboarding. +- Every fresh host runs the **cold** path — that's the one onboarding depends on. Test it deliberately: back up + remove the artifact, run cold, assert it was created correctly, then re-run to confirm the warm no-op. (Caught 2026-06-23 on rtj#75: the resolver-writer's first test plan only ran the warm path on a host that already had `/etc/resolver/`; a Plan-agent review flagged that the cold path — the one every new host takes — was untested. Fixed by `sudo rm`-ing the file and running cold before close.) +- Generalizes beyond shell: any "ensure X exists / converge to desired state" operation — Terraform resources, migrations, package installs — wants the from-absent path tested, not just the already-converged re-run. + +### A valid response is not a correct one — services fail in the shape of success +- An external service can answer **HTTP 200 with a structurally perfect payload that is not the thing you asked for**: a placeholder image, an empty-but-well-formed JSON envelope, a "your trial expired" page served as the resource. Every cheap assertion passes — status code, content type, dimensions, CRS, band count, schema — because the shape is right and only the *meaning* is wrong. +- This defeats the guard you already wrote. A fetch wrapper that returns `NULL` on failure never fires, because nothing failed. So the absence of an error is not evidence, and neither is a green suite: the artifact has to be **looked at**, or compared against something that knows what it should contain. +- Measured 2026-08 in gq#57: Carto made their basemaps key-only and began serving an "API KEY REQUIRED" watermark image. It rendered through a vignette build, `R CMD check`, and a pkgdown deploy onto the public web, watermark and all. Found by a human asking how good the maps were, which meant opening the PNG. +- **Do not reach for a content detector without measuring whether one can work.** The obvious fix — score the pixels, sniff the body — is often provably impossible, and shipping it is worse than shipping nothing because it *looks* like a check. Same measurement: the *watermarked* tile had **fewer** dark pixels (0.0068) than the *clean* one (0.0073), because the watermark is a small share of content and ordinary detail swamps it. No threshold separates them. +- What does work: + - **Prefer providers/endpoints that cannot enter the degraded state** (keyless where key-only is the failure; a pinned version where "latest" can drift). + - **Detect the degenerate cases that are actually separable**, and only those. A single-colour image, a zero-row response, an empty archive — cheap, and no false negatives on the case you can measure. + - **A canary that runs on a human's machine**, not in CI, asserting the live service still returns something real. CI can only tell you the code still runs. +- Note which direction each guard fails in, and prefer warning over discarding when a legitimate input is indistinguishable from a broken one — the cost of an unread warning is far below the cost of destroying valid data. + +### An inventory is only complete relative to a boundary — name the boundary +- "I enumerated every call site" is a claim about a **search scope**, not about the world. A `grep -rn` over one repo is complete for that repo and says nothing about the copy of the same snippet living in a docs site, a house skill, a template, a wiki page, or another team's codebase. The enumeration can be flawless and the fix still incomplete. +- The tell is when the thing being changed is a **pattern people copy** rather than a function people call. Anything that has ever been pasted into documentation has an unbounded number of call sites, and the repo boundary is exactly where the search stops being meaningful. +- Ask directly: *what do the downstream users actually read?* Often it is not the API docs. If the answer is a skill file, a README, or an onboarding doc, that file is a call site and belongs in the sweep. +- (gq#57, 2026-08: the provider inventory was complete within gq — 9 lines, 6 files, verified twice. The consumer projects read `soul/skills/cartography`, which shipped its own hand-rolled snippet naming the broken provider and never called gq's function at all. Fixing gq alone would have left every downstream repo pointed at the watermark. Caught by a reviewer asking what consumers read, not by the grep.) + +### Do not write to an artifact a human is testing on + +- Handing someone a deployed thing to test — a synced project, a staging + database, a preview build — and then continuing to push changes into it makes + two writers for one artifact. The tester chases versions, and any client-side + lock or "another process is running" error that follows is **yours**, not + theirs to debug. +- It also corrupts the evidence. When the tester reports a problem, you no longer + know which version they were on, so a symptom cannot be tied to a change. +- Caught 2026-08-26 in rfp#186/#196: three pushes into a live Mergin project + during a field test, taking it from v1 to v9 while the phone was syncing. The + app reported "another process is running" and the tester tried removing and + re-adding the project before the cause was identified as the other writer. +- Rule: **hand over one version and stop.** If a fix is needed mid-test, say so + and let the tester decide when to take it. Batch changes rather than pushing + each one. When you must push, say which version you pushed and what changed, so + a later report can be anchored to it. + +### A value nothing reads is wrong silently — get it from the consumer, not from reasoning + +- Serialized formats carry fields that are **redundant with a lookup that + actually happens**: a positional index beside a name, a declared length beside + a delimiter, a cached count beside the rows. Because the consumer resolves by + the *other* field, a wrong value here changes nothing observable. It is not + benign — it is a defect with no failure mode until something new starts reading + it, and then it fails far from the code that wrote it. +- Your own tests cannot catch this class, and neither can a reviewer: every + assertion goes through the same name-based lookup the consumer uses, so the + index is never read on either side. +- **The consumer's own output is the only oracle.** Find an artifact the real + application wrote, compute your value for the same input, and compare across + the whole set — not one example, which a plausible off-by-one survives. +- Caught 2026-08-26 in rfp#186: QGIS `` numbering. The obvious + reading is "position in the table", but the OGR provider excludes **both** the + geometry column and the integer primary key, so counting `fid` put every alias + one place out. QGIS resolves an alias by `field` name, so nothing broke and + nothing could have. Settled by computing indices for a QGIS-authored layer and + comparing against the aliases QGIS itself wrote — **99/99** — then pinning that + comparison as a test. +- Review check: for any field you write that your own code never reads back, + name what does read it and where the ground truth came from. "It seemed + right" is the whole hazard. +### Measure the output, not the input you handed in +- When you instrument something to find out what it *did*, check that the probe + reads downstream of the transformation. A probe that reads a value back from + the same object you populated is not a measurement — it is a round-trip + through your own assignment, and it agrees with you perfectly. +- The failure is invisible because the number looks like data. It has units, it + varies when you vary the input, it is stable across repeats — everything a + real measurement does, except it never consulted the thing that transforms. +- **Tells, in order of usefulness:** + - The result is *exactly* a constant you can derive from the input — no + rounding, no jitter. Real ink, real bytes and real timings are messy. + - The probe reads a field of an object you constructed or configured, rather + than an artifact the system emitted. + - Varying something you know matters (a shape, an encoding, a locale) does not + move the number. +- **Fix: measure at the furthest downstream point you can reach** — the rendered + primitive, the bytes on the wire, the row as the consumer's own client reads + it. Prefer a format that is inspectable and exact: an SVG's `` + beats a rasterised pixel count, and a captured request body beats a mock's + recorded arguments. +- Caught 2026-08-26 in gq#16, and only by a reviewer. A symbol-size conversion + was built on "tmap draws 5.08 mm per size unit", measured by reading + `pointsGrob$size` back off the grob — the value tmap had been *handed*. R's + graphics engine then applies a per-`pch` factor the grob slot never records, so + a circle actually draws **3.81 mm**. The fix shipped every symbol 25% + undersized *while documenting itself as exact*, which is worse than the bug it + replaced. 5.08 mm is 0.2 inch exactly — the roundness was the tell, and it read + as elegance instead. +- Sibling of the interop rule below, one step earlier: that one is about whether + the consumer accepts what you wrote, this one about whether your ruler is + touching the object at all. + +### Percent-encode a URL at construction, not at consumption + +- A URL built by string-concatenation from filenames inherits whatever those + filenames contain. An unencoded space is accepted by lenient clients — browsers, + `aws-cli` — and rejected by strict ones, so the break is deferred and then + arrives all at once. +- Caught 2026-07 in stac_dem_bc#25: hrefs carrying literal spaces worked for + months, then every strict `curl` fetch failed together — 90 items, 0-byte + fetches. Nothing changed about the hrefs; the consumer changed. +- Encode where the URL is **built**. Encoding at the point of use means every + future consumer has to remember, and the one that forgets is the one you find + out about in production. + +### A cache written before the work succeeds strands its inputs permanently + +- Change-detection caches ("which inputs have I already seen?") must be persisted + **after** the work they gate succeeds. Rewritten at detection time, any input + whose processing then fails is marked seen and never built — invisible to every + future run, because the cache is precisely what future runs consult. +- Caught 2026-02 in stac_dem_bc: 2,107 URLs stranded this way, found only by a + reconciliation script diffing the cache against actual outputs. Nothing errored + on any subsequent run; the work simply never happened. +- In CI, committing state at the end of a successful job gives this atomicity for + free. Elsewhere, write the cache last, or write it atomically alongside the + output it claims. +- Sibling of "Cache keys must cover every output-affecting input" above: that one + is about a cache returning the wrong thing, this one about a cache silently + returning nothing ever again. + +### A structure transcribed from an external form or API is a snapshot, not a contract + +- Recording an external system's field order — a web form, a report layout, an + undocumented API response — captures **one instance on one date**. The system is + free to reorder between revisions, and nothing tells you when it does. +- Where the fields are same-typed (all integers, all strings), a reordering is + **invisible**: the output stays structurally valid and becomes semantically + nonsense. No parser complains, because nothing in the pipeline knows what the + values mean. +- Caught 2026-08 in `template_permit_fish`: a paste-ready answers file built from a + submitted 2025 permit application encoded the portal's columns as + `UTM Zone | Northing | Easting`. The 2026 form revision shipped + `UTM Zone | Easting | Northing`. Pasting in order **transposed easting and + northing on four of five sites of a submitted permit application**. The same + revision also replaced the eligibility questions. +- Rules: + - Record **which instance and what date** the structure came from, beside the + structure itself. + - Re-check against a live instance before each use, not once at authoring time. + - Assert on **magnitude or format, not position**, wherever the types cannot tell + the fields apart. In UTM Zone 10 an easting is 6 digits and a northing is 7 + digits starting with 6 — an assertion that would have caught this one. +- Same diagnostic family as "a wrapper's exit 0 is not the work completed": the + output is structurally valid and semantically wrong, so every check that looks + only at shape passes it. + +### A round-trip through your own reader proves nothing about interop +- When code writes a format some **other** program consumes — a database table, a config file, an export another tool imports — a test that writes then reads it back with your own reader validates only that you are self-consistent. It cannot detect that the real consumer rejects what you wrote. +- Symptom when wrong: every test green, the artifact byte-perfect on inspection, and the feature silently does nothing in production. Failures on the consumer's side are often **silent by design** — a lookup that matches nothing returns "no result", not an error. +- Get the real consumer into the loop, even if awkward: run it in a container, shell out to its CLI, gate the test on the tool being installed and skip otherwise. Then keep a cheap structural assertion alongside for CI, so the invariant is still guarded when the heavy test skips. +- Best ground truth is **the consumer's own output**: have it write the artifact once, then diff yours against it. That surfaces required fields no documentation mentions. +- (rfp#17, 2026-08: `layer_styles` rows were written with `f_table_schema` NULL. QGIS looks a style up with an equality match passing `""`, and `NULL = ''` is never true in SQL, so every row was invisible — `loadDefaultStyle()` returned FALSE, layers drew with default symbols, nothing logged. The rows round-tripped perfectly through DBI, so the whole suite was green. Found only by asking QGIS in a container, then bisecting against a row QGIS wrote itself.) + +### Mocking the transport means the request is never built +- A network client mocked at its HTTP boundary — `local_mocked_bindings(.do_http = ...)`, `responses`, `nock`, a stubbed `fetch` — gives excellent coverage of *response* handling and **zero** coverage of the request. Status codes, retries, backoff, parse errors, partial bodies: all testable. Method, headers, content type, and body encoding: never exercised, because no test that stubs the transport constructs one. +- The gap is invisible in the usual way. The suite is green, the code reads correctly, and the first real call fails with a status that looks like the *server's* problem — a 400 or a 406 reads as a bad query or a rate limit long before it reads as "we sent the wrong content type". +- Sibling of the interop rule above, one level lower: that one is about what a consumer reads from your artifact, this one is about whether the request ever reaches the consumer at all. +- Fix pattern: make the wire format a **pure function** and assert it offline — `build_body(query)` returning a string, tested for its prefix, for a round-trip back through the decoder to the original input, and for no unescaped metacharacters surviving. Cheap, no network, and it guards the exact thing the mocks cannot. +- Verify the real encoding once against the live service and record the result, because the wrong choice is often the more obvious-looking API. (rfp#168, 2026-08: `curl::handle_setform()` reads like the way to send a form and sends `multipart/form-data`; the Overpass API answered **400** on every endpoint, having answered **406** to a raw body. Only `data=` url-encoded via `postfields` returns **200**. 130 tests passed while this was broken, and more of the same kind would not have helped.) + +### A fixture set that cannot reach the failure mode is not validation +- Hand-picked fixtures test the cases you thought of. If every one of them is structurally incapable of triggering the bug class you are fixing, a green run means nothing — and it is *more* dangerous than no test, because it licenses the claim "validated". +- Before declaring a fix verified, ask what the fixtures have in common and whether that shared property is the very thing the bug depends on. If it is, the set has a hole no amount of additions to it will close. +- Prefer a **global structural invariant** over more examples. Properties like antisymmetry, transitivity, "every node reaches a terminal", or a conservation total sweep the whole domain and cannot be gamed by fixture choice. +- (link#227 / fresh#214, 2026-08: a watershed drainage-closure fix was declared validated on 8 hydrology fixtures. All 8 compared groups with *differing* stream codes — the bug only manifests between groups sharing one code, so the set could not have caught it. The very next case tried, the Fraser, dropped the group the entire basin drains through. What actually earned the claim was a transitivity sweep: 0 violations across 3,537 triples, plus 0 cycles and every group reaching an outlet.) + +### A negative-case fixture rots when the positive set grows +- A test asserting "X is refused" has to pick a concrete X that nothing supplies. The moment someone adds support for that exact X — a new shipped resource, a new registry row, a new supported format — the assertion breaks, and it breaks in a way that reads as *the feature is wrong* rather than *the fixture is stale*. +- The failure is loud, which is lucky. The dangerous variant is the same change landing where the test would still pass: a refusal test whose chosen X quietly becomes supported and whose assertion is on something looser than the refusal itself now passes for nothing. +- Fix by **asserting the premise beside the assertion**, in the same test: + ```r + unshipped <- "EPSG:32609" + expect_false(nzchar(system.file("extdata", "srs", + paste0(gsub(":", "_", unshipped), ".xml"), package = "rfp"))) # <- the premise + expect_error(add_layer(qgs, crs = unshipped), "cannot be copied") # <- the property + ``` + Then a future addition to the shipped set fails on the premise line, naming the real cause, instead of on the behaviour line, blaming the code under test. +- The same shape applies to any "this input is unsupported" test: unsupported file extensions, unregistered layer types, unknown enum values. Ask what would have to become true for the chosen input to stop being unsupported, then assert it is still false. +- (rfp#139, 2026-08: shipping an `EPSG_4326.xml` `` block so a tracking layer could carry a CRS no template used made that CRS resolvable from a package resolver's third tier — breaking a raster test that had picked EPSG:4326 precisely because nothing supplied it. The behaviour was correct in both directions; only the fixture's premise had expired.) + +### A guard's escape hatches are where it goes to die — read them first +- Every guard grows two things that can silently disable it: an **exemption + list** ("these are allowed to fail the rule") and a **lookup** ("find the + thing I am checking"). Both fail toward *pass*, and both read as diligence on + the page. When reviewing a guard, read those two before reading the + assertion — the assertion is the part that is usually already right. +- **An exemption list that covers every input makes the assertion unreachable.** + It is not a weakened guard; it is a guard that cannot go red, and it looks + more careful than the correct version because it is longer. + ```r + legend_exempt <- c( + lake = "drawn and legended", # <- every one of these is a REASON + wetland = "drawn and legended", # to remove the entry, not to keep it + ... # all 9 drawn layers listed + ) + missing <- setdiff(drawn, c(legended, names(legend_exempt))) # always empty + ``` + **Tell:** an exemption whose reason says the rule *is* satisfied. "Drawn and + legended" is not a reason to exempt something from a drawn-must-be-legended + check — it is the check passing. An exemption is only ever for an input the + rule should genuinely not apply to, and `character(0)` is a normal and healthy + state that deserves a comment saying so. +- **A lookup that matches a container rather than the artifact reports success + and then dies — or worse, checks the wrong thing.** Test for the *file*, never + for a directory of the right name: + ```r + for (up in c("..", "../..", "../../..")) { + if (dir.exists(file.path(up, "vignettes"))) return(...) # matched SOME vignettes/ + } + ``` + Under `R CMD check` that walked out of the package into the temp tree, matched + an unrelated `vignettes/`, and blew up in `readLines()`. Had a same-named file + existed there it would have silently checked a stranger's copy. +- Both caught 2026-08-26 in gq#61, in the same 100-line test file, written by + someone who had just added the "fixture that cannot reach the failure mode" + rule below. Neither was visible by reading; the first surfaced by restoring + the bug, the second only under `R CMD check` — `devtools::test()` passed it + because the source tree happens to have the directory where it looked. +- **Corollary on where you verify.** A guard that reads repo layout behaves + differently under `devtools::test()`, `R CMD check`, and an installed package. + Green in the one you run locally says nothing about the one CI runs. Run both + before believing it. + +### Restore the bug and confirm the test fails +- The rule above says a fixture that cannot reach the failure mode is worthless. This is the thirty-second check that tells you which kind you just wrote: **put the defect back, run the test, watch it go red.** A test that stays green against the code it was written to reject is decoration, and reading it will not tell you that — every case below looked correct on the page. +- Cheapest form when the fix is inside a package: patch the namespace rather than editing the source back and forth. + ```r + ns <- asNamespace("pkg"); orig <- get("f", ns) + unlockBinding("f", ns); assign("f", broken_version, ns) + # run the assertion -- it must fail here + assign("f", orig, ns); lockBinding("f", ns) + ``` + For a data-shaped bug, feed the function the input the fix was about and assert the old answer is gone. +- Three instances in one PR (gq#52, 2026-08), all written by someone who had just read the fixture rule directly above: + - A scale-bar test asserting the bar stays within `share` of the frame — threshold hardcoded at **0.75** against a `share` of **0.35**, so a bar at 2.1x the requested size passed. Every width in the fixture also happened to round *down*, so none could overrun even at the right threshold. + - A clamp test for a bbox padded past ±90 — the box chosen padded the **x** axis, so the latitude clamp it was named for could never fire. + - An `options(str=)` independence test routed through real registry data whose values stayed distinct at one decimal. With the buggy key restored it still passed; a synthetic `1.32 / 1.34` pair made it fire. +- What they share is the tell: the **assertion** is correct and the **input** cannot reach it. So review the fixture against the bug, not the assertion against the spec — the assertion is the part that reads well and the part that is usually already right. +- Sibling of the interop rule above, at one remove: a test that inspects a structure its consumer would reject is the same failure. In that same PR, 18 tests read a legend object and none passed it to the renderer, which refused it outright. + +### Bare `y`, `n`, `on`, `off`, `yes`, `no` are booleans in YAML 1.1 +- The YAML 1.1 core schema resolves `y`, `Y`, `n`, `N`, `yes`, `no`, `on`, `off`, `true`, `false` (and their case variants) to **booleans**. Most parsers in wide use — libyaml, PyYAML, R's `yaml` — still do this. +- So a column, key, or field literally named `y` stops being a string the moment it is written unquoted: + ```yaml + cols: + - name: y # parses as logical TRUE, not "y" + ``` + Nothing errors. The consumer simply never matches that entry again, and whatever it was supposed to do to it silently does not happen. +- Bites hardest in **schema and config files**, where single-letter names are normal: coordinate columns (`x`, `y`, `z`), flags, short codes. Quote them: `- name: "y"`. +- Caught twice in one file 2026-08-24 (crate#9) — once in a canonical column list and once in a variant's column list. Both found by a guard that asserted every declared name `is.character()`; reading the YAML had not found either. +- Worth an assertion rather than vigilance: after parsing any config that carries user-chosen names, check they are all strings. The failure is invisible otherwise, because the wrong value is a perfectly valid one. + +### Canonicalize serialized documents before diffing them +- XML and JSON emitters are free to vary attribute order, whitespace, and regenerated ids without changing meaning. Comparing two such documents raw reports differences that are not differences — and the noise scales with document size, so it looks like a real signal. +- Normalize first: C14N for XML (`ET.canonicalize(strip_text=True)` sorts attributes), key-sorted dumps for JSON, and mask any regenerated identifiers (uuids, timestamps, generator version stamps). +- Then narrow the mask deliberately. Every field you normalize away is a field the comparison can no longer catch, so name each one and why — a mask that quietly grows turns a drift guard into decoration. +- (rfp#17, 2026-08: comparing two QGIS templates raw said 5 of 43 shared layers still matched, which read as severe drift and argued for restructuring how styles were stored. Canonicalized — attributes sorted, symbol uuids masked — it was **46 of 47**. The templates had not drifted at all; the difference was attribute order between two QGIS builds. The naive number nearly bought an architecture change nobody needed.) + +### A verification command can be shadowed by a shell function or alias +- The shell is initialized from the user's profile, so `diff`, `grep`, `ls`, `cat` and friends may resolve to a wrapper rather than the binary you assume. Measured 2026-08-24 in gq: `diff` was a shell **function** delegating to `git diff`, so `diff -q a b` — a byte-comparison in an idempotency check — died on ``unknown switch `q' `` and the step reported **NOT IDEMPOTENT** for two files that were in fact identical. +- That direction is survivable because it is loud. The dangerous one is a wrapper that exits 0 on a comparison it never performed, which reads as "verified". +- For anything whose output you are about to treat as evidence, bypass the lookup: `command diff`, `\diff`, or a tool with no common wrapper — `cmp -s` for byte-equality, `md5` / `sha256sum` for a value you can print. Printing the digest beats printing a verdict: it stays checkable after the fact. +- `type ` tells you what you actually have. Worth running the first time a verification step returns something surprising, before believing the surprise. + ### Documentation Staleness - Moving/renaming scripts: update CLAUDE.md, READMEs, usage comments - New variables: update .tfvars.example @@ -341,7 +1452,7 @@ For non-trivial issue-driven work, follow this checklist. Each step exists for a ## The Sequence -1. **Start with `/planning-init `** — given an issue number, scaffolds branch + PWF baseline from the issue body. One command replaces the manual issue → branch → plan dance. +1. **Start with `/planning-init `** — given an issue number, enters plan mode for codebase exploration, presents a phase breakdown for user approval, then scaffolds branch + PWF baseline with the approved phases. One command replaces the manual issue → explore → plan → branch → scaffold dance. 2. **Write robust tests first** — failing tests that reproduce the issue or document the new behavior. Tests are the contract; they fail until the work makes them pass. 3. **Name with intent** — functions, parameters, internal helpers carry the naming style of the package they live in. Look at existing exports as the guide; consistency over cleverness. (Per-package naming convention TBD — see soul issue tracking.) 4. **Examples that run** — every exported function gets a runnable `@examples` block. Pkgdown renders them; CI executes them. An example that doesn't run is documentation rot. @@ -362,6 +1473,41 @@ For one-line typo fixes, version-bump-only PRs, or trivial documentation edits, - `/gh-pr-push` — open the PR - `/gh-pr-merge` — merge with release bookkeeping +## Issue bodies get edited, not appended + +When work changes what an issue should say, **edit the body**. Don't add a +comment that corrects it, and retitle when the scope moves. + +**Why:** an issue is read as a spec by whoever picks it up. A body saying one +thing with a comment three screens down saying the opposite costs the reader the +reconciliation, every time. + +**How to apply:** `gh issue view N --json body -q .body` into a file, revise, +`gh issue edit N --body-file`. Name what changed and why when the correction is +load-bearing — the goal is a body that reads correctly top to bottom, not an +erasure of history. Comments are for genuine commentary: a merge notice, a +cross-repo pointer, a question. Applies to PR bodies too. Commit messages are +immutable history and are never rewritten this way. + +**The failure mode that keeps recurring: research findings feel like +commentary.** They are not — they are the spec. If a finding changes what +someone would *build*, it belongs in the body, with the durable version in +`research/` and the body linking to it. + +**Bodies drift at the moment work finishes, not while it is in flight.** Four +instances in a single day of rfp work, all of the same shape — the code learned +something and the issue did not: + +| drift | what a reader saw | +|---|---| +| premise disproved by measurement | an issue arguing for a fix that was no longer needed | +| a conclusion asserted in the body but never landed in code | body and tree contradicting each other | +| the shape of the work moved during exploration | a spec describing a design nobody built | +| a decision made and shipped, body still listing options A–D | "decision needed" on a decision a year old | + +Vigilance does not catch this, because the drift happens exactly when attention +moves to the merge. `/gh-pr-merge` reconciles at that moment — see its step 3b. + ## Why This Exists We've hit snags repeatedly when half-doing this — branches that mix concerns, tests bolted on after, code-check skipped (and then a bug ships in the diff), examples that fail in pkgdown. Each step is small; the cumulative reliability gain is real. The convention is here so it becomes the default expectation, not a thing the user has to remind every session about. @@ -434,10 +1580,233 @@ For multi-step tasks, state a brief plan: Strong success criteria let you loop independently. Weak criteria ("make it work") require constant clarification. +## 5. You Have No Clock Between Tool Calls + +**Every duration claim comes from `date`, never from how much waiting felt like +it happened.** + +Background `sleep` returns immediately from the agent's side, and the number of +times you have polled is not evidence of elapsed time. Two consecutive tool +calls can be 15 seconds apart by the clock while feeling like ten minutes of +waiting. + +The failure is stating it out loud before checking. Observed 2026-08: a CI run +was reported to the user as "pending for over an hour — unusually long, probably +a stuck runner", after roughly eight background sleeps. One `date -u` showed the +run was **three minutes old** and entirely normal. The whole diagnosis — stuck +runner, duplicate triggers, something wrong with the workflow — rested on a +duration that had been invented. + +**How to apply:** before saying *any* duration — "still running after N +minutes", "this has been X a while", "longer than usual" — run `date -u` and +subtract a real start time. `gh run list --json createdAt` gives it for CI. If a +claim about slowness would change what the user does next, it needs a measured +number or it does not get made. + +The same rule covers process state. `ps` and task-status listings have both been +observed wrong; check the artifact (an output file's size, its mtime, the +service's own API) rather than the wrapper. + +### The same blind spot picks the wrong waiting tool + +Not having a clock also makes a **chain of background sleeps** feel like +waiting when it is not. Observed 2026-08 on the same session as the above: +roughly a dozen `sleep 570; check` background tasks were spawned to wait out a +55-minute test suite and then CI. Two consecutive foreground checks printed the +*same minute* — no wall time had passed between them, because the sleeps run +detached and the polling happened around them rather than after them. Every one +of those tasks was waste, and killing them produced a batch of eleven +exit-code-144 notifications that read like failures. + +Pick the instrument by how many answers you need: + +| you need | use | +|---|---| +| one notification when a condition becomes true | `Bash(run_in_background)` with an `until` loop that exits | +| one per state change, ending on its own | `Monitor` with a command that emits and then exits | +| a value you must have before the next step | a **foreground** call, so the blocking is explicit | + +A repeated `sleep N; grep` is right in none of them. **Tell: if you are about to +spawn a second waiter for the same thing, the first one was the wrong shape.** + +A `Monitor` filter must also match the failure states, not just the success +one — silence looks identical to "still running", so a watcher that greps only +for the happy path stays quiet through a crash. + +## 6. Subagents Are Evidence, Not Dependencies + +**Don't block on one. Don't trust its status. Verify its claims in both directions.** + +### Don't block + +Spawn a background subagent, then keep working on the lowest-risk part of the +task — scaffolding, data files, tests. When findings arrive, treat them as a +review of landed work rather than a precondition for starting it. + +If a result genuinely must precede the next step, run it synchronously +(`run_in_background: false`) so the blocking is explicit and visible. + +Three observed cases where waiting would have been the expensive choice: + +- A research agent spawned 5 children and deadlocked for **~3 hours**, still + reporting as "running". The user caught it, not the agent. +- A `Plan` agent asked to review a `task_plan.md` *before the baseline commit* + returned after the issue was implemented, reviewed, merged and tagged. +- The same pattern on a later issue: findings arrived after all four phases had + shipped. Because the work had not waited, this cost nothing — three findings + were still new and landed as follow-up commits. + +That last one is the shape to aim for. Concurrent review is not a degraded +version of blocking review; it is often better, because the reviewer reads real +code instead of a plan. + +### Don't trust status + +**Never report an agent as "still running" without evidence.** Agent status and +`TaskList` have both been observed to be wrong — `TaskList` reported "No tasks +found" for an agent that was alive and later replied. Check the output file's +mtime before claiming progress, and say what you checked. + +### Verify claims, in both directions + +Subagent output is evidence, not verdict. Both failure modes are real: + +- **Acting on a wrong finding.** One labelled BLOCKER — "`glue()` will choke on + the literal braces in this fragment" — was disproved by a 30-second probe, + because glue does not re-parse interpolated values. Acting on it would have + meant rewriting a working generator. +- **Dismissing a late review wholesale.** In that same review 2 of 9 findings + were real, including a dead link. In a later one, a finding that a + `path|layername=` check would delete KML/GPX layers was correct, and was + confirmed against 207 real datasources before the fix landed. + +The rule that separates them: **cheap probe first, then act.** Reproduce the +claim before you fix it, and before you dismiss it. A finding you cannot +reproduce is a finding you do not yet understand. + **These guidelines are working if:** fewer unnecessary changes in diffs, fewer rewrites due to overcomplication, and clarifying questions come before implementation rather than after mistakes. +# pkgdown Publishing + +What a pkgdown deploy puts on the public internet, and the two ways that has +already gone wrong. + +## A pkgdown site publishes every root-level markdown file + +`pkgdown:::package_mds()` renders **every** `.md` in the package root except a +hardcoded allowlist — `README`, `LICENSE`, `NEWS`, and two GitHub templates. +There is **no config option to exclude a file**. + +So `CLAUDE.md` gets published. So would `INTERNAL.md`, `NOTES.md`, or a PWF +`task_plan.md` left at the root. + +**Repo visibility does not protect you.** GitHub Pages serves publicly +regardless of whether the repo is private, and there is no private Pages mode +below Enterprise Cloud. A private repo with a pkgdown deploy has public docs. + +Measured 2026-08-23: `CLAUDE.html` was live on six NGE sites. On `rfp` and `gq` — +both private repos, so their `CLAUDE.md` legitimately carried the internal-only +conventions — that put the SR&ED section on the public web: claim structure, +field code, fiscal year, and the consultant by name. + +The visibility filter was working correctly the whole time. It rests on an +assumption pkgdown breaks: that a private repo's `CLAUDE.md` stays private. + +### Remove it before the build, not after + +There are three copies, not one: + +| file | what it is | +|---|---| +| `CLAUDE.html` | the rendered page | +| `CLAUDE.md` | a **verbatim copy of the source**, served as-is | +| `search.json` | the full-text index, containing the text | + +Deleting `docs/CLAUDE.html` after the build leaves the other two. It looks like a +fix and achieves nothing. Remove the file from the CI checkout **before** +`build_site()` runs: + +```yaml +- name: Keep internal notes out of the published site + run: rm -f CLAUDE.md +``` + +### Gate on a declared allowlist + +Each repo states which extra root pages it *intends* to publish. Anything else +fails the build, so a new root markdown file cannot leak silently: + +```yaml +- name: Fail if an unexpected page reached the site + run: | + allowed="404 authors index LICENSE LICENSE-text" # + declared extras + ... +``` + +Add to `allowed` only after deciding the page should be public. `link` publishes +`NOTICE` and `RUNBOOK` deliberately — it is a public repo and both are genuine +documentation. That is the decision the allowlist is meant to record. + +Test the gate against **both** known answers before shipping it: it must exit +non-zero on a site that does contain the file, and zero on one that does not. A +guard that only ever returns one value is indistinguishable from a broken one. + +## Deploy with `clean: true` + +`JamesIves/github-pages-deploy-action` defaults matter here. With +`clean: false`, the action **never deletes** — every file ever deployed stays on +`gh-pages` forever, whether or not the source still produces it. + +Two consequences, both observed: + +- Removing a file from the repo does **not** unpublish it. Measured on `gq`: + `task_plan.html`, `progress.html` and `findings.html` were still returning 200 + long after the PWF documents had been moved out of the root. +- A leak cannot be fixed by fixing the build. The stale copies need a separate + explicit purge, which is a step people forget. + +`clean: true` makes the deployed site equal to what the build produced, so +removing a file from source removes it from the web on the next deploy. That is +the property you want, and it makes the site auditable. + +### Check before flipping it + +`clean: true` deletes anything on `gh-pages` not present in `docs/`. Confirm +none of these exist first: + +- **`CNAME`** — a custom domain file would be deleted and the domain would break. + (NGE repos have none; the domain comes from the org site repo, and project + sites inherit it as subpaths.) +- **`dev/`** — versioned docs from `development: mode: devel`, if the deploying + build is not the dev one. +- **hand-added assets** not produced by the build. Favicons and web manifests + under `pkgdown/favicon/` *are* produced by the build and are safe. + +Use `clean-exclude` for anything that must survive. + +```bash +gh api "repos/OWNER/REPO/contents?ref=gh-pages" --jq '.[] | "\(.type) \(.name)"' +``` + +## Removing something already published + +1. **Stop generating it** — the pre-build removal above. +2. **Remove the deployed copy** — automatic once `clean: true` is in; otherwise + an explicit purge. +3. **De-index** — a Search Console removal request per property, *after* the URL + 404s. + +Do **not** add a `robots.txt` block first. Blocking crawl prevents crawlers from +seeing the 404, which keeps stale search entries alive longer than doing +nothing. + +`gh-pages` history is not a problem the way normal git history is: on a private +repo the branch is not publicly browsable, and only the currently-served content +is public. Deleting the file genuinely ends the exposure — no history rewriting. + + # Planning Conventions How Claude manages structured planning for complex tasks using planning-with-files (PWF). @@ -453,15 +1822,33 @@ Skip planning for single-file edits, quick fixes, or tasks with obvious next ste ## The Workflow -1. **Explore first** — Enter plan mode (read-only). Read code, trace paths, understand the problem before proposing anything. +1. **Explore first** — Enter plan mode (read-only). Read code, trace paths, understand the problem before proposing anything. When the work codifies a pattern that already exists in multiple places (reference implementations across repos), read **every** reference in full, not just the canonical one — variation across references surfaces patches before v0.1 instead of as churn later (soul#52: reading all 4 references preempted 5 of the 7 fixes a dry-run would have found). Don't substitute Explore-agent summaries for direct reads; agents sometimes report existing files as absent. 2. **Plan to files** — Write the plan into 3 files in `planning/active/`: - `task_plan.md` — Phases with checkbox tasks - `findings.md` — Research, discoveries, technical analysis - `progress.md` — Session log with timestamps and commit refs -3. **Commit the plan** — Commit the planning files before starting implementation. This is the baseline. -4. **Work in atomic commits** — Each commit bundles code changes WITH checkbox updates in the planning files. The diff shows both what was done and the checkbox marking it done. -5. **Code check before commit** — Run `/code-check` on staged diffs before committing. Don't mark a task done until the diff passes review. -6. **Archive when complete** — Move `planning/active/` to `planning/archive/` via `/planning-archive`. Write a README.md in the archive directory with a one-paragraph outcome summary and closing commit/PR ref — future sessions scan these to catch up fast. +3. **Plan-review with the Plan agent — concurrently, not as a gate** — Once `task_plan.md` is scaffolded, spawn the Plan subagent (`Agent({subagent_type: "Plan", prompt: "..."}`) and ask it to critically review the task_plan against the issue body + actual codebase. Categorize findings as Blocker / Gap / Ordering / Assumption / Scope / Acceptance. The agent reads files fresh — it catches what you miss when you've been thinking about the design too long. Real example: caught 21 issues including hardcoded literals across 4 files not listed in the plan, untested DB column mismatches, and a baseline-cache-shadow that would have produced a 6-second no-op run. + + **Do not wait for it.** Spawn, then start the lowest-risk phase. Background agents have repeatedly returned late — in one case after the entire issue had shipped — so treating the review as a precondition stalls the work for as long as the agent takes (see `karpathy.md` §6). Fold findings in whenever they land: pre-baseline they edit the plan; mid-implementation they become follow-up commits. A review that arrives after the code is written is not wasted — the reviewer reads real code instead of a plan, which is how one late review still contributed three fixes that no earlier reading had found. If you genuinely cannot proceed without the result, run it with `run_in_background: false` so the blocking is explicit. + + Verify before acting, in both directions. Findings have been confidently wrong (a "BLOCKER" disproved by a 30-second probe) and confidently right about things nobody suspected. Reproduce the claim first. + + **Spawn review agents UNNAMED.** Passing `name` to the `Agent` tool changes what you get: a named spawn becomes a persistent *teammate* that goes **idle** rather than completing, so there is no final report to auto-deliver and its output must be pulled with `SendMessage`. An unnamed spawn is a fire-and-return subagent whose report arrives on its own in the completion notification. Measured 2026-08-25 on one machine, one session, unchanged settings: the unnamed spawn returned in **6.4s**; three named reviewers returned nothing at all, sending only empty idle pings. Pass `name` only for a collaborator you intend to keep messaging, and shut it down when done — it pings indefinitely otherwise. + + That mis-spawn is what produced the silent-delivery failures below, so check `name` before suspecting settings. Teammate mode (`CLAUDE_CODE_EXPERIMENTAL_AGENT_TEAMS=1` + `teammateMode`, merged globally from `soul/settings/defaults.json`) shapes what a *named* spawn becomes; it is not by itself why findings go missing, and an unnamed spawn delivers fine with it enabled. + + **Have the agent write findings to a file, and report only the path.** Message delivery has silently failed twice: one review arrived as idle notifications with no content, and one was routed to a different session on the user's phone — which only surfaced because the user mentioned it. From this side an idle ping is indistinguishable from an agent that had nothing to say, so the loss is invisible. A file (`planning/active/review-.md`) survives routing, survives the agent exiting, and is greppable later. Put the instruction in the first prompt, not as a follow-up. + + **Review the fixes, not just the code.** The second pass is where the value concentrates, because a fix written under a wrong assumption reproduces the same defect. Measured on gq#52: pass 1 found 13 defects, pass 2 found 7 more — including a blocker sitting *inside the fix* for pass 1's blocker, the same class twice (`lty`, then `fill_alpha`) because completeness was reasoned about rather than computed. Pass 3, scoped narrowly to the file edited most, found no new instances; **convergence is the signal to stop, not a fixed number of rounds.** + + Ask for the **mechanism**, not more instances. Pass 3's best finding was that an invariant was enforced by two lists happening to agree — which is what had produced instances two and three. + + The thing reviewers catch that self-probing does not is **interop**: 18 tests inspected a legend object and none handed it to the renderer, which rejected it outright. Ask the consumer. +4. **Lock naming before the baseline** — If naming feedback surfaces during planning (legacy filename, inconsistency with an existing file family), fold the rename into the convention + task_plan BEFORE the baseline commit, not as a follow-up. Pre-baseline it's free; retrofitting after implementation cascades (soul#52: `build_exec_pdf.R` → `run_pagedown_exec_summary.R` locked in pre-baseline meant zero downstream rework). +5. **Commit the plan** — After Plan-agent review + fixes. This is the baseline. +6. **Work in atomic commits** — Each commit bundles code changes WITH checkbox updates in the planning files. The diff shows both what was done and the checkbox marking it done. +7. **Code check before commit** — Run `/code-check` on staged diffs before committing. Don't mark a task done until the diff passes review. +8. **Archive when complete** — Move `planning/active/` to `planning/archive/` via `/planning-archive`. Write a README.md in the archive directory with a one-paragraph outcome summary and closing commit/PR ref — future sessions scan these to catch up fast. ## Atomic Commits (Critical) @@ -479,7 +1866,9 @@ This creates a git audit trail where `git log -- planning/` tells the full story Phases with checkboxes. This is the core tracking file. ```markdown -# Task Plan +# Task: (#) + + ## Phase 1: [Name] - [ ] Task description @@ -500,6 +1889,11 @@ Append-only research log. Discoveries, technical analysis, things learned. ## [Topic] [What was found, with source/date] + +## Errors Encountered + +| Error | Resolution | +|-------|------------| ``` ### progress.md @@ -515,6 +1909,38 @@ Session entries with commit references. - Next: [items] ``` + + + + + +## The Reboot Test + +The planning files exist so the work survives an interruption. Whether they +actually do is checkable: at any point mid-task, these five questions must be +answerable from the files alone, without the conversation. + +| Question | Answer source | +|----------|---------------| +| Where am I? | Current phase in `task_plan.md` | +| Where am I going? | Remaining phases in `task_plan.md` | +| What's the goal? | The `# Task: (#N)` frame and problem statement at the top of `task_plan.md` | +| What have I learned? | `findings.md` | +| What have I done? | `progress.md` | + +If an answer lives only in the session, **write it down and commit it**. Written +is not sufficient: an uncommitted `findings.md` does not move between machines, +and a repo whose `planning/` is gitignored accepts `git add planning/` with exit +0 while tracking nothing — see Directory Structure below. + +This is the operational check for the rule that every interruption should be a +resume point: a session death, sleep, or machine swap should cost a re-run at +most, never lost context. That rule states the goal; this tests it. + +Run it before any long wait, before compaction, and before switching machines — +the moments that take a session without warning. `/compact-prep` and +`/planning-update` are where it gets run; this section is what it asks. + ## Directory Structure ``` @@ -526,6 +1952,76 @@ planning/ If `planning/` doesn't exist in the repo, run `/planning-init` first. +**`planning/active/` must be tracked, not gitignored.** The atomic-commit rule +above requires each commit to carry its own checkbox flip in `task_plan.md`; an +ignored `active/` drops it silently, so `git log -- planning/` shows archives +appearing fully-formed with no history behind them. In-flight PWF also stops +surviving a move between machines. + +The failure is quiet in both directions. `git add planning/` reports nothing and +exits 0 on an ignored path, and files tracked *before* the rule existed keep +being tracked — including through a `git mv` into the ignored directory. So a +repo can look like it is working right up until the first genuinely new PWF file, +which simply never appears in a commit. + +Check rather than assume: + +```bash +git check-ignore -v planning/active/task_plan.md # expect no output +``` + +Found 2026-08-24 in gq, where the rule dated from the scaffold commit and the +#17 files had only survived because they predated their move into that +directory. gq and roli were the only 2 of 32 repos carrying it; roli still does. + +## When Something Keeps Failing + +Before a second attempt, name the failure class. A **deterministic** failure +returns the same result to the same inputs, so re-running unchanged only spends a +turn — change the inputs or change the approach. A **transient** failure +(network, a provider read, a rate limit, a resource still settling) is the case +where a re-run *is* the attempt: `code-check.md` prescribes exactly that for a +tofu plan that falsely reports a resource deleted. The rule is not "never retry"; +it is never retry unchanged while expecting a different answer. + +Escalate rather than iterate once the approach itself is in question. Report what +was tried and the exact error, and hand over the commands to run — the user is +assumed to be away, so a question answerable from a phone beats a retry loop they +cannot see. Escalating is not stopping: commit the current state, then move to +the lowest-risk independent part of the plan while the question is outstanding. + +Two classes escalate immediately rather than after retries, because further +attempts make them worse: + +- **A clamped session.** Once a live credential has been read, later + system-mutating commands are refused regardless of route — seven consecutive + refusals across unrelated routes is the documented case (`newgraph.md`, + "Reading a secret clamps the rest of the session"). Trying more phrasings is + the failure mode, not the remedy, and `/permissions` does not clear it. +- **Rate limits.** Retrying extends the block (`ci-monitoring.md`). + +### Log the errors that cost a retry + +An error that took more than one attempt to get past goes in `findings.md`, so +one task does not hit the same wall twice: + +```markdown +## Errors Encountered + +| Error | Resolution | +|-------|------------| +| `fatal: Unimplemented pathspec magic '_'` | Long-form `:(exclude)path` | +``` + +That row is also what graduation looks like: it began as one task's blocker and +now lives in `code-check.md` as a general rule about pathspec magic. Most rows +never make that trip and should not — the ledger's job is to stop one task +repeating itself. + +When a failure does generalize, it graduates to the convention that owns its +class: `code-check.md` for a bug class in a diff, `ci-monitoring.md` for CI +behaviour, the domain convention otherwise. + ## Skills | Skill | When to use | @@ -550,9 +2046,32 @@ test structure, hex sticker, etc.). - tidyverse style guide: snake_case, pipe operators (`|>` or `%>%`) - Match existing patterns in each codebase - Use `pak` for package installation (not `install.packages`) +- Prefer `fs::` helpers over base R for filesystem path operations in build + scripts and scaffolds: `fs::dir_create()` (creates parents by default, no + `recursive`/`showWarnings` fiddliness), `fs::path()`, `fs::file_delete()`, + `fs::file_exists()`, `fs::path_file()`. Avoids cross-platform separator + issues and silent no-ops on empty paths. - Prefix column name vectors with `cols_` for discoverability in the environment pane: `cols_all`, `cols_carry`, `cols_split`, `cols_writable`. Same principle for other grouped vectors (`params_`, `tbl_`, etc.) +- For SQL DDL+INSERT pairs that share a schema, use a single named + vector as the source of truth. Both `CREATE TABLE` and + `INSERT (cols) SELECT cols` derive their column lists from the same + `cols_*` vector. Avoids drift between table shape and write + projection — when columns change, you edit one place. Example: + ```r + cols_streams <- c( + id_segment = "integer NOT NULL", + watershed_group_code = "varchar(4) NOT NULL", + geom = "geometry(MultiLineStringZM, 3005)" + # … + ) + # CREATE TABLE consumes both names + types + ddl_body <- paste(names(cols_streams), unname(cols_streams), sep = " ", + collapse = ", ") + # INSERT consumes names only + proj <- paste(names(cols_streams), collapse = ", ") + ``` ## Package Structure @@ -584,7 +2103,7 @@ traceable record of what was planned, built, and verified. For new packages or major features, work on a branch and merge via PR: ``` -main ← scaffold-branch (PR closes with "Relates to NewGraphEnvironment/sred-2025-2026#N") +main ← scaffold-branch (PR closes with "Relates to NewGraphEnvironment/sred#N") ``` This gives one PR that contains all commits — a single SRED cross-reference @@ -609,6 +2128,41 @@ Close function issues via commit messages — see Closing Issues in newgraph con ``` For error context: `grep -E "(ERROR:|FAIL )" -A 10 | head -25` +### Common pitfalls + +- **`cli::cli_alert_warning()` is not `warning()`.** It's visual only — + callers can't catch it with `withCallingHandlers(warning = ...)` and + testthat's `expect_warning()` won't fire. When a function offers a + `warn` mode that callers may want to react to programmatically, use + `warning()`. Reserve `cli_alert_warning()` for FYI messages with no + programmatic contract. + +- **`expect_match(x, ..., all = FALSE)` passes silently on `character(0)`.** + If the input is empty (e.g. no warnings fired), the assertion succeeds + vacuously and defeats the test. Always pair with + `expect_gt(length(x), 0)` first when input may be empty. + +- **`skip_on_cran()` does not skip on GitHub Actions.** It skips when + `NOT_CRAN` is unset — and `devtools`, `usethis`'s check workflow and + `r-lib/actions` all set `NOT_CRAN=true`, precisely so your tests *do* run in + CI. So a network test guarded only by `skip_on_cran()` runs on every push, + and any upstream hiccup reddens the build for a reason unrelated to the + change under review. + - Use **`skip_on_ci()`** for a test that is meant for a human's machine — a + live canary against a third-party service, something slow, anything whose + failure needs a person to interpret it. + - `skip_if_offline()` is not a substitute: it tests whether the network is + reachable, not whether the *service* is behaving, and it calls + `skip_if_not_installed("curl")`, so add `curl` to Suggests or the guard + itself is what breaks. + - Caught 2026-08 in gq#57 by self-review: a comment claiming "skipped off-CI" + sat directly above code that did not skip off-CI. Read the guard, not the + comment above it. + +- **`local_mocked_bindings(.package = )` needs testthat >= 3.2.0.** A package + pinned at `testthat (>= 3.0.0)` errors rather than skipping on an older + install. Bump the pin when you first mock another package's binding. + ## Examples and Vignettes ### Runnable examples on every exported function @@ -629,9 +2183,19 @@ At least one vignette showing the full pipeline on real data: - Hosted on pkgdown so users can read it without installing **Output format:** Use `bookdown::html_vignette2` (not -`rmarkdown::html_vignette`) for figure numbering and cross-references. -Requires `bookdown` in Suggests and chunks must have `fig.cap` for -numbered figures. Cross-reference with `Figure \@ref(fig:chunk-name)`. +`rmarkdown::html_vignette`) for figure numbering. Requires `bookdown` in +Suggests and chunks must have `fig.cap` / `caption =` for numbered +figures and tables. + +**Gotcha — cross-references don't resolve in vignettes.** `Table \@ref(tab:foo)` +and `Figure \@ref(fig:foo)` markers compile to a literal `\@ref(...)` in +the rendered HTML rather than a numbered link. Bookdown's cross-ref +machinery isn't fully wired through `html_vignette2` under pkgdown. +Use natural language instead — "the table below", "the floodplain map", +"the parameter table" — and let the captions speak for themselves. If +you need real numbered cross-refs, use `bookdown::html_document2` +(matches the cd-style report-appendix pattern) and accept that the +output is no longer a true package vignette. **Vignettes that need external resources (DB, API, STAC):** Do NOT use the `.Rmd.orig` pre-knit pattern — it breaks `bookdown` figure numbering @@ -787,6 +2351,8 @@ Three tools, different purposes. Use the right one. **BBT citation key storage:** As of Feb 2025+, BBT stores citation keys as a `citationKey` field directly in `zotero.sqlite` (via Zotero's item data system), not in a separate BBT database. The old `better-bibtex.sqlite` and `better-bibtex.migrated` files are stale and no longer updated. Query citation keys with: `SELECT idv.value FROM items i JOIN itemData id ON i.itemID = id.itemID JOIN itemDataValues idv ON id.valueID = idv.valueID JOIN fields f ON id.fieldID = f.fieldID WHERE f.fieldName = 'citationKey'`. +**BBT citekey format is locally patched to strip `&`:** the `citekeyFormat` pref (`extensions.zotero.translators.better-bibtex.citekeyFormat` in `~/Library/Application Support/Zotero/Profiles/*/prefs.js`) has a `.replace(find = "&", replace = "")` segment added by hand. Without it, institutional authors containing `&` (e.g. "BC Species & Ecosystem Explorer", "WA Dept of Fish & Wildlife") leak `&` into the citekey, and pandoc's `@key` parser stops at `&` — so cites render broken in any bookdown/quarto build even though biblatex accepts the key. Reapply via Zotero → Tools → Run JavaScript: `Zotero.Prefs.set("translators.better-bibtex.citekeyFormat", val)` (also patch `citekeyFormatEditing` to match). Survives Zotero/BBT auto-updates; reverts only on a profile reset or a manual edit via the BBT preferences UI. Detect drift: `grep citekeyFormat ~/Library/Application\ Support/Zotero/Profiles/*/prefs.js` should show the `.replace(find = "&", ...)` chain. Teammates on Skeena/Fraser/restoration machines that hit the same `@key`-breaks-at-`&` drift should run the same `Zotero.Prefs.set`. + ## Adding References Workflow ### 1. Search and flag From 6a9d9d91771e6288c3b86e94c676f3916880b7c6 Mon Sep 17 00:00:00 2001 From: almac2022 <al@newgraphenvironment.com> Date: Thu, 27 Aug 2026 16:11:04 -0700 Subject: [PATCH 2/6] Initialize PWF baseline for #40 Relates to #40 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U9SAqmvFeENADk4rYcbtHS --- planning/active/findings.md | 80 ++++++++++++++++++++++++++++++++++++ planning/active/progress.md | 11 +++++ planning/active/task_plan.md | 68 ++++++++++++++++++++++++++++++ 3 files changed, 159 insertions(+) create mode 100644 planning/active/findings.md create mode 100644 planning/active/progress.md create mode 100644 planning/active/task_plan.md diff --git a/planning/active/findings.md b/planning/active/findings.md new file mode 100644 index 0000000..b540225 --- /dev/null +++ b/planning/active/findings.md @@ -0,0 +1,80 @@ +# Findings — fl_valley_attribute() (#40) + +## Why attribution rather than per-group VCA runs (measured 2026-08-27) + +Of the four criteria in `R/fl_valley_confine.R:140-160`, only the slope mask is independent of which +streams are supplied: + +| step | depends on stream set? | effect of dropping tributaries | +|---|---|---| +| slope mask | no | — | +| distance mask (`max_width/2`) | yes | corridor shrinks to the mainstem | +| cost distance | yes | more seeds can only lower cost, so a subset's cost is >= | +| flood model | yes | IDW interpolates the surface from *all* seed cells (`fl_flood_depth.R:68-80`) | +| cleanup (patch removal, hole fill) | yes, globally coupled | a sub-`size_threshold` patch may survive when merged | + +Measured on bundled Bulkley data, grouping by `gnis_name`: + +``` +FULL network: 53,635 cells (536.4 ha) + Bulkley River 37,837 | 0 cells outside the full run + Cesford Creek 20,169 | 127 outside (0.6%) + Richfield Creek 25,436 | 318 outside (1.3%) + Robert Hatch Creek 12,789 | 274 outside (2.1%) + unnamed 17,885 | 28 outside (0.2%) +UNION of per-group runs: 54,123 | 510 cells in a group run but NOT in the full run + | 22 full-run cells in no group run +``` + +Two conclusions: + +1. Per-group runs are not a decomposition of the whole-network run — they disagree in both + directions, worst on small tributaries (Robert Hatch 2.1%). "The Morice floodplain" would depend + on what else was in the run. This is the argument against floodplains#40's approach B. +2. Per-group areas sum to 1,141 ha against a 536 ha whole (2.1x). Confluence overlap is large, not a + thin seam — it must be represented, not resolved away. + +## Coverage guarantee + +Every valley cell belongs to >=1 group. A valley cell passed the global criteria, so its nearest +seed is within `max_width/2` and its global cost is under threshold; that seed belongs to some group +`g`, and cost from `g`'s seeds alone reproduces the global cost exactly (the min over a subset +containing the argmin equals the global min). Hence no orphans — provided the corridor crop is large +enough to contain the least-cost path, which the crop-safety test pins down. + +## Performance baseline (bundled tile, 648x800 = 518,400 cells) + +- `fl_valley_confine()`: 1.36 s +- `fl_cost_distance()`: 0.13 s + +Test suite stays fast; no `skip_on_ci()` needed. (Note the convention that `skip_on_cran()` does not +skip on CI — `NOT_CRAN=true` is set there.) + +## Field requirement driving the output shape + +The need is to filter polygons by watercourse and click a point to see whether it is in the Morice. +Overlapping per-group rows with the key column deliver both: an identify in the confluence band +returns both rows, and an attribute filter toggles a watercourse. The Morice polygon terminating +where the Morice mainstem does is what gives the longitudinal "within vs upstream of" boundary the +sampling frame needs. + +## Deferred — not built here + +- **Primary / contested split** — splitting each group's polygon into "primary" and "shared" parts. + The overlapping cover already answers the field question by filter-and-click; add only if a + sampling design needs a hard partition. +- **Converging `subset` / `break_points.csv` with this grouping abstraction** — driver-side, raised + in floodplains#40's design notes, a separate decision. + +## Unrelated defect noticed (not fixed on this branch) + +`vignettes/valley-confinement.Rmd` uses `\@ref(fig:...)` in 8 places. Under `bookdown::html_vignette2` +these do not resolve — the live site renders literal text: + +``` +<p>Burn the stream network onto the DEM grid (Figure @ref(fig:plot-dem)) +``` + +Confirmed against https://newgraphenvironment.github.io/flooded/articles/valley-confinement.html on +2026-08-27. Pre-existing and out of scope here (surgical changes); wants its own issue. The new +section added by this branch uses natural language instead. diff --git a/planning/active/progress.md b/planning/active/progress.md new file mode 100644 index 0000000..b33de79 --- /dev/null +++ b/planning/active/progress.md @@ -0,0 +1,11 @@ +# Progress — fl_valley_attribute() (#40) + +## Session 2026-08-27 + +- Read NewGraphEnvironment/floodplains#40; established that this is the package-side half and that + the driver half stays there (`floodplains/CLAUDE.md:9-11` — method in packages, driver in repo) +- Plan-mode exploration; measured per-group vs whole-network VCA disagreement before choosing the + mechanism (see findings.md) — phases approved by user +- Refreshed CLAUDE.md soul conventions on main (`5bf7e45`) and audited the plan against them +- Filed #40; created branch `40-fl-valley-attribute-attribute-valley-cel` off main +- Next: PWF baseline commit, spawn Plan-agent review, then Phase 2 tests diff --git a/planning/active/task_plan.md b/planning/active/task_plan.md new file mode 100644 index 0000000..42c6af9 --- /dev/null +++ b/planning/active/task_plan.md @@ -0,0 +1,68 @@ +# Task: fl_valley_attribute() — attribute valley cells to the stream groups that produced them (#40) + +A delineation from `fl_valley_confine()` answers only "where is this network's floodplain?", not +"where is the **Morice River's** floodplain?". Package-side half of +NewGraphEnvironment/floodplains#40; the driver half (config surface, key column on the gpkg) stays +there. + +Mechanism: delineate once on the full network (delineation never changes), then attribute each +valley cell to every stream group that can reach it, reusing the two stream-dependent geometric +criteria per group. The flood mask stays global — recomputing it per group is what makes per-group +runs unstable (measured; see findings.md). + +## Phase 1: Issue + PWF baseline + +- [x] Draft the mechanism issue, cross-referencing NewGraphEnvironment/floodplains#40 +- [x] File as #40; create branch `40-fl-valley-attribute-attribute-valley-cel` off main +- [x] Lock the name `fl_valley_attribute()` pre-baseline (verb form, matches `fl_valley_confine()`) +- [ ] Land PWF baseline (task_plan.md, findings.md, progress.md) +- [ ] Spawn concurrent Plan-agent review -> `planning/active/review-40.md` (do not wait on it) + +## Phase 2: Tests first — the contract + +`tests/testthat/test-fl_valley_attribute.R`, failing until Phase 3. Bundled `inst/testdata/` +(`dem.tif`, `streams.gpkg` — has `gnis_name`, `blue_line_key`, `stream_order`). + +- [ ] **Coverage, no orphans** — every valley cell falls in >=1 group +- [ ] **Containment** — each group's polygons lie within the global valley extent +- [ ] **Overlap preserved** — confluence cells belong to >=2 groups; count > 0 +- [ ] **Degenerate grouping** — a single constant group reproduces `fl_valley_poly()` output +- [ ] **Grouping invariance** — `gnis_name` vs `blue_line_key` give the same union +- [ ] **Crop safety** — one group attributed on a cropped window == on the full grid +- [ ] **Errors / edges** — missing `group` column, non-`sf` streams, geometry mismatch, `NA` group + values, a group whose streams fall outside the valley +- [ ] **Delineation untouched** — `fl_valley_confine()` output unchanged by this branch + +## Phase 3: Implement `fl_valley_attribute()` + +- [ ] `R/fl_valley_attribute.R` — validate inputs, `compareGeom()`, `group` names a column +- [ ] Derive slope from `dem` as `fl_valley_confine.R:135-138` when `slope = NULL` +- [ ] Per group: crop to the group's bbox + margin (`max_width`), reusing `fl_stream_rasterize()`, + `fl_mask_distance()`, `fl_cost_distance()`, `fl_mask()`, `fl_valley_poly()` +- [ ] Intersect with the cropped global valley raster, polygonize, tag with the group value +- [ ] Return `sf`, one row per group; overlapping rows where ground is shared +- [ ] `NA` group values form their own group (keeps the coverage guarantee); documented +- [ ] roxygen: runnable `@examples` on bundled data, `@seealso`, note that `max_width` / + `cost_threshold` must match the VCA run +- [ ] `devtools::document()` + +## Phase 4: Verify, document, release + +- [ ] Re-run the measurements against the new function; confirm coverage / overlap numerically +- [ ] Time it and note the scaling shape (MORR: k=33 by `gnis_name`, k=340 by `blue_line_key`) +- [ ] Vignette section in `vignettes/valley-confinement.Rmd` — natural language, no `\@ref()` + (does not resolve under `html_vignette2`); `fig.cap` on chunks +- [ ] `lintr::lint_package()`, `devtools::test()`, `devtools::check()` clean +- [ ] `NEWS.md` + version bump 0.3.2 -> 0.4.0 as the **final** commit + +## Phase 5: Hand off to the driver + +- [ ] **Edit** floodplains#40's body to fold in the measurement + the shipped API (findings are the + spec, not commentary); comment only as the cross-repo pointer + +## Validation + +- [ ] Tests pass +- [ ] `/code-check` clean on each commit +- [ ] PWF checkboxes match landed work +- [ ] `/planning-archive` on completion, then `/gh-pr-push` From 2e3477249489733abe66bfda371cfc79acc80cb6 Mon Sep 17 00:00:00 2001 From: almac2022 <al@newgraphenvironment.com> Date: Thu, 27 Aug 2026 16:14:21 -0700 Subject: [PATCH 3/6] Add fl_valley_attribute() test contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Failing tests that define per-group attribution: total coverage of valley cells, containment within the delineation, overlap preserved at confluences, the degenerate single-group case reproducing fl_valley_poly(), grouping invariance, and a no-crop oracle for corridor cropping. Encodes one thing the design has to handle: fl_valley_confine() adds cells after the mask intersection (cleanup, channel buffer, waterbodies — the last with no spatial filter), so a threshold test alone leaves orphan valley cells. Coverage needs a documented fallback, and complete = FALSE isolates the strict cover for testing. Relates to #40 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U9SAqmvFeENADk4rYcbtHS --- planning/active/task_plan.md | 20 +- tests/testthat/test-fl_valley_attribute.R | 212 ++++++++++++++++++++++ 2 files changed, 222 insertions(+), 10 deletions(-) create mode 100644 tests/testthat/test-fl_valley_attribute.R diff --git a/planning/active/task_plan.md b/planning/active/task_plan.md index 42c6af9..dbe19d6 100644 --- a/planning/active/task_plan.md +++ b/planning/active/task_plan.md @@ -15,23 +15,23 @@ runs unstable (measured; see findings.md). - [x] Draft the mechanism issue, cross-referencing NewGraphEnvironment/floodplains#40 - [x] File as #40; create branch `40-fl-valley-attribute-attribute-valley-cel` off main - [x] Lock the name `fl_valley_attribute()` pre-baseline (verb form, matches `fl_valley_confine()`) -- [ ] Land PWF baseline (task_plan.md, findings.md, progress.md) -- [ ] Spawn concurrent Plan-agent review -> `planning/active/review-40.md` (do not wait on it) +- [x] Land PWF baseline (task_plan.md, findings.md, progress.md) +- [x] Spawn concurrent Plan-agent review -> `planning/active/review-40.md` (do not wait on it) ## Phase 2: Tests first — the contract `tests/testthat/test-fl_valley_attribute.R`, failing until Phase 3. Bundled `inst/testdata/` (`dem.tif`, `streams.gpkg` — has `gnis_name`, `blue_line_key`, `stream_order`). -- [ ] **Coverage, no orphans** — every valley cell falls in >=1 group -- [ ] **Containment** — each group's polygons lie within the global valley extent -- [ ] **Overlap preserved** — confluence cells belong to >=2 groups; count > 0 -- [ ] **Degenerate grouping** — a single constant group reproduces `fl_valley_poly()` output -- [ ] **Grouping invariance** — `gnis_name` vs `blue_line_key` give the same union -- [ ] **Crop safety** — one group attributed on a cropped window == on the full grid -- [ ] **Errors / edges** — missing `group` column, non-`sf` streams, geometry mismatch, `NA` group +- [x] **Coverage, no orphans** — every valley cell falls in >=1 group +- [x] **Containment** — each group's polygons lie within the global valley extent +- [x] **Overlap preserved** — confluence cells belong to >=2 groups; count > 0 +- [x] **Degenerate grouping** — a single constant group reproduces `fl_valley_poly()` output +- [x] **Grouping invariance** — `gnis_name` vs `blue_line_key` give the same union +- [x] **Crop safety** — one group attributed on a cropped window == on the full grid +- [x] **Errors / edges** — missing `group` column, non-`sf` streams, geometry mismatch, `NA` group values, a group whose streams fall outside the valley -- [ ] **Delineation untouched** — `fl_valley_confine()` output unchanged by this branch +- [x] **Delineation untouched** — `fl_valley_confine()` output unchanged by this branch ## Phase 3: Implement `fl_valley_attribute()` diff --git a/tests/testthat/test-fl_valley_attribute.R b/tests/testthat/test-fl_valley_attribute.R new file mode 100644 index 0000000..02bbf0d --- /dev/null +++ b/tests/testthat/test-fl_valley_attribute.R @@ -0,0 +1,212 @@ +# Shared fixtures — fl_valley_confine() is ~1.4 s on the bundled tile, so build +# the delineation once and reuse it across tests. +attr_fixture <- local({ + cache <- NULL + function() { + if (is.null(cache)) { + dem <- terra::rast(testdata_path("dem.tif")) + streams <- sf::st_read(testdata_path("streams.gpkg"), quiet = TRUE) + precip_r <- fl_stream_rasterize(streams, dem, field = "map_upstream") + valleys <- fl_valley_confine(dem, streams, + field = "upstream_area_ha", precip = precip_r) + cache <<- list(dem = dem, streams = streams, valleys = valleys) + } + cache + } +}) + +# Rasterize an attribution result back onto the valley grid so cell sets can be +# compared exactly. Polygons come from cells, so their edges fall on cell edges +# and centre-based rasterization recovers the original cells. +cells_of <- function(x, template) { + r <- terra::rasterize(terra::vect(x), template, field = 1L, background = 0L) + v <- terra::values(r) == 1L + v[is.na(v)] <- FALSE + v +} + +valley_cells <- function(valleys) { + v <- terra::values(valleys) == 1L + v[is.na(v)] <- FALSE + v +} + +test_that("fl_valley_attribute returns one sf row per group, keyed by the group column", { + f <- attr_fixture() + + out <- fl_valley_attribute(f$valleys, f$streams, group = "gnis_name", dem = f$dem) + + expect_s3_class(out, "sf") + expect_true("gnis_name" %in% names(out)) + expect_setequal(out$gnis_name, unique(f$streams$gnis_name)) + expect_equal(sf::st_crs(out), sf::st_crs(f$valleys)) + expect_true(all(sf::st_is_valid(out))) +}) + +test_that("every valley cell is attributed to at least one group", { + f <- attr_fixture() + + out <- fl_valley_attribute(f$valleys, f$streams, group = "gnis_name", dem = f$dem) + + covered <- rep(FALSE, terra::ncell(f$valleys)) + for (i in seq_len(nrow(out))) covered <- covered | cells_of(out[i, ], f$valleys) + + expect_gt(sum(valley_cells(f$valleys)), 0) + expect_equal(sum(valley_cells(f$valleys) & !covered), 0) +}) + +test_that("attribution invents no ground outside the delineation", { + f <- attr_fixture() + + out <- fl_valley_attribute(f$valleys, f$streams, group = "gnis_name", dem = f$dem) + + for (i in seq_len(nrow(out))) { + expect_equal(sum(cells_of(out[i, ], f$valleys) & !valley_cells(f$valleys)), 0) + } +}) + +test_that("overlap at confluences is preserved, not resolved to one watercourse", { + f <- attr_fixture() + + out <- fl_valley_attribute(f$valleys, f$streams, group = "gnis_name", dem = f$dem) + + n_per_group <- vapply(seq_len(nrow(out)), + function(i) sum(cells_of(out[i, ], f$valleys)), integer(1)) + union_cells <- rep(FALSE, terra::ncell(f$valleys)) + for (i in seq_len(nrow(out))) union_cells <- union_cells | cells_of(out[i, ], f$valleys) + + # Shared ground means the parts sum to more than the whole. + expect_gt(sum(n_per_group), sum(union_cells)) +}) + +test_that("a single constant group reproduces the whole delineation", { + f <- attr_fixture() + streams <- f$streams + streams$one <- "all" + + out <- fl_valley_attribute(f$valleys, streams, group = "one", dem = f$dem) + + expect_equal(nrow(out), 1L) + expect_equal(cells_of(out, f$valleys), valley_cells(f$valleys)) +}) + +test_that("changing the grouping key relabels without moving the union", { + f <- attr_fixture() + + by_name <- fl_valley_attribute(f$valleys, f$streams, group = "gnis_name", dem = f$dem) + by_blk <- fl_valley_attribute(f$valleys, f$streams, group = "blue_line_key", dem = f$dem) + + u1 <- rep(FALSE, terra::ncell(f$valleys)) + for (i in seq_len(nrow(by_name))) u1 <- u1 | cells_of(by_name[i, ], f$valleys) + u2 <- rep(FALSE, terra::ncell(f$valleys)) + for (i in seq_len(nrow(by_blk))) u2 <- u2 | cells_of(by_blk[i, ], f$valleys) + + expect_equal(u1, u2) + expect_false(nrow(by_name) == nrow(by_blk)) +}) + +test_that("corridor cropping does not change the answer", { + # Oracle: compute one group's membership on the full grid with no cropping, + # using the same exported primitives the function is built from. + f <- attr_fixture() + streams <- f$streams + grp <- "Cesford Creek" + + sub <- streams[!is.na(streams$gnis_name) & streams$gnis_name == grp, ] + sub$seed <- 1 + slope_deg <- terra::terrain(f$dem, "slope", unit = "degrees") + slope <- tan(slope_deg * pi / 180) * 100 + seeds <- fl_stream_rasterize(sub, f$dem, field = "seed") + ref <- f$valleys * + fl_mask_distance(seeds, threshold = 2000 / 2) * + fl_mask(fl_cost_distance(slope, seeds), threshold = 2500, operator = "<") + ref_cells <- terra::values(ref) == 1L + ref_cells[is.na(ref_cells)] <- FALSE + + # complete = FALSE isolates the threshold test from the coverage fallback. + out <- fl_valley_attribute(f$valleys, streams, group = "gnis_name", + dem = f$dem, complete = FALSE) + got <- cells_of(out[!is.na(out$gnis_name) & out$gnis_name == grp, ], f$valleys) + + expect_equal(got, ref_cells) +}) + +test_that("complete = FALSE leaves unreachable valley cells unattributed and reports them", { + f <- attr_fixture() + + strict <- fl_valley_attribute(f$valleys, f$streams, group = "gnis_name", + dem = f$dem, complete = FALSE) + full <- fl_valley_attribute(f$valleys, f$streams, group = "gnis_name", dem = f$dem) + + u_strict <- rep(FALSE, terra::ncell(f$valleys)) + for (i in seq_len(nrow(strict))) u_strict <- u_strict | cells_of(strict[i, ], f$valleys) + + # Morphological cleanup, the channel buffer and waterbodies all add valley + # cells after the mask intersection in fl_valley_confine(), so the strict + # cover is a subset and the fallback is what makes coverage total. + n_fallback <- attr(full, "fl_fallback_cells") + expect_type(n_fallback, "integer") + expect_equal(sum(valley_cells(f$valleys) & !u_strict), n_fallback) +}) + +test_that("NA group values form their own group and stay covered", { + f <- attr_fixture() + expect_true(any(is.na(f$streams$gnis_name))) + + out <- fl_valley_attribute(f$valleys, f$streams, group = "gnis_name", dem = f$dem) + + expect_equal(sum(is.na(out$gnis_name)), 1L) + expect_gt(sum(cells_of(out[is.na(out$gnis_name), ], f$valleys)), 0) +}) + +test_that("fl_valley_attribute does not mutate its inputs", { + f <- attr_fixture() + before <- terra::values(f$valleys) + + fl_valley_attribute(f$valleys, f$streams, group = "gnis_name", dem = f$dem) + + expect_equal(terra::values(f$valleys), before) +}) + +test_that("fl_valley_attribute rejects bad input", { + f <- attr_fixture() + + expect_error( + fl_valley_attribute(f$valleys, f$streams, group = "not_a_column", dem = f$dem), + "not_a_column" + ) + expect_error( + fl_valley_attribute(f$valleys, sf::st_drop_geometry(f$streams), + group = "gnis_name", dem = f$dem), + "sf" + ) + expect_error( + fl_valley_attribute(f$valleys, f$streams, group = c("gnis_name", "blue_line_key"), + dem = f$dem), + "length" + ) + expect_error( + fl_valley_attribute(f$valleys, f$streams, group = "gnis_name"), + "slope|dem" + ) + + shifted <- terra::shift(f$dem, dx = 100000) + expect_error( + fl_valley_attribute(f$valleys, f$streams, group = "gnis_name", dem = shifted), + "extent|resolution|CRS" + ) +}) + +test_that("a group whose streams miss the valley yields an empty-but-present result", { + f <- attr_fixture() + streams <- f$streams + # Relabel the single segment furthest from the valley floor as its own group. + streams$grp <- ifelse(seq_len(nrow(streams)) == which.min(streams$upstream_area_ha), + "isolated", "main") + + out <- fl_valley_attribute(f$valleys, streams, group = "grp", dem = f$dem, + complete = FALSE) + + expect_true(all(c("isolated", "main") %in% out$grp) || nrow(out) == 1L) + expect_s3_class(out, "sf") +}) From 74a7919bab6955c35bf760f709f6f1e38ee1f598 Mon Sep 17 00:00:00 2001 From: almac2022 <al@newgraphenvironment.com> Date: Thu, 27 Aug 2026 17:01:46 -0700 Subject: [PATCH 4/6] Add fl_valley_attribute() for per-watercourse floodplain attribution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Attributes a finished delineation to the stream groups that produced it, without recomputing the delineation. A cell belongs to group g when it is a valley cell and satisfies, for g's streams alone, the two stream-dependent VCA criteria: within max_width/2 and cost-distance under cost_threshold. Rows overlap where ground is shared, which near a confluence it genuinely is. Per-group VCA runs were rejected on measurement, not taste: they disagree with the whole-network run in both directions (510 cells present in a group run and absent from the full run; 22 the reverse), so "the Morice floodplain" would depend on what else was in the run. Coverage is total by construction. fl_valley_confine() adds cells after intersecting its masks — cleanup, channel buffer, and waterbodies, the last with no spatial filter — so a threshold test alone orphans them (1,643 cells with the bundled waterbodies). Those are assigned to the nearest group, with the count reported and exposed as attr(x, "fl_fallback_cells"). Also fixes a latent bug in fl_valley_poly(): renaming an sf column by position detached the geometry column when as.polygons() returned no features, so fl_valley_poly() on a tile with no floodplain produced an sf that errored on every accessor. Same idiom that three review rounds kept finding; an internal assert on the return path now closes the family. Vignette gains a "Whose floodplain is it?" section. Relates to #40 Relates to NewGraphEnvironment/floodplains#40 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U9SAqmvFeENADk4rYcbtHS --- NAMESPACE | 1 + R/fl_valley_attribute.R | 347 ++++++++++++++++++++++ R/fl_valley_poly.R | 15 +- man/fl_valley_attribute.Rd | 144 +++++++++ planning/active/findings.md | 69 ++++- planning/active/progress.md | 21 +- planning/active/review-40.md | 198 ++++++++++++ planning/active/review-round1.md | 115 +++++++ planning/active/review-round2.md | 173 +++++++++++ planning/active/review-round3.md | 184 ++++++++++++ planning/active/task_plan.md | 39 ++- tests/testthat/test-fl_valley_attribute.R | 229 ++++++++++++-- tests/testthat/test-fl_valley_poly.R | 20 ++ vignettes/valley-confinement.Rmd | 76 +++++ 14 files changed, 1579 insertions(+), 52 deletions(-) create mode 100644 R/fl_valley_attribute.R create mode 100644 man/fl_valley_attribute.Rd create mode 100644 planning/active/review-40.md create mode 100644 planning/active/review-round1.md create mode 100644 planning/active/review-round2.md create mode 100644 planning/active/review-round3.md diff --git a/NAMESPACE b/NAMESPACE index 3250178..ce80733 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -14,6 +14,7 @@ export(fl_patch_conn) export(fl_patch_rm) export(fl_scenarios) export(fl_stream_rasterize) +export(fl_valley_attribute) export(fl_valley_confine) export(fl_valley_poly) import(terra) diff --git a/R/fl_valley_attribute.R b/R/fl_valley_attribute.R new file mode 100644 index 0000000..71efc3b --- /dev/null +++ b/R/fl_valley_attribute.R @@ -0,0 +1,347 @@ +#' Attribute valley cells to the stream groups that produced them +#' +#' Takes a completed delineation from [fl_valley_confine()] and works out which +#' part of it belongs to which watercourse (or reach, or any other grouping of +#' the stream network). The delineation itself is never recomputed, so grouping +#' changes relabel the output without moving a boundary. +#' +#' @param valleys A binary (`0`/`1`) `SpatRaster`, the output of +#' [fl_valley_confine()]. +#' @param streams An `sf` linestring object — the same network the delineation +#' was built from. +#' @param group Character. Name of the column in `streams` to group by, e.g. +#' `"gnis_name"` or `"blue_line_key"`. `NA` values form their own group. +#' @param dem A `SpatRaster` of elevation, used only to derive `slope`. Ignored +#' when `slope` is supplied; one of the two is required. +#' @param slope A `SpatRaster` of percent slope. If `NULL`, derived from `dem`. +#' @param max_width Numeric. Maximum valley width in map units (metres). +#' Default `2000`. Must match the value used for the delineation. +#' @param cost_threshold Numeric. Maximum accumulated cost distance. +#' Default `2500`. Must match the value used for the delineation. +#' @param crop_margin Numeric. Width in map units (metres) added around each +#' group's bounding box before its cost distance is computed. Default +#' `max_width`. See Details. +#' @param complete Logical. If `TRUE` (default), valley cells that no group +#' reaches within the thresholds are assigned to the group whose streams are +#' nearest, so every valley cell is attributed. If `FALSE`, they are left +#' unattributed. See Details. +#' +#' @return An `sf` polygon object with one row per group: a `valley` column and +#' a column named after `group`. Rows overlap where ground is shared between +#' watercourses. Groups that yield no cells are omitted, with a warning naming +#' them. The number of valley cells that fell outside every group's thresholds +#' is attached as the attribute `"fl_fallback_cells"` — these are assigned to +#' the nearest group when `complete = TRUE` and left unattributed otherwise, so +#' the count reports the same quantity in both modes. +#' +#' @details +#' A cell is attributed to group `g` when it is a valley cell **and** it +#' satisfies, for `g`'s streams alone, the two stream-dependent criteria the +#' VCA already applies to the whole network: +#' +#' ``` +#' member(cell, g) <=> valley(cell) +#' AND distance(cell, streams_g) <= max_width / 2 +#' AND cost(cell, streams_g) < cost_threshold +#' ``` +#' +#' A cell can satisfy this for more than one group, and near a confluence it +#' usually does — ground there genuinely belongs to both floodplains, so the +#' output rows overlap rather than partitioning the valley. +#' +#' The flood mask is deliberately **not** recomputed per group. Re-running the +#' delineation on a subset of the network changes it: the flood surface is +#' interpolated from every seed cell (see [fl_flood_depth()]), the distance and +#' cost criteria loosen as seeds are added, and morphological cleanup couples +#' patches. Attributing a single delineation instead keeps "the floodplain of +#' this river" independent of whatever else was in the run. +#' +#' ## Coverage +#' +#' [fl_valley_confine()] adds cells after intersecting its masks — morphological +#' closing, hole filling, the channel buffer, and waterbody polygons, which get +#' no spatial filter at all. Those cells can fall outside every group's distance +#' and cost thresholds. With `complete = TRUE` they are assigned to the group +#' with the nearest streams so the attribution covers the delineation exactly; +#' the count is reported and available as `attr(x, "fl_fallback_cells")`. Use +#' `complete = FALSE` to see only cost-reachable ground. +#' +#' ## Corridor cropping +#' +#' Each group's cost distance is computed on a crop around that group's own +#' streams, expanded by `crop_margin`. This is an approximation, not a bound: a +#' least-cost path can in principle leave the crop and return, and across +#' near-flat ground a long detour costs very little. On the bundled test data the +#' default of `max_width` — twice the corridor half-width — reproduced the +#' uncropped cost surface exactly, while `max_width / 2` left 200 corridor cells +#' differing by up to 217 cost units. Those particular cells sat far enough from +#' the threshold that membership did not change, but a tighter crop does drop +#' ground silently (at `crop_margin = 500` on the same tile, 33,860 cells). +#' Widen it if group corridors are unusually convoluted. +#' +#' ## Performance +#' +#' Attributing the bundled tile by `gnis_name` (5 groups, 518,400 cells) takes +#' about 0.7 s against 1.4 s for the delineation itself. That saving comes from +#' the crop, so it shrinks as a group's bounding box approaches the full grid — +#' a long sinuous mainstem is the worst case, and a run with hundreds of groups +#' on a multi-million-cell raster has not been measured. Supplying `slope` +#' avoids re-deriving it from `dem`. +#' +#' @seealso [fl_valley_confine()], [fl_valley_poly()], [fl_cost_distance()], +#' [fl_mask_distance()] +#' +#' @examples +#' dem <- terra::rast(system.file("testdata/dem.tif", package = "flooded")) +#' streams <- sf::st_read( +#' system.file("testdata/streams.gpkg", package = "flooded"), +#' quiet = TRUE +#' ) +#' precip_r <- fl_stream_rasterize(streams, dem, field = "map_upstream") +#' valleys <- fl_valley_confine(dem, streams, +#' field = "upstream_area_ha", precip = precip_r) +#' +#' # Which part of the floodplain belongs to which watercourse? +#' by_stream <- fl_valley_attribute(valleys, streams, group = "gnis_name", +#' dem = dem) +#' by_stream[, c("gnis_name")] +#' +#' # One named river's floodplain, on its own — it ends where the river does +#' terra::plot(dem, main = "Bulkley River floodplain") +#' plot(sf::st_geometry(by_stream[!is.na(by_stream$gnis_name) & +#' by_stream$gnis_name == "Bulkley River", ]), +#' add = TRUE, col = "#0000ff40", border = "blue") +#' +#' @export +fl_valley_attribute <- function(valleys, streams, group, + dem = NULL, slope = NULL, + max_width = 2000, cost_threshold = 2500, + crop_margin = max_width, + complete = TRUE) { + stopifnot( + inherits(valleys, "SpatRaster"), + inherits(streams, "sf"), + is.character(group), length(group) == 1L, + is.numeric(max_width), length(max_width) == 1L, max_width > 0, + is.numeric(cost_threshold), length(cost_threshold) == 1L, cost_threshold > 0, + is.numeric(crop_margin), length(crop_margin) == 1L, crop_margin > 0, + is.logical(complete), length(complete) == 1L + ) + + if (identical(group, "geometry")) { + stop("`group` cannot be \"geometry\" — it would collide with the output's ", + "geometry column.", call. = FALSE) + } + + if (!group %in% names(streams)) { + stop("`group` '", group, "' not found in `streams`. Available columns: ", + paste(setdiff(names(streams), attr(streams, "sf_column")), collapse = ", "), + call. = FALSE) + } + + if (is.null(slope)) { + if (is.null(dem)) { + stop("Supply either `slope` or `dem` so friction can be derived.", call. = FALSE) + } + stopifnot(inherits(dem, "SpatRaster")) + if (!terra::compareGeom(valleys, dem, stopOnError = FALSE)) { + stop("`dem` must have the same extent, resolution, and CRS as `valleys`.", + call. = FALSE) + } + slope_deg <- terra::terrain(dem, "slope", unit = "degrees") + slope <- tan(slope_deg * pi / 180) * 100 + } else { + stopifnot(inherits(slope, "SpatRaster")) + if (!terra::compareGeom(valleys, slope, stopOnError = FALSE)) { + stop("`slope` must have the same extent, resolution, and CRS as `valleys`.", + call. = FALSE) + } + } + + if (sf::st_crs(streams) != terra::crs(valleys)) { + streams <- sf::st_transform(streams, terra::crs(valleys)) + } + + keys <- streams[[group]] + levels_grp <- unique(keys) + levels_grp <- c(sort(levels_grp[!is.na(levels_grp)]), levels_grp[is.na(levels_grp)][1]) + levels_grp <- levels_grp[!(is.na(levels_grp) & !any(is.na(keys)))] + + valley_cells <- which(terra::values(valleys, mat = FALSE) == 1L) + + # --- Per-group membership on a corridor crop --- + cells_by_group <- vector("list", length(levels_grp)) + reasons <- rep(NA_character_, length(levels_grp)) + for (i in seq_along(levels_grp)) { + g <- levels_grp[i] + rows <- if (is.na(g)) is.na(keys) else !is.na(keys) & keys == g + res <- fl_group_cells(streams[rows, ], valleys, slope, + max_width, cost_threshold, crop_margin) + cells_by_group[[i]] <- res$cells + reasons[i] <- res$reason + } + + covered <- unique(unlist(cells_by_group, use.names = FALSE)) + uncovered <- setdiff(valley_cells, covered) + + # --- Coverage fallback: nearest group by distance --- + n_assigned <- 0L + if (length(uncovered) > 0L) { + usable <- isTRUE(complete) && nrow(streams) > 0L && + !all(sf::st_is_empty(sf::st_geometry(streams))) + if (usable) { + idx_streams <- streams + idx_streams$fl_group_idx <- match(keys, levels_grp) + idx_r <- fl_stream_rasterize(idx_streams, valleys, field = "fl_group_idx") + # No burned cells means nothing to measure distance from. + if (!all(is.na(terra::values(idx_r, mat = FALSE)))) { + nearest <- terra::distance(idx_r, target = NA, values = TRUE) + assigned <- terra::values(nearest, mat = FALSE)[uncovered] + for (i in seq_along(levels_grp)) { + add <- uncovered[!is.na(assigned) & assigned == i] + if (length(add) > 0L) { + cells_by_group[[i]] <- c(cells_by_group[[i]], add) + n_assigned <- n_assigned + length(add) + } + } + } + } + cli::cli_alert_info(paste( + "{length(uncovered)} valley cell{?s} outside every group's thresholds -", + "{n_assigned} assigned to the nearest group." + )) + } + + # A group that still has no cells gets no row, which breaks the one-row-per-group + # contract silently — at k in the hundreds that reads as "this river has no + # floodplain" rather than "this river was not processed". Warn only after the + # fallback has run: a group that scored zero on the thresholds can still pick up + # cells there, and naming it as omitted would be false. + empty_grp <- lengths(cells_by_group) == 0L + if (any(empty_grp)) { + dropped <- vapply(which(empty_grp), function(i) { + why <- if (is.na(reasons[i])) "no valley cells" else reasons[i] + paste0(levels_grp[i], " (", why, ")") + }, character(1)) + warning("No valley cells attributed to ", length(dropped), " group", + if (length(dropped) > 1L) "s" else "", ", omitted from the output: ", + paste(dropped, collapse = "; "), call. = FALSE) + } + + # --- Polygonize each group --- + parts <- list() + for (i in seq_along(levels_grp)) { + cells <- cells_by_group[[i]] + if (length(cells) == 0L) next + poly <- fl_cells_poly(cells, valleys) + poly[[group]] <- levels_grp[i] + parts[[length(parts) + 1L]] <- poly + } + + if (length(parts) == 0L) { + empty <- list(valley = integer(0)) + empty[[group]] <- keys[0] + empty$geometry <- sf::st_sfc(crs = sf::st_crs(valleys)) + out <- fl_check_out(sf::st_sf(empty, sf_column_name = "geometry"), group) + attr(out, "fl_fallback_cells") <- length(uncovered) + return(out) + } + + out <- fl_check_out(do.call(rbind, parts), group) + attr(out, "fl_fallback_cells") <- length(uncovered) + out +} + +#' Assert the output kept its geometry and group columns +#' +#' Three rounds of review found the same family of defect: an `sf` mutated in +#' place losing track of which column is the geometry. Cheaper to assert than to +#' keep remembering the rule. +#' +#' @param out The `sf` about to be returned. +#' @param group Name of the group column. +#' +#' @return `out`, unchanged. +#' @noRd +fl_check_out <- function(out, group) { + if (!identical(attr(out, "sf_column"), "geometry") || !group %in% names(out)) { + stop("Internal error: attribution output lost its geometry or group column.", + call. = FALSE) + } + out +} + +#' Cell indices a single stream group reaches +#' +#' @param streams_g An `sf` subset for one group. +#' @param valleys The binary valley `SpatRaster`. +#' @param slope Percent-slope `SpatRaster` matching `valleys`. +#' @param max_width,cost_threshold VCA thresholds. +#' @param crop_margin Width added around the group's bounding box. +#' +#' @return List of `cells` (integer cell indices into `valleys`) and `reason` +#' (`NA` when cells were found, otherwise why none were). +#' @noRd +fl_group_cells <- function(streams_g, valleys, slope, max_width, cost_threshold, + crop_margin) { + none <- function(reason) list(cells = integer(0), reason = reason) + if (nrow(streams_g) == 0L) return(none("no stream segments")) + + # Empty geometries are routine after st_intersection() clipping, and + # terra::ext() errors on them rather than returning an empty extent — one bad + # row would otherwise abort every other group. + streams_g <- streams_g[!sf::st_is_empty(sf::st_geometry(streams_g)), ] + if (nrow(streams_g) == 0L) return(none("all geometries empty")) + + e <- terra::ext(terra::vect(sf::st_geometry(streams_g))) + e <- terra::extend(e, crop_margin) + e <- terra::intersect(e, terra::ext(valleys)) + # terra::intersect() returns NULL, not an empty extent, when they miss. + if (is.null(e)) return(none("streams outside the valley raster")) + + slope_c <- terra::crop(slope, e, snap = "out") + valleys_c <- terra::crop(valleys, e, snap = "out") + + streams_g$fl_seed <- 1 + seeds <- fl_stream_rasterize(streams_g, slope_c, field = "fl_seed") + # fl_stream_rasterize() uses touches = FALSE, so segments that never cross a + # cell centre burn nothing. + if (all(is.na(terra::values(seeds, mat = FALSE)))) { + return(none("segments do not cross a cell centre")) + } + + member <- valleys_c * + fl_mask_distance(seeds, threshold = max_width / 2) * + fl_mask(fl_cost_distance(slope_c, seeds), threshold = cost_threshold, + operator = "<") + + local_cells <- which(terra::values(member, mat = FALSE) == 1L) + if (length(local_cells) == 0L) return(none("no valley cells within the thresholds")) + + list(cells = terra::cellFromXY(valleys, terra::xyFromCell(member, local_cells)), + reason = NA_character_) +} + +#' Polygonize a set of cell indices +#' +#' Builds the raster on the cells' own bounding box rather than the full grid, +#' so per-group polygonization stays proportional to the group's footprint. +#' +#' @param cells Integer vector of cell indices into `template`. +#' @param template The full-grid `SpatRaster`. +#' +#' @return An `sf` polygon object with a `valley` column. +#' @noRd +fl_cells_poly <- function(cells, template) { + xy <- terra::xyFromCell(template, cells) + half <- terra::res(template) / 2 + e <- terra::ext(min(xy[, 1]) - half[1], max(xy[, 1]) + half[1], + min(xy[, 2]) - half[2], max(xy[, 2]) + half[2]) + tmpl <- terra::crop(template, e, snap = "out") + + r <- terra::rast(tmpl) + terra::values(r) <- NA_integer_ + r[terra::cellFromXY(tmpl, xy)] <- 1L + + fl_valley_poly(r) +} diff --git a/R/fl_valley_poly.R b/R/fl_valley_poly.R index 7f6f1fa..d85caf8 100644 --- a/R/fl_valley_poly.R +++ b/R/fl_valley_poly.R @@ -31,10 +31,19 @@ fl_valley_poly <- function(x, dissolve = TRUE) { # Polygonize polys <- terra::as.polygons(x_mask, dissolve = dissolve) - # Convert to sf and clean + # Name the column on the SpatVector, before it becomes an sf. Renaming by + # position afterwards hits the geometry column when as.polygons() returns no + # features, producing an sf whose sf_column no longer points at a geometry. + names(polys) <- "valley" + out <- sf::st_as_sf(polys) |> sf::st_make_valid() - names(out)[1] <- "valley" - out + # An all-NA mask yields a SpatVector with no attribute columns, so the name + # above lands nowhere and the promised `valley` column would be missing. + if (!"valley" %in% names(out)) { + out[["valley"]] <- integer(0) + } + + out[, "valley"] } diff --git a/man/fl_valley_attribute.Rd b/man/fl_valley_attribute.Rd new file mode 100644 index 0000000..cd0f155 --- /dev/null +++ b/man/fl_valley_attribute.Rd @@ -0,0 +1,144 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/fl_valley_attribute.R +\name{fl_valley_attribute} +\alias{fl_valley_attribute} +\title{Attribute valley cells to the stream groups that produced them} +\usage{ +fl_valley_attribute( + valleys, + streams, + group, + dem = NULL, + slope = NULL, + max_width = 2000, + cost_threshold = 2500, + crop_margin = max_width, + complete = TRUE +) +} +\arguments{ +\item{valleys}{A binary (\code{0}/\code{1}) \code{SpatRaster}, the output of +\code{\link[=fl_valley_confine]{fl_valley_confine()}}.} + +\item{streams}{An \code{sf} linestring object — the same network the delineation +was built from.} + +\item{group}{Character. Name of the column in \code{streams} to group by, e.g. +\code{"gnis_name"} or \code{"blue_line_key"}. \code{NA} values form their own group.} + +\item{dem}{A \code{SpatRaster} of elevation, used only to derive \code{slope}. Ignored +when \code{slope} is supplied; one of the two is required.} + +\item{slope}{A \code{SpatRaster} of percent slope. If \code{NULL}, derived from \code{dem}.} + +\item{max_width}{Numeric. Maximum valley width in map units (metres). +Default \code{2000}. Must match the value used for the delineation.} + +\item{cost_threshold}{Numeric. Maximum accumulated cost distance. +Default \code{2500}. Must match the value used for the delineation.} + +\item{crop_margin}{Numeric. Width in map units (metres) added around each +group's bounding box before its cost distance is computed. Default +\code{max_width}. See Details.} + +\item{complete}{Logical. If \code{TRUE} (default), valley cells that no group +reaches within the thresholds are assigned to the group whose streams are +nearest, so every valley cell is attributed. If \code{FALSE}, they are left +unattributed. See Details.} +} +\value{ +An \code{sf} polygon object with one row per group: a \code{valley} column and +a column named after \code{group}. Rows overlap where ground is shared between +watercourses. Groups that yield no cells are omitted, with a warning naming +them. The number of valley cells that fell outside every group's thresholds +is attached as the attribute \code{"fl_fallback_cells"} — these are assigned to +the nearest group when \code{complete = TRUE} and left unattributed otherwise, so +the count reports the same quantity in both modes. +} +\description{ +Takes a completed delineation from \code{\link[=fl_valley_confine]{fl_valley_confine()}} and works out which +part of it belongs to which watercourse (or reach, or any other grouping of +the stream network). The delineation itself is never recomputed, so grouping +changes relabel the output without moving a boundary. +} +\details{ +A cell is attributed to group \code{g} when it is a valley cell \strong{and} it +satisfies, for \code{g}'s streams alone, the two stream-dependent criteria the +VCA already applies to the whole network: + +\if{html}{\out{<div class="sourceCode">}}\preformatted{member(cell, g) <=> valley(cell) + AND distance(cell, streams_g) <= max_width / 2 + AND cost(cell, streams_g) < cost_threshold +}\if{html}{\out{</div>}} + +A cell can satisfy this for more than one group, and near a confluence it +usually does — ground there genuinely belongs to both floodplains, so the +output rows overlap rather than partitioning the valley. + +The flood mask is deliberately \strong{not} recomputed per group. Re-running the +delineation on a subset of the network changes it: the flood surface is +interpolated from every seed cell (see \code{\link[=fl_flood_depth]{fl_flood_depth()}}), the distance and +cost criteria loosen as seeds are added, and morphological cleanup couples +patches. Attributing a single delineation instead keeps "the floodplain of +this river" independent of whatever else was in the run. +\subsection{Coverage}{ + +\code{\link[=fl_valley_confine]{fl_valley_confine()}} adds cells after intersecting its masks — morphological +closing, hole filling, the channel buffer, and waterbody polygons, which get +no spatial filter at all. Those cells can fall outside every group's distance +and cost thresholds. With \code{complete = TRUE} they are assigned to the group +with the nearest streams so the attribution covers the delineation exactly; +the count is reported and available as \code{attr(x, "fl_fallback_cells")}. Use +\code{complete = FALSE} to see only cost-reachable ground. +} + +\subsection{Corridor cropping}{ + +Each group's cost distance is computed on a crop around that group's own +streams, expanded by \code{crop_margin}. This is an approximation, not a bound: a +least-cost path can in principle leave the crop and return, and across +near-flat ground a long detour costs very little. On the bundled test data the +default of \code{max_width} — twice the corridor half-width — reproduced the +uncropped cost surface exactly, while \code{max_width / 2} left 200 corridor cells +differing by up to 217 cost units. Those particular cells sat far enough from +the threshold that membership did not change, but a tighter crop does drop +ground silently (at \code{crop_margin = 500} on the same tile, 33,860 cells). +Widen it if group corridors are unusually convoluted. +} + +\subsection{Performance}{ + +Attributing the bundled tile by \code{gnis_name} (5 groups, 518,400 cells) takes +about 0.7 s against 1.4 s for the delineation itself. That saving comes from +the crop, so it shrinks as a group's bounding box approaches the full grid — +a long sinuous mainstem is the worst case, and a run with hundreds of groups +on a multi-million-cell raster has not been measured. Supplying \code{slope} +avoids re-deriving it from \code{dem}. +} +} +\examples{ +dem <- terra::rast(system.file("testdata/dem.tif", package = "flooded")) +streams <- sf::st_read( + system.file("testdata/streams.gpkg", package = "flooded"), + quiet = TRUE +) +precip_r <- fl_stream_rasterize(streams, dem, field = "map_upstream") +valleys <- fl_valley_confine(dem, streams, + field = "upstream_area_ha", precip = precip_r) + +# Which part of the floodplain belongs to which watercourse? +by_stream <- fl_valley_attribute(valleys, streams, group = "gnis_name", + dem = dem) +by_stream[, c("gnis_name")] + +# One named river's floodplain, on its own — it ends where the river does +terra::plot(dem, main = "Bulkley River floodplain") +plot(sf::st_geometry(by_stream[!is.na(by_stream$gnis_name) & + by_stream$gnis_name == "Bulkley River", ]), + add = TRUE, col = "#0000ff40", border = "blue") + +} +\seealso{ +\code{\link[=fl_valley_confine]{fl_valley_confine()}}, \code{\link[=fl_valley_poly]{fl_valley_poly()}}, \code{\link[=fl_cost_distance]{fl_cost_distance()}}, +\code{\link[=fl_mask_distance]{fl_mask_distance()}} +} diff --git a/planning/active/findings.md b/planning/active/findings.md index b540225..2f17af0 100644 --- a/planning/active/findings.md +++ b/planning/active/findings.md @@ -13,7 +13,15 @@ streams are supplied: | flood model | yes | IDW interpolates the surface from *all* seed cells (`fl_flood_depth.R:68-80`) | | cleanup (patch removal, hole fill) | yes, globally coupled | a sub-`size_threshold` patch may survive when merged | -Measured on bundled Bulkley data, grouping by `gnis_name`: +Measured on bundled Bulkley data, grouping by `gnis_name`. The exact call matters — recording it +so the numbers stay falsifiable, because bare `fl_valley_confine(dem, streams)` gives 16,268 cells, +not 53,635: + +```r +precip_r <- fl_stream_rasterize(streams, dem, field = "map_upstream") +valleys <- fl_valley_confine(dem, streams, field = "upstream_area_ha", precip = precip_r) +``` + ``` FULL network: 53,635 cells (536.4 ha) @@ -34,18 +42,57 @@ Two conclusions: 2. Per-group areas sum to 1,141 ha against a 536 ha whole (2.1x). Confluence overlap is large, not a thin seam — it must be represented, not resolved away. -## Coverage guarantee +## Coverage guarantee — as first stated it was FALSE -Every valley cell belongs to >=1 group. A valley cell passed the global criteria, so its nearest +The original argument: every valley cell belongs to >=1 group. A valley cell passed the global criteria, so its nearest seed is within `max_width/2` and its global cost is under threshold; that seed belongs to some group `g`, and cost from `g`'s seeds alone reproduces the global cost exactly (the min over a subset -containing the argmin equals the global min). Hence no orphans — provided the corridor crop is large -enough to contain the least-cost path, which the crop-safety test pins down. +containing the argmin equals the global min). + +The premise is wrong for real `fl_valley_confine()` output. That function returns the mask +intersection **plus** morphological cleanup, the channel buffer, and waterbodies — and waterbodies +get no spatial filter at all (`fl_valley_confine.R:213-220`), so a lake can sit outside every +group's distance and cost thresholds. Measured with `inst/testdata/waterbodies.gpkg`: + +``` +valley cells orphans under the strict criteria +53,635 0 # no waterbodies +55,345 1,643 # + waterbodies -> 3.0% orphaned +``` + +Cleanup does it too, but only where the thresholds bind — at `cost_threshold = 300`, 56 cells +(0.53%); at defaults on this tile, 0. So a coverage test written only at defaults passes for the +wrong reason. + +Resolution: `complete = TRUE` (default) assigns leftover valley cells to the group with the nearest +streams, so coverage holds by construction rather than by an argument about an input the function +does not control. The count is reported and exposed as `attr(x, "fl_fallback_cells")`. +`complete = FALSE` exposes the strict cover for testing. ## Performance baseline (bundled tile, 648x800 = 518,400 cells) - `fl_valley_confine()`: 1.36 s - `fl_cost_distance()`: 0.13 s +- `fl_valley_attribute()` by `gnis_name` (k=5): 0.74 s — cheaper than the delineation itself + +The saving comes from the corridor crop, and it shrinks as a group's bounding box approaches the +full grid. On this tile the crops are already 39-74% of the grid, and a long sinuous mainstem is the +worst case — the Morice's bbox is close to its whole AOI. k=340 by `blue_line_key` on a 27M-cell +raster is therefore NOT claimed; it needs measuring on a production tile before the driver leans on +it. + +## Crop margin is an approximation, not a bound + +Per-group cost on a bbox+margin crop vs the full grid, unnamed-group streams: + +``` +margin = 1000 m (= max_width/2) : 200 corridor cells differ, max delta 216.9 cost units +margin = 2000 m (= max_width) : 0 cells differ +``` + +Least-cost paths really are truncated at `max_width/2`. No finite margin is a bound in general — +across near-flat ground a detour of arbitrary length costs arbitrarily little. Hence `crop_margin` +is an argument (default `max_width`) documented as an approximation. Test suite stays fast; no `skip_on_ci()` needed. (Note the convention that `skip_on_cran()` does not skip on CI — `NOT_CRAN=true` is set there.) @@ -66,6 +113,18 @@ sampling frame needs. - **Converging `subset` / `break_points.csv` with this grouping abstraction** — driver-side, raised in floodplains#40's design notes, a separate decision. +## Pre-existing bug found during review — filed as #41, not fixed here + +`fl_cost_distance()` sets stream cells to 0 and calls `costDist(target = 0)`, so **every** cell whose +friction is exactly 0 becomes a seed, not just stream cells — and the roxygen claims otherwise. On a +synthetic grid a flat patch 380 m from the only stream cell reported cost 0 instead of ~1400. + +The bundled DEM hides it (0 cells exactly zero; min slope 1.42e-14), but hydro-flattened or +integer-metre DEMs do not. It matters more under attribution than under the global VCA: a flat patch +is a free zero-cost source for whichever group's crop contains it, so cost stops discriminating +between groups exactly where floodplains are. Filed as #41 because the fix changes VCA output on +affected DEMs and deserves its own diff. + ## Unrelated defect noticed (not fixed on this branch) `vignettes/valley-confinement.Rmd` uses `\@ref(fig:...)` in 8 places. Under `bookdown::html_vignette2` diff --git a/planning/active/progress.md b/planning/active/progress.md index b33de79..22ddc97 100644 --- a/planning/active/progress.md +++ b/planning/active/progress.md @@ -8,4 +8,23 @@ mechanism (see findings.md) — phases approved by user - Refreshed CLAUDE.md soul conventions on main (`5bf7e45`) and audited the plan against them - Filed #40; created branch `40-fl-valley-attribute-attribute-valley-cel` off main -- Next: PWF baseline commit, spawn Plan-agent review, then Phase 2 tests +- PWF baseline `6a9d9d9`; test contract `2e34772` (failing by design) +- Implemented `fl_valley_attribute()` + two internal helpers; 33 tests green +- Concurrent Plan-agent review returned 19 findings -> `planning/active/review-40.md`. It confirmed + the waterbody coverage hole independently (1,643 orphaned cells, already fixed before it landed) + and found several I had missed: + - grouping-invariance test was vacuous (`gnis_name` / `blue_line_key` are a bijection on this + tile) -> replaced with a genuine coarsening test + - coverage only tested at non-binding thresholds -> added a `cost_threshold = 300` case + - crop margin unproven -> `crop_margin` is now an argument, documented as an approximation + - a group whose segments miss every cell centre vanished silently -> now warns, with a test + - argument order fought `fl_valley_confine()` -> `dem` before `slope` +- Filed #41 for a pre-existing `fl_cost_distance()` bug the review found (every zero-friction cell + is treated as a seed); not fixed here — it changes VCA output on affected DEMs +- 42 tests green; vignette section added +- Code-check rounds 1-3 (`review-round{1,2,3}.md`): 4 + 3 + 2 findings, all fixed. Round 2 found a + bug inside round 1's fix; round 3 converged and identified the mechanism (renaming an `sf` column + by position), which also fixed a latent corrupt-`sf` bug in `fl_valley_poly()` on an empty + delineation and added an internal assert on the return path +- 233 tests green; `R CMD check` 0 errors / 0 warnings / 1 pre-existing NOTE (`pkgdown/` at top level) +- Next: NEWS + version bump, archive, PR, reconcile both issue bodies diff --git a/planning/active/review-40.md b/planning/active/review-40.md new file mode 100644 index 0000000..f8fd07a --- /dev/null +++ b/planning/active/review-40.md @@ -0,0 +1,198 @@ +# Review — fl_valley_attribute() plan (#40) + +Plan-agent review, reviewed against the code at `6a9d9d9`. Every claim was reproduced by running +the real functions on `inst/testdata/`. 19 findings: 3 Blocker / 6 Gap / 2 Ordering / 3 Assumption / +2 Scope / 3 Acceptance. + +Verdict: the mechanism is sound as an algorithm on the raw criteria, and the seed-decomposition +argument was confirmed empirically. But the plan's central guarantee is stated against +`fl_valley_confine()`'s mask intersection (lines 140-160) while the function returns mask +intersection PLUS cleanup PLUS channel buffer PLUS waterbodies (lines 162-220). + +## BLOCKER 1 — Coverage guarantee false for real fl_valley_confine() output + +`fl_valley_confine.R:213-220` ORs waterbodies in after the mask intersection; those cells never +passed the distance or cost criterion, so no group's predicate can hold. + +``` +valley cells orphans (valley==1 but outside dist<=1000 AND cost<2500) +53,635 0 # no waterbodies +55,345 1,643 # + inst/testdata/waterbodies.gpkg -> 3.0% orphaned +53,261 0 # channel_buffer = FALSE +``` + +Fix: residual pass — assign leftover valley cells to the nearest / argmin-cost group so coverage +holds by construction, and report the residual count as a diagnostic. + +STATUS: FIXED before this review landed. `complete = TRUE` fallback assigns by nearest group; +`attr(x, "fl_fallback_cells")` reports the count; the waterbody case is now a non-vacuous test. + +## BLOCKER 2 — Cleanup breaks coverage too, but not at the defaults + +``` +max_width cost_threshold valley orphans +2000 2500 16,268 0 + 200 2500 6,396 5 (0.08%) +2000 300 10,574 56 (0.53%) +2000 150 7,255 57 (0.79%) +``` + +At package defaults the coverage test passes for the wrong reason — the criteria are not binding. +Fix: parameterise the coverage test; run at a binding `cost_threshold`, not only at defaults. + +STATUS: FIXED — added a binding-threshold coverage test. + +## BLOCKER 3 — fl_cost_distance() seeds every zero-friction cell + +`fl_cost_distance.R:51-52` sets stream cells to 0 then calls `costDist(target = 0)`, so ANY cell +with friction exactly 0 becomes a seed. Roxygen at `:9-12` says only stream cells are seeds — the +documentation is wrong about the function's own behaviour. + +Synthetic 50x50 grid, friction 10%, one stream cell, a flat (0%) patch 380 m away: + +``` +cost at flat-patch centre : 0 (should be ~1400) +cost one cell beyond the patch : 50 (should be ~1450) -> 29x understated +cells with cost 0 : 122 (stream cells: 1) +``` + +Bundled DEM masks this (0 cells exactly zero). Production DEMs — integer-metre, hydro-flattened +lakes, void-filled plateaus — do produce exact zeros. Matters MORE under attribution: a flat patch +is a free zero-cost source for whichever group's crop contains it, so cost stops discriminating +between groups exactly where floodplains are. + +Fix: floor the friction before seeding (`ifel(friction <= 0, 1e-6, friction)`). Verified to restore +correct values with no change on bundled data. + +STATUS: NOT fixed here — pre-existing behaviour in a different function, filed as its own issue. + +## GAP 4 — Required crop margin unproven + +``` +margin = 1000 m (= max_width/2) : 200 corridor cells differ, max delta 216.9 cost units +margin = 2000 m (= max_width) : 0 cells differ, max delta 0.000 +``` + +Least-cost paths ARE truncated at max_width/2. No finite bound exists in general — across near-flat +ground a detour of arbitrary length costs arbitrarily little. + +Fix: expose `crop_margin` (default `max_width`), document as an approximation. + +STATUS: FIXED — `crop_margin` argument added and documented as an approximation. + +## GAP 5 — Crop-safety test near-vacuous; perf framing overstated + +Group bbox + 2000 m margin crops are 39-74% of the bundled grid, so the crop can barely fail, and +at k=5 the crops sum to 276% of a full-grid pass. + +Fix: soften the "scales with network length" claim; make the crop test meaningful. + +STATUS: PARTIALLY FIXED — claim softened in docs/issue; a small-margin test now pins the +approximation. A genuine large-grid synthetic remains unbuilt. + +## GAP 6 — Grouping-invariance test vacuous twice over + +On bundled data `gnis_name` and `blue_line_key` are a bijection (5 groups each), so any +implementation passes. The union over groups is also a tautology in exact arithmetic. + +Fix: test a genuine coarsening — coarse group == union of its fine members, cell for cell. + +STATUS: FIXED — replaced with a coarsening test. + +## GAP 7 — A group whose streams rasterize to zero cells is unhandled + +`touches = FALSE` means a segment that misses every cell centre burns nothing; the group then +vanishes silently. Near-certain at k=340 on MORR. + +Fix: `warning()` naming the group (not `cli_alert_warning()`, per CLAUDE.md — not catchable). + +STATUS: FIXED — warning names the dropped group values. + +## GAP 8 — NA propagation, and slope must never be derived on a crop + +`terra::terrain()` puts NA in the outer one-cell ring (2,892 cells on the bundled DEM = exactly the +border), and `fl_cost_distance()` treats NA friction as impassable — so deriving slope per crop +would wall every crop. + +STATUS: ALREADY CORRECT — slope is derived once on the full DEM, then cropped per group. + +## GAP 9 — Signature omits `field`; template should be `valleys` + +STATUS: NOT APPLICABLE to the implementation — it adds its own constant seed column and rasterizes +onto the cropped slope grid, which is derived from `valleys`. + +## ORDERING 10 — Argument order fights the existing convention + +`fl_valley_confine(dem, streams, field, slope, ...)` puts `dem` first. Fix: `dem` before `slope`. + +STATUS: FIXED. + +## ORDERING 11 — Review concurrent with Phase 2 invalidates the test contract + +STATUS: MOOT — the two contract-changing findings (BLOCKER 1, GAP 6) were resolved by edit. + +## ASSUMPTION 12 — Parameter drift enforced only by a roxygen note + +Nothing detects a `valleys` raster produced at `max_width = 1000` being attributed at 2000. + +STATUS: ACCEPTED AS-IS — the fallback count is reported at runtime and exposed as an attribute, +which surfaces drift as an unusually large residual. A hard warning would fire on the legitimate +waterbody case, which is common. + +## ASSUMPTION 13 — Performance story unmeasured at production scale + +A bbox is a poor proxy for a long sinuous mainstem: the Morice's bbox is close to the whole AOI, so +the largest group gets no saving. + +STATUS: CLAIM SOFTENED — measured k=5 at 0.74 s (vs 1.36 s for the delineation itself); k=340 on a +27M-cell raster is explicitly not claimed. + +## ASSUMPTION 14 — findings.md numbers not reproducible without the exact call + +`fl_valley_confine(dem, streams)` gives 16,268 cells; the reported 53,635 needs +`field = "upstream_area_ha"` and `precip = fl_stream_rasterize(streams, dem, "map_upstream")`. + +STATUS: FIXED — the exact parameterisation is now recorded in findings.md. + +## SCOPE 15 — waterbodies / channel_buffer / size_threshold invisible to the API + +STATUS: DOCUMENTED — `@details` names waterbodies specifically as a source of fallback cells. + +## SCOPE 16 — New parameters want a row in the parameter legend + +STATUS: DECLINED — `fl_params()` documents VCA model parameters; `crop_margin` is an implementation +knob, not part of the model. + +## ACCEPTANCE 17 — "Delineation untouched" not testable as written + +STATUS: FIXED — replaced with an input-mutation test (terra objects are C++ pointers, so this is a +real risk), plus `git diff -- R/fl_valley_confine.R` being empty on the branch. + +## ACCEPTANCE 18 — Vacuous assertions in the Phase 2 list + +Containment is true by construction and passes on a 0-row result; overlap "count > 0" passes on a +single cell. Measured anchors at `channel_buffer = FALSE` defaults, by `gnis_name`: + +``` +valley cells 16,268 | Bulkley 14,842 | Richfield 6,528 | Cesford 5,944 | unnamed 4,184 +Robert Hatch 3,003 | sum 34,501 = 2.12x the valley | uncovered 0 +``` + +STATUS: FIXED — assertions anchored to group count and an overlap-ratio band. + +## ACCEPTANCE 19 — Both issue bodies need editing, not just the driver one + +flooded#40's Acceptance list contains the two bullets disproved above. + +STATUS: FIXED — flooded#40 body edited with the disproof and the fallback policy. + +## What the reviewer checked and could not fault + +- The subset/argmin decomposition is correct on the raw criteria: union of all five group + memberships covers all 16,268 valley cells, 0 uncovered. +- Overlap is genuinely large (2.12x by cell count), so overlapping rows are the right output shape. +- Keeping the flood mask global is right: `fl_flood_depth.R:68-71` builds the IDW point set from ALL + non-NA flood_surface cells, so a per-group flood surface really would move the answer. +- Operator consistency: `fl_mask_distance()` uses `<=`, the cost mask uses `<`. The predicate + matches both. +- Test-suite runtime: `skip_on_ci()` genuinely unnecessary. diff --git a/planning/active/review-round1.md b/planning/active/review-round1.md new file mode 100644 index 0000000..70fe79c --- /dev/null +++ b/planning/active/review-round1.md @@ -0,0 +1,115 @@ +# Review round 1 — `fl_valley_attribute()` staged diff + +Target: `git diff --cached` at review time (`R/fl_valley_attribute.R`, +`tests/testthat/test-fl_valley_attribute.R`, `man/`, `NAMESPACE`). +Line numbers below are from the **staged** blob (`git show :R/fl_valley_attribute.R`). + +Note: the working tree has already moved past the index (adds `crop_margin`, reorders +`dem`/`slope`, adds a `no_seeds` warning, returns a list from `fl_group_cells()`). All +probes below were run against the **staged** code in a sandbox copy so the review matches +what is about to be committed. Where a worktree change already addresses a finding it is +called out. + +Staged tests pass: `FAIL 0 | WARN 0 | SKIP 0 | PASS 33`. + +## Findings + +- **[bug]** `R/fl_valley_attribute.R:188-193` — the zero-parts branch returns a + structurally **corrupt `sf`** that cannot be printed, plotted, or written. + `sf::st_sf(valley = integer(0), sf::st_sfc(crs = ...))` makes the unnamed `sfc` the + *geometry* column with an auto-generated name (`sf..st_sfc.crs...sf..st_crs.valleys..`). + `names(out)[2] <- group` then renames **the geometry column** to e.g. `"gnis_name"` + while `attr(out, "sf_column")` still points at the old name. The result has no group + column at all, and every sf accessor errors: + `attr(obj, "sf_column") does not point to a geometry column.` + Reproduced end-to-end against the staged code on the bundled tile: + ```r + out <- fl_valley_attribute(valleys * 0, streams, group = "gnis_name", dem = dem) + names(out) # "valley" "gnis_name" + attr(out, "sf_column") # "sf..st_sfc.crs...sf..st_crs.valleys.." + st_geometry(out); print(out); plot(out); st_write(out, f) # all error + ``` + Reachable whenever no group yields a cell — an empty/over-thresholded delineation, or a + tile with no floodplain. No test exercises the branch, so it ships untested. Build the + group column explicitly and let `st_sf()` name the geometry, e.g. + `sf::st_sf(valley = integer(0), setNames(list(keys[0]), group), geometry = sf::st_sfc(crs = sf::st_crs(valleys)))` + — never rename an sf column by position. Still present in the working tree. + +- **[bug]** `R/fl_valley_attribute.R:180-186` (with `:210-215`) — a group whose streams lie + **entirely outside the valley raster** is silently dropped from the output: no row, no + warning, no error. `fl_group_cells()` returns `integer(0)` via the `is.null(e)` guard and + the polygonize loop `next`s. Reproduced: appending one stream segment translated 500 km + and grouping on it returns rows for `main` only, with `far` absent. `@return` promises + "one row per group", so a caller doing + `merge(attribution, per_group_stats, by = "gnis_name")` loses that group with no signal — + the missing row reads as "this river has no floodplain" rather than "this river was not + processed". The staged test at `test-fl_valley_attribute.R:213-225` explicitly permits + the drop (`|| nrow(out) == 1L`) rather than pinning the behaviour, so tests pass while the + contract is broken. The working tree added a `no_seeds` warning for the sibling + "rasterized to nothing" case but left this path silent. + +- **[fragile]** `R/fl_valley_attribute.R:191,196` — `fl_fallback_cells` is set to + `length(uncovered)` unconditionally, including under `complete = FALSE` where **nothing + was assigned**. `@return` and the Coverage section both describe it as "the number of + cells assigned by the coverage fallback". Measured on the waterbody fixture: + `complete = FALSE` returns `fl_fallback_cells = 1643` for a run that attributed zero + fallback cells. Anyone using the attribute as a QA metric ("how much ground was guessed?") + gets a false positive. Either zero it when `complete` is `FALSE` or redocument it as + "cells outside every group's thresholds". + +- **[fragile]** `R/fl_valley_attribute.R:143-145` — the comment claims the crop margin makes + the answer exact: *"Margin is max_width (twice the corridor half-width) so a least-cost + path to a candidate cell cannot be clipped by the crop edge."* That is not a bound. + `terra::costDist` treats everything outside the crop as impassable, and a least-cost + detour across flat ground can leave and re-enter the corridor over an arbitrary distance, + so cropping can only ever *over*-estimate cost and silently drop edge cells. Verified + exact on the bundled tile — cropped vs full-grid oracle for **all five** `gnis_name` + groups gave `missing = 0, extra = 0` — so there is no observable defect here, but the + guarantee in the comment is false and the single-group test + (`test-fl_valley_attribute.R:109-133`) cannot catch it on other terrain. The working tree + has already reworded this and exposed `crop_margin`; it should not land in the index in + its current form. + +## Probed and clean + +Each of these was reproduced against the staged code on `inst/testdata/`, not reasoned about: + +- **Cropped-cell → global-cell mapping is exact.** `cellFromXY(valleys, xyFromCell(member, …))` + matched a full-grid oracle for every group (`ref == got`, 0 missing / 0 extra; 49,989 / + 20,087 / 25,187 / 12,651 / 23,367 cells). The half-cell tolerance in `cellFromXY` swamps + any float drift from the `snap = "out"` crop. +- **`terra::extend(SpatExtent, n)` and `terra::intersect(SpatExtent, SpatExtent)` are the + right calls.** terra 1.9.34: `extend` grows all four sides by `n`; `intersect` returns + **`NULL`** (not an empty extent, no warning) when the extents miss, so the `is.null(e)` + guard on line 215 fires correctly. Partly-outside groups clamp to the raster via `crop`. +- **`levels_grp` NA handling is correct** for: no NAs (trailing `NA` correctly filtered), + all NAs (single `NA` group kept), the literal string `"NA"` (treated as an ordinary + group, and correctly distinct from a real `NA` when both are present), numeric keys, and + factor keys. `match(keys, levels_grp)` also matches `NA` to the `NA` level, so the + fallback index raster is right. (Only a factor built with `exclude = NULL` — an explicit + `NA` *level* — produces a duplicated group; `sf::st_read()` never yields that.) +- **The `cli` glue string renders correctly**, including `{ifelse(...)}` with single-quoted + branches and `{?s}` pluralization: verified singular, plural, `complete = TRUE`, and + `complete = FALSE`. `cli` is declared in `Imports`. +- **No input mutation.** `identical(snapshot, streams)` is `TRUE` after a call (the + `streams_g$fl_seed` / `idx_streams$fl_group_idx` assignments hit R copies), and + `terra::values(valleys)` is unchanged. No terra pointer aliasing — `terra::rast(tmpl)` + builds a fresh raster and `crop()` returns new objects. +- **`attr(out, "fl_fallback_cells")` survives `do.call(rbind, parts)`** (it is set after the + rbind) and is `integer`, as the test asserts. +- **`fl_cells_poly()` is fine at both ends of the range**: a single cell yields a polygon of + exactly one cell area (100 m² at 10 m), and the full grid (518,400 cells, exercised by the + "single constant group" test) completes without issue. `terra::values(r) <- NA_integer_` + recycles correctly and keeps integer type. +- **The roxygen `@examples` block runs** end to end and produces 5 rows. + +## Notes, not findings + +- `fl_stream_rasterize(..., fun = "max")` on the fallback index raster shadows the + lower-indexed group where two groups burn the same cell — 4 cells out of 1,607 on the + bundled tile. Only nudges nearest-group ties at confluences; not worth changing. +- Passing `group = "valley"` silently overwrites the `valley` marker column that + `fl_valley_poly()` adds. Output stays a valid `sf` with the group labels in `valley`, so + nothing breaks — just be aware the schema collapses to one column. +- The cli message on line 161 is ~190 characters, over the 120-char `line_length_linter` + in the project `.lintr` config. Style only, listed so it isn't a surprise in CI. diff --git a/planning/active/review-round2.md b/planning/active/review-round2.md new file mode 100644 index 0000000..2a65a36 --- /dev/null +++ b/planning/active/review-round2.md @@ -0,0 +1,173 @@ +# Review round 2 — the four round-1 fixes + +Target: `git diff --cached` on branch `40-fl-valley-attribute-attribute-valley-cel`. +Worktree == index at review time, so line numbers below are live file line numbers in +`R/fl_valley_attribute.R`. + +Baseline: `devtools::test()` → `FAIL 0 | WARN 0 | SKIP 0 | PASS 221`. `devtools::document()` +produces no diff (man/ and NAMESPACE are in sync). `vignettes/valley-confinement.Rmd` +renders end to end (67/67 chunks) under `bookdown::html_document2`. + +## Findings + +- **[bug]** `R/fl_valley_attribute.R:180-187` — the dropped-group `warning()` is emitted + **before** the coverage fallback runs, so with the default `complete = TRUE` it can name a + group as *"omitted from the output"* that is in fact present in the output with + substantial area. The warning is built from `reasons[]`, which only records the outcome of + the threshold pass (lines 168-175). Lines 202-214 then hand uncovered valley cells to the + nearest group — including a group that scored zero on thresholds — and line 219-225 + polygonizes it into a row. + + Reproduced on the bundled tile: a 30 m synthetic segment placed on non-valley ground + 22 m from the valley edge and 1022 m from any real stream, run with + `max_width = 10` (so nothing is within `max_width / 2`): + + ``` + WARNING: No valley cells attributed to 1 group, omitted from the output: + Ghost Creek (no valley cells within the thresholds) + output groups: Bulkley River | Cesford Creek | Ghost Creek | Richfield Creek | + Robert Hatch Creek | NA + area of the "omitted" group: 394,700 m2 (39.5 ha) + ``` + + `max_width = 10` is only the cheapest trigger; the general condition is *a group whose + streams are further than `max_width / 2` from every valley cell yet still the nearest + stream to some valley cell*. That is exactly the shape of the case the Coverage section + documents — waterbody polygons get no spatial filter, so a lake 1.5 km from the mapped + network with a tributary group 1.2 km away hits it at package defaults. + + Consequence: the warning asserts something demonstrably false about the returned object. + The round-1 finding was "silently dropped groups"; the fix now mis-reports which groups + were dropped, which is worse than the count being silent — a caller that filters or + re-queues on the warning text will drop or reprocess a group that is already attributed. + The `reason` plumbing in `fl_group_cells()` is fine; only the point at which the warning + is raised is wrong. Warn after the fallback, driven by + `length(cells_by_group[[i]]) == 0L` at polygonize time (keeping `reasons[i]` as the + explanatory text), not by `reasons` alone. + +- **[fragile]** `R/fl_valley_attribute.R:257` — **not every no-cell path returns a reason**; + one of them aborts the whole call. A group whose geometries are all empty + (`st_linestring()` — routine after `st_intersection()` clipping or a sloppy export) + reaches `terra::ext(terra::vect(sf::st_geometry(streams_g)))` with an empty vector and + the call dies: + + ``` + ERR: [ext] invalid extent + ``` + + No reason, no warning, no partial result — one bad row kills the attribution of every + other group. `nrow(streams_g) == 0L` (line 255) does not catch it because the rows exist. + Same class: `fl_valley_attribute(valleys, streams[0, ], …)` with `complete = TRUE` gets + past the (correct) `"no stream segments"` reason and then dies at line 206 with + `[distance] no locations to compute distance from`, after having already printed + `ℹ 53635 valley cells … assigned to the nearest group`. This is pre-existing rather than + introduced by the fix, but it is the gap in the "every no-cell path is covered" claim the + fix is making. + +- **[fragile]** `R/fl_valley_attribute.R:223` (and `:229-231`) — the round-1 geometry-column + clobbering is fixed in the empty branch but survives in the populated one. `fl_cells_poly()` + returns an `sf` whose geometry column is named `geometry`, so + `poly[[group]] <- levels_grp[i]` overwrites it when `group == "geometry"`. Reproduced on + a GeoPackage-read `streams` (its sf column is `geom`, so a plain data column *can* be + named `geometry` and passes the `group %in% names(streams)` check): + + ```r + s$geometry <- ifelse(is.na(s$gnis_name), "unnamed", s$gnis_name) + fl_valley_attribute(valleys, s, group = "geometry", dem = dem) + # ERR: attr(obj, "sf_column") does not point to a geometry column. + ``` + + It fails loudly rather than returning a corrupt object, so it is milder than the round-1 + bug, but it is the same trap. In the empty branch the collision is silent instead: + line 229 writes the group column, line 230 overwrites it with the `sfc`, and the caller + gets a structurally valid `sf` with **no group column at all**. Rejecting + `group == "geometry"` alongside the existing name check closes both. + +## Verified — the four fixes hold on everything else probed + +Each was reproduced by running code, not by reading it. + +**1. Empty sf (`:227-233`).** Genuinely usable, and the group column carries the right type. +For `gnis_name` (character), `blue_line_key` (integer) and a `factor` group column, all of +`names()`, `attr(out, "sf_column") == "geometry"`, `st_geometry()`, `print()` and +`st_write()` to a real `.gpkg` succeed, and the group column comes back `character` / +`integer` / `factor` respectively — `keys[0]` carries the type correctly in all three. +`rbind()` works in both directions with a non-empty result (`rbind(empty, ne)` and +`rbind(ne, empty)` → 5 rows), because the non-empty path also names its geometry column +`geometry` and both carry an `integer` `valley` column. `plot()` errors with +`NA value(s) in bounding box` — but so does a canonical `st_sf(a = integer(0), geometry = +st_sfc(crs = 3005))`, so that is sf's behaviour for any zero-row `sf`, not a defect of this +construction. + +**2. Reason coverage and warning mechanics.** Every `return()` in `fl_group_cells()` except +the crash in the finding above carries a reason, and the success return sets +`NA_character_`, so `any(!is.na(reasons))` cannot misfire on a successful group. Confirmed +each reason actually fires for the case it names: `"streams outside the valley raster"` for +the 500 km translation, `"segments do not cross a cell centre"` for the sub-pixel segment, +`"no valley cells within the thresholds"` for a hillslope segment, `"no stream segments"` +for an empty subset. `warning()` (not `cli::cli_alert_warning()`) is the right call — +`cli_alert_warning()` does not signal a `warning` condition, so `expect_warning()` would not +catch it, `suppressWarnings()` would not suppress it, and `options(warn = 2)` would not +promote it. CLAUDE.md imposes no cli-only convention. Boundary cases around the new crop +are clean: a segment 300 m outside the raster edge (inside `crop_margin`, so +`terra::intersect()` returns a sliver rather than `NULL`) and a segment exactly touching +`xmax` both return `"segments do not cross a cell centre"` without error. + +**3. `fl_fallback_cells` semantics.** `length(uncovered)` is computed at line 190 from the +threshold pass only, before any fallback assignment, in every exit path — the normal return +(`:237`) and the empty-result early return (`:232`). Measured on the waterbody fixture: +`complete = TRUE` → 1643, `complete = FALSE` → 1643. That matches the reworded +`@return` ("cells outside every group's thresholds … the count reports the same quantity in +both modes") exactly. The Coverage section's "covers the delineation exactly" claim also +holds: `sum(valley_cells & !covered) == 0` under `complete = TRUE`. `attr()` is `integer`. + +**4. `crop_margin`.** Threading verified by instrumenting `fl_group_cells()` via +`assignInNamespace()`: `max_width = 600` with no `crop_margin` → every call receives `600` +(the lazy default picks up the caller's `max_width`, as intended); `crop_margin = 137` +explicit → every call receives `137`. There is no second crop site to thread it to — +`fl_cells_poly()` crops to the cells' own bounding box, which needs no margin. +`terra::extend(SpatExtent, n)` confirmed to add `n` in **map units** on all four sides +(`ext(0,10,0,10)` → `-5 15 -5 15`), so the "metres" wording is right. Validation +(`is.numeric`, `length == 1`, `> 0`) is evaluated after `max_width`'s own checks, so the +default expression can never be forced against an invalid `max_width`. Against a full-grid +oracle for all five `gnis_name` groups: `crop_margin = 2000` → 0 missing / 0 extra; +`crop_margin = 500` → 33,860 missing (silently, as documented). + +**Test-suite interaction.** Full suite is `FAIL 0 | WARN 0 | SKIP 0 | PASS 221`, and that +zero is real, not masked: the only two tests that trigger the new `warning()` wrap it in +`expect_warning()`, and the empty-valleys test uses `suppressWarnings()`. The +`cost_threshold = 300` test does not warn (every group keeps its own stream cells, which are +valley cells at cost 0, so no group empties). + +**The `far` test.** `sf::st_geometry(far) <- sf::st_geometry(far) + c(5e5, 5e5)` does drop +the CRS (`st_crs(far)$input` is `NA` afterwards), and without the `st_crs(far) <- …` line +the subsequent `rbind()` fails with `arguments have different crs` — so the reassignment is +load-bearing and the test is not passing by accident. The warning it matches really is +`Elsewhere River (streams outside the valley raster)`, i.e. the `is.null(e)` branch it +claims to exercise. The sub-pixel test likewise fires the reason it advertises +(`Subpixel Creek (segments do not cross a cell centre)`) and the group is genuinely absent +from the output. + +**Vignette.** The new "Whose floodplain is it?" section renders under +`bookdown::html_document2` with no errors or warnings. `valleys_wb` (line 284), `dem`, +`streams`, `sf` and `terra` are all in scope at line 421. Nothing is environment-dependent: +no `system.file()` beyond the bundled testdata already loaded, no network, no `whitebox`, +no absolute paths. The numeric claims check out — sum of attributed parts / delineated area +is 2.40 on `valleys_wb` (text says "roughly 2.4 times"), and `fl_fallback_cells` renders as +`1643`. The one chunk with a figure carries a `fig.cap`, so bookdown numbering works. + +## Notes, not findings + +- The dropped-group warning renders an `NA` group as the literal string `NA` + (`paste0(NA, " (...)")`), indistinguishable from a group whose value is the string + `"NA"`. Diagnostic text only; no behavioural consequence. +- The `crop_margin` docs say the default "reproduced the uncropped answer exactly on the + bundled test data, where `max_width / 2` did not (200 corridor cells differed, by up to + 217 cost units)". Against a full-grid oracle, `crop_margin = max_width / 2` (1000 m) gives + **0 missing / 0 extra** membership on this tile — the 200-cell difference is in the cost + surface, not in the attributed answer. The parenthetical discloses this ("cost units"), + but the lead clause overstates it. The important half of the round-1 fix — that the crop + is an approximation and not a bound — is now stated correctly. +- `vignettes/valley-confinement.Rmd` `plot-attribute` caption says "the hatched overlap near + the confluence belongs to both", but the plot draws two alpha-blended fills and no + hatching. Cosmetic. diff --git a/planning/active/review-round3.md b/planning/active/review-round3.md new file mode 100644 index 0000000..3aa0619 --- /dev/null +++ b/planning/active/review-round3.md @@ -0,0 +1,184 @@ +# Review round 3 — do the round-2 fixes converge? + +Target: worktree == index on `40-fl-valley-attribute-attribute-valley-cel`, so line numbers are +live file line numbers. Baseline re-measured: `devtools::test()` → `FAIL 0 | WARN 0 | SKIP 0 | +PASS 226`; `devtools::document()` produces no diff. + +Short answer: **the four round-2 fixes converge inside `fl_valley_attribute.R`.** Every case the +brief asked about was reproduced and all of them behave. But the *mechanism* behind rounds 1-3 is +still live one call down, and it still produces a corrupt `sf`. + +## Findings + +- **[bug]** `R/fl_valley_poly.R:38` — the round-1 defect was fixed at the instance, not at the + idiom, and the idiom survives in the function that builds **every row** `fl_valley_attribute()` + returns. `names(out)[1] <- "valley"` renames an `sf` column by position. When + `terra::as.polygons()` yields zero features, `sf::st_as_sf()` returns an `sf` whose *only* + column is the geometry column, so line 38 renames **the geometry column** to `"valley"` while + `attr(out, "sf_column")` still says `"geometry"`. Reproduced: + + ```r + o <- fl_valley_poly(valleys * 0L) + nrow(o) # 0 + names(o) # "valley" + attr(o, "sf_column") # "geometry" + sf::st_geometry(o); print(o); plot(o); sf::st_write(o, f) + # all: attr(obj, "sf_column") does not point to a geometry column. + ``` + + That is byte-for-byte the round-1 failure mode. It is **not** reachable from + `fl_valley_attribute()` — `fl_cells_poly()` only calls it with `length(cells) > 0`, so at least + one cell is `1L` and `as.polygons()` always returns a feature (verified: the one-cell case gives + `names = valley, geometry`). It **is** reachable on the path the roxygen advertises: + `fl_valley_poly()` is exported and documented as "the natural final step after + `fl_valley_confine()`", and `fl_valley_confine()` returns an all-zero raster on a tile with no + floodplain — the same object `test-fl_valley_attribute.R:276` builds deliberately. Fix is the + round-1 fix applied here: name the column at construction (`names(polys) <- "valley"` on the + `SpatVector`, before `st_as_sf()`), never by position afterwards. `grep -rn "names(.*)\[[0-9]" + R/` returns exactly this one line, so one edit closes the family. + +- **[fragile]** `R/fl_valley_attribute.R:188-196` — round 2's finding was "a diagnostic emitted + before the state it describes". The fix moved the `warning()` after the fallback and left the + `cli::cli_alert_info()` in front of it, so the same defect shape remains at the line above. + `fate` is chosen from `complete` alone (`:189-193`), then printed (`:194-196`), and only then do + `usable` (`:197`) and the all-NA `idx_r` guard (`:203`) decide whether any assignment happens. + When either guard fires the message asserts an assignment that did not occur. Reproduced, all + three shapes: + + ``` + streams[0, ], complete = TRUE + ℹ 53635 valley cells outside every group's thresholds - assigned to the nearest group. + → 0 rows returned, 0 cells assigned, and NO warning (levels_grp is empty), so this + false line is the only diagnostic the caller gets. + + all geometries empty (usable = FALSE) + ℹ 53635 valley cells ... - assigned to the nearest group. → 0 assigned, 0 rows + + every segment sub-pixel (idx_r all NA) + ℹ 53635 valley cells ... - assigned to the nearest group. → 0 assigned, 0 rows + ``` + + Informational only — the returned object and `fl_fallback_cells` are both correct in all three — + which is why this is `fragile` and not `bug`. But it is the identical mistake round 2 filed, so + it belongs in the same fix: compute the fallback, then report what it did. Note the two + guard-fires can only coincide with a zero-row result (see below), so no run that returns rows + can mis-report. + +## The four round-2 fixes, item by item + +**1. Warning moved after the fallback — converged.** The warning predicate +(`lengths(cells_by_group) == 0L`, `:221`) and the polygonize skip (`length(cells) == 0L`, `:236`) +are now the *same expression over the same object at the same point in time*, with only the +fallback assignment between them and nothing that can empty a group. So it cannot stay silent +about an absent group and cannot name a present one — the two sets are equal by construction, not +by coincidence. `test-fl_valley_attribute.R:314-357` pins the direction that regressed. + +Reason-text accuracy after the fallback: only the `"no valley cells within the thresholds"` reason +can be attached to a group that later wins fallback cells, and such a group is no longer in +`empty_grp`, so its stale reason is never printed. The other three reasons +(`"streams outside the valley raster"`, `"segments do not cross a cell centre"`, +`"all geometries empty"`) all describe groups that burn nothing into `idx_r` either — the fallback +rasterizes with the same `fl_stream_rasterize()` / `touches = FALSE` — so those groups can never +gain fallback cells and their reason cannot go stale. `reasons` is not read anywhere else. + +The `if (is.na(reasons[i])) "no valley cells"` fallback text at `:224` is unreachable (a group with +`reason = NA` returned cells, so it is never in `empty_grp`). Harmless. + +**2. `group == "geometry"` rejected up front — complete for what it guards.** The output geometry +column is unconditionally named `geometry` on both paths (`fl_valley_poly()` via `st_as_sf()`; +`st_sf(..., sf_column_name = "geometry")` at `:246`), verified on the one-cell, full-grid and +zero-row cases. So `"geometry"` is the only value of `group` that can collide with the *output*, +and the guard is exhaustive. + +Probed the three adjacent collisions the brief names, none of them a defect: + +- *`streams` whose active sf column is named something else* — the bundled GeoPackage's is `geom`, + which is why round 2's data column named `geometry` was constructible. `group = "geom"` gets past + both guards (`group %in% names(streams)` does not exclude `attr(streams, "sf_column")`) and dies + at `sort()` on an `sfc` with `both operands of the expression should be "units" objects`. Loud, + no corrupt object, no data loss, and pre-existing — the round-2 guard was never aimed at it. +- *`group == "valley"`* — `poly[["valley"]] <- levels_grp[i]` overwrites the marker column, so the + output has one data column instead of the two `@return` promises. Both branches agree (the empty + branch's `empty[["valley"]] <- keys[0]` overwrites the same slot with the same type), the result + is a structurally valid `sf` with `sf_column = "geometry"` and the right 5 group labels, and + `rbind()` still works. Round 1 already logged this as a note; re-verified, still not a defect. +- *collision after `rbind()`* — `levels_grp` is `unique()`, both branches emit columns + `valley, <group>, geometry` in that order with matching types, so no duplicate key and no column + mismatch can reach `do.call(rbind, parts)`. + +**3. Empty-geometry filter + `usable` + all-NA guards — all four cases clean.** Run on the bundled +tile: + +| case | result | +|---|---| +| `streams[0, ]`, `complete = TRUE` | 0-row `sf`, `fl_fallback_cells = 53635`, no crash | +| all geometries empty, `complete = TRUE` | 0 rows, warning names all 5 groups `(all geometries empty)` | +| all geometries empty, `complete = FALSE` | 0 rows, `fl_fallback_cells = 53635` | +| mix of empty + valid **within one group** (4 of Cesford Creek's 9 rows blanked) | 5 rows, all groups present, `fl_fallback_cells = 94` | +| one group entirely empty, others fine, **with the fallback firing** (`cost_threshold = 300`) | 5 rows, `Empty Creek (all geometries empty)` warned, `fl_fallback_cells = 25051` | +| every segment sub-pixel (`idx_r` all NA), `usable = TRUE` | 0 rows, all 5 warned `(segments do not cross a cell centre)`, no crash | + +The last two matter most. The empty-geometry filter lives in `fl_group_cells()` (`:275`) but the +fallback at `:201` rasterizes the **unfiltered** `streams` — the obvious place for a third variant. +It is clean: `terra::vect()` on an `sf` with empty linestrings does not error, unlike +`terra::ext()`. Worth knowing that the committed test +(`test-fl_valley_attribute.R:359-372`) does **not** cover this — `fl_fallback_cells` is `0` at +package defaults on the plain tile, so that test never enters the fallback block at all. The +`cost_threshold = 300` variant in the table above is what actually exercises it. + +**4. Nested `if` control flow — correct, despite the mis-indentation.** The braces do what they +look like: `:198` gates on `complete && usable`, `:203` gates on `idx_r` having a burned cell, and +on both false paths `cells_by_group` is untouched, so `uncovered` cells are left unattributed — +never mis-assigned. The third leak path is inside the loop: +`terra::distance(target = NA, values = TRUE)` returns **`NaN` at source cells**, not the cell's own +value (verified on a 5x5 toy raster), so a burned cell appearing in `uncovered` would fall through +`!is.na(assigned)` and be dropped rather than mis-assigned. That intersection is empty in practice +and provably so — a cell burned in `idx_r` is burned in its own group's `seeds` by the same +`touches = FALSE` rasterization on a `snap = "out"`-aligned crop, giving distance 0 and cost 0, so +it is always already covered. Measured: at `cost_threshold = 300`, 25,051 uncovered cells, **0** of +them burned in `idx_r`; `sum(valley_cells & !covered) == 0` under `complete = TRUE` on both the +plain and the waterbody fixtures. + +`fl_fallback_cells` is `length(uncovered)` computed once at `:185`, before any assignment, and read +identically at `:247` (empty branch) and `:252` (normal). Measured equal under +`complete = TRUE`/`FALSE` on the waterbody fixture (1643/1643) and on every row of the table above. +Same quantity on every exit path. + +## Mechanism + +Three rounds, three findings, one property: **`fl_valley_attribute()` builds its return value by +mutating an `sf` in place, and reports on it from a different point in the function than where the +value is decided.** Both halves are the same underlying thing — no single place owns the output +object, so nothing can check it. + +- Rounds 1 and 2 (empty branch, `group == "geometry"`) were the column half: `names(out)[2] <- group` + and `poly[[group]] <- ...` write into an `sf` by position or by a name the caller supplies, with + no barrier between "a data column" and "the geometry column". +- Round 2 (warning before the fallback) and finding 2 above are the ordering half: a message is + composed from state that a later block is still free to change. + +Each round fixed the instance it was shown. The idiom was not fixed, which is why the family +regenerates: `names(x)[i] <- ` is still in the tree at `fl_valley_poly.R:38`, and it still yields +the exact corrupt object round 1 described. **One change closes the column half of the family** — +delete the positional rename (name the `SpatVector` column before `st_as_sf()`), after which no +line in `R/` renames an `sf` column by position and the sole remaining name-based write is +`poly[[group]]`, already guarded. **One change closes the ordering half** — move the +`cli_alert_info()` below the fallback block so both diagnostics describe settled state, which is +the rule round 2 already established for the `warning()` and simply did not apply to the line above +it. + +A cheap structural backstop, if you want one that does not depend on remembering the rule: a +three-line assertion before each `return()` (`identical(attr(out, "sf_column"), "geometry")`, +`"valley" %in% names(out)`, `group %in% names(out)`) would have caught round 1, round 2's +`"geometry"` collision, and the `fl_valley_poly()` bug above, none of which any test caught. + +## Notes, not findings + +- `lintr::lint("R/fl_valley_attribute.R")` reports the un-reindented fallback body at `:204` + (6 spaces, should be 8) — the visible residue of fix 4's brace change — and a false-positive + `object_usage_linter` on `fate` (used inside the `cli` glue string). Style only, but the project + `.lintr` is configured to be clean, so CI will show them. +- `group = "geom"` (the streams' own sf column on a GeoPackage read) fails with + `both operands of the expression should be "units" objects` rather than the intended + "column not found" message, because `group %in% names(streams)` does not exclude + `attr(streams, "sf_column")`. Pre-existing, loud, no corruption. diff --git a/planning/active/task_plan.md b/planning/active/task_plan.md index dbe19d6..d114f93 100644 --- a/planning/active/task_plan.md +++ b/planning/active/task_plan.md @@ -35,26 +35,37 @@ runs unstable (measured; see findings.md). ## Phase 3: Implement `fl_valley_attribute()` -- [ ] `R/fl_valley_attribute.R` — validate inputs, `compareGeom()`, `group` names a column -- [ ] Derive slope from `dem` as `fl_valley_confine.R:135-138` when `slope = NULL` -- [ ] Per group: crop to the group's bbox + margin (`max_width`), reusing `fl_stream_rasterize()`, +- [x] `R/fl_valley_attribute.R` — validate inputs, `compareGeom()`, `group` names a column +- [x] Derive slope from `dem` as `fl_valley_confine.R:135-138` when `slope = NULL` +- [x] Per group: crop to the group's bbox + margin (`max_width`), reusing `fl_stream_rasterize()`, `fl_mask_distance()`, `fl_cost_distance()`, `fl_mask()`, `fl_valley_poly()` -- [ ] Intersect with the cropped global valley raster, polygonize, tag with the group value -- [ ] Return `sf`, one row per group; overlapping rows where ground is shared -- [ ] `NA` group values form their own group (keeps the coverage guarantee); documented -- [ ] roxygen: runnable `@examples` on bundled data, `@seealso`, note that `max_width` / +- [x] Intersect with the cropped global valley raster, polygonize, tag with the group value +- [x] Return `sf`, one row per group; overlapping rows where ground is shared +- [x] `NA` group values form their own group (keeps the coverage guarantee); documented +- [x] roxygen: runnable `@examples` on bundled data, `@seealso`, note that `max_width` / `cost_threshold` must match the VCA run -- [ ] `devtools::document()` +- [x] `devtools::document()` ## Phase 4: Verify, document, release -- [ ] Re-run the measurements against the new function; confirm coverage / overlap numerically -- [ ] Time it and note the scaling shape (MORR: k=33 by `gnis_name`, k=340 by `blue_line_key`) -- [ ] Vignette section in `vignettes/valley-confinement.Rmd` — natural language, no `\@ref()` +- [x] Re-run the measurements against the new function; confirm coverage / overlap numerically +- [x] Time it and note the scaling shape (MORR: k=33 by `gnis_name`, k=340 by `blue_line_key`) +- [x] Vignette section in `vignettes/valley-confinement.Rmd` — natural language, no `\@ref()` (does not resolve under `html_vignette2`); `fig.cap` on chunks -- [ ] `lintr::lint_package()`, `devtools::test()`, `devtools::check()` clean +- [x] `lintr::lint_package()`, `devtools::test()`, `devtools::check()` clean - [ ] `NEWS.md` + version bump 0.3.2 -> 0.4.0 as the **final** commit +## Code review (3 rounds, per planning conventions) + +- [x] Round 1 -> `review-round1.md`: 2 bugs (corrupt empty `sf`; silently dropped groups), + 2 fragile. All fixed. +- [x] Round 2 -> `review-round2.md`: a bug *inside* round 1's fix (dropped-group warning fired + before the coverage fallback, so it named groups that were in the output with 39 ha), plus + empty geometries aborting the whole call and a `group = "geometry"` collision. All fixed. +- [x] Round 3 -> `review-round3.md`: converged inside `fl_valley_attribute()`. Named the mechanism + — renaming an `sf` column by position — and found the last instance of it at + `fl_valley_poly.R:38`. Fixed there too, plus an internal assert so the family cannot return. + ## Phase 5: Hand off to the driver - [ ] **Edit** floodplains#40's body to fold in the measurement + the shipped API (findings are the @@ -62,7 +73,7 @@ runs unstable (measured; see findings.md). ## Validation -- [ ] Tests pass -- [ ] `/code-check` clean on each commit +- [x] Tests pass +- [x] `/code-check` clean on each commit - [ ] PWF checkboxes match landed work - [ ] `/planning-archive` on completion, then `/gh-pr-push` diff --git a/tests/testthat/test-fl_valley_attribute.R b/tests/testthat/test-fl_valley_attribute.R index 02bbf0d..e56289f 100644 --- a/tests/testthat/test-fl_valley_attribute.R +++ b/tests/testthat/test-fl_valley_attribute.R @@ -20,13 +20,13 @@ attr_fixture <- local({ # and centre-based rasterization recovers the original cells. cells_of <- function(x, template) { r <- terra::rasterize(terra::vect(x), template, field = 1L, background = 0L) - v <- terra::values(r) == 1L + v <- as.vector(terra::values(r)) == 1L v[is.na(v)] <- FALSE v } valley_cells <- function(valleys) { - v <- terra::values(valleys) == 1L + v <- as.vector(terra::values(valleys)) == 1L v[is.na(v)] <- FALSE v } @@ -90,19 +90,63 @@ test_that("a single constant group reproduces the whole delineation", { expect_equal(cells_of(out, f$valleys), valley_cells(f$valleys)) }) -test_that("changing the grouping key relabels without moving the union", { +test_that("a coarser grouping is exactly the union of its finer members", { + # gnis_name and blue_line_key are a bijection on this tile (5 groups each), so + # comparing them proves nothing. Build a genuine two-level coarsening instead. f <- attr_fixture() + streams <- f$streams + fine <- ifelse(is.na(streams$gnis_name), "unnamed", streams$gnis_name) + streams$fine <- fine + streams$coarse <- ifelse(fine %in% c("Bulkley River", "unnamed"), "A", "B") + + by_fine <- fl_valley_attribute(f$valleys, streams, group = "fine", dem = f$dem) + by_coarse <- fl_valley_attribute(f$valleys, streams, group = "coarse", dem = f$dem) + + expect_equal(nrow(by_fine), 5L) + expect_equal(nrow(by_coarse), 2L) + + for (cg in c("A", "B")) { + members <- unique(streams$fine[streams$coarse == cg]) + u_fine <- rep(FALSE, terra::ncell(f$valleys)) + for (m in members) { + u_fine <- u_fine | cells_of(by_fine[by_fine$fine == m, ], f$valleys) + } + expect_equal(cells_of(by_coarse[by_coarse$coarse == cg, ], f$valleys), u_fine) + } +}) + +test_that("coverage still holds when the cost threshold is binding", { + # At package defaults the criteria are not binding on this tile, so a coverage + # test there can pass for the wrong reason. Squeeze cost_threshold until + # morphological cleanup can push cells past it. + f <- attr_fixture() + + out <- fl_valley_attribute(f$valleys, f$streams, group = "gnis_name", + dem = f$dem, cost_threshold = 300) + + covered <- rep(FALSE, terra::ncell(f$valleys)) + for (i in seq_len(nrow(out))) covered <- covered | cells_of(out[i, ], f$valleys) + + expect_gt(attr(out, "fl_fallback_cells"), 0) + expect_equal(sum(valley_cells(f$valleys) & !covered), 0) +}) - by_name <- fl_valley_attribute(f$valleys, f$streams, group = "gnis_name", dem = f$dem) - by_blk <- fl_valley_attribute(f$valleys, f$streams, group = "blue_line_key", dem = f$dem) +test_that("group counts and overlap are anchored, not merely non-zero", { + f <- attr_fixture() - u1 <- rep(FALSE, terra::ncell(f$valleys)) - for (i in seq_len(nrow(by_name))) u1 <- u1 | cells_of(by_name[i, ], f$valleys) - u2 <- rep(FALSE, terra::ncell(f$valleys)) - for (i in seq_len(nrow(by_blk))) u2 <- u2 | cells_of(by_blk[i, ], f$valleys) + out <- fl_valley_attribute(f$valleys, f$streams, group = "gnis_name", dem = f$dem) - expect_equal(u1, u2) - expect_false(nrow(by_name) == nrow(by_blk)) + expect_equal(nrow(out), 5L) + + per_group <- vapply(seq_len(nrow(out)), + function(i) sum(cells_of(out[i, ], f$valleys)), integer(1)) + expect_true(all(per_group > 0)) + + # Measured 2.45x on this tile; assert a band so the test fails if overlap + # collapses to a partition or explodes. + ratio <- sum(per_group) / sum(valley_cells(f$valleys)) + expect_gt(ratio, 1.5) + expect_lt(ratio, 3.5) }) test_that("corridor cropping does not change the answer", { @@ -120,7 +164,7 @@ test_that("corridor cropping does not change the answer", { ref <- f$valleys * fl_mask_distance(seeds, threshold = 2000 / 2) * fl_mask(fl_cost_distance(slope, seeds), threshold = 2500, operator = "<") - ref_cells <- terra::values(ref) == 1L + ref_cells <- as.vector(terra::values(ref)) == 1L ref_cells[is.na(ref_cells)] <- FALSE # complete = FALSE isolates the threshold test from the coverage fallback. @@ -131,22 +175,34 @@ test_that("corridor cropping does not change the answer", { expect_equal(got, ref_cells) }) -test_that("complete = FALSE leaves unreachable valley cells unattributed and reports them", { +test_that("waterbody cells beyond every group's thresholds are still covered", { + # fl_valley_confine() ORs waterbody polygons into the output with no spatial + # filter (fl_valley_confine.R:213-220), so a lake can sit outside every + # group's distance and cost thresholds. The coverage fallback is what keeps + # those cells attributed. f <- attr_fixture() + wb <- sf::st_read(testdata_path("waterbodies.gpkg"), quiet = TRUE) + precip_r <- fl_stream_rasterize(f$streams, f$dem, field = "map_upstream") + valleys_wb <- fl_valley_confine(f$dem, f$streams, field = "upstream_area_ha", + precip = precip_r, waterbodies = wb) - strict <- fl_valley_attribute(f$valleys, f$streams, group = "gnis_name", + strict <- fl_valley_attribute(valleys_wb, f$streams, group = "gnis_name", dem = f$dem, complete = FALSE) - full <- fl_valley_attribute(f$valleys, f$streams, group = "gnis_name", dem = f$dem) + full <- fl_valley_attribute(valleys_wb, f$streams, group = "gnis_name", + dem = f$dem) - u_strict <- rep(FALSE, terra::ncell(f$valleys)) - for (i in seq_len(nrow(strict))) u_strict <- u_strict | cells_of(strict[i, ], f$valleys) + u_strict <- rep(FALSE, terra::ncell(valleys_wb)) + for (i in seq_len(nrow(strict))) u_strict <- u_strict | cells_of(strict[i, ], valleys_wb) + u_full <- rep(FALSE, terra::ncell(valleys_wb)) + for (i in seq_len(nrow(full))) u_full <- u_full | cells_of(full[i, ], valleys_wb) - # Morphological cleanup, the channel buffer and waterbodies all add valley - # cells after the mask intersection in fl_valley_confine(), so the strict - # cover is a subset and the fallback is what makes coverage total. n_fallback <- attr(full, "fl_fallback_cells") expect_type(n_fallback, "integer") - expect_equal(sum(valley_cells(f$valleys) & !u_strict), n_fallback) + + # The fallback must actually fire here, or the rest of this test is vacuous. + expect_gt(n_fallback, 0) + expect_equal(sum(valley_cells(valleys_wb) & !u_strict), n_fallback) + expect_equal(sum(valley_cells(valleys_wb) & !u_full), 0) }) test_that("NA group values form their own group and stay covered", { @@ -197,16 +253,131 @@ test_that("fl_valley_attribute rejects bad input", { ) }) -test_that("a group whose streams miss the valley yields an empty-but-present result", { +test_that("a group whose streams fall outside the raster is named, not silently dropped", { f <- attr_fixture() - streams <- f$streams - # Relabel the single segment furthest from the valley floor as its own group. - streams$grp <- ifelse(seq_len(nrow(streams)) == which.min(streams$upstream_area_ha), - "isolated", "main") + far <- f$streams[1, ] + sf::st_geometry(far) <- sf::st_geometry(far) + c(5e5, 5e5) + sf::st_crs(far) <- sf::st_crs(f$streams) + far$gnis_name <- "Elsewhere River" + combined <- rbind(f$streams, far) + + expect_warning( + out <- fl_valley_attribute(f$valleys, combined, group = "gnis_name", dem = f$dem), + "Elsewhere River" + ) + # Dropped rather than returned as an empty row — but never silently. + expect_false("Elsewhere River" %in% out$gnis_name) +}) - out <- fl_valley_attribute(f$valleys, streams, group = "grp", dem = f$dem, - complete = FALSE) +test_that("a delineation with no valley cells returns a usable empty sf", { + # The zero-parts branch is easy to build wrong: renaming an sf column by + # position detaches the geometry column and every accessor then errors. + f <- attr_fixture() + empty_valleys <- f$valleys * 0L + + out <- suppressWarnings( + fl_valley_attribute(empty_valleys, f$streams, group = "gnis_name", dem = f$dem) + ) - expect_true(all(c("isolated", "main") %in% out$grp) || nrow(out) == 1L) expect_s3_class(out, "sf") + expect_equal(nrow(out), 0L) + expect_true("gnis_name" %in% names(out)) + expect_equal(attr(out, "sf_column"), "geometry") + expect_silent(sf::st_geometry(out)) + expect_equal(sf::st_crs(out), sf::st_crs(f$valleys)) + expect_output(print(out)) +}) + +test_that("a group whose segments miss every cell centre warns instead of vanishing", { + # fl_stream_rasterize() uses touches = FALSE, so a sub-cell segment burns + # nothing. At k = 340 blue_line_keys this is near-certain, not hypothetical. + f <- attr_fixture() + streams <- f$streams + + # Put a 0.4 m segment at a cell CORNER, where no cell centre can be crossed. + centre <- terra::xyFromCell(f$valleys, terra::ncell(f$valleys) %/% 2L) + corner <- centre + terra::res(f$valleys) / 2 + coords <- rbind(corner + 0.1, corner + 0.5) + + tiny <- streams[1, ] + sf::st_geometry(tiny) <- sf::st_sfc(sf::st_linestring(coords), + crs = sf::st_crs(streams)) + tiny$gnis_name <- "Subpixel Creek" + combined <- rbind(streams, tiny) + + expect_warning( + fl_valley_attribute(f$valleys, combined, group = "gnis_name", dem = f$dem), + "Subpixel Creek" + ) +}) + +test_that("a group named as omitted is genuinely absent from the output", { + # The dropped-group warning must be raised AFTER the coverage fallback: a group + # with zero threshold cells can still win uncovered cells as nearest-group, and + # naming it "omitted" while returning it is worse than saying nothing. + f <- attr_fixture() + vals <- as.vector(terra::values(f$valleys)) + + # A stream sitting on non-valley ground, close enough to the valley edge to be + # the nearest group for some uncovered cells. + valley_xy <- terra::xyFromCell(f$valleys, which(!is.na(vals) & vals == 1L)[1]) + non_valley <- which(!is.na(vals) & vals == 0L) + nv_xy <- terra::xyFromCell(f$valleys, non_valley) + d <- sqrt((nv_xy[, 1] - valley_xy[1])^2 + (nv_xy[, 2] - valley_xy[2])^2) + seed_xy <- nv_xy[which.min(d), ] + + ghost <- f$streams[1, ] + sf::st_geometry(ghost) <- sf::st_sfc( + sf::st_linestring(rbind(seed_xy, seed_xy + c(20, 0))), + crs = sf::st_crs(f$streams) + ) + ghost$gnis_name <- "Ghost Creek" + combined <- rbind(f$streams, ghost) + + # max_width = 10 makes the distance criterion bite, so most cells go to the + # fallback and Ghost Creek can pick some up despite scoring zero on thresholds. + warned <- character(0) + out <- withCallingHandlers( + fl_valley_attribute(f$valleys, combined, group = "gnis_name", dem = f$dem, + max_width = 10), + warning = function(w) { + warned <<- c(warned, conditionMessage(w)) + invokeRestart("muffleWarning") + } + ) + + groups <- as.character(stats::na.omit(unique(combined$gnis_name))) + named <- groups[vapply(groups, function(g) any(grepl(g, warned, fixed = TRUE)), + logical(1))] + + # Ghost Creek scores zero on the thresholds but wins fallback cells, which is + # the precondition for the bug this pins — assert it, or the test is vacuous. + expect_true("Ghost Creek" %in% as.character(out$gnis_name)) + expect_equal(intersect(named, as.character(out$gnis_name)), character(0)) +}) + +test_that("empty geometries do not abort the whole attribution", { + # Routine after st_intersection() clipping; terra::ext() errors on them. + f <- attr_fixture() + broken <- f$streams[1, ] + sf::st_geometry(broken) <- sf::st_sfc(sf::st_linestring(), crs = sf::st_crs(f$streams)) + broken$gnis_name <- "Empty Creek" + combined <- rbind(f$streams, broken) + + expect_warning( + out <- fl_valley_attribute(f$valleys, combined, group = "gnis_name", dem = f$dem), + "Empty Creek" + ) + expect_equal(nrow(out), 5L) +}) + +test_that("group = 'geometry' is rejected rather than clobbering the geometry column", { + f <- attr_fixture() + streams <- f$streams + streams$geometry <- ifelse(is.na(streams$gnis_name), "unnamed", streams$gnis_name) + + expect_error( + fl_valley_attribute(f$valleys, streams, group = "geometry", dem = f$dem), + "geometry" + ) }) diff --git a/tests/testthat/test-fl_valley_poly.R b/tests/testthat/test-fl_valley_poly.R index 484928c..c0d39a1 100644 --- a/tests/testthat/test-fl_valley_poly.R +++ b/tests/testthat/test-fl_valley_poly.R @@ -40,3 +40,23 @@ test_that("fl_valley_poly returns empty sf for all-zero raster", { expect_s3_class(poly, "sf") expect_equal(nrow(poly), 0L) }) + +test_that("fl_valley_poly returns a usable sf when there are no valley cells", { + # An all-zero raster used to yield an sf whose sf_column pointed at a column + # that had been renamed out from under it — every accessor then errored. + dem <- terra::rast(testdata_path("dem.tif")) + empty <- dem * 0L + + out <- fl_valley_poly(empty) + + expect_s3_class(out, "sf") + expect_equal(nrow(out), 0L) + expect_true("valley" %in% names(out)) + expect_equal(attr(out, "sf_column"), "geometry") + expect_silent(sf::st_geometry(out)) + expect_output(print(out)) + + f <- tempfile(fileext = ".gpkg") + on.exit(unlink(f), add = TRUE) + expect_silent(sf::st_write(out, f, quiet = TRUE)) +}) diff --git a/vignettes/valley-confinement.Rmd b/vignettes/valley-confinement.Rmd index 476e1a2..ab29ea7 100644 --- a/vignettes/valley-confinement.Rmd +++ b/vignettes/valley-confinement.Rmd @@ -408,6 +408,82 @@ for (id in names(results)) { } ``` +## Whose floodplain is it? + +The delineation above answers "where is this network's floodplain?" It cannot +answer "where is the *Bulkley River's* floodplain?" — a valley cell carries no +memory of which stream made it one. `fl_valley_attribute()` adds that memory +after the fact: it takes the finished delineation and works out which part of it +belongs to which watercourse. + +```{r attribute} +by_stream <- fl_valley_attribute( + valleys_wb, + streams, + group = "gnis_name", + dem = dem +) + +by_stream[, "gnis_name"] +``` + +A cell is attributed to a watercourse when it is a valley cell *and* it sits +within `max_width / 2` of that watercourse *and* its cost-distance from it is +under `cost_threshold` — the same two stream-dependent criteria the VCA applies +to the whole network. Nothing is re-delineated, so changing the grouping key +relabels the output without moving a boundary. + +Near a confluence, ground legitimately belongs to more than one floodplain, so +the rows overlap rather than partitioning the valley. On this tile the parts sum +to roughly 2.4 times the whole: + +```{r attribute-areas} +areas <- data.frame( + watercourse = ifelse(is.na(by_stream$gnis_name), "unnamed", by_stream$gnis_name), + area_ha = round(as.numeric(sf::st_area(by_stream)) / 1e4, 1) +) +knitr::kable( + areas[order(-areas$area_ha), ], + row.names = FALSE, + col.names = c("Watercourse", "Area (ha)"), + caption = "Floodplain area attributable to each watercourse. Overlapping rows mean these do not sum to the total delineated area." +) +``` + +The practical payoff is in the field. Filter to one watercourse and its +floodplain terminates where that watercourse does, which is what makes "inside +the Bulkley floodplain" and "upstream of it" answerable at a point: + +```{r plot-attribute, fig.cap = "Floodplain attributed per watercourse. Blue is the Bulkley River mainstem, orange is Richfield Creek; the hatched overlap near the confluence belongs to both."} +plot(dem, main = "Floodplain by watercourse", col = grey.colors(100)) +plot(st_geometry(by_stream[which(by_stream$gnis_name == "Bulkley River"), ]), + add = TRUE, col = "#2166ac60", border = "#2166ac") +plot(st_geometry(by_stream[which(by_stream$gnis_name == "Richfield Creek"), ]), + add = TRUE, col = "#d6604d60", border = "#d6604d") +plot(st_geometry(streams), add = TRUE, col = "steelblue", lwd = 0.8) +legend("topright", legend = c("Bulkley River", "Richfield Creek"), + fill = c("#2166ac60", "#d6604d60"), border = c("#2166ac", "#d6604d"), + bty = "n", cex = 0.8) +``` + +Why attribute a single delineation instead of running the VCA once per +watercourse? Because a per-watercourse run is not that watercourse's share of +the whole-network run. The flood surface is interpolated from every seed cell, +the distance and cost criteria loosen as seeds are added, and morphological +cleanup couples patches — so dropping the tributaries changes the mainstem's own +floodplain. Attributing one delineation keeps the answer independent of whatever +else happened to be in the run. + +Morphological cleanup, the channel buffer, and waterbody polygons all add cells +to the delineation after the masks are intersected, and those can fall outside +every watercourse's thresholds. By default they are assigned to the nearest +watercourse so the attribution covers the delineation exactly; the count is +reported and stored on the result: + +```{r attribute-fallback} +attr(by_stream, "fl_fallback_cells") +``` + ## Performance Several `terra` operations inside `fl_valley_confine()` support multi-threading From eda4986be63f3aa62bb8e53d8475e1ad30d897e9 Mon Sep 17 00:00:00 2001 From: almac2022 <al@newgraphenvironment.com> Date: Thu, 27 Aug 2026 17:03:43 -0700 Subject: [PATCH 5/6] Archive PWF for #40 Outcome summary in the archive README: mechanism chosen on measurement, three code-check rounds plus a concurrent plan review, and the two follow-ups (#41, floodplains#40 driver half). Relates to #40 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U9SAqmvFeENADk4rYcbtHS --- .../README.md | 30 +++++++++++++++++++ .../findings.md | 0 .../progress.md | 0 .../review-40.md | 0 .../review-round1.md | 0 .../review-round2.md | 0 .../review-round3.md | 0 .../task_plan.md | 0 8 files changed, 30 insertions(+) create mode 100644 planning/archive/2026-08-issue-40-fl-valley-attribute/README.md rename planning/{active => archive/2026-08-issue-40-fl-valley-attribute}/findings.md (100%) rename planning/{active => archive/2026-08-issue-40-fl-valley-attribute}/progress.md (100%) rename planning/{active => archive/2026-08-issue-40-fl-valley-attribute}/review-40.md (100%) rename planning/{active => archive/2026-08-issue-40-fl-valley-attribute}/review-round1.md (100%) rename planning/{active => archive/2026-08-issue-40-fl-valley-attribute}/review-round2.md (100%) rename planning/{active => archive/2026-08-issue-40-fl-valley-attribute}/review-round3.md (100%) rename planning/{active => archive/2026-08-issue-40-fl-valley-attribute}/task_plan.md (100%) diff --git a/planning/archive/2026-08-issue-40-fl-valley-attribute/README.md b/planning/archive/2026-08-issue-40-fl-valley-attribute/README.md new file mode 100644 index 0000000..17f1c2d --- /dev/null +++ b/planning/archive/2026-08-issue-40-fl-valley-attribute/README.md @@ -0,0 +1,30 @@ +# fl_valley_attribute() — per-watercourse floodplain attribution (#40) + +**Outcome:** shipped `fl_valley_attribute()`, which takes a finished `fl_valley_confine()` +delineation and returns one overlapping `sf` row per stream group, so "where is the Morice River's +floodplain?" is answerable and a field point can be tested against a single watercourse. The +delineation is never recomputed — grouping changes relabel without moving a boundary. The mechanism +was chosen on measurement: per-group VCA runs disagree with the whole-network run in both directions +(510 cells present in a group run and absent from the full run, 22 the reverse), so approach B would +have made "the Morice floodplain" depend on what else was in the run. Coverage is total by +construction via a nearest-group fallback, because `fl_valley_confine()` adds cells after +intersecting its masks — waterbodies orphan 1,643 cells (3.0%) on the bundled tile with no spatial +filter at all. + +**Reviews earned their keep.** A concurrent Plan agent returned 19 findings (`review-40.md`), +independently confirming the waterbody coverage hole and killing two tests that were vacuous — +`gnis_name` and `blue_line_key` are a bijection on the test tile, so the "grouping invariance" test +would have passed for any implementation. Three code-check rounds (`review-round{1,2,3}.md`) found +4 + 3 + 2 issues; round 2 found a bug *inside* round 1's fix, and round 3 converged and named the +mechanism — renaming an `sf` column by position — which turned up a latent corrupt-`sf` bug in +`fl_valley_poly()` on an empty delineation. An internal assert on the return path now closes that +family. + +**Also filed:** #41 — `fl_cost_distance()` seeds every zero-friction cell, not just stream cells. +Pre-existing, deliberately not fixed here because it changes VCA output on any DEM with exact zeros. + +**Driver half:** NewGraphEnvironment/floodplains#40, body updated with the shipped API and the +measurement that rules out per-group runs. + +**Closing commits:** `74a7919` (implementation), PR to follow. 233 tests green; +`R CMD check` 0 errors / 0 warnings / 1 pre-existing NOTE (`pkgdown/` at top level). diff --git a/planning/active/findings.md b/planning/archive/2026-08-issue-40-fl-valley-attribute/findings.md similarity index 100% rename from planning/active/findings.md rename to planning/archive/2026-08-issue-40-fl-valley-attribute/findings.md diff --git a/planning/active/progress.md b/planning/archive/2026-08-issue-40-fl-valley-attribute/progress.md similarity index 100% rename from planning/active/progress.md rename to planning/archive/2026-08-issue-40-fl-valley-attribute/progress.md diff --git a/planning/active/review-40.md b/planning/archive/2026-08-issue-40-fl-valley-attribute/review-40.md similarity index 100% rename from planning/active/review-40.md rename to planning/archive/2026-08-issue-40-fl-valley-attribute/review-40.md diff --git a/planning/active/review-round1.md b/planning/archive/2026-08-issue-40-fl-valley-attribute/review-round1.md similarity index 100% rename from planning/active/review-round1.md rename to planning/archive/2026-08-issue-40-fl-valley-attribute/review-round1.md diff --git a/planning/active/review-round2.md b/planning/archive/2026-08-issue-40-fl-valley-attribute/review-round2.md similarity index 100% rename from planning/active/review-round2.md rename to planning/archive/2026-08-issue-40-fl-valley-attribute/review-round2.md diff --git a/planning/active/review-round3.md b/planning/archive/2026-08-issue-40-fl-valley-attribute/review-round3.md similarity index 100% rename from planning/active/review-round3.md rename to planning/archive/2026-08-issue-40-fl-valley-attribute/review-round3.md diff --git a/planning/active/task_plan.md b/planning/archive/2026-08-issue-40-fl-valley-attribute/task_plan.md similarity index 100% rename from planning/active/task_plan.md rename to planning/archive/2026-08-issue-40-fl-valley-attribute/task_plan.md From 8779f308a3a5a90de5c2e4b6ae314a45575f9fd5 Mon Sep 17 00:00:00 2001 From: almac2022 <al@newgraphenvironment.com> Date: Thu, 27 Aug 2026 17:19:32 -0700 Subject: [PATCH 6/6] Release v0.4.0 New fl_valley_attribute(), plus the fl_valley_poly() empty-delineation fix. R CMD check: 0 errors, 0 warnings, 1 pre-existing NOTE (pkgdown/ at top level). 233 tests green. Relates to #40 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U9SAqmvFeENADk4rYcbtHS --- DESCRIPTION | 2 +- NEWS.md | 6 ++++++ R/fl_valley_attribute.R | 2 +- 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/DESCRIPTION b/DESCRIPTION index bc868fa..e2acf1a 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -1,6 +1,6 @@ Package: flooded Title: Portable Floodplain Delineation from DEM and Stream Network -Version: 0.3.2 +Version: 0.4.0 Authors@R: c( person("Allan", "Irvine", , "al@newgraphenvironment.com", role = c("aut", "cre"), comment = c(ORCID = "0000-0002-3495-2128")), diff --git a/NEWS.md b/NEWS.md index 96f0b23..32be3b2 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,3 +1,9 @@ +# flooded 0.4.0 + +- New `fl_valley_attribute()` — attribute a finished `fl_valley_confine()` delineation to the stream groups that produced it, so a floodplain can be filtered and queried per watercourse or reach rather than only per network (#40). Returns one `sf` row per group; rows overlap where ground is genuinely shared between watercourses, which near a confluence is most of it. The delineation is never recomputed, so changing the grouping key relabels the output without moving a boundary. +- Per-group VCA runs were measured and rejected: they disagree with the whole-network run in both directions, which would make a river's floodplain depend on what else was in the run. See the function's Details and the vignette section "Whose floodplain is it?" for the mechanism and its limits. +- Fix `fl_valley_poly()` on a delineation with no valley cells — it renamed an `sf` column by position, which detached the geometry column and made every accessor error. + # flooded 0.3.2 - Drop the internal `rtj/docs/dem-sources.md` reference from `fl_dem_aoi()` documentation and NEWS — MRDEM-30 is described as the default DEM source without pointing readers at a private doc they can't access. diff --git a/R/fl_valley_attribute.R b/R/fl_valley_attribute.R index 71efc3b..804d2f0 100644 --- a/R/fl_valley_attribute.R +++ b/R/fl_valley_attribute.R @@ -129,7 +129,7 @@ fl_valley_attribute <- function(valleys, streams, group, ) if (identical(group, "geometry")) { - stop("`group` cannot be \"geometry\" — it would collide with the output's ", + stop("`group` cannot be \"geometry\": it would collide with the output's ", "geometry column.", call. = FALSE) }