Downstream-state guard: fail loud when downstream dams aren't persisted - #245
Merged
Merged
Conversation
Single-WSG downstream-state guard. Phases derived from plan-mode exploration (2 Explore + 1 Plan agent) and verified against live fwapg. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_014kSEJoad4Bc553qU8eFiE6
link#227 refactors .lnk_pipeline_prep_dams so the downstream guard can share its exact filters rather than keep a second copy that drifts. That SQL is the most delicate in the package, so pin current behaviour before touching it. Uses explicit assertions, not expect_snapshot(): snapshots skip on CRAN, which would leave the regression net silently absent in exactly the run that matters. Counts pinned against docker fwapg (cabd.dams 2594 rows) on 2026-08-28 — ADMS 8 dams, all psc = 1, all snapped to a blue line, no duplicate dam_id (which is what DISTINCT ON (c.dam_id) buys). Also pins the cabd_additions psc-NULL rule: the `usa` CTE hardcodes NULL::integer for passability_status_code and the barrier_status CASE has no NULL arm, so the US placeholder dams are structurally incapable of becoming barriers despite barrier_ind = t in the CSV (verified: 0 rows in fresh.barriers). The guard must mirror this or it demands an override for the whole Columbia. Latent bug, filed separately in Phase 5 — pinned here so any fix is deliberate rather than incidental. Prerequisites confirmed: fresh (>= 0.33.0) pinned for frs_wsg_outlets(); whse_basemapping.fwa_downstream present (3 overloads). Baseline captured out-of-band for ADMS/KOTL/PARS to diff the Phase 1 refactor against. Worth recording: PARS has 0 dams in-WSG — all three of its blocking dams are downstream in UPCE/PCEA, which is exactly why the pre-flight exists. Refs #227 Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_014kSEJoad4Bc553qU8eFiE6
The link#227 guard has to apply link's exact dam filters — exclusions, blue-line xref, passability overrides, the 65 m lateral snap — or it flags dams the pipeline treats as passable, operators reach for the override reflexively, and the guard stops meaning anything. That is the issue's own "make or break" caveat. Solved by construction rather than by comment: the `cabd` and `matched` CTE bodies now live once in R/lnk_wsg_downstream_check.R and are consumed by both .lnk_pipeline_prep_dams and (next phase) the guard. .lnk_dams_cabd_sql is parameterized on its source, so the pipeline passes its staged <schema>.cabd_* tables while the guard will inline (VALUES ...) and touch nothing — the same pattern fresh 0.33.0 used to retire public.wsg_outlet. .lnk_dams_edit_values_sql builds those VALUES fragments. Absent or empty CSVs yield a typed single-NULL-row sentinel, because a VALUES list with no rows is a syntax error and an untyped NULL makes Postgres reject the join predicate. Behaviour-preserving, and verified as such rather than assumed: output diffed against the Phase 0 capture for ADMS (8 dams), KOTL (41) and PARS (0) — byte-identical on dam_id, psc, blue_line_key and measure in all three. Refs #227 Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_014kSEJoad4Bc553qU8eFiE6
lnk_wsg_downstream_check() verifies the DS-first precondition instead of asking the operator to assert it: find the blocking dams on the focal WSG's downstream flow path, confirm each is already persisted as a barrier, and fail loud naming them when they are not. Path, not membership. The issue proposed flagging when a downstream watershed group *contains* a blocking dam. Measured against the live DB, that over-fires: BULK's closure holds 18 blocking dams across LSKE/KISP/KLUM and none of them sit below its outlet, so a membership guard cries wolf on the issue's own motivating example and trains operators to reach for the override. The path form tests each dam with the measure-aware fwa_downstream() from the outlet shipped by fresh::frs_wsg_outlets(). It is complete, not merely cheaper: access walks downstream from every segment and every focal segment exits through the focal outlet, so the out-of-WSG barriers reachable from ANY focal segment are exactly those below it. Verified live — PARS names Peace Canyon, Site C and W.A.C. Bennett; BULK, KOTL and ADMS are clean; SLOC finds Brilliant. 0.5-0.6 s, against a 5 s budget. Three filters decide what actually blocks, and all three live downstream of prep_dams: psc IN (1,2), a real linear_feature_id join, and blue_line_key = watershed_key. The psc filter is why the cabd_additions US placeholders never appear — they carry NULL. Persistence is checked at DAM level, not WSG level. .lnk_wsg_persisted() cannot distinguish a group persisted with dams = FALSE, which would let the guard pass on a schema holding the streams but not the barriers. Dams in species-less WSGs are dropped: lnk_wsg_resolve() species-filters the closure, and such a dam is never persisted as a barrier, so demanding it would be an unfixable false alarm. A bare TRUE override is rejected. The justification is the mechanism — an override without one is precisely the hole tier 3 exists to close — and it is carried into the note so lnk_log_read() can later report that this network was built on a stated assumption. Missing fwa_downstream errors rather than auto-passing; a silent pass is the bug being fixed. Refs #227 Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_014kSEJoad4Bc553qU8eFiE6
…ition
wsg_run_one.R calls the guard between the species skip and lnk_pipeline_run,
and threads guard$note into the run log so an override or a deferral is
recorded alongside wsg_upstream — provable from two directions afterwards.
An unrecognised LNK_GUARD_DOWNSTREAM value errors rather than falling back to
a default: a typo must not silently disable the guard.
study_area_run.sh exports LNK_GUARD_DOWNSTREAM=warn on BOTH legs — the local
subshell and inside the ssh string. Missing the second is the silent failure
mode: cyphers would hard-fail, and because per-WSG failures soft-fail with a
WARN the WSG would be SKIPPED, which lnk_access(merge=TRUE) cannot repair.
That is strictly worse than the bug being fixed.
wsg_recompute_one.R re-runs the guard in error mode after consolidate. This is
what makes warn a deferral rather than a hole: by then every WSG is persisted,
so it must pass, and a failure means the consolidate genuinely dropped a
downstream group.
Also fixes a defect shipped in v0.45.0: .lnk_log_create_tables built the log
tables but never the schema. The run log is opened before lnk_persist_init (the
open row must predate any write so wsg_upstream reflects the starting state),
so on a brand-new persist schema it failed with "schema does not exist". It
only worked until now because every schema tested already existed. Found by
running the guard's smoke tests into scratch schemas.
Verified live, end to end:
typo mode -> errors, names the valid values
PARS, empty schema -> exit 1, names Bennett/Peace Canyon/Site C + DS-first order
ADMS, new schema -> guard passes, models in 2.1 min, log row lands
override -> proceeds; note reads "guard(override): 3 unmodelled
downstream dam(s) - PCEA(1), UPCE(2) - Site C fishway
operational, ref doc-123"
warn -> proceeds; note reads "guard(warn): ... at open"
Refs #227
Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_014kSEJoad4Bc553qU8eFiE6
…e v0.46.0 RUNBOOK 8c carries the membership-vs-path measurement — BULK's closure holds 18 blocking dams and none are below its outlet — precisely so nobody "simplifies" the guard back to a membership test later. It also records the known bound: the guard inherits frs_wsg_drainage's one-outlet-per-group model, so a WSG draining by two independent paths would be under-covered. RUNBOOK 6b gains the guard(...) note strings so lnk_log_read() output reads cleanly, with the cross-check against wsg_upstream spelled out. Follow-up #244 filed: cabd_additions dams carry barrier_ind = t but the usa CTE hardcodes psc NULL and the barrier_status CASE has no NULL arm, so the US placeholders can never become barriers. Left as a decision rather than fixed here — options (1) and (2) change modelled access in the Columbia and need a parity check; the behaviour is pinned by a test either way. Suite 1513 pass, 0 failures. Refs #227 Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_014kSEJoad4Bc553qU8eFiE6
lintr line_length_linter; no new lint classes in the new file now. The 13 remaining indentation lints are the package-wide sprintf-SQL idiom (317 across 28 files in R/). Refs #227 Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_014kSEJoad4Bc553qU8eFiE6
Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_014kSEJoad4Bc553qU8eFiE6
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Enforces the DS-first precondition
data-raw/wsg_run_one.Rhas stated in its own header since #175 and that nothing checked. Accessibility is not computed from the focal watershed group alone — link reads the already-persisted barriers of the groups downstream — so modelling out of order writesstreams_access/streams_mapping_codemarking segments accessible that are in fact dammed off, printsdone in N min, and exits 0. A wrong answer indistinguishable from a right one.New exported
lnk_wsg_downstream_check()verifies it: find the blocking dams on the focal WSG's downstream flow path, confirm each is already persisted as a barrier, fail loud naming them when not.The finding that reshaped the design
The issue proposed checking for blocking dams in the downstream closure's WSGs — a membership test. Measured live, it cries wolf on the issue's own motivating example:
At that rate operators reach for the override reflexively and the guard stops meaning anything — the failure the issue's own "make or break" caveat warns about, though it attributes the cause to edit-CSV fidelity rather than the predicate.
The path form tests each dam with the measure-aware
whse_basemapping.fwa_downstream()fromfresh::frs_wsg_outlets(). It is complete, not merely cheaper: access walks downstream from every segment and every focal segment exits through the focal outlet, so the out-of-WSG barriers reachable from any focal segment are exactly those below it. ~0.5 s against a 5 s budget.RUNBOOK.md§8c carries the measurement so it isn't "simplified" back.Three further corrections to the issue body
fwa_watershed_groups_polyfor the proposed 2.4 s spatial join — code columns are NULL on docker fwapg (test-lnk_wsg_resolve live tests route to wrong DB (skip-gate vs lnk_db_conn mismatch) #222).watershed_group_codecomes from the snappedfwa_stream_networks_sprow, asprep_damsitself does; also 8× faster..lnk_wsg_persisted()is the wrong granularity — cannot distinguish a WSG persisted withdams = FALSE, which would pass a schema holding streams but not barriers. Persistence checked per dam.lnk_access(merge=TRUE)cannot repair. Strictly worse than the bug. Resolved withwarnon both legs + a post-condition inwsg_recompute_one.R.Anti-drift by construction, not by comment
The
cabdandmatchedCTE bodies now live once and are consumed by both.lnk_pipeline_prep_dams()and the probe, parameterized on source — staged tables for the pipeline, inline(VALUES …)for the guard. A guard that snapped or filtered differently would flag dams the pipeline treats as passable.A golden capture was taken before the refactor and diffed after: byte-identical for ADMS (8 dams), KOTL (41), PARS (0).
Also fixes a v0.45.0 defect
.lnk_log_create_tables()built the run-log tables but never the schema, so a brand-new persist schema failed withschema does not exist. The log opens beforelnk_persist_initby design (the open row must predate any write sowsg_upstreamreflects the starting state), and every schema tested until now already existed. Found by running the guard's smoke tests into scratch schemas.Verified live, end to end
LNK_GUARD_DOWNSTREAMguard(override): 3 unmodelled downstream dam(s) — PCEA(1), UPCE(2) — Site C fishway operational, ref doc-123guard(warn): … at openSuite 1513 pass, 0 failures. No new lint classes.
Reviewer notes
TRUEis rejected — the justification is the mechanism, and it lands in<persist>.log.notesbesidewsg_upstreamsolnk_log_read()reports afterwards that a network was built on a stated assumption.study_area_run.shexportswarnon both legs — local subshell and inside the ssh string. Missing the second is the silent failure mode.frs_wsg_drainage()'s one-outlet-per-group model.cabd_additionsdams carrybarrier_ind = tbut can never become barriers (psc NULL). Two of three remedies change modelled access in the Columbia and need a parity check; pinned by a test either way.Closes #227
🤖 Generated with Claude Code
https://claude.ai/code/session_014kSEJoad4Bc553qU8eFiE6