S1 (#182) — appwrite_env logs before it raises; ADR-008 pinned across both entry validators

reports/technical_risk_register.md

@@ -4,9 +4,9 @@
 |-------------------|--------------------------------------|
 | Project           | views-postprocessing                 |
 | Owner             | Dylan Pinheiro / PRIO MD&D Team      |
-| Last Updated      | 2026-08-01                           |
-| Total Concerns    | 73                                   |
-| Open Concerns     | 24                                   |
+| Last Updated      | 2026-08-02                           |
+| Total Concerns    | 74                                   |
+| Open Concerns     | 25                                   |
 | Resolved Concerns | 49                                   |
 
 ---
@@ -45,9 +45,9 @@ covered a single open entry (see Historical clusters below).
 
 ### Cluster I: Governance-artifact drift
 **Root cause:** the register, ADR prose, and issue bodies are hand-maintained mirrors of cross-repo state that moves under them.
-**Entries:** C-44, C-46, C-47, C-57 (the cross-repo instance: a registry referenced by URL cannot be diffed by a local test) — plus this register's own findings at review-rr 2026-07-31 (header miscount, two RESOLVED entries misfiled under Open, eight stale `unfao.py` line ranges after the manager grew 273→636 lines, two unnamespaced foreign-register IDs). Historical precedent: the entire C-48–C-55 ADR-013 audit series, and C-42/C-47.
+**Entries:** C-44, C-46, C-47, C-57, C-74 (a guard whose declared scan roots silently stopped existing — the cluster's disease inside the cluster's own prescription) — plus this register's own findings at review-rr 2026-07-31 (header miscount, two RESOLVED entries misfiled under Open, eight stale `unfao.py` line ranges after the manager grew 273→636 lines, two unnamespaced foreign-register IDs). Historical precedent: the entire C-48–C-55 ADR-013 audit series, and C-42/C-47.
 **Highest tier:** 3
-**Fix strategy:** this repo already solved this disease once — the ADR-013 audit series ended with **40 permanent guard tests** (`tests/test_falsify_adr013_*.py`), and the same pattern now guards the þing-01 invariants (`tests/test_env_declaration.py`, `tests/test_redaction_guard.py`). There is **no equivalent for the register**. A small `tests/test_register_integrity.py` — header counts match section counts; no RESOLVED body under `## Open Concerns`; every `C-\d+`/`D-\d+` reference resolves or is namespaced to a foreign register — would make this class self-detecting.
+**Fix strategy:** this repo already solved this disease once — the ADR-013 audit series ended with **40 permanent guard tests** (`tests/test_falsify_adr013_*.py`), and the same pattern now guards the þing-01 invariants (`tests/test_env_declaration.py`, `tests/test_redaction_guard.py` — the latter **only over the roots that still exist**, see C-74: a guard is only as good as the assertion that its inputs are real). There is **no equivalent for the register**. A small `tests/test_register_integrity.py` — header counts match section counts; no RESOLVED body under `## Open Concerns`; every `C-\d+`/`D-\d+` reference resolves or is namespaced to a foreign register — would make this class self-detecting.
 **Resolution scope:** Full for the mechanical half.
 
 ### Cluster J: Delivery aftercare has no mechanism
@@ -481,9 +481,11 @@ This repo's half is well guarded. `tests/test_env_declaration.py` pins that ever
 
 Two named changes are already anticipated and will fire this trigger: the **retirement of the legacy `APPWRITE_DATASTORE_API_KEY`** in favour of the three-tier read/write/provision slots (D4), and any target-coordinate addition for the second store (issue #97). Tier 3 — coordination and cost-of-change across a repo boundary; the failure is loud, not silent, and D6's entry validation is the backstop that keeps it that way.
 
-**Deliberately out of scope here:** the þing-01 redaction clause is already mechanically enforced (`tests/test_redaction_guard.py` — the delivery modules stay credential-blind and the provenance description is a closed keyset), and D2's ruling that **integration tests against the production Appwrite project are FORBIDDEN** (no non-production project exists) is a standing prohibition, not a drift risk.
+**Deliberately out of scope here:** the þing-01 redaction clause is mechanically enforced (`tests/test_redaction_guard.py` — the delivery modules stay credential-blind and the provenance description is a closed keyset), and D2's ruling that **integration tests against the production Appwrite project are FORBIDDEN** (no non-production project exists) is a standing prohibition, not a drift risk.
 
-Cross-refs: C-33 (store identity still hardcoded per store — the same env surface, different concern), C-58 (what happens when a coordinate is wrong rather than missing), C-44 (the pipeline-core version coupling that would carry a registry change), issues #134/#135/#138 (this repo's discharged þing-01 obligations), #104 (README env block placeholders).
+**⚠ CORRECTED 2026-08-02.** The word *already* above overclaimed: **C-74** showed that guard scans one of its five declared roots, four having pointed at paths that #153 moved. The keyset half is enforced; the credential-blindness half is enforced over `delivery/` only until C-74 lands.
+
+Cross-refs: C-74 (the guard this paragraph vouched for), C-33 (store identity still hardcoded per store — the same env surface, different concern), C-58 (what happens when a coordinate is wrong rather than missing), C-44 (the pipeline-core version coupling that would carry a registry change), issues #134/#135/#138 (this repo's discharged þing-01 obligations), #104 (README env block placeholders).
 
 ---
 
@@ -701,6 +703,38 @@ Cross-refs: **C-25** (whose resolution this corrects), **C-40** (the inherited p
 
 ---
 
+### C-74: The þing-01 redaction guard scans four paths that stopped existing — it reports success while covering one root of five
+
+| Field | Value |
+|-------|-------|
+| ID | C-74 |
+| Tier | 3 — **not a leak today**: the relocated modules were checked directly and are still credential-blind (zero hits for `os.environ`, `getenv`, `load_dotenv`, `API_KEY`, `credentials`). What is gone is the thing that would notice them ceasing to be. A security-adjacent control that cannot fail is a maintainability defect until the day it is a correctness one. |
+| Source | `review-diff` (2026-08-02) — S1/#182 review; found while reading the redaction discipline the new ADR-008 test cites as "the wider rule" |
+| Trigger | When any module under `contract/wire`, `contract/historical.py`, `contract/track_a_source.py` or `contract/frame_extraction.py` gains environment access — the #135 guard will not report it. Also fires on **the next package move**: `rglob` on a vanished root yields silence, not an error, so any future relocation narrows the scan again with no signal |
+| Location | `tests/test_redaction_guard.py:25-31` (`_CREDENTIAL_BLIND`), `:34-39` (`_python_sources`) |
+
+The þing-01 delivery-log redaction audit (#135, orð_09 §3) certified five module trees as credential-blind and pinned that finding as a permanent guard. Epic #148's S5 (#153) then moved the machinery out from under `unfao/` into `contract/`. **Four of the five roots were never re-pointed:**
+
+| declared root | exists | files scanned |
+|---|---|---|
+| `unfao/wire` | no | 0 |
+| `delivery` | yes | 6 |
+| `unfao/historical.py` | no | 0 |
+| `unfao/track_a_source.py` | no | 0 |
+| `unfao/frame_extraction.py` | no | 0 |
+
+**The failure is silent by construction.** `_python_sources` branches `if path.is_file(): … else: path.rglob("*.py")`, and `rglob` on a **nonexistent** directory yields an empty iterator rather than raising. A missing root and a clean root are therefore indistinguishable to the test, which passes either way. The guard reports an audited fact as pinned while pinning roughly a third of it.
+
+**Why this is Cluster I and not merely a stale path.** The register's own text asserts the opposite. **C-57** says, in its "deliberately out of scope" paragraph, that *"the þing-01 redaction clause is **already mechanically enforced** (`tests/test_redaction_guard.py` — the delivery modules stay credential-blind…)"* — and **Cluster I's fix strategy cites this very file** as an example of the guard pattern working. Both were written in good faith and both are now overclaims. That is the cluster's disease reproducing inside the cluster's own prescription.
+
+It is also the **sixth** instance of the stale-claim class found in two days, after C-63 and C-47 (filed Open with the defect fixed), C-43/C-59/C-61 (filed Open with their stated closing conditions met), #158 (closed with the code half undone) and `tests/test_validation.py` (claiming fidelity to a method it no longer resembles — **C-03**). The common shape is not carelessness: it is that a *move* leaves prose and paths behind, and nothing in this repo asserts that a declared path exists.
+
+**Mitigation:** re-point the four roots at `contract/`, and add a root-existence assertion so a future relocation fails loudly instead of silently narrowing. The second half is the load-bearing one — re-pointing fixes today, asserting existence fixes the class. Tracked as a story under **epic #181**.
+
+Cross-refs: **C-57** (whose "already mechanically enforced" claim this falsifies — corrected in place), **C-03** (the same class in `test_validation.py`), **C-46** (a different guard that also does not run, by a different mechanism), **C-63**, **C-47**, **Cluster I**, #135, #153, #182.
+
+---
+
 ## Disagreements
 
 ### D-12: Post-Run-0 infrastructure & naming intents — repo rename, internal-store transport, compute co-location

tests/test_env_declaration.py

@@ -7,16 +7,20 @@
 verdict retired it — the launcher declares its env sourcing (views-models M3,
 merged), and this package only validates (verdict D6).
 
-Imports only `unfao.appwrite_env` (dependency-light by design); the manager
-module itself needs views-pipeline-core, absent in test environments, so the
-manager-side facts are pinned by source scan — the repo's standing pattern.
+Imports `unfao.appwrite_env` and its sibling `contract.launch_config` — both are
+dependency-light by design (no views-pipeline-core, no pandas), which is what
+lets them be exercised directly here. The manager module is not: it needs
+views-pipeline-core, absent in test environments, so the manager-side facts are
+pinned by source scan instead — the repo's standing pattern.
 """
 
+import logging
 import re
 from pathlib import Path
 
 import pytest
 
+from views_postprocessing.contract import launch_config
 from views_postprocessing.unfao import appwrite_env
 
 
@@ -96,6 +100,89 @@ def test_declared_names_match_the_manager_reads():
     assert text.count("appwrite_env.assert_env_declared(") == 2
 
 
+# ── ADR-008 across BOTH entry validators (S1 / #182, register C-71) ──────────
+#
+# `appwrite_env` asserts the launcher assembled the *environment*; `launch_config`
+# asserts it declared the *delivery mode*. They are deliberate siblings, NOT a
+# shared abstraction (WET before DRY — the trigger to extract is a THIRD such
+# module, #181). Siblings drift: `launch_config` was written in #149 by mirroring
+# `appwrite_env`, inherited its missing log call, was fixed in review, and left
+# the module it copied as the odd one out for two weeks.
+#
+# Parametrising the ADR-008 obligation over both is what stops that recurring —
+# and adding a third validator is one line here, not a new test.
+_REFUSALS = (
+    pytest.param(
+        lambda: appwrite_env.assert_env_declared(("VPP_S1_ABSENT_VAR",), store="test_store"),
+        EnvironmentError,
+        "VPP_S1_ABSENT_VAR",
+        id="appwrite_env.assert_env_declared",
+    ),
+    pytest.param(
+        lambda: launch_config.assert_contract_mode({}),
+        launch_config.LaunchConfigError,
+        launch_config.WIRE_CONTRACT_KEY,
+        id="launch_config.assert_contract_mode",
+    ),
+    pytest.param(
+        lambda: launch_config.assert_frame_native_historical("pandas_dataframe"),
+        launch_config.LaunchConfigError,
+        launch_config.FEATURE_FRAME_FORMAT,
+        id="launch_config.assert_frame_native_historical",
+    ),
+)
+
+
+@pytest.mark.parametrize("refuse,exc,expected_token", _REFUSALS)
+def test_every_entry_validator_logs_before_it_raises(refuse, exc, expected_token, caplog):
+    """ADR-008:48/51 — a structural refusal must leave a persistent record.
+
+    :48 requires raised structural failures to be logged at ERROR or higher; :51
+    that *"raising is not a substitute for logging."* A launcher misconfiguration
+    is a structural failure by any reading, and these are the two seams whose
+    whole job is to make one visible. An operator reading logs after a refused
+    run must find the reason there, not only in a traceback they no longer have.
+    """
+    with caplog.at_level(logging.ERROR):
+        with pytest.raises(exc):
+            refuse()
+
+    errors = [r for r in caplog.records if r.levelno >= logging.ERROR]
+    assert len(errors) == 1, (
+        f"expected exactly one ERROR record from the refusal, got {len(errors)}: "
+        f"{[r.getMessage() for r in errors]}"
+    )
+    assert expected_token in errors[0].getMessage(), (
+        "the log record must name what was missing — a record that says only "
+        "'refused' sends the operator back to the traceback it was meant to replace"
+    )
+
+
+def test_the_environment_refusal_logs_names_and_never_values(monkeypatch, caplog):
+    """The one place ADR-008 and the redaction discipline could collide.
+
+    `CONNECTION_ENV` carries `APPWRITE_DATASTORE_API_KEY` — a secret slot. Logging
+    a refusal is only safe because `missing` holds NAMES: membership is decided by
+    `os.getenv(name)` being falsy and the resolved value is never read. This test
+    pins that, so a future "let's log the current environment for debuggability"
+    cannot land quietly. See tests/test_redaction_guard.py for the wider rule.
+    """
+    sentinel = "s1-sentinel-secret-value-must-never-be-logged"
+    monkeypatch.setenv("APPWRITE_DATASTORE_API_KEY", sentinel)
+    monkeypatch.delenv("APPWRITE_ENDPOINT", raising=False)
+
+    with caplog.at_level(logging.ERROR):
+        with pytest.raises(EnvironmentError):
+            appwrite_env.assert_env_declared(appwrite_env.CONNECTION_ENV, store="unfao_bucket")
+
+    logged = "\n".join(r.getMessage() for r in caplog.records)
+    assert "APPWRITE_ENDPOINT" in logged, "the missing NAME must be reported"
+    assert sentinel not in logged, "a secret VALUE reached a log record"
+    assert "APPWRITE_DATASTORE_API_KEY" not in logged, (
+        "the key was set, so it is not missing — it must not appear at all"
+    )
+
+
 def test_secret_env_names_follow_the_platform_naming_rule():
     # PLATFORM-001 D3: suffix _API_KEY/_PASSWORD/_TOKEN ⇒ secret. Exactly one
     # declared name is a secret; every other declared name is a coordinate.

views_postprocessing/unfao/appwrite_env.py

@@ -13,8 +13,11 @@
 
 from __future__ import annotations
 
+import logging
 import os
 
+logger = logging.getLogger(__name__)
+
 CONNECTION_ENV = (
     "APPWRITE_ENDPOINT",
     "APPWRITE_DATASTORE_PROJECT_ID",
@@ -40,12 +43,30 @@
 
 def assert_env_declared(names: tuple, *, store: str) -> None:
     """Entry validation (þing-01 D6): every required name resolved and non-empty,
-    or raise naming ALL missing variables — never a partial config that half-works."""
+    or raise naming ALL missing variables — never a partial config that half-works.
+
+    Logs at ERROR before raising (ADR-008): the refusal must survive in the run's
+    log, not only in a traceback the operator no longer has. Names only — the
+    resolved value of a variable is never read, and never logged.
+
+    Args:
+        names: the environment-variable names this store requires.
+        store: the store being configured, for the message.
+
+    Raises:
+        EnvironmentError: naming every missing variable.
+    """
     missing = [name for name in names if not os.getenv(name)]
     if missing:
-        raise EnvironmentError(
+        # `missing` holds NAMES, never values: membership is decided by
+        # `os.getenv(name)` being falsy, and the resolved value is never read.
+        # That is what makes this loggable at all — CONNECTION_ENV carries the
+        # APPWRITE_DATASTORE_API_KEY secret slot.
+        err_msg = (
             f"{store}: the launcher did not assemble the required environment — "
             f"missing {missing}. Coordinates come from the PLATFORM-001 registry "
             "(views-models run.sh declares its sourcing); the secret is the "
             "operator slot. This package no longer loads any dotenv (#134)."
         )
+        logger.error(err_msg)  # ADR-008: logged persistently AND raised
+        raise EnvironmentError(err_msg)