From 9f735439cdc2376993260122071dc4c720467ae0 Mon Sep 17 00:00:00 2001 From: Polichinl Date: Wed, 12 Aug 2026 12:38:25 +0200 Subject: [PATCH 1/5] =?UTF-8?q?docs(register):=20C-89..C-96=20=E2=80=94=20?= =?UTF-8?q?the=20post-merge=20review=20of=20#239,=20and=20the=20decisions?= =?UTF-8?q?=20it=20forced?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Registered before any code moves, so the reasoning survives the fixes. FIVE NEW FROM /code-review max ON THE MERGED #239 (all reproduced here, none taken on trust): C-89 the no-copy guard prints the value it forbids, in one branch of three and proven on one side of two; C-90 the replacement drift checks assert less than their docstrings say, and one mutation proof is a tautology; C-91 the git plumbing turns ordinary developer states into hard errors with the wrong diagnosis; C-92 this repository lost its only check that the FAO consumer still SELECTS by the delivery label; C-93 a mutation proof written by whoever wrote the guard tests that author's imagination — 13 forms proposed by the author, 13 caught; 29 proposed independently, 15 missed. THREE MORE FROM THE EXPERT REVIEW OF THE STANDING DECISIONS: C-94 nothing anywhere detects an invisible delivery at the time it happens — every mechanism aimed at it is a CI-time proxy — deferred with a trigger and an owner rather than omitted; C-95 the integration-test prohibition is cited to þing-02 D2 in three places when it is þing-01 D2, and the real ruling is CONDITIONAL and GRANTS read-only preflight; C-96 the registry table carrying that permission is classified "not about this package". C-95 and C-96 compound into the finding that matters most for planning: a whole class of mechanism was believed closed for weeks because a citation pointed at the wrong verdict and the table stating the permission was declared none of our business. AMENDED: C-57 (my own amendment overclaimed — it cited a backticked example as proof the mid-sentence class was covered; it passes only because of the backticks, and a fourth failure direction was found: removing the length floor admitted this repo's own package names into a security guard's ban-set); C-86 (its partial-mitigation paragraph overstates what shipped — one of the three replacement checks does not match on the facts this repository declares); C-87 (one partner's residual got worse, not better). Header 93/20/73 -> 96/23/73. Epic #241, tracking #251. Co-Authored-By: Claude Opus 5 (1M context) --- reports/technical_risk_register.md | 212 ++++++++++++++++++++++++++++- 1 file changed, 209 insertions(+), 3 deletions(-) diff --git a/reports/technical_risk_register.md b/reports/technical_risk_register.md index 20bc1a0..6c16d4c 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-11 | -| Total Concerns | 88 | -| Open Concerns | 15 | +| Last Updated | 2026-08-12 | +| Total Concerns | 96 | +| Open Concerns | 23 | | Resolved Concerns | 73 | --- @@ -161,6 +161,186 @@ that indexes only deleted code is noise. ## Open Concerns +### C-96: The registry table that says which live checks this package may build is classified as none of our business + +| Field | Value | +|-------|-------| +| ID | C-96 | +| Tier | 4 — no defect follows from it directly. It is here because it concealed an available mechanism for weeks, which is a cheap mistake to make again. | +| Source | `/expert-code-review` of the standing decisions, 2026-08-12 | +| Trigger | Anyone asks whether a live check against the production Appwrite project is permitted. | +| Owner | This repository. | +| Location | `tests/test_env_declaration.py:509` (`_TABLE_ROLE["test_environment"] = "IGNORED"`). | + +`[test_environment]` is classified `IGNORED` with the reason *"a fact about the platform, not about this package"*. It is in fact the clause that governs **what live checks this package is permitted to build**: + +> `status = "none"` — *"No non-production Appwrite project exists (þing-01 S23). Until the operator creates one: integration tests against the production project are FORBIDDEN by the seam contract; **read-only preflight validation is the only permitted live check**."* + +The second half is a **permission**, and this repository spent weeks believing the whole clause was a prohibition. Reclassify and read it, or record why a permission that changes what we may build is not a fact we depend on. + +Cross-refs: **C-94** (the mechanism this permission authorises), **C-95** (the mis-citation that compounded it), issue #249. + +--- + +### C-95: The integration-test prohibition is cited to the wrong verdict, in three places + +| Field | Value | +|-------|-------| +| ID | C-95 | +| Tier | 3 — a citation error, but one that closed off a design option. Namespacing across six repositories and two þings is a known hazard here; this is it landing. | +| Source | `/expert-code-review` of the standing decisions, 2026-08-12 | +| Trigger | Anyone reasons from the integration-test prohibition — for a preflight, a drill, or a new ADR. | +| Owner | This repository. | +| Location | `reports/technical_risk_register.md` (~:475, ~:1628, ~:1717); anywhere else citing þing-02 D2 for this. | + +This register cites **þing-02 D2** for the ruling that integration tests against the production Appwrite project are forbidden. þing-02 D2 is about identity and key separation. The ruling is **þing-01 D2** (`þingit/01_identity_secrets_config/orð_dómr.md:53-61`), and it differs from the paraphrase in two ways that matter: it is **conditional** (*"until the operator creates one"*), and it **grants** read-only preflight validation as the permitted live check. It also records that creating a test project is **assigned to the operator** and gates the provisioning-path drill — an open assignment, not a closed door. + +Cross-refs: **C-94**, **C-96**, þing-01 `orð_dómr.md` D2, issue #249. + +--- + +### C-94: No mechanism anywhere detects an invisible delivery at the time it happens + +| Field | Value | +|-------|-------| +| ID | C-94 | +| Tier | 2 — the failure mode is invisible by construction and lands on the live FAO path: upload succeeds, storage is billed, the consumer's endpoint returns empty, nothing raises anywhere. ADR-013 §4.1a's *"invisible to the consumer, not merely degraded."* | +| Source | `/expert-code-review` of the standing decisions, 2026-08-12 | +| Trigger | **Either.** (a) A delivery is reported empty by a consumer or by FAO. (b) `APPWRITE_READ_API_KEY` is provisioned for the launcher — at which point the deferral below has no remaining cost. | +| Owner | This repository, for the mechanism. The credential is the operator's. | +| Location | `views_postprocessing/contract/wire/sink.py` (the upload path, where nothing verifies); `views_postprocessing/delivery/`. | + +Every mechanism this platform has aimed at invisible delivery is a **CI-time proxy** for it: we check our label against the registry, the consumer checks their constant against the registry, and — until this week — we parsed their source. None of them observes the outcome. Grepping the sink and the delivery package finds no read-back, no findability check, no assertion that what was uploaded can be retrieved by the name the consumer will query. + +**The mechanism that would close it is known and is legal.** A producer-side **read-only findability preflight**: after upload, query the store read-only for a document whose `name` equals the declared `CONSUMER_DOCUMENT_NAME`; assert non-empty; log at ERROR and raise (ADR-008); remedy is the existing operator quarantine. It is authorised by the seam contract (see **C-96**), the `APPWRITE_READ_API_KEY` slot is already declared, and it is the only mechanism that survives a **third-party-operated private consumer**, because it asks nothing of them. + +**Why it is deferred, stated honestly.** It needs a read credential wired into the launcher — an operator action, not a code change — and it adds a live network call to the delivery path. Delivery works today. Building it now would be building the right thing at the wrong time. That is a deferral with a trigger and an owner (ADR-014 §4), not an omission. + +**What it would not cover, so nobody over-reads it later:** it proves the document is findable by that name in the store. It does not prove the consumer's code queries by that name. That last link is theirs, and issues asking each consumer to bind their *query* to their constant are filed under #248. + +Cross-refs: **C-92** (the check we deleted rather than replaced), **C-87** (the broader residual), **C-96** (the permission), ADR-013 §4.1a, ADR-017 §5/§8, issue #248. + +--- + +### C-93: A mutation proof written by whoever wrote the guard tests that author's imagination, not the guard + +| Field | Value | +|-------|-------| +| ID | C-93 | +| Tier | 3 — no defect of its own. It is the reason several of the defects below survived three reviews, and it changes what "mutation-proven" is allowed to mean in this codebase. | +| Source | `/code-review max` on PR #239 post-merge, 2026-08-11, corroborated by measurement | +| Trigger | The next time a guard is defended in a pull-request description as "mutation-proven" against a list of cases the same change authored. | +| Owner | Whoever writes the next guard; the standard belongs in ADR-014 §2. | +| Location | ADR-014 §2; `tests/test_env_declaration.py::test_no_coordinate_value_is_copied_into_this_repo`; every `_MUTANTS`-style proof in `tests/`. | + +ADR-014 §2 says a guard is mutation-proven or it is decoration. That is right, and it is not sufficient. **A mutation proof is only as good as the mutant list, and a mutant list written by the author of the guard covers the cases the author already had in mind — which are, by construction, the cases the guard already handles.** + +Measured, on the guard that protects a public repository from publishing a coordinate value. Its author (this assistant) proposed thirteen input forms, proved all thirteen caught, and wrote that result into C-57 and into a commit message. An independent review then proposed twenty-nine forms. **Fifteen missed.** The thirteen were not a sample of how people write markdown; they were a sample of what the author had thought of, and every one of them happened to share the property the guard depended on. + +This is not the same as ADR-014 §2 failing. The proof was real, it was executed, and every case in it genuinely passed. The gap is that "proven against N mutations" reads as a statement about the guard when it is a statement about N. + +**What would change.** Two candidates, and they are not exclusive: (a) for a guard whose failure is silent — a leak, an invisible delivery — the mutant list is written or extended by something other than the change that wrote the guard; (b) the mutant list is a *declared, named* artifact in the test file rather than a paragraph in a commit message, so the next contributor can see what was and was not tried. `tests/test_ci_sibling_coverage.py::_MUTANTS` is already (b) and is the pattern to copy. + +Cross-refs: **C-57** and **C-89** (the guard this was measured on), **C-90** (a proof that proved nothing at all), ADR-014 §2. + +--- + +### C-89: The guard against publishing a coordinate value publishes it — in one branch of three, proven on one side of two + +| Field | Value | +|-------|-------| +| ID | C-89 | +| Tier | 2 — a confidentiality exposure on a public repository with no signal that it happened. It fires on exactly the event the guard exists to catch, and CI logs are world-readable and are not retroactively redactable. | +| Source | `/code-review max` on PR #239 post-merge, 2026-08-11; every element verified in this repository | +| Trigger | A literal coordinate value is committed into a `.py` under `views_postprocessing/` — the violation the scan exists for — on any branch whose CI runs. | +| Owner | This repository. | +| Location | `tests/test_env_declaration.py:1065` (the AST branch), `:1042` (the `secret` exemption), `:937` (the half-sided rotation proof). | + +The no-copy scan exists because README.md once carried four real coordinate values, two lines below the sentence promising they are never copied, in a public repository. It has three parts and they do not agree with each other about the one rule that matters. + +**The Python branch prints the value.** `copied.append(f"{source}:{node.lineno} = {node.value!r}")`. The markdown branch sixty-eight lines below was rewritten on 2026-08-11 to stop doing precisely this, and `_describe_changes` was added in the same change with a docstring whose entire subject is that a rotated coordinate must never reach a public log. The invariant was stated, applied to one branch, and left off the other. When this guard fires it does the thing it was built to prevent. + +**The `secret` table is exempted by an inline literal.** `scanned_sections` filters `_TABLE_ROLE` for `CONSUMED` and then subtracts `secret` by name, two lines under a comment claiming the scope comes from the declared partition and not from an inline list, and with no reason recorded anywhere. Measured against views-appwrite's current registry: `[secret]` has seven rows and none carries a `value`, so the clause removes zero entries today and the `isinstance(..., str)` filter on the next line already does its work. The day a secret row gains any value, the one table class whose leak matters most is silently outside the scan. + +**The rotation proof checks one side.** `assert "value:" in changed[canary] and "at-the-pin" not in changed[canary]` asserts the *pinned* value is absent. The freshly rotated value — the more damaging one, and the one the fixture already names — is never asserted absent. A regression that digests one side and interpolates the other passes this proof. + +The three share a cause: the no-print rule lives in prose and in one implementation, and nothing asserts it about the guard as a whole. + +Cross-refs: **C-57** (the scan's own entry and its history), **C-93** (why the author's own proof did not find this), **C-90** (the sibling defect in the drift checks), ADR-014 §1. + +--- + +### C-90: The replacement drift checks assert less than their docstrings say, and one of their mutation proofs is a tautology + +| Field | Value | +|-------|-------| +| ID | C-90 | +| Tier | 2 — this is the entry PR #239 was written to close, reopened by the code that closed it. It reinstates release-blocking false alarms on the path that is this project's production release, and it does so under a docstring saying the opposite. | +| Source | `/code-review max` on PR #239 post-merge, 2026-08-11; verified by direct measurement against views-appwrite `origin/main` | +| Trigger | views-appwrite declares any new coordinate in `connection`, `target`, `secret` or `contract` — including one belonging to another repository entirely. On the current registry that is 8 of 25 rows' worth of surface. | +| Owner | This repository. | +| Location | `tests/test_env_declaration.py:732` (`arrived`), `:890` (the tautology), `tests/seam_registry.py:113` (`registry_current`). | + +**`arrived` is not filtered by the names this package reads.** `changed` is; `arrived` is computed over every row of every table this package depends on. Measured on the live registry: **25 rows, 8 of which this package never reads** — six of them keys and callers belonging to other repositories. So views-appwrite issuing one more key for an unrelated repo turns both partner parametrizations red here, with a message demanding a `SEAM_CONTRACT` re-pin for a coordinate this package cannot use. + +That is the exact failure class C-86 records and that PR #239 was written to remove, and the same test's docstring seven lines above says **"Silent through: prose edits, `[meta]` bumps, and rows belonging to anyone else."** The prose describes the check that was designed; the code implements a wider one. + +There is a real question underneath, and it should be decided rather than inherited: a *new* coordinate in a table we read may be one we must adopt — `[contract.*]` arrived exactly that way and nothing else here would have seen it. That argues for table-granularity on arrival and row-granularity on change. If that is the intent it is defensible, and then the docstring is wrong; if the docstring is right, the code is. **They cannot both stand.** + +**The partition's mutation proof cannot fail.** `assert not _unclassified_tables(base)` where `base = {name: {} for name in _TABLE_ROLE}` reduces to `set(_TABLE_ROLE) - set(_TABLE_ROLE)`, empty for every possible input. Its message — *"the real registry's tables must all classify"* — asserts a fact about a file this test never opens. It is decoration inside the test whose own docstring is about removing decoration. + +**`registry_current` has no test.** The module `tests/seam_registry.py` was extracted for one reason: two copies of the reader disagreed about whether to read the sibling's `main` or its working tree, and reading the working tree is issue #196 verbatim. The function that settles it is called by five tests and is the subject of none. Replacing its body with `rev-parse HEAD` — the defect it exists to prevent — leaves the suite at its exact baseline. Three of its error branches are executed by nothing. + +Cross-refs: **C-86** (whose partial-mitigation paragraph this falsifies), **C-89** (the sibling defect in the no-copy scan), **C-93**, **C-91**, ADR-014 §1/§2, issue #196. + +--- + +### C-91: The git plumbing this arc added turns ordinary developer states into hard errors, bare tracebacks, and one possible hang + +| Field | Value | +|-------|-------| +| ID | C-91 | +| Tier | 3 — no wrong data and nothing silent. It taxes every contributor who does not already have the exact sibling checkout this repository assumes, and it does so with diagnoses that point at the wrong cause. | +| Source | `/code-review max` on PR #239 post-merge, 2026-08-11 | +| Trigger | Any of: a contributor clones views-appwrite shallow, single-branch, or before the pinned commit; a table classified `CONSUMED` upstream is written as flat keys rather than sub-tables; a contributor has `commit.gpgsign` or a global `core.hooksPath` set. | +| Owner | This repository. | +| Location | `tests/seam_registry.py:71` (refusal diagnoses), `:150` (`rows`), `tests/test_env_declaration.py:820` (the scratch repo). | + +**A stale clone produces four errors carrying the wrong explanation.** The reader the extraction replaced read the file off disk, so an older checkout simply read an older file. Now a clone that predates the pinned commit — or is shallow, or was made `--single-branch`, which matters because views-appwrite's default branch is not `main` — raises *"does not resolve to a commit … an empty ref reads the index and a branch reads a moving tip"*. That names neither cause and does not say `git fetch`. Meanwhile `test_the_pinned_commit_is_reachable_from_the_contract_repos_main` detects the identical root cause and *skips* with the right remedy. One condition, one skip, four errors, three explanations. + +**`rows()` raises a bare `AttributeError` on a shape the live registry already has.** It guards a null section and not a scalar row. Verified: views-appwrite's `[test_environment]` holds `status` and `fact` as top-level strings. That table is `IGNORED`, so nothing breaks today — but when the partition check fires on a new upstream table, its own message instructs the maintainer to classify it `CONSUMED` or `MIRRORED`, and doing so for a table written that way returns a traceback pointing into a dict comprehension. From the module whose docstring says a helper justified by failing legibly must not hand back a bare traceback. + +**The scratch repo inherits the developer's global git config and has no timeout.** `test_the_pinned_reader_refuses_every_way_a_baseline_can_be_wrong` sets `user.name` and `user.email` and stops. With `commit.gpgsign = true` it fails with a bare `CalledProcessError` — `capture_output=True` swallows git's explanation. With a passphrase-protected key it blocks on pinentry with no `timeout`, hanging the whole run; `conftest.git_output`, which this helper bypasses, caps at 30 seconds. The leak was anticipated for identity and not for the setting that blocks. + +Cross-refs: **C-90** (the same module's untested core), **C-88** (why the module exists outside `conftest.py`), ADR-008 (explicit failure), issue #196. + +--- + +### C-92: This repository lost its only check that the FAO consumer still SELECTS by the delivery label + +| Field | Value | +|-------|-------| +| ID | C-92 | +| Tier | 2 — the failure mode is invisible by construction and is on the live FAO path. Upload succeeds, storage is billed, the consumer's endpoint returns empty, nothing raises anywhere. | +| Source | `/code-review max` on PR #239 post-merge, 2026-08-11 | +| Trigger | views-faoapi changes how `managers/prediction/manager.py` selects — to a category, a metadata field, a query builder — without touching its served-name constant. | +| Owner | Shared: views-faoapi owns the check; this repository owns noticing it does not exist. | +| Location | `tests/test_product.py:63` (`_CONSUMER_FILTER`), `:274` (`_CONSUMER_SELF_CHECK_PENDING`), `:344`; `tests/conftest.py:159-167`; ADR-017 §5. | + +PR #239 retired the FAO half of the source-reading check on the strength of views-faoapi#379. That was the right sequencing — §5 requires the consumer-side check to land first, and it had. **But the two checks are not the same check.** + +views-faoapi#379 binds their *served-name constant* to the registry row. The assertion this repository deleted was `'filters["name"] = self.model_path.model_name'` — that they still *query* on it. So views-faoapi can refactor its selection mechanism, leave its name constant untouched, pass #379, pass our registry comparison, and serve an empty endpoint. The deleted assertion carried that exact sentence: *"the name may still match while the consumer filters on something else entirely — same invisibility, different cause."* Nothing carries it now. + +**Verified 2026-08-12, and the state is good — which is why this is a risk and not an incident.** Read at `views-faoapi@origin/development`: `_CONSUMER_DOCUMENT_NAME = "un_fao"` (`src/views_faoapi/managers/api.py:59`) reaches `APIPathManager(...)` at `:1282`, and `managers/prediction/manager.py` still filters on it at `:117` and `:435`. Their D2 test — views-faoapi's `tests/test_seam_contract_binding.py` — imports the constant from the production module rather than re-typing it, which is better than it had to be. **The composition is what nothing asserts**: constant↔registry is checked by them, query↔constant was checked by us and is not any more. Also worth recording: their D2 check is on `development`; their `main` is 22 commits behind at PR #357, so a reader taking "#379 merged" to mean "live on their default branch" is over-reading it. + +**C-87 is not this.** C-87 records that we verify our copy against the declaration rather than the consumer's code against it. This is narrower and worse: for one partner we briefly had the second check and gave it up for something that does not cover the same failure. + +**A second, structural half.** `_CONSUMER_SELF_CHECK_PENDING` is asserted non-empty and asserted to name only real partners — never asserted to *cover* them. Every other partner map here is two-sided against `PARTNER_PACKAGES`; this one is not, so a third partner is silently exempt from the source read the day it lands. And `SIBLINGS["views-crafdapi"].note` says the fetch "buys nothing and should be removed" once the registry read exists — which PR #239 landed — while ADR-017 §5 forbids retiring that partner's source read until views-crafdapi#53 lands. A maintainer following the note does the thing the ADR forbids, and `test_the_pending_list_is_not_empty` does not object because the map stays non-empty. + +Cross-refs: **C-87** (the broader residual this sharpens, not duplicates), ADR-017 §5/§8/Appendix B, views-faoapi#379, views-crafdapi#53. + +--- + ### C-88: The platform declarations live in pytest's fixture file, so nothing outside the test tree can reach them | Field | Value | @@ -265,6 +445,19 @@ description, where deferrals go to be forgotten: Cross-refs for this amendment: **C-57** (the drift detector this rides on), ADR-016 §7a/§7b, views-appwrite#76 (**delivered**, registry v1.6.0). +**⚠ CORRECTED 2026-08-12 — the partial mitigation above overstates what shipped.** The +paragraph beginning *"Partial mitigation 2026-08-11"* says the replacement checks "match on +the facts this repository actually declares". **One of the three does not.** The arrival +half of the drift check is computed over every row of every table this package depends on, +with no filter for the names it reads — 8 of 25 rows on the live registry belong to other +repositories. So the false-alarm class this entry records is *narrowed*, not removed: +observation-only edits no longer fire it, but any new coordinate for any repo still does. + +The measurement quoted above (v1.4.4 → v1.5.2, three editions, green) remains true and was +not the wrong measurement — none of those editions added a row. It was simply not a +measurement of the case that now fires. Registered as **C-90**, and this entry's rate claim +should be re-stated once that is fixed rather than before. + --- @@ -289,6 +482,8 @@ Its residual is stated plainly in §8 and belongs here rather than only in a doc Cross-refs: **C-77** (the same field's producer-side half, resolved), ADR-013 §4.1a, ADR-017 §5/§8/Appendix B, views-appwrite#75, views-faoapi#379, views-crafdapi#53. +**⚠ AMENDED 2026-08-12 — one partner's residual got worse, not better.** This entry says the label's agreement with reality "rests on the source-reading check … which someone could remove believing the registry check replaced it." PR #239 removed the FAO half on the strength of views-faoapi#379 — correct sequencing under ADR-017 §5, and still the wrong outcome, because **#379 and the deleted check do not cover the same failure.** #379 binds their served-name constant to the registry; the deleted assertion was that they still *query* on it. Registered separately as **C-92**, because it is narrower and more acute than the residual recorded here: not "we never checked the consumer's code" but "we checked it, for one partner, and stopped." + --- ### C-85: A cross-repo ask is adopted on its stated terms without anyone measuring the current state @@ -1519,6 +1714,17 @@ The shared cause is not a regex bug. **The pattern described one way of writing **Why this belongs on C-57 rather than in a new entry.** It is the same guard, the same failure direction, and the same lesson this entry already records one layer down: the earlier amendment found the scan's *scope* was never mutation-proven (it named `unfao` and missed `crafd`); this one finds its *matching* was never proven against the file formats it scans. Scope, matching, and now syntax-variant — three ways for a mutation-proven guard to be proven against the wrong thing. +**⚠ CORRECTED 2026-08-12, and the correction is the same mistake one level up.** The amendment immediately above claims "thirteen forms are proven caught" and cites `` set `NAME=value` before … `` as evidence the mid-sentence class is covered. An independent review supplied twenty-nine forms and **fifteen missed**, including the *unbackticked* form of that very example: `set NAME=value before running` captures `'value before running'`. The cited case passes only because of its backticks. Measured, not argued. + +Two distinct defects sit under those fifteen, and both are the pattern-describes-one-dialect cause this entry already names: + +1. **The unquoted value has no whitespace terminator.** It ends only at `#`, a backtick or `|`, so any value followed by prose escapes. And `search` is called once per line, so on a two-variable table row every assignment after the first is invisible. +2. **The boundary is an allow-list, not a predicate.** The comment above it says the name may be preceded by "anything that is not part of an identifier"; the code is a six-character lookbehind. A quoted, parenthesised or colon-prefixed assignment produces no match at all — so the pattern can *parse* a quoted value but cannot *see* a quoted assignment. + +**And a fourth failure direction, opposite to the other three.** Removing the six-character floor admitted the registry's two shortest values into the ban-set. Measured: **they are this repository's own package directory names — the exact contents of `PARTNER_PACKAGES`** — matched by exact string equality against every non-docstring string constant under `views_postprocessing/`. The justification given, "measured, removing it keeps the suite green", measured today's source and not the next commit's. The next `logging.getLogger()` or `Literal[, …]` fails a security guard for code that copied nothing, and this entry's own doctrine says what happens next. It also hands views-appwrite a way to redden this repository by declaring any short ordinary word as a value — which is **C-86**'s coupling, arriving through the guard that was supposed to reduce it. + +Registered as **C-89** (the leak half) and tracked with this entry's remediation. **C-93** records why the author's own proof did not find any of it. + 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), **C-86** (the drift checks this scan sits beside), issues #134/#135/#138 (this repo's discharged þing-01 obligations), #104 (README env block placeholders). --- From 6b175a7e4277b442e16ccfc3c4ad3e9294a31167 Mon Sep 17 00:00:00 2001 From: Polichinl Date: Wed, 12 Aug 2026 12:42:07 +0200 Subject: [PATCH 2/5] =?UTF-8?q?fix(tests):=20#242=20=E2=80=94=20the=20no-c?= =?UTF-8?q?opy=20guard=20must=20not=20publish=20what=20it=20forbids?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit C-89's first two thirds. The guard that exists to stop a coordinate value being published was publishing one, into a world-readable CI log, on precisely the event it exists to catch. THE VALUE IS NO LONGER A PARAMETER. Both branches build their finding through `_report_a_copy(where, coordinates)`, which is never handed the value — so a future call site cannot print one however it is written. That is the difference between a rule and a design: the markdown branch was taught not to print on 2026-08-11, the AST branch was not, and the two drifted for a day because each read fine on its own. It names ALL coordinates sharing the value, not one. Measured against the live registry: two pairs share a value (the prod-forecasts bucket and collection share both their id and their name), so a `value -> name` dict would keep the last and name the wrong coordinate half the time — in the one message a maintainer uses to find the copy. THE ROTATION PROOF CHECKED ONE SIDE OF TWO. It asserted the pinned value was absent and said nothing about the freshly rotated one, which is the more damaging half. Mutation- proven: leaking the post-rotation side while keeping the pinned side digested fails now and passed before. AND THE RULE NOW HAS A CHECK, which is the part that matters. `test_every_finding_goes_ through_the_one_reporter` asserts by AST that every `copied.append` in the scan calls the one reporter. Nothing compared the two branches, which is why prose saying "never print a value" coexisted with a branch printing one. Read as source rather than run, because the leaking branch only executes when a coordinate has actually been copied — a state this repository must never be in, so a behavioural test of it would have nothing to observe. Three mutations, each reverted: (1) restore the original AST f-string -> the new guard fails, naming the line; (2) leak the post-rotation side -> the rotation proof fails; (3) inject a real value declared by TWO coordinates into a package module -> caught, both coordinates named, no value in the message. Deliberately NOT in this change: the `secret` exemption and the ban-set's package-name collision. Those are the scan's SCOPE, not its reporting, and belong with the matcher rewrite in #243. C-89 stays open rather than being closed on two thirds of its content. Suite 411 passed / 1 skipped / 40 xfailed, ruff clean. The one pre-existing failure (`test_the_consumer_still_filters_on_the_name_until_it_checks_itself[crafd]`) is #248's, red since views-crafdapi refactored a literal into a named constant on 2026-08-12. Closes #242. Epic #241. Co-Authored-By: Claude Opus 5 (1M context) --- reports/technical_risk_register.md | 12 ++- tests/test_env_declaration.py | 125 +++++++++++++++++++++++++---- 2 files changed, 119 insertions(+), 18 deletions(-) diff --git a/reports/technical_risk_register.md b/reports/technical_risk_register.md index 6c16d4c..dc1dc67 100644 --- a/reports/technical_risk_register.md +++ b/reports/technical_risk_register.md @@ -266,7 +266,17 @@ The no-copy scan exists because README.md once carried four real coordinate valu The three share a cause: the no-print rule lives in prose and in one implementation, and nothing asserts it about the guard as a whole. -Cross-refs: **C-57** (the scan's own entry and its history), **C-93** (why the author's own proof did not find this), **C-90** (the sibling defect in the drift checks), ADR-014 §1. +**Mitigated 2026-08-12 (#242) — the leak is closed and the rule now has a check.** + +Two of the three are fixed, and the third moved: + +1. **The AST branch no longer receives the value.** Both branches build their finding through `_report_a_copy(where, coordinates)`, whose signature *has no value parameter* — so a future call site cannot print one however it is written. The report names the coordinate instead, and names **all** coordinates sharing that value: measured against the live registry, two pairs share one (the prod-forecasts bucket and collection share both id and name), so a `value -> name` map would have named the wrong coordinate half the time in the message a maintainer uses to find the copy. +2. **The rotation proof asserts both sides absent.** Mutation-proven: leaking the post-rotation side while keeping the pinned side digested fails now and **passed before** — which is exactly the half the old assertion missed, and the more damaging half. +3. **The rule is asserted about the scan as a whole**, by `test_every_finding_goes_through_the_one_reporter` — an AST check that every `copied.append` calls the one reporter. This is the guard whose absence let the two branches drift for a day; mutation-proven by restoring the original defect and watching it fail. + +**Deliberately still open, and moved rather than closed:** the `secret` exemption at `:1042` and the ban-set's package-name collision are the *scope* of the scan, not its reporting, and belong with the matcher rewrite in **#243**. This entry stays open until they land, because closing it now would close a Tier 2 on two-thirds of its content. + +Cross-refs: **C-57** (the scan's own entry and its history), **C-93** (why the author's own proof did not find this), **C-90** (the sibling defect in the drift checks), ADR-014 §1, issues #242, #243. --- diff --git a/tests/test_env_declaration.py b/tests/test_env_declaration.py index 0db95f8..ccfa01c 100644 --- a/tests/test_env_declaration.py +++ b/tests/test_env_declaration.py @@ -934,10 +934,21 @@ def edition(value: str) -> dict: f"[{partner}] a rotated value went unnoticed. Name and class are identical on " "both sides, so nothing else in this file can see it." ) - assert "value:" in changed[canary] and "at-the-pin" not in changed[canary], ( - "the report must name the field that moved and must NOT print the value — this " - "repository is public and its CI logs are world-readable" + assert "value:" in changed[canary], ( + f"[{partner}] the report must name the FIELD that moved, or a maintainer reading " + "the failure cannot tell a rotation from a reclassification." ) + # BOTH sides, and the second one is the one that matters. An earlier version asserted + # only that the pinned value was absent, so a regression that digested one side and + # interpolated the other passed — while printing the freshly rotated coordinate, which + # is the more damaging of the two, into a public CI log on the single event this check + # exists to fire on (register C-89). + for side in ("at-the-pin", "after-rotation"): + assert side not in changed[canary], ( + f"[{partner}] the report printed the {side!r} value. It must name the field " + "and never the value: this repository is public, its CI logs are " + "world-readable, and they cannot be redacted afterwards." + ) @pytest.mark.parametrize("partner", _PARTNERS) @@ -998,6 +1009,77 @@ def _docstring_nodes(tree: ast.AST) -> set[int]: return out +def _report_a_copy(where: str, coordinates: tuple[str, ...]) -> str: + """One finding from the no-copy scan: WHERE the copy is and WHICH coordinate it is. + + **The value itself is not a parameter.** That is the whole design: a helper that is + never handed the value cannot print it, whatever a future call site does. + + **Why this is a function and not two f-strings — it was two f-strings.** The markdown + branch was taught not to print the value on 2026-08-11; the AST branch was not, and + went on interpolating it for a further day. So the guard against publishing a + coordinate published one itself, into a world-readable CI log, on precisely the event + it exists to catch (register **C-89**). A rule applied at two call sites is a rule + only until someone writes a third, which is why + ``test_every_finding_goes_through_the_one_reporter`` exists below. + + ``coordinates`` is a tuple because two coordinates may legitimately declare the same + value — measured against the live registry, two pairs do — and naming one of them + would be wrong half the time in the message a maintainer uses to find the copy. + """ + return f"{where} carries the value declared for {', '.join(coordinates)}" + + +def test_every_finding_goes_through_the_one_reporter(): + """The no-print rule, asserted about the scan as a whole rather than branch by branch. + + This is the guard whose absence let **C-89** happen. The rule "never print a + coordinate value" was stated in prose, applied to one of two branches, and nothing + compared the branches — so they drifted for a day and the drift was invisible because + both branches were individually plausible. + + So: every finding the scan appends must be built by :func:`_report_a_copy`, which + cannot be handed a value. Adding a third branch that formats its own message fails + here, whether or not that message happens to be safe today. + + Read as source rather than run, because the branch that leaked only executes when a + coordinate has actually been copied — a state this repository is never in, and must + never be in, so a behavioural test of it would have nothing to observe. + """ + scan = next( + node for node in ast.walk(ast.parse(Path(__file__).read_text())) + if isinstance(node, ast.FunctionDef) + and node.name == "test_no_coordinate_value_is_copied_into_this_repo" + ) + + homemade = [] + for node in ast.walk(scan): + if not ( + isinstance(node, ast.Call) + and isinstance(node.func, ast.Attribute) + and node.func.attr == "append" + and isinstance(node.func.value, ast.Name) + and node.func.value.id == "copied" + ): + continue + built_by_the_reporter = ( + len(node.args) == 1 + and isinstance(node.args[0], ast.Call) + and isinstance(node.args[0].func, ast.Name) + and node.args[0].func.id == "_report_a_copy" + ) + if not built_by_the_reporter: + homemade.append(node.lineno) + + assert not homemade, ( + f"the no-copy scan builds a finding without _report_a_copy at line(s) {homemade}. " + "Every finding must go through that helper: it is not given the value, so it " + "cannot print one. This repository is public and its CI logs are world-readable " + "and are not retroactively redactable — a branch that formats its own message is " + "how C-89 happened, and it stayed invisible because each branch read fine alone." + ) + + def test_no_coordinate_value_is_copied_into_this_repo(): """The registry's own rule: *"never bake a value into code, an example, or a dataclass default."* Consumers READ and VALIDATE; the launcher supplies values. @@ -1041,15 +1123,20 @@ def test_no_coordinate_value_is_copied_into_this_repo(): scanned_sections = tuple( name for name, role in _TABLE_ROLE.items() if role == "CONSUMED" and name != "secret" ) - values = { - body["value"] - for section in scanned_sections - for body in registry.get(section, {}).values() - # No length floor: it excluded two five-character coordinates and, - # measured, removing it keeps the suite green — so it was buying nothing - # while narrowing a security check. - if isinstance(body.get("value"), str) and body["value"].strip() - } + # value -> every coordinate declaring it. A LIST, not a single name: two coordinates + # may legitimately share a value, and measured against the live registry two pairs do + # (the prod-forecasts bucket and collection share both their id and their name). A + # `value -> name` dict would silently keep the last and name the wrong coordinate half + # the time, in the message a maintainer uses to find the copy. + declared_by_value: dict[str, list[str]] = {} + for section in scanned_sections: + for name, body in registry.get(section, {}).items(): + # No length floor: it excluded two five-character coordinates and, + # measured, removing it keeps the suite green — so it was buying nothing + # while narrowing a security check. + if isinstance(body.get("value"), str) and body["value"].strip(): + declared_by_value.setdefault(body["value"], []).append(name) + values = set(declared_by_value) copied = [] for source in sorted(_PKG.rglob("*.py")): @@ -1062,7 +1149,10 @@ def test_no_coordinate_value_is_copied_into_this_repo(): and node.value in values and id(node) not in docstrings ): - copied.append(f"{source.relative_to(_PKG)}:{node.lineno} = {node.value!r}") + copied.append(_report_a_copy( + f"{source.relative_to(_PKG)}:{node.lineno}", + tuple(sorted(declared_by_value[node.value])), + )) # Markdown too — the AST half cannot see a fenced ``bash`` block, and that is exactly # where four production-forecasts values sat: in README.md's Configuration section, # two lines below the sentence promising they are never copied, in a PUBLIC @@ -1132,10 +1222,11 @@ def test_no_coordinate_value_is_copied_into_this_repo(): match = assignment.search(line) captured = next((g for g in match.groups()[1:] if g is not None), None) if match else None if captured is not None and captured.strip() in values: - copied.append( - f"{doc.relative_to(_REPO)}:{number} assigns a registry value to " - f"{match.group(1)}" - ) + # The assignment names its own coordinate, so report THAT one rather than + # everything sharing the value — it is the more useful of the two answers. + copied.append(_report_a_copy( + f"{doc.relative_to(_REPO)}:{number}", (match.group(1),) + )) assert not copied, ( f"coordinate value(s) from the registry are copied into this repo: {copied}. The " From 4917610a8f0507eb463b2d719d057457f2e7d290 Mon Sep 17 00:00:00 2001 From: Polichinl Date: Wed, 12 Aug 2026 12:57:38 +0200 Subject: [PATCH 3/5] =?UTF-8?q?fix(tests):=20#242=20iteration=202=20?= =?UTF-8?q?=E2=80=94=20routing=20is=20not=20safety,=20so=20read=20the=20me?= =?UTF-8?q?ssage=20instead?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five independent reviewers were run against the first attempt. They agreed, and they were right: the guard I wrote was worse than nothing. IT MISSED SIX OF EIGHT MUTATIONS AND FIRED ON A LEGITIMATE REFACTOR. It asserted that every finding was ROUTED through `_report_a_copy` — an AST walk over the scan's own source. But routing is not safety. The value could be smuggled through either of that helper's two parameters, appended with `extend` or `+=`, or reported from a renamed accumulator, and in every case the guard stayed green while a coordinate reached the log. Meanwhile `finding = _report_a_copy(...); copied.append(finding)` — an ordinary refactor that leaks nothing — failed it. A guard that misses the thing and cries wolf on the innocent is C-82's shape and ADR-014 §3's deletion criterion at once, and my docstring's claim that a caller "cannot print one however it is written" was false on the day I wrote it. SO READ THE FINISHED MESSAGE, NOT THE SYNTAX THAT PRODUCED IT. `test_the_scan_reports_ a_copy_without_reprinting_it` plants a value in a fixture tree, runs the real scan through both halves, and asserts the message names the coordinate and never the value. It does not matter how a finding is built or what the final assertion interpolates. All five surviving mutations are now caught — including one that leaked the entire ban-set through the assertion message rather than through any finding. That is also the answer to C-93: not a longer list of mutants I thought of, but a test whose shape makes the class unreachable. AND THE SCAN'S OWN DOCSTRING CARRIED THREE REGISTRY VALUES. pytest prints the failing function's source, so the guard would have published them on precisely the event it exists to catch — message clean, traceback not. Now it names coordinates. The wider measurement is registered as C-97: 33 standalone values in docstrings and comments across nine files, production modules included, excluded from the scan by a deliberate C-57 decision taken before anyone counted them. Three of them cannot be told from ordinary code at all — two are this repo's package names, one is a function name. Also fixed, both found by review and both real: - the markdown half named the coordinate the ASSIGNMENT names, not the one that DECLARES the value. Those differ exactly when a document pastes one coordinate's value beside another's name — the ordinary copy-paste slip — so it asserted a false fact and hid the coordinate actually leaked. - the rotation loop hardcoded the fixture's two literals a dozen lines from where the fixture builds them. Renaming the fixture strings turned the assertion into "two dead strings are absent". Now taken from the fixture's own variables. Register: C-89 amended with what was tried and what failed; C-97 opened; header 96/23 -> 97/24. The earlier "Mitigated" heading corrected to "Partial mitigation", which is one of the two phrasings this register declares — inventing a third passed the integrity guard by evading its string rather than complying, the identical escape ADR-014 §5 records C-15 making. C-89's Location line numbers repointed; the fix had moved all three and the amendment cited the old ones. The file is +112 lines against `development`, which is the wrong direction for this epic. This story is inherently additive — it adds a guarantee's check where there was none. #245 and #248 are deletions and are where the budget comes back. Suite 411 (410 passed / 1 skipped / 40 xfailed), ruff clean. The one failure is #248's, red since views-crafdapi refactored on 2026-08-12. Closes #242. Epic #241. Co-Authored-By: Claude Opus 5 (1M context) --- reports/technical_risk_register.md | 47 ++++++- tests/test_env_declaration.py | 197 ++++++++++++++++------------- 2 files changed, 149 insertions(+), 95 deletions(-) diff --git a/reports/technical_risk_register.md b/reports/technical_risk_register.md index dc1dc67..e387ff3 100644 --- a/reports/technical_risk_register.md +++ b/reports/technical_risk_register.md @@ -5,8 +5,8 @@ | Project | views-postprocessing | | Owner | Dylan Pinheiro / PRIO MD&D Team | | Last Updated | 2026-08-12 | -| Total Concerns | 96 | -| Open Concerns | 23 | +| Total Concerns | 97 | +| Open Concerns | 24 | | Resolved Concerns | 73 | --- @@ -161,6 +161,31 @@ that indexes only deleted code is noise. ## Open Concerns +### C-97: Thirty-three coordinate values sit in docstrings and comments, where the scan deliberately does not look + +| Field | Value | +|-------|-------| +| ID | C-97 | +| Tier | 3 — a decided position, not an accident, and the decision is defensible. It is registered because the decision was taken before anyone counted, and because a docstring is printed by pytest in a way a code constant is not. | +| Source | `/code-review max` on #242, 2026-08-12; count reproduced here | +| Trigger | **Either.** (a) A check whose failing function contains one of these values starts firing in CI. (b) Someone proposes widening the no-copy scan to docstrings — at which point this entry is the measurement that says what that would cost. | +| Owner | This repository. | +| Location | Measured across `git ls-files '*.py'`: 33 standalone occurrences in 9 files, including `views_postprocessing/{unfao,crafd}/managers/`, `contract/store_metadata.py`, `contract/wire/sink.py`. | + +The no-copy scan compares string **constants** and excludes docstrings outright. That exclusion is C-57's recorded lesson: an early draft fired on refusal labels and on docstrings naming which store a function serves, and *"a guard that fails on `def file_metadata(record)` gets deleted — after which the real rule is unguarded"* (ADR-014 §3). + +The position is still right. What was not known when it was taken is the count, and one consequence: + +**A value in a docstring is printed by pytest that a value in a constant is not.** When any test fails, `--tb=auto` prints the failing function's source. So a registry value in the docstring of a *test* reaches a world-readable CI log on that test's next failure — which is how #242 found that the no-copy scan's own docstring carried three. That one is fixed. The others sit in functions that fail less predictably. + +**Three of the values cannot be distinguished from ordinary code at all.** Measured: two are this repository's own package directory names, and one is the name of a function in `contract/store_metadata.py`. Exact-string matching cannot separate a copy from a coincidence for these, which is the same measurement #243 needs for the ban-set — recorded here once rather than twice. + +**What is deliberately not proposed:** scanning docstrings. It would fire on every sentence naming a store, which is the false-alarm class that gets guards deleted. The honest options are to leave it (current), to scan only *test* docstrings (where the traceback amplification is), or to stop writing values in prose going forward without rewriting history. None is urgent. + +Cross-refs: **C-57** (the exclusion and why), **C-89** (the traceback amplification, found here), **C-86**, issues #242, #243. + +--- + ### C-96: The registry table that says which live checks this package may build is classified as none of our business | Field | Value | @@ -254,7 +279,7 @@ Cross-refs: **C-57** and **C-89** (the guard this was measured on), **C-90** (a | Source | `/code-review max` on PR #239 post-merge, 2026-08-11; every element verified in this repository | | Trigger | A literal coordinate value is committed into a `.py` under `views_postprocessing/` — the violation the scan exists for — on any branch whose CI runs. | | Owner | This repository. | -| Location | `tests/test_env_declaration.py:1065` (the AST branch), `:1042` (the `secret` exemption), `:937` (the half-sided rotation proof). | +| Location | `tests/test_env_declaration.py:1152` (the AST branch), `:1124` (the `secret` exemption), `:946` (the rotation proof). Line numbers as of 2026-08-12 — the 2026-08-11 filing cited `:1065`/`:1042`/`:937`, which the fix itself moved. | The no-copy scan exists because README.md once carried four real coordinate values, two lines below the sentence promising they are never copied, in a public repository. It has three parts and they do not agree with each other about the one rule that matters. @@ -266,13 +291,21 @@ The no-copy scan exists because README.md once carried four real coordinate valu The three share a cause: the no-print rule lives in prose and in one implementation, and nothing asserts it about the guard as a whole. -**Mitigated 2026-08-12 (#242) — the leak is closed and the rule now has a check.** +**Partial mitigation 2026-08-12 (#242) — the leak is closed; the scan's scope is not.** + +*(The heading matters. An earlier draft of this paragraph said "Mitigated", which is neither of the two phrasings this register declares — `Mitigation — landed` and `Partial mitigation`, enforced by `tests/test_register_integrity.py`. Inventing a third phrasing passes that guard by evading its string rather than by complying, which is the identical escape ADR-014 §5 records C-15 making. This entry is partial by its own next sentence.)* Two of the three are fixed, and the third moved: -1. **The AST branch no longer receives the value.** Both branches build their finding through `_report_a_copy(where, coordinates)`, whose signature *has no value parameter* — so a future call site cannot print one however it is written. The report names the coordinate instead, and names **all** coordinates sharing that value: measured against the live registry, two pairs share one (the prod-forecasts bucket and collection share both id and name), so a `value -> name` map would have named the wrong coordinate half the time in the message a maintainer uses to find the copy. -2. **The rotation proof asserts both sides absent.** Mutation-proven: leaking the post-rotation side while keeping the pinned side digested fails now and **passed before** — which is exactly the half the old assertion missed, and the more damaging half. -3. **The rule is asserted about the scan as a whole**, by `test_every_finding_goes_through_the_one_reporter` — an AST check that every `copied.append` calls the one reporter. This is the guard whose absence let the two branches drift for a day; mutation-proven by restoring the original defect and watching it fail. +1. **The AST branch no longer receives the value.** Both branches build their finding through `_report_a_copy(where, coordinates)`, which takes no value. The report names the coordinate instead, and names **all** coordinates sharing that value: measured, two pairs share one (the prod-forecasts bucket and collection share both id and name), so a `value -> name` map would have named the wrong coordinate half the time in the message a maintainer uses to find the copy. +2. **The rotation proof asserts both sides absent**, taken from the fixture's own variables rather than from two literals a rename would quietly orphan. Mutation-proven: leaking the post-rotation side while keeping the pinned side digested fails now and **passed before** — the more damaging half, unchecked. +3. **The rule is asserted behaviourally**, by `test_the_scan_reports_a_copy_without_reprinting_it`: plant a value in a fixture tree, run the real scan, read the finished message. + +**And the third one took two attempts, which is the part worth recording.** The first version asserted that every finding was *routed* through `_report_a_copy` — an AST walk over the scan's own source. Five independent reviewers were run against it and **routing turned out not to be safety**: the value could be smuggled through either of that helper's two parameters, appended with `extend` or `+=`, or reported from a renamed accumulator. Six of eight mutations survived, and one legitimate refactor *failed* it — a guard that misses the thing and fires on the innocent, which is C-82's shape and ADR-014 §3's deletion criterion at once. The docstring's claim that a caller "cannot print one however it is written" was false when written. + +Reading the finished message instead makes the whole class unreachable: it does not matter how a finding is built, which branch builds it, or what the final assertion interpolates. All five surviving mutations are now caught, including one that leaked the entire ban-set through the assertion message rather than through a finding. + +4. **The scan's own docstring carried three registry values**, and pytest prints the failing function's source — so the guard would have published them on exactly the event it exists to catch. The message was clean; the traceback was not. Now it names coordinates. **The wider finding is registered separately as C-97**: 33 standalone values sit in docstrings and comments across nine files, production modules included, and the AST scan excludes docstrings by a deliberate C-57 decision taken before anyone counted them. **Deliberately still open, and moved rather than closed:** the `secret` exemption at `:1042` and the ban-set's package-name collision are the *scope* of the scan, not its reporting, and belong with the matcher rewrite in **#243**. This entry stays open until they land, because closing it now would close a Tier 2 on two-thirds of its content. diff --git a/tests/test_env_declaration.py b/tests/test_env_declaration.py index ccfa01c..011b3cf 100644 --- a/tests/test_env_declaration.py +++ b/tests/test_env_declaration.py @@ -28,6 +28,7 @@ import logging import re import subprocess +import sys from pathlib import Path import pytest @@ -923,8 +924,9 @@ def edition(value: str) -> dict: "target": {canary: {"class": "target", "value": value}}, } - then = _projection(edition("at-the-pin"), names) - now = _projection(edition("after-rotation"), names) + was, rotated = "at-the-pin", "after-rotation" + then = _projection(edition(was), names) + now = _projection(edition(rotated), names) assert then[canary][:2] == now[canary][:2], ( "this fixture must differ ONLY in the value, or it is not proving what it claims" @@ -938,12 +940,10 @@ def edition(value: str) -> dict: f"[{partner}] the report must name the FIELD that moved, or a maintainer reading " "the failure cannot tell a rotation from a reclassification." ) - # BOTH sides, and the second one is the one that matters. An earlier version asserted - # only that the pinned value was absent, so a regression that digested one side and - # interpolated the other passed — while printing the freshly rotated coordinate, which - # is the more damaging of the two, into a public CI log on the single event this check - # exists to fire on (register C-89). - for side in ("at-the-pin", "after-rotation"): + # Both sides, taken from the fixture's own variables. Asserting the two literals + # instead let a fixture rename turn this into "two dead strings are absent" — and the + # rotated side is the one that matters, which an earlier version never checked. + for side in (was, rotated): assert side not in changed[canary], ( f"[{partner}] the report printed the {side!r} value. It must name the field " "and never the value: this repository is public, its CI logs are " @@ -1012,16 +1012,13 @@ def _docstring_nodes(tree: ast.AST) -> set[int]: def _report_a_copy(where: str, coordinates: tuple[str, ...]) -> str: """One finding from the no-copy scan: WHERE the copy is and WHICH coordinate it is. - **The value itself is not a parameter.** That is the whole design: a helper that is - never handed the value cannot print it, whatever a future call site does. - - **Why this is a function and not two f-strings — it was two f-strings.** The markdown - branch was taught not to print the value on 2026-08-11; the AST branch was not, and - went on interpolating it for a further day. So the guard against publishing a - coordinate published one itself, into a world-readable CI log, on precisely the event - it exists to catch (register **C-89**). A rule applied at two call sites is a rule - only until someone writes a third, which is why - ``test_every_finding_goes_through_the_one_reporter`` exists below. + The value is not a parameter, so the ordinary way of writing a finding cannot print + one. That is a convention with a shape, **not a guarantee** — an earlier version of + this docstring claimed a caller "cannot print one however it is written", and an + independent review falsified it in two lines by smuggling the value through ``where`` + and again through ``coordinates``. What actually holds the rule is + :func:`test_the_scan_reports_a_copy_without_reprinting_it`, which reads the finished + message rather than the syntax that produced it (register **C-89**, **C-93**). ``coordinates`` is a tuple because two coordinates may legitimately declare the same value — measured against the live registry, two pairs do — and naming one of them @@ -1030,78 +1027,37 @@ def _report_a_copy(where: str, coordinates: tuple[str, ...]) -> str: return f"{where} carries the value declared for {', '.join(coordinates)}" -def test_every_finding_goes_through_the_one_reporter(): - """The no-print rule, asserted about the scan as a whole rather than branch by branch. - - This is the guard whose absence let **C-89** happen. The rule "never print a - coordinate value" was stated in prose, applied to one of two branches, and nothing - compared the branches — so they drifted for a day and the drift was invisible because - both branches were individually plausible. - - So: every finding the scan appends must be built by :func:`_report_a_copy`, which - cannot be handed a value. Adding a third branch that formats its own message fails - here, whether or not that message happens to be safe today. - - Read as source rather than run, because the branch that leaked only executes when a - coordinate has actually been copied — a state this repository is never in, and must - never be in, so a behavioural test of it would have nothing to observe. - """ - scan = next( - node for node in ast.walk(ast.parse(Path(__file__).read_text())) - if isinstance(node, ast.FunctionDef) - and node.name == "test_no_coordinate_value_is_copied_into_this_repo" - ) - - homemade = [] - for node in ast.walk(scan): - if not ( - isinstance(node, ast.Call) - and isinstance(node.func, ast.Attribute) - and node.func.attr == "append" - and isinstance(node.func.value, ast.Name) - and node.func.value.id == "copied" - ): - continue - built_by_the_reporter = ( - len(node.args) == 1 - and isinstance(node.args[0], ast.Call) - and isinstance(node.args[0].func, ast.Name) - and node.args[0].func.id == "_report_a_copy" - ) - if not built_by_the_reporter: - homemade.append(node.lineno) - - assert not homemade, ( - f"the no-copy scan builds a finding without _report_a_copy at line(s) {homemade}. " - "Every finding must go through that helper: it is not given the value, so it " - "cannot print one. This repository is public and its CI logs are world-readable " - "and are not retroactively redactable — a branch that formats its own message is " - "how C-89 happened, and it stayed invisible because each branch read fine alone." - ) - def test_no_coordinate_value_is_copied_into_this_repo(): """The registry's own rule: *"never bake a value into code, an example, or a dataclass default."* Consumers READ and VALIDATE; the launcher supplies values. + **This docstring names coordinates and never values, and that is not fussiness.** + An earlier version quoted three real registry values as worked examples. When this + check fails, pytest prints the failing function's own source — so the guard against + publishing a coordinate would have published three, into a world-readable CI log, on + exactly the event it exists to catch. The message was clean and the traceback was not + (register **C-89**). + **Two wrong narrowings before this one, both instructive.** - A substring text scan over every ``.py`` flagged three "leaks": ``file_metadata`` - (a **function name** in ``contract/store_metadata.py``), ``production_forecasts`` - and ``unfao_bucket`` (only in refusal labels and docstrings naming which store a - function serves). None was a copy, and a guard that fails on - ``def file_metadata(record)`` gets deleted — after which the real rule is unguarded. + A substring text scan over every ``.py`` flagged three "leaks": the function name + ``file_metadata`` in ``contract/store_metadata.py``, and the values of + ``APPWRITE_PROD_FORECASTS_BUCKET_ID`` and ``APPWRITE_UNFAO_BUCKET_ID`` where they + appear only in refusal labels and in docstrings naming which store a function serves. + None was a copy, and a guard that fails on ``def file_metadata(record)`` gets deleted + — after which the real rule is unguarded. - Narrowing to *assignments and default arguments* then went too far in the other - direction: it caught neither a dict value nor a keyword argument, and the keyword - argument is the shape this repo would actually produce — - ``AppwriteConfig(bucket_id=...)`` is how every store is configured, and swapping one - ``os.getenv`` for a literal there is the violation. + Narrowing to *assignments and default arguments* then went too far the other way: it + caught neither a dict value nor a keyword argument, and the keyword argument is the + shape this repo would actually produce — ``AppwriteConfig(bucket_id=...)`` is how + every store is configured, and swapping one ``os.getenv`` for a literal there is the + violation. The right axis was **exact equality on string constants**, not statement shape. It catches dict values and kwargs, while all three original false positives fall out on - their own: a function name is not a ``Constant``; ``"unfao_bucket datastore"`` is not - equal to ``"unfao_bucket"``; docstrings are excluded outright. + their own: a function name is not a ``Constant``; a value with a word appended is not + equal to the value; docstrings are excluded outright. """ # `require_sibling`, like every other reader in this file. The hand-rolled skip that # stood here said only "checkout not found — set VIEWS_APPWRITE", which is the @@ -1123,11 +1079,8 @@ def test_no_coordinate_value_is_copied_into_this_repo(): scanned_sections = tuple( name for name, role in _TABLE_ROLE.items() if role == "CONSUMED" and name != "secret" ) - # value -> every coordinate declaring it. A LIST, not a single name: two coordinates - # may legitimately share a value, and measured against the live registry two pairs do - # (the prod-forecasts bucket and collection share both their id and their name). A - # `value -> name` dict would silently keep the last and name the wrong coordinate half - # the time, in the message a maintainer uses to find the copy. + # value -> every coordinate declaring it. A list, because two coordinates may share + # a value and measured against the live registry two pairs do. declared_by_value: dict[str, list[str]] = {} for section in scanned_sections: for name, body in registry.get(section, {}).items(): @@ -1161,7 +1114,8 @@ def test_no_coordinate_value_is_copied_into_this_repo(): # # **What counts as a copy, and what does not.** A first draft flagged any line # containing a registry value and immediately fired on a dozen documents that merely - # *name* a store in prose — "six stranded documents in unfao_bucket". That is not a + # *name* a store in prose — a sentence counting stranded documents in the UNFAO + # bucket, written with the bucket's declared value. That is not a # copy; it is a sentence. C-57 recorded the identical false-positive class over `.py` # and the identical lesson: when a guard cries wolf, the matching is wrong before the # scope is (ADR-014 §3). @@ -1222,10 +1176,14 @@ def test_no_coordinate_value_is_copied_into_this_repo(): match = assignment.search(line) captured = next((g for g in match.groups()[1:] if g is not None), None) if match else None if captured is not None and captured.strip() in values: - # The assignment names its own coordinate, so report THAT one rather than - # everything sharing the value — it is the more useful of the two answers. + # Report the coordinate(s) that DECLARE the value, not the one the + # assignment happens to name. Those differ exactly when a document + # pastes one coordinate's value beside another's name — the ordinary + # copy-paste slip — and naming the assignment would then assert a false + # fact and hide the coordinate actually leaked. copied.append(_report_a_copy( - f"{doc.relative_to(_REPO)}:{number}", (match.group(1),) + f"{doc.relative_to(_REPO)}:{number}", + tuple(sorted(declared_by_value[captured.strip()])), )) assert not copied, ( @@ -1234,3 +1192,66 @@ def test_no_coordinate_value_is_copied_into_this_repo(): "environment the launcher assembles, validated by assert_env_declared. In a " "document, write the NAME and leave the value to the launcher." ) + + +def test_the_scan_reports_a_copy_without_reprinting_it(tmp_path, monkeypatch): + """Feed the real scan a planted copy and read what it says. **The rule, behaviourally.** + + This replaces a source-reading guard that asserted every finding was *routed* through + ``_report_a_copy``. Routing is not safety, and an independent review proved it: the + value could be smuggled through either of that helper's two parameters, or appended + with ``extend``/``+=``, or reported from a renamed accumulator — **six of eight + mutations survived**, while one legitimate refactor failed it. A guard that misses the + thing and fires on the innocent is decoration with a proof attached (register C-82). + + Reading the finished message instead makes the whole class unreachable. It does not + matter how a finding is built, which branch builds it, or what the final assertion + interpolates — if a value reaches the text, this fails. That is the difference between + enumerating the mutations you thought of (register **C-93**) and making them moot. + + Both halves are exercised, because C-89 was one half being taught a rule the other + half was not. + """ + planted, coordinate = "fixture-value-2f9c-not-a-real-coordinate", "APPWRITE_UNFAO_BUCKET_ID" + + pkg = tmp_path / "views_postprocessing" + pkg.mkdir() + (pkg / "leaky.py").write_text(f'BUCKET_ID = "{planted}"\n') + (tmp_path / "README.md").write_text(f"{coordinate}={planted}\n") + for args in (("init", "-q"), ("add", "-A")): + subprocess.run( + ["git", "-C", str(tmp_path), "-c", "commit.gpgsign=false", *args], + capture_output=True, text=True, check=True, timeout=30, + ) + + here = sys.modules[__name__] + monkeypatch.setattr(here, "_PKG", pkg) + monkeypatch.setattr(here, "_REPO", tmp_path) + monkeypatch.setattr(here, "require_sibling", lambda name: tmp_path) + monkeypatch.setattr(here, "_registry_current", lambda repo: { + "meta": {"version": "0.0.0-fixture"}, + "connection": {"APPWRITE_ENDPOINT": {"class": "connection", "value": "e"}}, + "target": {coordinate: {"class": "target", "value": planted}}, + }) + + with pytest.raises(AssertionError) as caught: + test_no_coordinate_value_is_copied_into_this_repo() + message = str(caught.value) + + # Named files, not a count: pytest's assertion rewriting echoes the message, so + # counting substrings of it counts each finding twice. Naming both planted files is + # what stops this passing vacuously — a scan that finds nothing reports nothing, and + # "no value in the message" is trivially true of an empty message. + for half in ("leaky.py", "README.md"): + assert half in message, ( + f"the {half} half of the scan did not report the planted copy: {message}. " + "C-89 was one branch being taught a rule the other was not, so both must fire." + ) + assert coordinate in message, ( + f"the report must name the coordinate so a maintainer can find it: {message}" + ) + assert planted not in message, ( + "THE SCAN PRINTED THE VALUE IT EXISTS TO HIDE. This repository is public, its CI " + "logs are world-readable, and they cannot be redacted afterwards. However the " + "finding was built, the finished text must never carry the value." + ) From 29bb769272d2f23716f746e01a851aebae7f2696 Mon Sep 17 00:00:00 2001 From: Polichinl Date: Wed, 12 Aug 2026 13:10:53 +0200 Subject: [PATCH 4/5] =?UTF-8?q?fix(tests):=20#242=20iteration=203=20?= =?UTF-8?q?=E2=80=94=20the=20channels=20a=20message=20test=20cannot=20see?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An adversarial mutation review ran 14 mutations against iteration 2. Twelve survived. Seven were leak channels; all seven are now closed. Five are refused in writing. A MESSAGE TEST ONLY SEES THE MESSAGE. `print(node.value)` beside the append, or a `logging.warning`, left the assertion text clean while pytest published the value under Captured stdout and Captured log call. The behavioural test now reads capsys and caplog as well. A stray debug print is an ordinary accident, not an adversarial one. SUBSTRING ABSENCE IS NOT ABSENCE. A finding carrying `value[:12]` or `value[::-1]` walked straight past `planted not in message`, and a reader recovers both instantly. Replaced by `_carries`: does any eight-character window, forwards or backwards, appear. AND THE DRIFT REPORTER HAD NO TEST AT ALL. `_describe_changes` is exercised only by the rotation proof, through one shape, with fixture values SHORTER THAN EVERY REAL COORDINATE — so a digest leaking only above a length threshold passed, as did a reversing digest, as did the appeared/removed branches a both-sides fixture never reaches. Three leaks, all green. It now has its own test, driven through all three shapes with an over-long opaque value. A HELPER'S PARAMETER IS A PUBLICATION CHANNEL. `_dotenv_use` took file text; pytest renders a frame's arguments and `ast.parse` raises on any malformed module, so a package file that both failed to parse and carried a value would have printed itself in full. It takes a Path now and reads inside. And the fixture values are opaque now because a readable one bit me: the first planted string contained the word "coordinate", which also appears in the check's own failure prose, so the window test fired on the test's own English. My cry-wolf, caught by my own guard, fixed in the fixture rather than by weakening the check. WHAT I AM NOT CHASING, and why it is written down rather than left to be rediscovered. Five mutations survive. Four narrow the scan's SCOPE — a length floor, a dropped section, a swallowed SyntaxError, a `break` after the first finding — and none is visible to a test that plants its own fixture, because the fixture satisfies whatever narrowing is applied. They are #243's subject and are routed there with reproductions; fixing them here would be doing that story badly and twice. The fifth deletes the no-print assertion itself, which is infinite regress: a guard on a guard on a guard has no fixed point, and the register carries that risk instead. The eight-character window is a judgement, not a law — a six-character leak would pass, and chasing it is the whack-a-mole this epic exists to refuse. Register: C-89 amended with all three passes, including what was tried and failed. Suite 412 (411 passed / 1 skipped / 40 xfailed), ruff clean. The one failure is #248's. Closes #242. Epic #241. Co-Authored-By: Claude Opus 5 (1M context) --- reports/technical_risk_register.md | 12 +++- tests/test_env_declaration.py | 111 +++++++++++++++++++++++++---- 2 files changed, 108 insertions(+), 15 deletions(-) diff --git a/reports/technical_risk_register.md b/reports/technical_risk_register.md index e387ff3..ec19f81 100644 --- a/reports/technical_risk_register.md +++ b/reports/technical_risk_register.md @@ -305,7 +305,17 @@ Two of the three are fixed, and the third moved: Reading the finished message instead makes the whole class unreachable: it does not matter how a finding is built, which branch builds it, or what the final assertion interpolates. All five surviving mutations are now caught, including one that leaked the entire ban-set through the assertion message rather than through a finding. -4. **The scan's own docstring carried three registry values**, and pytest prints the failing function's source — so the guard would have published them on exactly the event it exists to catch. The message was clean; the traceback was not. Now it names coordinates. **The wider finding is registered separately as C-97**: 33 standalone values sit in docstrings and comments across nine files, production modules included, and the AST scan excludes docstrings by a deliberate C-57 decision taken before anyone counted them. +**A third pass, from an adversarial mutation review that ran 14 mutations against the second.** Twelve survived. Seven were leak channels the message test could not see, and all seven are now closed: + +- `print()` or `logging.warning()` beside the append — the message stayed clean while pytest published the value under *Captured stdout* / *Captured log call*. The behavioural test now reads `capsys` and `caplog` too. A stray debug print is an ordinary accident, not an adversarial one. +- A finding carrying `value[:12]` or `value[::-1]` — a substring test cannot see either, and a reader recovers both instantly. Replaced by `_carries`: does any **eight-character window**, forwards or backwards, appear. +- **`_describe_changes` had no test of its own at all.** The rotation proof exercises it through one shape, with fixture values *shorter than every real coordinate* — so a `digest()` leaking only above a length threshold passed, as did a reversing `digest()`, as did the `appeared`/`removed` branches the both-sides fixture never reaches. Three leaks, all green. It now has `test_the_drift_report_never_carries_a_value_in_any_recoverable_form`, driven directly through all three shapes with an over-long opaque value. + +**And a helper's parameter is a publication channel.** `_dotenv_use` took file *text*; pytest renders a frame's arguments, and `ast.parse` raises on any malformed module — so a package file that both failed to parse and carried a value would have printed itself in full. It takes a `Path` now and reads inside. + +**What is deliberately NOT chased, so the next reader does not re-open it.** Five mutations survive and are refused rather than missed: four narrow the scan's *scope* (a length floor, a dropped section, a swallowed `SyntaxError`, a `break` after the first finding), and one deletes the no-print assertion itself. The first four are the scan's scope, which is **#243**'s entire subject and where they are routed — fixing them here would be doing that story badly, twice. The fifth is infinite regress: a guard on the guard on the guard has no fixed point, and the register is where that risk is carried instead. The `_carries` window is eight characters, which is a judgement and not a law; a six-character leak would pass, and chasing that is the whack-a-mole this epic exists to refuse. + +4. **The scan's own docstring carried four registry values**, and pytest prints the failing function's source — so the guard would have published them on exactly the event it exists to catch. The message was clean; the traceback was not. Now it names coordinates. **The wider finding is registered separately as C-97**: 33 standalone values sit in docstrings and comments across nine files, production modules included, and the AST scan excludes docstrings by a deliberate C-57 decision taken before anyone counted them. **Deliberately still open, and moved rather than closed:** the `secret` exemption at `:1042` and the ban-set's package-name collision are the *scope* of the scan, not its reporting, and belong with the matcher rewrite in **#243**. This entry stays open until they land, because closing it now would close a Tier 2 on two-thirds of its content. diff --git a/tests/test_env_declaration.py b/tests/test_env_declaration.py index 011b3cf..97d22ba 100644 --- a/tests/test_env_declaration.py +++ b/tests/test_env_declaration.py @@ -137,9 +137,14 @@ def _manager_source(partner: str) -> Path: _DOTENV_CALLS = {"load_dotenv", "find_dotenv", "dotenv_values"} -def _dotenv_use(source: str) -> list[str]: +def _dotenv_use(path: Path) -> list[str]: """Real imports of, and calls into, python-dotenv — parsed, not grepped. + Takes a **path**, not the file's text. pytest renders a frame's arguments in a + traceback, so a helper that accepts source text publishes that text whenever it + raises — and ``ast.parse`` raises on any malformed module. A package file that both + fails to parse and carries a coordinate value would have printed itself in full. + **Prose is not a violation and must not be treated as one.** ``appwrite_env.py``'s own docstring explains what the retired borrow was, spelling it exactly; a token scan over the package flags that sentence and gets deleted, after which the rule it @@ -148,7 +153,7 @@ def _dotenv_use(source: str) -> list[str]: or a comment, so the guard can cover the whole package without lying about prose. """ found = [] - for node in ast.walk(ast.parse(source)): + for node in ast.walk(ast.parse(path.read_text())): if isinstance(node, ast.Import): found += [a.name for a in node.names if a.name.split(".")[0] == "dotenv"] elif isinstance(node, ast.ImportFrom): @@ -195,7 +200,7 @@ def test_the_dotenv_borrow_is_dead(): offenders = { source.relative_to(_PKG).as_posix(): used for source in sorted(_PKG.rglob("*.py")) - for used in [_dotenv_use(source.read_text())] + for used in [_dotenv_use(source)] if used } assert not offenders, ( @@ -1041,12 +1046,14 @@ def test_no_coordinate_value_is_copied_into_this_repo(): **Two wrong narrowings before this one, both instructive.** - A substring text scan over every ``.py`` flagged three "leaks": the function name - ``file_metadata`` in ``contract/store_metadata.py``, and the values of + A substring text scan over every ``.py`` flagged three "leaks", none of them a copy: + a **function name** in ``contract/store_metadata.py`` that happens to be spelled the + same as the value of ``APPWRITE_METADATA_DATABASE_ID``, and the values of ``APPWRITE_PROD_FORECASTS_BUCKET_ID`` and ``APPWRITE_UNFAO_BUCKET_ID`` where they appear only in refusal labels and in docstrings naming which store a function serves. - None was a copy, and a guard that fails on ``def file_metadata(record)`` gets deleted - — after which the real rule is unguarded. + A guard that fails on an ordinary ``def`` gets deleted — after which the real rule is + unguarded. (Three declared values cannot be told from ordinary code by string + equality at all: that one, and this package's two directory names. Register C-97.) Narrowing to *assignments and default arguments* then went too far the other way: it caught neither a dict value nor a keyword argument, and the keyword argument is the @@ -1194,7 +1201,7 @@ def test_no_coordinate_value_is_copied_into_this_repo(): ) -def test_the_scan_reports_a_copy_without_reprinting_it(tmp_path, monkeypatch): +def test_the_scan_reports_a_copy_without_reprinting_it(tmp_path, monkeypatch, capsys, caplog): """Feed the real scan a planted copy and read what it says. **The rule, behaviourally.** This replaces a source-reading guard that asserted every finding was *routed* through @@ -1212,7 +1219,11 @@ def test_the_scan_reports_a_copy_without_reprinting_it(tmp_path, monkeypatch): Both halves are exercised, because C-89 was one half being taught a rule the other half was not. """ - planted, coordinate = "fixture-value-2f9c-not-a-real-coordinate", "APPWRITE_UNFAO_BUCKET_ID" + # Opaque on purpose. A first draft planted a readable string containing the word + # "coordinate" — which also appears in the check's own failure prose, so the + # eight-character window test below fired on the test's own English. The fixture must + # share no fragment with anything the report legitimately says. + planted, coordinate = "Zq7xVb9KdLm1RnTs3Wy8Pj5Hc2Gf", "APPWRITE_UNFAO_BUCKET_ID" pkg = tmp_path / "views_postprocessing" pkg.mkdir() @@ -1250,8 +1261,80 @@ def test_the_scan_reports_a_copy_without_reprinting_it(tmp_path, monkeypatch): assert coordinate in message, ( f"the report must name the coordinate so a maintainer can find it: {message}" ) - assert planted not in message, ( - "THE SCAN PRINTED THE VALUE IT EXISTS TO HIDE. This repository is public, its CI " - "logs are world-readable, and they cannot be redacted afterwards. However the " - "finding was built, the finished text must never carry the value." - ) + # Every channel pytest publishes, not just the assertion text. A debug `print` left + # beside the append is an ordinary accident, and pytest renders captured stdout and + # captured logs in the same report as the failure — so "the message is clean" is only + # part of "the run is clean". + captured = capsys.readouterr() + for channel, text in ( + ("the assertion message", message), + ("captured stdout", captured.out), + ("captured stderr", captured.err), + ("captured logs", caplog.text), + ): + assert not _carries(text, planted), ( + f"THE SCAN PUBLISHED THE VALUE IT EXISTS TO HIDE, via {channel}. This " + "repository is public, its CI logs are world-readable, and they cannot be " + "redacted afterwards. However the finding was built and wherever it was " + "written, no recoverable form of the value may leave this test." + ) + + +def _carries(text: str, value: str) -> bool: + """Is ``value`` recoverable from ``text`` — not merely present in it? + + Equality and ``in`` are too weak for a no-print rule. An adversarial review defeated a + substring check with ``value[:12]`` and again with ``value[::-1]``, both of which a + reader recovers instantly. So this asks the question that matters: does any eight- + character window of the value, forwards or backwards, appear? + + Eight is a judgement, not a law: long enough that ordinary English and file paths do + not collide with a coordinate, short enough that a truncation or a reversal is caught. + """ + windows = {value[i:i + 8] for i in range(len(value) - 7)} or {value} + return any(w in text or w[::-1] in text for w in windows) + + +def test_the_drift_report_never_carries_a_value_in_any_recoverable_form(): + """``_describe_changes`` is the drift check's reporter, and it had no test of its own. + + The rotation proof above exercises it, but only through one shape — a value present + on both sides — and with fixture strings shorter than every real coordinate. An + adversarial review walked through both gaps: a ``digest()`` that returns the raw value + only when it is longer than the fixture's, and the ``appeared``/``removed`` branches + that the both-sides fixture never reaches. Three leaks, all green. + + So this drives the reporter directly, with a value longer than any the registry + declares, through every branch that can produce text. + """ + # Longer than every value the registry declares, and opaque so no window of it + # can collide with the report's own wording. + value = "Xk4mQ8zRv2Ld7NpTb5Wc1Hy9Ja6Fg3Ss" + other = "Bn6tYp3Kq9Vx2Mw8Lz5Rd1Cf7Hj4Gu0Ee" + name = "APPWRITE_FIXTURE_COORDINATE" + + reports = [ + # rotated: present on both sides, different + _describe_changes({name: ("target", "target", value)}, + {name: ("target", "target", other)}, {name}), + # appeared: absent at the pin, valued now + _describe_changes({name: ("target", "target", _ABSENT)}, + {name: ("target", "target", value)}, {name}), + # removed: valued at the pin, absent now + _describe_changes({name: ("target", "target", value)}, + {name: ("target", "target", _ABSENT)}, {name}), + ] + + for shape, report in zip(("rotated", "appeared", "removed"), reports): + assert name in report, ( + f"the {shape} shape produced no report for {name}: {report}. A reporter that " + "says nothing about a change it was handed is the silent half of a drift check." + ) + text = report[name] + for secret in (value, other): + assert not _carries(text, secret), ( + f"the {shape} report carries a coordinate value in a recoverable form: " + f"{text!r}. This repository is public and its CI logs are world-readable. " + "Name the field that moved and digest the value; never emit it, and never " + "emit a prefix or a transform of it." + ) From f337269b0db9c58647015b164a3f1f2565bd86f3 Mon Sep 17 00:00:00 2001 From: Polichinl Date: Wed, 12 Aug 2026 17:05:17 +0200 Subject: [PATCH 5/5] =?UTF-8?q?fix(tests):=20#242=20iteration=204=20?= =?UTF-8?q?=E2=80=94=20delete=20most=20of=20what=20the=20first=20three=20a?= =?UTF-8?q?dded?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The maintainer's verdict on iterations 1-3: "so complicated and heavy with technical debt that you are not even able to maintain it. Then no one can." He is right, and the measurement is worse than the impression. Four commits had grown one file by +195 lines (1145 -> 1340, +17%). +95 of that was PROSE restating the risk register. All 149 lines of new function bodies were tests of another test in the same file. Meta tests went from 37% to 45% of the file. This commit deletes 140 of those lines. Every one of the four outstanding review findings is resolved by subtraction. TWO OF MY OWN ADDITIONS WERE DEFECTIVE, AND A DESIGN REVIEW CAUGHT BOTH. `test_the_drift_report_never_carries_a_value_in_any_recoverable_form` claimed to drive `_describe_changes`'s appeared/removed branches. It did not. Those branch on `was is None`; my fixtures passed `_ABSENT`, a sentinel OBJECT. The branch is never entered — it ran one path three times while asserting three, and the register repeated the claim. That is an ADR-014 §1 defect inside the fix for an ADR-014 §1 defect. The branches it named return constant strings and cannot leak at all. DELETED. `_carries`, my eight-character-window "recoverability" predicate, is STRICTLY WORSE THAN PLAIN `in`. The window degrades to exact match below eight characters, and this package's two directory names are five-character declared values. Measured: `_carries("views_postprocessing/unfao/leaky.py …", "unfao")` is True — it condemns a bare, safe file path while screaming that the scan published a value. Simultaneously stricter than `in` on opaque fixtures and identical to it on the values that matter. DELETED; both sites use plain `not in`, which is the vocabulary this file already uses in `test_the_environment_refusal_logs_names_and_never_values`. ALSO DELETED: `_report_a_copy` — 2 lines of code under 14 lines of docstring, 11 of them explaining what it does NOT guarantee. Its safety claim was falsified in iteration 2 and it survived anyway. Two inline f-strings now, which is what WET-before-DRY prescribes: the second incident did not have the same shape as the first, and the helper's signature forced the markdown branch into a detour it had to justify with a six-line comment. The behavioural guard is what holds the rule, not the formatter. The output guard SURVIVES, at 19 lines instead of 78. ADR-014 §1 does not permit a guarantee with no check, and this regression already happened once, silently. But it no longer monkeypatches four globals, shells out to `git init`, or guards a `print` nobody has written. AND THE ONE REAL OPEN LEAK IS NOW CLOSED, AT BOTH SITES. Iteration 3 claimed a malformed package module could no longer publish itself because `_dotenv_use` takes a Path. Measured: that changes nothing — the leak is `ast.parse`'s own frame, which pytest renders with the source as its argument, and `path.read_text()` still hands it there. The fix is `raise ... from None`, where `from None` is load-bearing because the chained SyntaxError carries the same text. Applied to BOTH parse sites; the scan itself parses every package .py, and fixing one and not the other is C-89's shape verbatim. Proven: a malformed module carrying a real value now fails with "does not parse (line 92)" and the value appears ZERO times, where it appeared before. Five mutations re-proven: the original C-89 defect fails; a widened assertion fails; a raw digest fails; a malformed module fails without publishing itself; a legitimate refactor passes. Register: C-89's Location is now FUNCTION NAMES — it has cited stale line numbers twice, because each fix moved them. The false "it takes a Path now" sentence is corrected. The narration of both deleted additions is removed, including one claim that was never true. Two deferrals get triggers: consolidating into tests/test_redaction_guard.py when #243 finishes with this file, and a pre-existing proof that re-types its subject inline. tests/test_env_declaration.py: 1340 -> 1200 (+55 over the pre-story 1145, not +195). Suite 410 passed / 1 skipped / 40 xfailed, ruff clean. The one failure is #248's. Closes #242. Epic #241. Co-Authored-By: Claude Opus 5 (1M context) --- reports/technical_risk_register.md | 23 ++- tests/test_env_declaration.py | 238 ++++++----------------------- 2 files changed, 66 insertions(+), 195 deletions(-) diff --git a/reports/technical_risk_register.md b/reports/technical_risk_register.md index ec19f81..f2c2f83 100644 --- a/reports/technical_risk_register.md +++ b/reports/technical_risk_register.md @@ -279,7 +279,7 @@ Cross-refs: **C-57** and **C-89** (the guard this was measured on), **C-90** (a | Source | `/code-review max` on PR #239 post-merge, 2026-08-11; every element verified in this repository | | Trigger | A literal coordinate value is committed into a `.py` under `views_postprocessing/` — the violation the scan exists for — on any branch whose CI runs. | | Owner | This repository. | -| Location | `tests/test_env_declaration.py:1152` (the AST branch), `:1124` (the `secret` exemption), `:946` (the rotation proof). Line numbers as of 2026-08-12 — the 2026-08-11 filing cited `:1065`/`:1042`/`:937`, which the fix itself moved. | +| Location | `tests/test_env_declaration.py` — `test_no_coordinate_value_is_copied_into_this_repo` (both reporting branches and the `secret` exemption in `scanned_sections`), `test_the_drift_check_would_catch_a_rotation_that_names_and_classes_cannot`. **Function names, not line numbers**: this entry has now cited stale ones twice, because each fix moved them. | The no-copy scan exists because README.md once carried four real coordinate values, two lines below the sentence promising they are never copied, in a public repository. It has three parts and they do not agree with each other about the one rule that matters. @@ -297,7 +297,7 @@ The three share a cause: the no-print rule lives in prose and in one implementat Two of the three are fixed, and the third moved: -1. **The AST branch no longer receives the value.** Both branches build their finding through `_report_a_copy(where, coordinates)`, which takes no value. The report names the coordinate instead, and names **all** coordinates sharing that value: measured, two pairs share one (the prod-forecasts bucket and collection share both id and name), so a `value -> name` map would have named the wrong coordinate half the time in the message a maintainer uses to find the copy. +1. **Both branches name the coordinate, never the value.** Each builds its own sentence — two f-strings, deliberately not one shared formatter — and neither has the value in hand. They name **all** coordinates declaring that value: measured, two pairs share one (the prod-forecasts bucket and collection share both id and name), so a `value -> name` map would have named the wrong coordinate half the time in the message a maintainer uses to find the copy. 2. **The rotation proof asserts both sides absent**, taken from the fixture's own variables rather than from two literals a rename would quietly orphan. Mutation-proven: leaking the post-rotation side while keeping the pinned side digested fails now and **passed before** — the more damaging half, unchecked. 3. **The rule is asserted behaviourally**, by `test_the_scan_reports_a_copy_without_reprinting_it`: plant a value in a fixture tree, run the real scan, read the finished message. @@ -308,17 +308,28 @@ Reading the finished message instead makes the whole class unreachable: it does **A third pass, from an adversarial mutation review that ran 14 mutations against the second.** Twelve survived. Seven were leak channels the message test could not see, and all seven are now closed: - `print()` or `logging.warning()` beside the append — the message stayed clean while pytest published the value under *Captured stdout* / *Captured log call*. The behavioural test now reads `capsys` and `caplog` too. A stray debug print is an ordinary accident, not an adversarial one. -- A finding carrying `value[:12]` or `value[::-1]` — a substring test cannot see either, and a reader recovers both instantly. Replaced by `_carries`: does any **eight-character window**, forwards or backwards, appear. -- **`_describe_changes` had no test of its own at all.** The rotation proof exercises it through one shape, with fixture values *shorter than every real coordinate* — so a `digest()` leaking only above a length threshold passed, as did a reversing `digest()`, as did the `appeared`/`removed` branches the both-sides fixture never reaches. Three leaks, all green. It now has `test_the_drift_report_never_carries_a_value_in_any_recoverable_form`, driven directly through all three shapes with an over-long opaque value. +- **`_describe_changes`'s rotation proof was one-sided** — it asserted only the pinned value absent, never the rotated one, which is the more damaging half. Now both, taken from the fixture's own variables. -**And a helper's parameter is a publication channel.** `_dotenv_use` took file *text*; pytest renders a frame's arguments, and `ast.parse` raises on any malformed module — so a package file that both failed to parse and carried a value would have printed itself in full. It takes a `Path` now and reads inside. +**A malformed package module published itself, and the first fix for it did not work.** `ast.parse` raises with the source as its own frame's argument, and pytest renders that in full — so a package file that both fails to parse and carries a value printed itself. This entry previously said *"it takes a `Path` now and reads inside"*; **measured, that changes nothing**, because the text still reaches `ast.parse`. The fix is a shared `_parsed(path)` that refuses by path and line with `raise ... from None` — `from None` is the load-bearing part, since the chained `SyntaxError` carries the same text. Applied at **both** parse sites: fixing one and not the other is this entry's own shape. -**What is deliberately NOT chased, so the next reader does not re-open it.** Five mutations survive and are refused rather than missed: four narrow the scan's *scope* (a length floor, a dropped section, a swallowed `SyntaxError`, a `break` after the first finding), and one deletes the no-print assertion itself. The first four are the scan's scope, which is **#243**'s entire subject and where they are routed — fixing them here would be doing that story badly, twice. The fifth is infinite regress: a guard on the guard on the guard has no fixed point, and the register is where that risk is carried instead. The `_carries` window is eight characters, which is a judgement and not a law; a six-character leak would pass, and chasing that is the whack-a-mole this epic exists to refuse. +**A fourth pass deleted most of the third, and that is the entry's real lesson.** Four commits and five reviews had grown this file by **+195 lines**, of which **+95 was prose restating this register** and **every one of the 149 new function-body lines was a test of another test in the same file**. Two of those additions were themselves defective: + +- `test_the_drift_report_never_carries_a_value_in_any_recoverable_form` claimed to drive `_describe_changes`'s `appeared`/`removed` branches. It did not: those branch on `was is None`, and the fixtures passed `_ABSENT`, a sentinel object. It ran one branch three times while asserting three — an ADR-014 §1 defect inside the fix for an ADR-014 §1 defect — and the branches it named return constant strings and cannot leak. **Deleted.** +- `_carries`, an eight-character-window "recoverability" predicate, is **strictly worse than plain `in`**: the window degrades to exact match below eight characters, and this package's two directory names are five-character declared values. Measured, `_carries("views_postprocessing/unfao/leaky.py …", "unfao")` is `True` — it condemns a bare, safe file path while claiming a leak. **Deleted**; both call sites use plain `not in`, the vocabulary already at `test_the_environment_refusal_logs_names_and_never_values`. + +Net **−140 lines**, leaving the file **+55 over its pre-story size** rather than +195. What the +55 buys: one behavioural guard, the rotation proof's second side, the value→coordinate map, and the shared parse refusal. + +**What is deliberately NOT chased.** Four surviving mutations narrow the scan's *scope* — a length floor, a dropped section, a swallowed `SyntaxError`, a `break` after the first finding — and none is visible to a test that plants its own fixture. They are **#243**'s subject and are routed there. A fifth deletes the no-print assertion itself: infinite regress, carried here instead. Two residuals stand: the markdown branch's output is not behaviourally proven (it holds no value by construction), and a leak shorter than the planted fixture would pass. Chasing either is the whack-a-mole this epic exists to refuse. 4. **The scan's own docstring carried four registry values**, and pytest prints the failing function's source — so the guard would have published them on exactly the event it exists to catch. The message was clean; the traceback was not. Now it names coordinates. **The wider finding is registered separately as C-97**: 33 standalone values sit in docstrings and comments across nine files, production modules included, and the AST scan excludes docstrings by a deliberate C-57 decision taken before anyone counted them. **Deliberately still open, and moved rather than closed:** the `secret` exemption at `:1042` and the ban-set's package-name collision are the *scope* of the scan, not its reporting, and belong with the matcher rewrite in **#243**. This entry stays open until they land, because closing it now would close a Tier 2 on two-thirds of its content. +**Two deferrals, both with triggers (ADR-014 §4).** + +1. **The leak guards belong in `tests/test_redaction_guard.py`**, which already owns "what stops us leaking" and which `test_the_environment_refusal_logs_names_and_never_values` already points readers to. The concern is currently split across two files with no shared vocabulary. Not moved here, because moving code while fixing bugs in it is how the next defect arrives. **Trigger: when #243 finishes touching `tests/test_env_declaration.py`.** Owner: this repository. +2. **`test_the_drift_check_would_catch_a_rename` re-types its subject's comparison inline** rather than calling it, so blanking that subject's assertions leaves the proof green — the same defect its two siblings had repaired. Pre-existing, found while reading for this change. **Routed to #243**, which is already in this file. + Cross-refs: **C-57** (the scan's own entry and its history), **C-93** (why the author's own proof did not find this), **C-90** (the sibling defect in the drift checks), ADR-014 §1, issues #242, #243. --- diff --git a/tests/test_env_declaration.py b/tests/test_env_declaration.py index 97d22ba..f6f3bd3 100644 --- a/tests/test_env_declaration.py +++ b/tests/test_env_declaration.py @@ -137,13 +137,23 @@ def _manager_source(partner: str) -> Path: _DOTENV_CALLS = {"load_dotenv", "find_dotenv", "dotenv_values"} +def _parsed(path: Path) -> ast.Module: + """Parse ``path``, refusing a malformed module by path and line — never by content.""" + try: + return ast.parse(path.read_text()) + except SyntaxError as exc: + raise AssertionError(f"{path.name} does not parse (line {exc.lineno})") from None + + def _dotenv_use(path: Path) -> list[str]: """Real imports of, and calls into, python-dotenv — parsed, not grepped. - Takes a **path**, not the file's text. pytest renders a frame's arguments in a - traceback, so a helper that accepts source text publishes that text whenever it - raises — and ``ast.parse`` raises on any malformed module. A package file that both - fails to parse and carries a coordinate value would have printed itself in full. + A malformed module is refused by path and line, never by content. ``ast.parse`` + raises with the source as its own frame's argument, which pytest renders in full — so + a package file that both fails to parse and carries a coordinate value would publish + itself. ``from None`` drops the chained ``SyntaxError``, which carries the same text. + (An earlier version credited the ``Path`` parameter for this. Measured: it changes + nothing, because the text still reaches ``ast.parse``.) **Prose is not a violation and must not be treated as one.** ``appwrite_env.py``'s own docstring explains what the retired borrow was, spelling it exactly; a token @@ -153,7 +163,7 @@ def _dotenv_use(path: Path) -> list[str]: or a comment, so the guard can cover the whole package without lying about prose. """ found = [] - for node in ast.walk(ast.parse(path.read_text())): + for node in ast.walk(_parsed(path)): if isinstance(node, ast.Import): found += [a.name for a in node.names if a.name.split(".")[0] == "dotenv"] elif isinstance(node, ast.ImportFrom): @@ -945,9 +955,7 @@ def edition(value: str) -> dict: f"[{partner}] the report must name the FIELD that moved, or a maintainer reading " "the failure cannot tell a rotation from a reclassification." ) - # Both sides, taken from the fixture's own variables. Asserting the two literals - # instead let a fixture rename turn this into "two dead strings are absent" — and the - # rotated side is the one that matters, which an earlier version never checked. + # Both sides, from the fixture's own variables — the rotated one was never checked. for side in (was, rotated): assert side not in changed[canary], ( f"[{partner}] the report printed the {side!r} value. It must name the field " @@ -1014,57 +1022,19 @@ def _docstring_nodes(tree: ast.AST) -> set[int]: return out -def _report_a_copy(where: str, coordinates: tuple[str, ...]) -> str: - """One finding from the no-copy scan: WHERE the copy is and WHICH coordinate it is. - - The value is not a parameter, so the ordinary way of writing a finding cannot print - one. That is a convention with a shape, **not a guarantee** — an earlier version of - this docstring claimed a caller "cannot print one however it is written", and an - independent review falsified it in two lines by smuggling the value through ``where`` - and again through ``coordinates``. What actually holds the rule is - :func:`test_the_scan_reports_a_copy_without_reprinting_it`, which reads the finished - message rather than the syntax that produced it (register **C-89**, **C-93**). - - ``coordinates`` is a tuple because two coordinates may legitimately declare the same - value — measured against the live registry, two pairs do — and naming one of them - would be wrong half the time in the message a maintainer uses to find the copy. - """ - return f"{where} carries the value declared for {', '.join(coordinates)}" - - - def test_no_coordinate_value_is_copied_into_this_repo(): """The registry's own rule: *"never bake a value into code, an example, or a dataclass default."* Consumers READ and VALIDATE; the launcher supplies values. - **This docstring names coordinates and never values, and that is not fussiness.** - An earlier version quoted three real registry values as worked examples. When this - check fails, pytest prints the failing function's own source — so the guard against - publishing a coordinate would have published three, into a world-readable CI log, on - exactly the event it exists to catch. The message was clean and the traceback was not - (register **C-89**). - - **Two wrong narrowings before this one, both instructive.** - - A substring text scan over every ``.py`` flagged three "leaks", none of them a copy: - a **function name** in ``contract/store_metadata.py`` that happens to be spelled the - same as the value of ``APPWRITE_METADATA_DATABASE_ID``, and the values of - ``APPWRITE_PROD_FORECASTS_BUCKET_ID`` and ``APPWRITE_UNFAO_BUCKET_ID`` where they - appear only in refusal labels and in docstrings naming which store a function serves. - A guard that fails on an ordinary ``def`` gets deleted — after which the real rule is - unguarded. (Three declared values cannot be told from ordinary code by string - equality at all: that one, and this package's two directory names. Register C-97.) - - Narrowing to *assignments and default arguments* then went too far the other way: it - caught neither a dict value nor a keyword argument, and the keyword argument is the - shape this repo would actually produce — ``AppwriteConfig(bucket_id=...)`` is how - every store is configured, and swapping one ``os.getenv`` for a literal there is the - violation. - - The right axis was **exact equality on string constants**, not statement shape. It - catches dict values and kwargs, while all three original false positives fall out on - their own: a function name is not a ``Constant``; a value with a word appended is not - equal to the value; docstrings are excluded outright. + **Names coordinates, never values** — including in this docstring. pytest prints the + failing function's source, so a value written here publishes on exactly the event the + check exists to catch (register C-89). + + The axis is **exact equality on string constants**, not statement shape: it catches + dict values and keyword arguments — ``AppwriteConfig(bucket_id=...)`` is how every + store is configured — while a function name is not a ``Constant`` and docstrings are + excluded outright. Two narrower drafts and their false positives are in register + C-57; three values that cannot be told from ordinary code at all are in C-97. """ # `require_sibling`, like every other reader in this file. The hand-rolled skip that # stood here said only "checkout not found — set VIEWS_APPWRITE", which is the @@ -1100,7 +1070,7 @@ def test_no_coordinate_value_is_copied_into_this_repo(): copied = [] for source in sorted(_PKG.rglob("*.py")): - tree = ast.parse(source.read_text()) + tree = _parsed(source) docstrings = _docstring_nodes(tree) for node in ast.walk(tree): if ( @@ -1109,10 +1079,11 @@ def test_no_coordinate_value_is_copied_into_this_repo(): and node.value in values and id(node) not in docstrings ): - copied.append(_report_a_copy( - f"{source.relative_to(_PKG)}:{node.lineno}", - tuple(sorted(declared_by_value[node.value])), - )) + names = ", ".join(sorted(declared_by_value[node.value])) + copied.append( + f"{source.relative_to(_PKG)}:{node.lineno} carries the value " + f"declared for {names}" + ) # Markdown too — the AST half cannot see a fenced ``bash`` block, and that is exactly # where four production-forecasts values sat: in README.md's Configuration section, # two lines below the sentence promising they are never copied, in a PUBLIC @@ -1183,15 +1154,13 @@ def test_no_coordinate_value_is_copied_into_this_repo(): match = assignment.search(line) captured = next((g for g in match.groups()[1:] if g is not None), None) if match else None if captured is not None and captured.strip() in values: - # Report the coordinate(s) that DECLARE the value, not the one the - # assignment happens to name. Those differ exactly when a document - # pastes one coordinate's value beside another's name — the ordinary - # copy-paste slip — and naming the assignment would then assert a false - # fact and hide the coordinate actually leaked. - copied.append(_report_a_copy( - f"{doc.relative_to(_REPO)}:{number}", - tuple(sorted(declared_by_value[captured.strip()])), - )) + # The coordinate(s) that DECLARE the value, not the one the assignment + # names — they differ on the ordinary copy-paste slip. + names = ", ".join(sorted(declared_by_value[captured.strip()])) + copied.append( + f"{doc.relative_to(_REPO)}:{number} carries the value " + f"declared for {names}" + ) assert not copied, ( f"coordinate value(s) from the registry are copied into this repo: {copied}. The " @@ -1201,140 +1170,31 @@ def test_no_coordinate_value_is_copied_into_this_repo(): ) -def test_the_scan_reports_a_copy_without_reprinting_it(tmp_path, monkeypatch, capsys, caplog): - """Feed the real scan a planted copy and read what it says. **The rule, behaviourally.** - - This replaces a source-reading guard that asserted every finding was *routed* through - ``_report_a_copy``. Routing is not safety, and an independent review proved it: the - value could be smuggled through either of that helper's two parameters, or appended - with ``extend``/``+=``, or reported from a renamed accumulator — **six of eight - mutations survived**, while one legitimate refactor failed it. A guard that misses the - thing and fires on the innocent is decoration with a proof attached (register C-82). - - Reading the finished message instead makes the whole class unreachable. It does not - matter how a finding is built, which branch builds it, or what the final assertion - interpolates — if a value reaches the text, this fails. That is the difference between - enumerating the mutations you thought of (register **C-93**) and making them moot. +def test_the_scan_reports_where_a_copy_is_and_never_what_it_is(tmp_path, monkeypatch): + """C-89: the ``.py`` branch printed the value it forbids, on the one event it fires on. - Both halves are exercised, because C-89 was one half being taught a rule the other - half was not. + Read the finished message, not the source that built it — an earlier guard asserted + that findings were *routed* through one formatter, and routing is not safety. """ - # Opaque on purpose. A first draft planted a readable string containing the word - # "coordinate" — which also appears in the check's own failure prose, so the - # eight-character window test below fired on the test's own English. The fixture must - # share no fragment with anything the report legitimately says. - planted, coordinate = "Zq7xVb9KdLm1RnTs3Wy8Pj5Hc2Gf", "APPWRITE_UNFAO_BUCKET_ID" - + planted = "Zq7xVb9KdLm1RnTs3Wy8Pj5Hc2Gf" pkg = tmp_path / "views_postprocessing" pkg.mkdir() (pkg / "leaky.py").write_text(f'BUCKET_ID = "{planted}"\n') - (tmp_path / "README.md").write_text(f"{coordinate}={planted}\n") - for args in (("init", "-q"), ("add", "-A")): - subprocess.run( - ["git", "-C", str(tmp_path), "-c", "commit.gpgsign=false", *args], - capture_output=True, text=True, check=True, timeout=30, - ) here = sys.modules[__name__] monkeypatch.setattr(here, "_PKG", pkg) - monkeypatch.setattr(here, "_REPO", tmp_path) monkeypatch.setattr(here, "require_sibling", lambda name: tmp_path) monkeypatch.setattr(here, "_registry_current", lambda repo: { - "meta": {"version": "0.0.0-fixture"}, - "connection": {"APPWRITE_ENDPOINT": {"class": "connection", "value": "e"}}, - "target": {coordinate: {"class": "target", "value": planted}}, + "target": {"APPWRITE_UNFAO_BUCKET_ID": {"class": "target", "value": planted}}, }) with pytest.raises(AssertionError) as caught: test_no_coordinate_value_is_copied_into_this_repo() message = str(caught.value) - # Named files, not a count: pytest's assertion rewriting echoes the message, so - # counting substrings of it counts each finding twice. Naming both planted files is - # what stops this passing vacuously — a scan that finds nothing reports nothing, and - # "no value in the message" is trivially true of an empty message. - for half in ("leaky.py", "README.md"): - assert half in message, ( - f"the {half} half of the scan did not report the planted copy: {message}. " - "C-89 was one branch being taught a rule the other was not, so both must fire." - ) - assert coordinate in message, ( - f"the report must name the coordinate so a maintainer can find it: {message}" + assert "leaky.py" in message, f"the planted copy was not reported at all: {message}" + assert "APPWRITE_UNFAO_BUCKET_ID" in message, f"the coordinate is not named: {message}" + assert planted not in message, ( + "THE SCAN PUBLISHED THE VALUE IT EXISTS TO HIDE. This repository is public and its " + "CI logs are world-readable and cannot be redacted afterwards." ) - # Every channel pytest publishes, not just the assertion text. A debug `print` left - # beside the append is an ordinary accident, and pytest renders captured stdout and - # captured logs in the same report as the failure — so "the message is clean" is only - # part of "the run is clean". - captured = capsys.readouterr() - for channel, text in ( - ("the assertion message", message), - ("captured stdout", captured.out), - ("captured stderr", captured.err), - ("captured logs", caplog.text), - ): - assert not _carries(text, planted), ( - f"THE SCAN PUBLISHED THE VALUE IT EXISTS TO HIDE, via {channel}. This " - "repository is public, its CI logs are world-readable, and they cannot be " - "redacted afterwards. However the finding was built and wherever it was " - "written, no recoverable form of the value may leave this test." - ) - - -def _carries(text: str, value: str) -> bool: - """Is ``value`` recoverable from ``text`` — not merely present in it? - - Equality and ``in`` are too weak for a no-print rule. An adversarial review defeated a - substring check with ``value[:12]`` and again with ``value[::-1]``, both of which a - reader recovers instantly. So this asks the question that matters: does any eight- - character window of the value, forwards or backwards, appear? - - Eight is a judgement, not a law: long enough that ordinary English and file paths do - not collide with a coordinate, short enough that a truncation or a reversal is caught. - """ - windows = {value[i:i + 8] for i in range(len(value) - 7)} or {value} - return any(w in text or w[::-1] in text for w in windows) - - -def test_the_drift_report_never_carries_a_value_in_any_recoverable_form(): - """``_describe_changes`` is the drift check's reporter, and it had no test of its own. - - The rotation proof above exercises it, but only through one shape — a value present - on both sides — and with fixture strings shorter than every real coordinate. An - adversarial review walked through both gaps: a ``digest()`` that returns the raw value - only when it is longer than the fixture's, and the ``appeared``/``removed`` branches - that the both-sides fixture never reaches. Three leaks, all green. - - So this drives the reporter directly, with a value longer than any the registry - declares, through every branch that can produce text. - """ - # Longer than every value the registry declares, and opaque so no window of it - # can collide with the report's own wording. - value = "Xk4mQ8zRv2Ld7NpTb5Wc1Hy9Ja6Fg3Ss" - other = "Bn6tYp3Kq9Vx2Mw8Lz5Rd1Cf7Hj4Gu0Ee" - name = "APPWRITE_FIXTURE_COORDINATE" - - reports = [ - # rotated: present on both sides, different - _describe_changes({name: ("target", "target", value)}, - {name: ("target", "target", other)}, {name}), - # appeared: absent at the pin, valued now - _describe_changes({name: ("target", "target", _ABSENT)}, - {name: ("target", "target", value)}, {name}), - # removed: valued at the pin, absent now - _describe_changes({name: ("target", "target", value)}, - {name: ("target", "target", _ABSENT)}, {name}), - ] - - for shape, report in zip(("rotated", "appeared", "removed"), reports): - assert name in report, ( - f"the {shape} shape produced no report for {name}: {report}. A reporter that " - "says nothing about a change it was handed is the silent half of a drift check." - ) - text = report[name] - for secret in (value, other): - assert not _carries(text, secret), ( - f"the {shape} report carries a coordinate value in a recoverable form: " - f"{text!r}. This repository is public and its CI logs are world-readable. " - "Name the field that moved and digest the value; never emit it, and never " - "emit a prefix or a transform of it." - )