From a26ed9cbcfccb98eeee578bbde827174cfe6bed7 Mon Sep 17 00:00:00 2001 From: almac2022 Date: Fri, 28 Aug 2026 11:54:47 -0700 Subject: [PATCH] Sync CLAUDE.md with current soul conventions Regenerates the section below the soul marker. No project-specific content above the marker is touched. Co-Authored-By: Claude Opus 5 --- CLAUDE.md | 1971 +++++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 1829 insertions(+), 142 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index b988a02..59ce2ed 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -143,6 +143,136 @@ The two-vignette template is the foundation for additional regional reporting ap +# 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 When this repo has GitHub Actions workflows, scan recent runs on session start. Catches failed pkgdown deploys, broken vignette builds, and stale citation regenerations that would otherwise linger until the user manually checks. @@ -175,6 +305,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 @@ -183,12 +352,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 +### A preview flag is only safe if it previews + +- `--dry-run`, `DRY=1`, `--plan` conventionally mean "show me what would happen". + **Nothing enforces that.** A flag that skips the *expensive* step while still + performing the *destructive* one is worse than no flag, because it is exactly + what people reach for when they are unsure. +- Symptom: you run the preview to check something unrelated, and `git status` + afterwards shows deletions you never asked for. +- Caught 2026-08-27 in floodplains#44: `run_region.R` prints + `[DRY] plan + configs written; no pipeline runs` — it skips the pipeline, not + the config write. A `DRY=1` run to verify an unrelated one-line change deleted a + watershed group's second-species scenario rows, every literature citation in two + `flood_scenarios.csv` files, and a `break_points.csv`. 50 deletions from a + command documented as "plan only". +- Before trusting one, read what it actually gates. If you own it, make the flag + return **before the first write**, not before the first slow call. +- Cheap audit either way: run `git status` immediately after a dry run. + +### `git add -A` after a generator sweeps its side effects into your commit + +- Config regenerators, formatters, codegen, lockfile updaters and "plan" commands + all rewrite files you did not edit. Staged wholesale, they ride into a commit + whose message describes something else, and the diff stat is the only warning. +- Stage the paths you actually changed (`git add `), or read + `git diff --cached --stat` before committing and reconcile every file against + your intent. **A commit touching six files when you edited one is the signal.** +- Caught 2026-08-27 in floodplains, same session as the dry-run entry above and + compounding with it: the preview created the unexpected changes and `git add -A` + committed them — a "one-line config change" of 6 files, 28 insertions and + **50 deletions**. Reset before it left the branch, but only because the file + count looked wrong. + +### Never silence stderr on a mutating command, and never chain one with `;` + +- `cmd_that_moves_things 2>/dev/null; next_command` combines two mistakes that + cover for each other. The redirect hides the diagnostic, and `;` runs the next + command regardless — so a mutation that "succeeded" doing the **wrong thing** + leaves no trace, and the only symptom is an unrelated error one command later. +- Caught 2026-08 archiving a planning directory: + ```bash + git mv planning/active $(basename planning/active) 2>/dev/null; mv planning/active "$dest" + ``` + The `git mv` succeeded — it moved `planning/active` to `./active` at the repo + root, which is not what was meant. Nothing said so. The failure surfaced as + `mv: cannot stat 'planning/active'` from the *next* command, which reads like + the directory was never there. +- Two rules, and the first is the load-bearing one: + - **`2>/dev/null` belongs on reads, not writes.** A probe that may legitimately + fail (`grep -q`, `test`, a `gh` call you expect to 404) can be quiet. A + command that moves, deletes, or writes must be allowed to speak. + - **Chain mutations with `&&`.** `;` between two steps of one operation says + "these are unrelated", which is exactly what they are not. +- Same class as the `set -e` / pipefail entries above, but it survives them: `;` + defeats `set -e` for the preceding command by design, so a script with + `set -euo pipefail` at the top is not protected. + +### `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. @@ -219,6 +640,34 @@ Add new checks here when a bug class is discovered — they compound over time. - 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. @@ -226,149 +675,45 @@ Add new checks here when a bug class is discovered — they compound over time. - 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`). + - **A third cause, and the one that bites agents: an alias in the invoking shell.** Measured 2026-08-28 — in an agent Bash call `ls` was aliased to `command ls --color`, so `ls -A dir | grep -v '^\.gitkeep$'` returned `^[[0m^[[00m.gitkeep^[[0m`, the grep failed to filter it, and a directory-empty guard false-failed on a correct tree. The identical command was fine inside a script file, where no alias applies and `ls` resolved to GNU coreutils — so testing it from a script *proves nothing about how it will run inline*. `CLICOLOR_FORCE` was not involved in that instance; check `type ls` before trusting either. - 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`. +### `&` binds to the whole `&&` list, so assignments never reach the parent + +- `cmd1 && VAR=$(...) && nohup prog > "$VAR.log" & disown` backgrounds the + **entire list**, not just `nohup`. `VAR` is assigned inside the background + subshell, so it is empty in the parent — and a following `tail -f "$VAR.log"` + reads the wrong path or errors while the job runs fine, writing somewhere you + are not looking. +- The symptom lies about which side failed: the `tail` says + `No such file or directory`, which reads as "the job never started". It started. +- Fix: assign **before** the list — `VAR=$(...); cmd1 && nohup ... &` — or + `printf` the resolved path from inside the backgrounded shell so the parent can + read it from output. +- Hit twice in one floodplains session (2026-08-27) launching detached runs. + +### `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` / `gh pr create` with heredoc bodies fail 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. The two are written interchangeably, so the trap applies to both: `gh pr create --body "$(cat <<'EOF' … EOF)"` breaks the parser on a prose apostrophe and bash reports `unexpected EOF while looking for matching '"'`, aborting the whole command before anything runs. +- **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 -## Cloud-Init (YAML) - -### ASCII -- Must be pure ASCII — em dashes, curly quotes, arrows cause silent parse failure -- Check with: `perl -ne 'print "$.: $_" if /[^\x00-\x7F]/' file.yaml` - -### YAML flow-mapping in runcmd -- Any runcmd item containing both `{` and `:` is at risk of being parsed as a YAML flow-mapping (dict), not a literal string. Cloud-init's shellify hits a non-string and throws TypeError, **aborting all subsequent runcmd steps silently** while `final_message` still fires. -- Don't write: `- test -s /file || { echo "FATAL: ..." }` — the `:` inside braces makes YAML see a dict. -- Do write: use `- |` block scalar with explicit `if/then/fi`: - ```yaml - - | - if [ ! -s /file ]; then - echo "FATAL: ..." >&2 - exit 1 - fi - ``` -- Validate post-edit: `python3 -c "import yaml; runcmd=yaml.safe_load(open('cloud-init.yaml').read().split(chr(10),1)[1])['runcmd']; print([type(x).__name__ for x in runcmd if not isinstance(x,str)] or 'all strings')"`. If the output is anything other than `all strings`, the runcmd will fail. - -### State -- `cloud-init clean` causes full re-provisioning on next boot — almost never what you want before snapshot -- Use `tailscale logout` not `tailscale down` before snapshot (deregister vs disconnect) -- Wipe `/var/lib/tailscale/*` before snapshot too — `tailscale logout` deauthorizes server-side but local node identity blob persists in tailscaled.state. Snapshot restored elsewhere inherits prior key material until `tailscale up` runs again. -- Wipe `/etc/ssh/ssh_host_*` before snapshot — otherwise droplets spawned from the same image share host identity. - -### Template Variables -- Secrets rendered via `templatefile()` are readable at `169.254.169.254` metadata endpoint -- Acceptable for ephemeral machines, document the tradeoff -- Heredocs in runcmd that write secrets: `<<'EOF'` (quoted) prevents bash from re-expanding `$X` sequences in already-substituted credential strings. AWS keys rarely contain `$` but base64-padded secrets might. - -### Repo + key install ordering -- `apt-key adv --keyserver` is deprecated on Ubuntu 24.04 noble — silently fails AND APT ignores resulting keyring. Use `gpg --dearmor` + `signed-by=` keyring file pattern. -- Repo .list files in `write_files:` trigger the implicit `package_update` BEFORE runcmd installs the keyring → first apt-get update fails with NO_PUBKEY. Put the repo line in runcmd alongside the key install, not in write_files. - -### Cloud-init users vs DO SSH key injection -- DO injects `ssh_key_ids` only into `/root/.ssh/authorized_keys` (cloud-init's `cc_ssh` module). Cloud-init `users:` block with `ssh_authorized_keys: []` does NOT pick those up. -- Non-root users that need SSH access must copy from root's keys in runcmd: - ```yaml - - mkdir -p /home//.ssh - - cp /root/.ssh/authorized_keys /home//.ssh/authorized_keys - - chown -R : /home//.ssh - ``` -- Guard with `test -s /root/.ssh/authorized_keys` to fail loudly if `cc_ssh` hasn't run before runcmd (rare race). - -## OpenTofu / Terraform - -### State -- Parsing `tofu state show` text output is fragile — use `tofu output` instead -- Missing outputs that scripts need — add them to main.tf -- Snapshot/image IDs in tfvars after deleting the snapshot — stale reference - -### 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. - -## DigitalOcean - -### Snapshot disk-size constraint -- DO snapshots include the source droplet's disk size. New droplets from a snapshot must have disk **>=** snapshot disk. Resize **up** is fine; resize **down** below the snapshot disk is impossible without rebuilding. -- Build the snapshot at the smallest droplet size you'd ever want to spin from it. Sizes vs disks at writing: `g-4vcpu-16gb` = 50 GB, `g-8vcpu-32gb` / `m-4vcpu-32gb` = 100 GB, `m-8vcpu-64gb` = 200 GB. -- If your workload requires X GB RAM minimum, your snapshot floor is whatever droplet has X GB AND the smallest disk class. - -### Reserved IP detach behavior -- Targeted destroy (`tofu destroy -target=module.droplet -target=...assignment...`) preserves the reserved IP at $4/mo. Full `tofu destroy` releases it (next apply gets a NEW IP). - -### Reserved IP assignment race (rtj#55, rtj#85) -- DO returns 422 "Droplet already has a pending event" when reserved IP assignment fires immediately after droplet+firewall creation. The droplet's internal event queue takes time to drain. -- **Every DO droplet module that uses a reserved IP MUST have:** - 1. `time_sleep` resource between droplet creation and IP assignment, with `create_duration ≥ 60s` (10s and 30s have both been observed to race; 60s has more headroom) - 2. `depends_on = [time_sleep.]` on the `digitalocean_reserved_ip_assignment` resource - 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. - -## Docker / Postgres - -### Postgis init time -- `imresamu/postgis` (and similar postgis images) on first cold start (empty data volume) take **5-12 min** to install all extensions — varies with disk IO and noisy-neighbor lottery on cloud hosts. Health-wait scripts must allow 15 min minimum, ideally with hard-fail + log dump on timeout. - -### Tuning vs host RAM -- fresh's `docker/docker-compose.yml` defaults are tuned for a 128 GB host (`shared_buffers=32GB`, `shm_size=36gb`). On smaller hosts, postgres OOMs at startup with "could not map anonymous shared memory". -- 32 GB host floor: use the M1/cypher 32 GB-host preset (`scripts/fwapg/compose.override.m1.yml`) which sets `shared_buffers=8GB, shm_size=12gb`. -- Below 32 GB: postgres can technically start with smaller `shared_buffers` but fwapg work becomes painful. Don't run fwapg pipelines on <32 GB hosts. - -### `search_path` is data, not config -- `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. +## Spatial CLIs (bcdata, ogr, gdal) -### 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." +### 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. -### 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 -- Tailscale SSH ACL `"users": ["autogroup:nonroot"]` for `tag:compute` blocks `ssh root@` over the tailnet. Use `ssh @` + sudo for root operations. -- For SSH-as-root from off-tailnet (regular OpenSSH on the public IP), the ACL doesn't apply — but you need the SSH key registered on the node. - -### Reusable + ephemeral auth keys -- Cypher-style ephemeral compute droplets need both flags on the auth key: **Reusable** (same key works across destroy/recreate) + **Ephemeral** (tailnet entries auto-clean when offline >5 min). -- Tag the key (e.g. `tag:compute`) at creation time. Nodes joining with that key inherit the tag automatically — no `--advertise-tags` needed at `tailscale up` time. +### 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. ## Security @@ -386,8 +731,234 @@ 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. + +### `fs::dir_ls(glob = )` matches the FULL path, so a bare filename pattern matches nothing + +- `fs::dir_ls(dir, glob = "form_*.gpkg")` returns **zero** for a directory full of + `form_*.gpkg` files. The glob is tested against the whole path + (`/Users/.../project/form_pscis.gpkg`), which does not start with `form_`. +- It fails **silently and in the safe-looking direction** — an empty result reads + as "this project has none", not as "the pattern was wrong". Seen 2026-08-27 in + rtj#221: a harvest driver found no forms in a project holding four, and a + second glob (`"*/form_*.gpkg"`) masked it by accidentally matching. +- Use an anchored `regexp` instead, which is matched the same way but says so: + `fs::dir_ls(dir, regexp = "/form_[^/]+\\.gpkg$", recurse = FALSE, type = "file")`. +- Set `recurse` deliberately while you are there. A recursive search of a Mergin + project picks up `.mergin/`'s own cache copies and anything under `hold/` — + stale duplicates that then get processed as if they were live. + +### Do not build an exact-match edit from a formatted display + +- Reading a file through a pretty-printer and then writing a string replacement + against what you saw will fail whenever the formatter changed the bytes. + `sed -n '10,20p' file | sed 's/^/ /'` adds two spaces to every line; a + subsequent `replace(old, new)` built from that output silently matches nothing. +- The failure looks like the file changed under you, so the instinct is to re-read + it — through the same formatter — and conclude the text is right and the tool is + broken. Cost two failed edit rounds in rtj#221 before `repr()` on the raw lines + showed the real indent was **two** spaces where the padded display implied four. +- Print the bytes you intend to match: `repr()` in Python, `cat -A`, or + `writeLines()` — never a column-shifted copy. +- Same family as diagnosing PATH in the shell that actually runs: inspect the + thing you are acting on, not a convenience rendering of it. + +### `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. + +### `f(g(x)) <- v` needs a `g<-`, not an evaluated `g(x)` + +- R parses **any** call on the left of `<-` as a replacement function, all the + way down. `xml2::xml_text(node_for(ml)) <- expr` does not evaluate + `node_for(ml)` and assign into the result — it looks for `` `node_for<-` `` + and errors with `could not find function "node_for<-"`. +- It reads as correct because the single-call form is idiomatic and works: + `xml_text(node) <- v`, `names(x) <- v`, `levels(f) <- v`. Only the *nested* + form breaks, so the habit is what leads you into it. +- Fix: assign the inner result first. + ```r + target <- node_for(ml) # not xml_text(node_for(ml)) <- expr + xml2::xml_text(target) <- expr + ``` +- The error names a function nobody wrote, which sends you looking for a missing + import or a typo rather than at the line's shape. Caught 2026-08-27 in rfp#201 + with `xml2::xml_text(.qgs_preview_node(ml)) <- expr`. +- Applies to every replacement form — `attr<-`, `[[<-`, `dim<-`, `st_crs<-`. If + the left side has two calls, one of them has to move to its own line. + +### `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 @@ -397,6 +968,28 @@ 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. @@ -406,8 +999,153 @@ Add new checks here when a bug class is discovered — they compound over time. - 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): @@ -417,6 +1155,353 @@ 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 binding rather than editing the source back and forth. For a data-shaped bug, feed the function the input the fix was about and assert the old answer is gone. +- **In R, patching only `asNamespace()` gives a false green for anything test code calls directly.** `pkgload::load_all()` (so `devtools::test()`, so every local run) creates **two** bindings: the namespace, and an attached `package:` on the search path. Test code resolves through the search path and never consults the namespace, so the obvious recipe leaves the test calling the *original* function while reporting success. + Measured 2026-08-28 in flooded#41, restoring a fully-reverted bug under `test_file()`: + ``` + unpatched FAIL = 0 zeros = 1 (fixed behaviour) + asNamespace("flooded") patched FAIL = 0 zeros = 1 <- false green, bug not reached + + as.environment("package:flooded") FAIL = 3 zeros = 400 <- broken code actually runs + ``` + Which binding you want depends on who calls: + + | the call under test | patch | + |---|---| + | test code -> an exported function | `as.environment("package:")` | + | one package function -> another (internal call path) | `asNamespace("")` | + | either, on testthat >= 3.2.0 | `local_mocked_bindings(f = ..., .package = "")` | + + ```r + for (e in list(asNamespace("pkg"), as.environment("package:pkg"))) { + unlockBinding("f", e); assign("f", broken_version, e) + } + ``` +- **Do not reason about which environment — print a value that proves the patch took.** One line, before the assertion, whose output can only come from the broken version. That is what turns "I patched it" into "the broken code ran", and it is the same check whether the language is R, Python or JS. Assigning into `globalenv()` also appears to work in R, by *shadowing* the attached copy earlier on the search path — a workaround that happens to produce the right answer for the wrong reason, and silently fails the moment the caller is inside the package. +- 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. +- A fourth, of a different kind, from the entry above: the restoration *harness* can be the thing that is broken. A green run proves nothing until you have evidence the defect was actually executing. +- 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. + +### List the container; do not construct the sibling path +- Probing for a related object by editing a known-good path — swapping a + directory, appending a suffix, substituting a product name — assumes the + naming convention is uniform across the whole store. It usually is not, and + the places it is not are invisible from any single example. +- The failure is a **404, which reads as "that product does not exist"** rather + than "I guessed the name wrong". So the wrong conclusion arrives looking like + evidence, and it is the confident kind: a checked path that returned nothing. +- Measured 2026-08-27 in the BC LidarBC objectstore. Swapping `/dem/` for + `/dsm/` in a tile's URL: + ``` + 2022 dem/bc_082f037_xli1m_utm11_2022.tif + dsm/bc_082f037_xli1m_utm11_2022.tif <- same basename, swap works + 2017 dem/bc_082f037_xli1m_utm11_2017.tif + dsm/bc_082f037_xli1m_utm11_2017_dsm.tif <- suffixed, swap 404s + ``` + A probe run against a 2017 tile concluded "there is no surface model and no + CHM", and that became the central constraint of a project plan — ruling out + canopy measurement entirely — for weeks. Listing the prefix instead showed + `dem, dsm, orthophoto, pointcloud` immediately, with DSM present in 25 of 38 + mapsheet-years. +- **Enumerate the container** (`?list-type=2&delimiter=/&prefix=…`, `ls`, the + API's own listing endpoint) and match on the fields that actually identify the + thing — tile, date, product — rather than on a name you assembled. +- When pairing two families this way, **report the unpaired members**. An item + with no partner is a real gap, and silently dropping it turns a coverage hole + into an apparently complete result. + +### A verifier built on the writer's own library shares its blind spot +- After rewriting a file, the natural check is to parse both versions and + compare their structure. That check is worth much less than it looks when + the same library does both jobs: **anything the library does not model, it + will neither preserve nor miss.** The comparison comes back identical, and + the loss is invisible precisely where it matters. +- Live case, 2026-08-27: Python's `ElementTree` **silently drops the + ``** when it writes. A `.qgs` carries one. The structural check — + root tag, layer count, theme count, tree nodes — reported IDENTICAL, because + `ElementTree` does not need a DOCTYPE to parse either. R's `xml2::write_xml` + preserves it, which is why the same operation through the package's own + writer had never shown the problem. +- The prologue is the usual casualty in XML (DOCTYPE, processing + instructions, comments, namespace prefixes, attribute order), but the shape + is general: JSON writers drop key order and numeric precision, YAML writers + drop anchors and comments, image libraries drop EXIF. +- Two habits that catch it: + - **Diff the bytes at the boundaries**, not just the parsed structure — + `head -2` and `tail -2` on both files costs nothing and is exactly where + prologue loss shows. + - **Check what the real consumer needs**, then assert that specifically. The + generic "did the structure survive" question cannot ask it for you. +- Sibling of *"A round-trip through your own reader proves nothing about + interop"* above, one level meaner: there the reader was too generous, here + the **verifier** is, so the failure survives a check that was written + specifically to catch it. + +### 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. + +### A comparison test proves nothing if the fixture makes both sides identical +- A test of the form "configuration A and configuration B produce the same result" is only a test when A and B *can* differ in the data it runs on. When the fixture makes them equivalent, the assertion holds for every implementation — correct or broken — and the green tick is indistinguishable from a real pass. +- Measured 2026-08-27 in flooded#40: a test asserted that grouping a floodplain by `gnis_name` and by `blue_line_key` produced the same union of ground. In the bundled test data those two columns are a **bijection** — 5 groups each, one-to-one — so the two runs were the same run with different labels. The test could not fail. Replaced with a constructed coarsening (merge the 5 fine groups into 2, assert each coarse group equals the union of its members, cell for cell), which is the property that actually mattered and can be broken. +- The tell is that both sides come from the *same* fixture column set. Before writing the assertion, compute the cross-tabulation — `table(a, b)` — and look at it. A diagonal means you have one test, not two. +- Same shape, different dress: comparing two code paths that a small fixture drives down the same branch, or two parameter values that both fall outside a threshold the data never approaches. Check that the fixture reaches the distinction, not just that the code contains it. +- Generally: when a test passes on the first run, ask what edit to the code under test would make it fail. If you cannot name one, the test is documentation, not verification. + ### Documentation Staleness - Moving/renaming scripts: update CLAUDE.md, READMEs, usage comments - New variables: update .tfvars.example @@ -431,7 +1516,7 @@ For non-trivial issue-driven work, follow this checklist. Each step exists for a 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.) +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. For files rather than functions — shell scripts and operational R scripts under `scripts/` or `data-raw/` — the standard is the `noun_verb-detail` pattern in `newgraph.md`, noun first. 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. 5. **Code-check before each commit** — `/code-check` on staged diff. Catches what tests miss: edge cases, hard-coded paths, unguarded variables, security issues. 6. **Atomic commits** — each commit bundles code change + checkbox flip in `task_plan.md`. The diff and the progress live in the same commit; `git log -- planning/` tells the full story. @@ -450,6 +1535,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. @@ -522,10 +1642,415 @@ 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. + +### Don't edit files a long-running suite is still reading + +`devtools::test()` and its equivalents 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 mid-run are half-applied and the result describes a tree that never +existed. + +Cost two full Docker suites (~1 hour) on rfp#178, both reporting `FAIL 1`. The +failure was a test written *during* the run, executing against source from *before* +the fix that made it pass — nearly reported as a regression. **The tell is a moving +denominator:** 3490 passes, then 3496, then 3500, on "the same" tree. + +Before a long run, commit. While it runs, do work that touches nothing it reads — +issue bodies, PR text, reading, planning. If an edit cannot wait, kill the run +rather than let it produce a result that has to be re-litigated. 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. + +## 6. Subagents Are Evidence, Not Dependencies + +**Spawn on your own judgment. Don't block on one. Don't trust its status. Verify its claims in both directions.** + +### Spawning is your call, not the user's + +Deciding to spawn a subagent is an engineering judgment, the same kind as choosing +to write a test or run a grep. **Do not ask permission for it.** + +The user is usually not positioned to answer. Knowing whether a fan-out beats a +sequential read requires knowing the shape of the work — which you have and they do +not, so the question forces them to guess at a technical call. Under **Always Away** +it is worse than useless: the work stalls until they wake up, for an answer that was +yours to make. *"I wouldn't be in the know enough to know when that is"* +(airvine, 2026-08-27) is the whole problem in one line. + +This does not soften §1's asks — *"if uncertain, ask"* and *"if something is unclear, +stop and ask"*. Those are about **what the user wants**: intent, scope, an ambiguous +requirement, a tradeoff only they can weigh. This is about **how you carry it out**. +Ask about intent; decide about mechanism. A question starting "should I use…" is +almost always the second kind, and almost always yours to answer. + +**Spawn without asking when:** + +- A skill or convention mandates it — `/code-check`'s review rounds, the Plan review + in `planning.md`. That decision is already made; re-asking it is friction carrying + no information. +- You want fresh eyes on your own work. The mechanism and the measurements behind it + are in `code-check/SKILL.md`. +- A sweep over many files will **locate** what matters faster than reading serially. + The sweep finds candidates; it does not replace the read — `planning.md` is + explicit that agents sometimes report existing files as absent, so read directly + whatever you are going to act on. +- Independent items can run concurrently and nothing downstream needs them ordered. + +**Do it yourself when:** + +- One grep answers it. +- The work depends on conversation context a subagent will not have. +- You would sit idle waiting — spawn and keep working, or do it inline. + +**Bounds and defaults you enforce yourself, rather than converting into questions:** + +- **Two or three concurrent is the working default, and about five per task** is + where spend stops being incidental. Concurrency and cumulative total are different + quantities — `/code-check`'s three rounds plus a Plan review plus an ad-hoc sweep + never exceeds three at once while spending well past a handful. Bound both. +- Past that total, **say so in your next message.** An escape you grant yourself + silently is not a bound; it has to land in front of the user, after the fact. +- **Do not let a subagent fan out again.** Intent does not enforce this — the child + decides what it calls — so use the structure: the `Explore` and `Plan` types are + defined without the `Agent` tool and *cannot* spawn. `general-purpose` can, so when + you use it (as `/code-check` does), put "do not spawn subagents" in the prompt. The + one case on record — a research agent that had spawned 5 children and deadlocked + for **~3 hours** while still reporting as running (below) — never had a root cause + established, which is exactly why this bound is structural rather than advisory. +- Unnamed, delivering by file — `planning.md` carries the mechanics. +- **Report after, not before.** Say what you spawned, and relay what it found (per + `code-check/SKILL.md` — a subagent's report never reaches the user on its own). A + user can object to a spawn that already happened; they cannot usefully approve one + that has not. + +**What is genuinely the user's call is budget, not mechanism.** A workflow or +deep-research run fanning out dozens of agents is a spending decision and needs an +explicit ask. Two or three reviewers is not — that is just doing the work. + +Worth being concrete about the value, because the cost is the visible half and the +benefit is not: on 2026-08-27 two reviewers over one conventions draft returned +**20 findings**, caught **six** false factual claims in it, and killed a section that +would otherwise have shipped contradicting `code-check.md`. None of that review +happens if the spawn waits on a user who is away. + +### 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. + + +## 7. Evidence, Not Impressions + +**Measure before you characterise. Presence is not provenance. "Unknowable" is a +claim.** + +Six principles that all fail the same way: something *feels* established — because +it is visible, because it is present, because someone said so — and gets offered +with the confidence of a measurement. + +### Measure before you characterise + +When a decision turns on **what something contains**, open it and count. Do not +describe it from its structure, from an issue's claim about it, or from a tag list. +A heading tells you a thing is *present*, never that it is *populated* — an empty +`` and one with rules look identical in a list of child names. + +Four instances in one rfp session, each corrected by the user's follow-up question +rather than by review: a tradeoff described as three times its real size; an issue's +stale claim repeated as current; an installed version reported as sixteen releases +behind when a parallel session had updated it eighteen minutes earlier; and "nothing +on main addresses this" from a local `main` three commits behind — one `git fetch` +away from the truth. + +**A measurement carries the time it was taken.** One made earlier in the same +session is not a current one, least of all for anything another session can change +underneath it. For anything git-backed, `git fetch` first: reading a local clone and +reporting it as the state of the world is the same error with a longer fuse. + +**And before hand-rolling a parser for a probe, check whether the code already has +one.** A bespoke parser silently narrows the population it can see, and the result +looks like a measurement rather than a sample — worse than not measuring, because it +carries a number. Measured 10 of 80 with a hand-written matcher; routed through the +package's own resolver it was 14 of 117. + +### Presence is not provenance + +When something's **presence** is offered as evidence for **how it got there**, find +the fact that actually discriminates. A QGIS project's `3.30.1` stamp was offered as +evidence a desktop had opened it — but the template it was copied from carries that +stamp, so a never-opened project reads the same. What actually proved it was a +tracking key the template does not contain. + +The tell: reaching for the *most visible* fact rather than the *discriminating* one, +because the visible fact is consistent with the conclusion. **Consistency is not +support.** Before offering "X shows Y", ask what else would produce X. If anything +would, X is not evidence. + +When the user pushes back on an inference, re-derive rather than defend. The +conclusion often survives; the reasoning that reaches it is usually different. + +### "It can only be answered by testing" is a claim with an author + +An issue or a colleague saying a question needs a field season, a device or a deploy +is stating a claim, not a property of the problem. Spend the cheap probe first. + +rfp#186 opened with "three questions decide whether this is viable, and none can be +answered by reading." Two fell in about twenty minutes — one to reading a call +graph, one to re-reading a file already on disk — turning "run a field season, then +decide what to build" into "build it, then confirm one thing." + +The claim is usually made by someone who knows the domain, at a moment before they +looked. Not wrong so much as **unexamined**, which is what lets it survive into the +plan. Then **bound what the probe closed**: reading a desktop plugin says nothing +about the mobile app. An over-claimed probe is worse than none. + +### A real bug is not necessarily the reported bug + +A defect found while investigating a symptom is **evidence, not the answer**. Before +offering it as the cause, check that it produces *exactly* the symptom described, +including the details that sound incidental. + +Two confident wrong causes in a row on rfp#196 — a layer missing from a map theme +(a real bug, fixed) and a sub-pixel geometry (a real measurement). Both true; +neither explained the report. The actual cause was draw order, and the user named it +himself. The discriminating fact was in his words all along: *"as soon as I stop +tracking I can't see the track"* rules out both theories in one line. + +Finding a genuine defect feels like finding *the* defect — the relief of having an +explanation is what stops the check. Write the reported symptom out and ask whether +the proposed cause produces **all** of it. Say which parts are still unexplained: +"this is a real bug and it may not be your bug" is honest and cheap. + +### An enumeration is not a checklist + +A probe listing what exists — subkeys present, columns found, files listed — answers +"what is here", never "what do we want". Scope arriving this way looks +evidence-backed, so it survives review. + +On rfp#68, "the two Mergin subkeys that exist" became "the settings to verify", +then an item on a field checklist a human had to walk outdoors to complete. Nothing +in the codebase read or wrote `PhotoNaming`. Before a probe's output becomes work, +grep for each item and ask whether anything consumes it. When it duplicates +something already done another way, name the comparison — the existing approach +usually wins for a reason worth stating. + **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). @@ -541,16 +2066,40 @@ 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. **Plan-review with the Plan agent before committing the plan** — After scaffolding `task_plan.md` but BEFORE the baseline commit, 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. Address each before committing. 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, unfixable test-literal-string assertions, and a baseline-cache-shadow that would have produced a 6-second no-op run. Cost: ~5 min agent. Saves: hours of mid-implementation rework. -4. **Commit the plan** — After Plan-agent review + fixes. This is the baseline. -5. **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. -6. **Code check before commit** — Run `/code-check` on staged diffs before committing. Don't mark a task done until the diff passes review. -7. **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. + + **Get the findings into a file — but check who is doing the writing.** 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, surfacing only 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. + + **The `Plan` and `Explore` agent types have no Write tool, so they cannot write that file.** Both plan reviews on 2026-08-26 (gq#61, gq#40) were instructed to and were structurally unable to; one said so outright — *"I have no Write/Edit tools and am explicitly barred from creating files; an agent instruction can't lift that"* — and returned the full review as reply text instead. Both arrived intact, ~26 findings each. So: + + - **Read-only agent** (`Plan`, `Explore`): ask for the findings **in the reply**, then write them to `planning/active/review-.md` yourself. The file is still the deliverable; you are just the one creating it. + - **Agent type that can write**: put the file-path instruction in the first prompt, not as a follow-up. + + Asking for a file the agent cannot produce costs a round-trip, and — worse — sets you up to read an absent file as an absent review. Check the agent type's tools before writing the instruction. + + **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) @@ -568,7 +2117,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 @@ -589,6 +2140,11 @@ Append-only research log. Discoveries, technical analysis, things learned. ## [Topic] [What was found, with source/date] + +## Errors Encountered + +| Error | Resolution | +|-------|------------| ``` ### progress.md @@ -604,6 +2160,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 ``` @@ -615,6 +2203,77 @@ 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-infra.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 (or `code-check-infra.md` when it +is specific to provisioning), `ci-monitoring.md` for CI +behaviour, the domain convention otherwise. + ## Skills | Skill | When to use | @@ -639,6 +2298,11 @@ 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.) @@ -730,6 +2394,27 @@ Close function issues via commit messages — see Closing Issues in newgraph con 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 @@ -918,6 +2603,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