From 6b38ccf2aefa15c27d5a9ba5577b96e4a58969d3 Mon Sep 17 00:00:00 2001 From: Polichinl Date: Sun, 2 Aug 2026 00:31:20 +0200 Subject: [PATCH] =?UTF-8?q?fix(env):=20S1=20#182=20=E2=80=94=20appwrite=5F?= =?UTF-8?q?env=20logs=20before=20it=20raises,=20and=20a=20guard=20over=20b?= =?UTF-8?q?oth=20entry=20validators?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ADR-008:48 requires raised structural failures to be logged at ERROR; :51 that "raising is not a substitute for logging." `assert_env_declared` raised `EnvironmentError` naming every missing variable and wrote no log record, so an operator reading logs after a refused run found nothing (register C-71). The two-line fix matters less than why it was missing. `contract/launch_config.py` was written in #149 by mirroring this module, inherited the flaw, was fixed in review, and left the module it copied as the odd one out. The new test therefore parametrises the ADR-008 obligation over BOTH validators rather than pinning one: adding a third is one line, and the pair cannot drift again. Deliberately NOT extracted into a shared helper. WET before DRY — they answer different questions (did the launcher assemble the environment / did it declare the delivery mode) against different sources (os.getenv / a passed dict). Named trigger to revisit: a third entry-validation module. Recorded in epic #181. Logging here is only safe because `missing` holds NAMES: membership is decided by `os.getenv(name)` being falsy and the resolved value is never read. CONNECTION_ENV carries the APPWRITE_DATASTORE_API_KEY secret slot, so a second test sets a sentinel value and asserts it never reaches a record — a future "log the current environment for debuggability" cannot land quietly. Mutation-tested: removing the logger.error line fails 2 of the new tests. Also registers C-74, found by this story's own review. tests/test_redaction_guard.py pins the þing-01 #135 redaction audit over five module roots; #153 moved the machinery from unfao/ to contract/ and four were never re-pointed. rglob on a nonexistent directory yields nothing rather than raising, so the guard scans 6 files instead of ~16 and passes. Not a leak — the relocated modules are still credential-blind — but C-57 and Cluster I both vouched for that guard in writing, and both are corrected in place. Tracked as #192. Co-Authored-By: Claude Opus 5 (1M context) --- reports/technical_risk_register.md | 48 +++++++++-- tests/test_env_declaration.py | 93 +++++++++++++++++++++- views_postprocessing/unfao/appwrite_env.py | 25 +++++- 3 files changed, 154 insertions(+), 12 deletions(-) diff --git a/reports/technical_risk_register.md b/reports/technical_risk_register.md index 6222797..5ed255a 100644 --- a/reports/technical_risk_register.md +++ b/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 diff --git a/tests/test_env_declaration.py b/tests/test_env_declaration.py index 5ad57f7..8ba81f6 100644 --- a/tests/test_env_declaration.py +++ b/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. diff --git a/views_postprocessing/unfao/appwrite_env.py b/views_postprocessing/unfao/appwrite_env.py index 6e2724e..b60a28d 100644 --- a/views_postprocessing/unfao/appwrite_env.py +++ b/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)