diff --git a/docs/ADRs/016_ci_read_access_to_private_siblings.md b/docs/ADRs/016_ci_read_access_to_private_siblings.md index ae5cf56..ee77a84 100644 --- a/docs/ADRs/016_ci_read_access_to_private_siblings.md +++ b/docs/ADRs/016_ci_read_access_to_private_siblings.md @@ -44,9 +44,11 @@ this repository makes about a different repository** is still true. The clearest example. Each partner's `appwrite_env.py` records which edition of the shared configuration registry it was verified against, by naming a commit in another repository. Tests open that repository at that commit and ask whether anything this -package reads has moved since — a rename, a reclassification, a rotated value, or a new -coordinate that has arrived. *(Until 2026-08-11 they asked a cruder question — whether the -edition **label** still matched — which fired on every upstream edit. See §7b's erratum.)* +package reads has moved since — a rename, a reclassification, a rotated value. *(Until +2026-08-11 they asked a cruder question — whether the edition **label** still matched — +which fired on every upstream edit. A second attempt also asked whether any coordinate had +**arrived**, which fired on other repositories' rows and was deleted on 2026-08-12. See +§7b's erratum.)* This is not hypothetical housekeeping. On 2026-08-05 that test failed — the registry had moved to a new edition while nobody here was looking. Two days earlier, the same family of @@ -248,9 +250,14 @@ So the two categories above become three: - **Reachability checks** read a pinned commit; movement is noise. - **Differential tripwires** read **both** — a pinned edition as the baseline, the sibling's `main` as the comparison — and fire when something *we declare* differs between - them, or when a coordinate arrives in a table we read. This is the shape the registry - check now has. It catches a rotation, which is invisible to the other two, and it is - silent through prose edits and version bumps. + them. This is the shape the registry check now has. It catches a rotation, which is + invisible to the other two, and it is silent through prose edits and version bumps. + + *(Erratum, 2026-08-12. This paragraph read "…or when a coordinate **arrives** in a table + we read" and called that the check's current shape. That half was deleted the same day: it + compared every row of every table this package depends on, so another repository's key + reddened this one. The cost of removing it — a coordinate issued **for** this package now + goes unnoticed until a human reads the registry — is carried in register C-90.)* *(Corrected twice on 2026-08-11, and the second time is the instructive one. The first implementation compared against the sibling's **working tree** — so a developer whose clone diff --git a/reports/technical_risk_register.md b/reports/technical_risk_register.md index f2c2f83..504a11c 100644 --- a/reports/technical_risk_register.md +++ b/reports/technical_risk_register.md @@ -334,27 +334,35 @@ Cross-refs: **C-57** (the scan's own entry and its history), **C-93** (why the a --- -### C-90: The replacement drift checks assert less than their docstrings say, and one of their mutation proofs is a tautology +### C-90: A mutation proof that cannot fail, and the untested function a module was extracted to create | 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. | +| Trigger | **Both remaining halves are proof defects, not runtime ones.** (a) Someone mutates `_unclassified_tables` or `_TABLE_ROLE` and believes the tautological proof covers it. (b) Someone changes `registry_current` — the reason `tests/seam_registry.py` exists — and the suite stays green. *(The original trigger, an unrelated coordinate arriving upstream, died with `arrived` in #245.)* | | Owner | This repository. | -| Location | `tests/test_env_declaration.py:732` (`arrived`), `:890` (the tautology), `tests/seam_registry.py:113` (`registry_current`). | +| Location | `tests/test_env_declaration.py` — `test_the_table_partition_would_catch_a_new_table_and_a_vanished_one` (the tautology); `tests/seam_registry.py::registry_current` (untested). Function names, not line numbers: this entry has cited stale ones before. | -**`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. +**~~`arrived` is not filtered by the names this package reads.~~ RESOLVED 2026-08-12 (#245) — deleted; see the mitigation below. Left visible because the reasoning it prompted is the entry's most useful part.** `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.** +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. That argues for table-granularity on arrival and row-granularity on change. **They cannot both stand.** + +**DECIDED 2026-08-12 (#245): the docstring won, and the cost is real.** The arrival half was deleted. A coordinate views-appwrite issues *for this package* — or a second `[contract.*]` row for a future partner such as views-productionapi — is now **silent** until a human reads the registry: no test, no run-time assert, nothing. `assert_env_declared` cannot see it, because it iterates the names this package already declares. That is the accepted price of not being reddened by every unrelated row, and it is recorded here rather than left to be discovered. *(The half's own defence — that `[contract.*]` "arrived exactly this way and nothing else here would have seen it" — was false at table granularity, where the partition check catches it, and true at row granularity, which is exactly the cost now accepted.)* **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. +**Partial mitigation 2026-08-12 (#245) — the false-alarm half is gone; the two proof defects are not.** + +`arrived` is **deleted**. Measured before deleting: each partner reads 13 of the 25 rows in the tables this package depends on, and 8 of those rows belong to no repository here — so the check subscribed this repository to another repo's changelog. Mutation-proven after: an unrelated API key and a third partner's contract row are silent; a rotation and a removal still fire. The docstring and the code now agree, and the stopping rule sits above the check. + +**Still open, and routed to #246**: the partition's mutation proof is a tautology (`base` is built from `_TABLE_ROLE`, so `_unclassified_tables(base)` is empty for every possible input), and `registry_current` — the function `tests/seam_registry.py` was extracted to create — has no test, so replacing it with a working-tree read leaves the suite at baseline. This entry stays open until both land. + 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. --- @@ -509,18 +517,64 @@ 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. +**⚠ CORRECTED 2026-08-12, and PARTLY RESOLVED the same day (#245).** The paragraph +beginning *"Partial mitigation 2026-08-11"* claimed the replacement checks "match on the +facts this repository actually declares". **Two of the three did not**, and an earlier +draft of this correction said one — the review that caught it is the reason this paragraph +is longer than it wants to be. + +**The first, now fixed.** The drift check's arrival half ran over every row of every table +this package depends on, with no filter for the names it reads. Measured on the live +registry: each partner reads **13 of 25** rows, and **6 belong to no repository here** — +three caller keys for other consumers, and three platform key slots still marked +`status = "planned"`, one of which names *"un_fao delivery"* among the identities it would +be issued for. C-90's original wording was "belong to no repository here"; an earlier draft +of this paragraph upgraded it to "other repositories entirely", which is the same +overstatement this paragraph exists to correct, two sentences from correcting it. The other two rows this package does not +read are its own delivery labels — both declare `producer = "views-postprocessing"` — and +they are covered by `tests/test_product.py`, not by this check. *An earlier draft of this +paragraph said "8 belong to no repository here", which upgraded a careful claim in C-90 +into a false one and propagated it to four places.* + +**And it never actually fired in anger.** Measured: **zero rows have arrived in the +depended-on tables since the pin** — the arrival half was green on every real edition it +ever saw, from its introduction on 2026-08-11 to its deletion on 2026-08-12. An earlier +draft said an upstream key "reddened this repository and blocked a release". That is a +mutation result written in the past tense. What is true: a mutation shows it *would* fire, +and it *would* block once C-81's required check lands — `protect_main` today carries +`deletion, non_fast_forward, pull_request` and no required status check, which is this +entry's own "latent rather than live" paragraph. + +**The arrival half is now deleted** (#245). The drift check fires on exactly one condition — +a row this partner declares differs between the pinned edition and the sibling's `main` — +and that stopping rule is written above the check itself. Its self-defence turned out to be +false: it claimed `[contract.*]` *"arrived exactly this way and nothing else here would have +seen it"*, but `[contract]` is a top-level **table**, caught by the partition check directly +above it. + +Mutation-proven both ways against the live registry: an unrelated API key and a third +partner's contract row are now **silent**; a rotation and a removal of a row this partner +reads still **fire**. A new test asserts the silence direction, so a third widening meets +an objection rather than a paragraph. + +*(That silence test needed two attempts, and the first is worth recording because it is +this entry's own disease. It called `_describe_changes` directly — the helper underneath +the check — and was worthless: re-adding the deleted arrival half to the real check left +the whole suite green, because that half never lived in the helper the test was asking. +The "mutation proof" offered for it was invalid too: it mutated the test rather than the +code, which proves nothing. It now drives the real check through monkeypatched readers, +and re-adding the arrival half fails it. **A guard must be pointed at the thing it claims +to guard**, which is the sentence this whole entry keeps re-learning.)* + +**The second inaccuracy stands, and is this entry's remaining rate risk.** *"Every +top-level table upstream must be classified here"* fires on any new table regardless of +whether this repository declares anything about it — and **it has already fired for exactly +that reason**: `[edition]` arrived at v1.6.0, an edition the registry's own `[meta]` calls +*"additive and opt-in; obliges nobody"*, and it reddened this repository. That event is +recorded **earlier** in this entry — the partition "went red on it unprompted", "which is +the whole reason that check is directional" — and that record is true of the *detection* +and silent about the *cost*. Adopting `[edition].obliges_consumers` is the deferral that +would fix it, and its trigger is below. --- diff --git a/tests/test_env_declaration.py b/tests/test_env_declaration.py index f6f3bd3..2672ed6 100644 --- a/tests/test_env_declaration.py +++ b/tests/test_env_declaration.py @@ -736,6 +736,30 @@ def test_nothing_this_repo_reads_has_changed_since_the_pin(partner): **Silent through:** prose edits, ``[meta]`` bumps, and rows belonging to anyone else. Measured across the real v1.4.4 -> v1.5.2 window (three editions, one week): green. + + **The stopping rule, because this check has been widened once already.** It fires on + exactly one condition: *a row named in this partner's* ``expected_class`` *differs + between the pinned edition and the sibling's* ``main``. + + Any proposal to widen it must state two things: which delivery failure it prevents, + **and its false-alarm surface measured against the live registry**. If that surface + includes rows this package does not read, it is refused. (An earlier version of this + rule asked instead whether ``assert_env_declared`` already prevented the failure at + run time. That rule is wrong in both directions: an unadopted upstream coordinate is + not prevented by it, so arrival detection would pass — and a removed coordinate *is* + caught by it, so the removal detection this check does perform would fail.) + + An arrival half failed the surface test and was deleted (#245): it compared every row + of every depended-on table, and measured, each partner reads 13 of 25 while 6 belong + to no repository here. It defended itself by claiming ``[contract.*]`` + "arrived exactly this way and nothing else here would have seen it" — false at table + granularity, where ``test_every_table_in_the_registry_is_classified_here`` catches it. + + **What that deletion costs, stated rather than implied.** A coordinate views-appwrite + issues *for this package* is now silent until a human reads the registry — no test, no + run-time assert, nothing. So is a second ``[contract.*]`` row for a future partner. + That is the accepted price of not being reddened by every unrelated row, and it is + carried in register C-90 rather than here. """ repo = require_sibling("views-appwrite") module, _, expected_class = _PARTNER_ENV[partner] @@ -745,19 +769,6 @@ def test_nothing_this_repo_reads_has_changed_since_the_pin(partner): names = set(expected_class) then, now = _projection(pinned, names), _projection(current, names) - arrived = sorted( - set(_rows(current, _TABLES_WE_DEPEND_ON)) - set(_rows(pinned, _TABLES_WE_DEPEND_ON)) - ) - assert not arrived, ( - f"[{partner}] the registry gained coordinate(s) {arrived} in a table this package " - f"reads, since the edition it was verified against " - f"(v{module.SEAM_CONTRACT_VERSION}). Decide whether this package must adopt them " - "— that is a human read, which is why this reports rather than guesses — then " - "move SEAM_CONTRACT_VERSION and SEAM_CONTRACT_COMMIT together. This is the half " - "of the deleted edition check that was worth keeping: `[contract.*]` arrived " - "exactly this way, and nothing else here would have seen it." - ) - changed = _describe_changes(then, now, names) assert not changed, ( f"[{partner}] the registry changed something this package reads, since the " @@ -964,6 +975,84 @@ def edition(value: str) -> dict: ) +@pytest.mark.parametrize("partner", _PARTNERS) +def test_the_drift_check_is_silent_on_a_row_this_partner_does_not_read(partner, monkeypatch): + """The stopping rule above, as a check rather than a paragraph. + + Paired with ``test_the_drift_check_fires_when_a_row_this_partner_reads_rotates``, + which is the positive direction through the same harness. Neither is optional: alone, + this one is a check that can only pass, and alone the twin says nothing about what the + check ignores. + + An earlier version of this docstring vouched for the two proofs either side — "rotation + fires, a rename fires" — and neither is about this check. One calls the helper + underneath it; the other belongs to the class check. That is this file's own recurring + defect (C-80, C-82), so the vouching is gone and the twin is real. + + **It drives the real check**, through the real registry readers, rather than calling + the projection helpers underneath it. A first version called ``_describe_changes`` + directly and was worthless: re-adding the deleted arrival half to the check left the + whole suite green, because that half never lived in the helper this was asking. A + guard has to be pointed at the thing it claims to guard. + """ + expected_class = _PARTNER_ENV[partner][2] + targets = sorted(n for n, cls in expected_class.items() if cls == "target") + assert targets, ( + f"[{partner}] declares no coordinate of class 'target', so this fixture has " + "nothing to file under [target] and would prove nothing. (An earlier version " + "asserted that the chosen name's class was 'target' — true by construction, " + "since the name was selected by that condition. A proof that cannot fail is the " + "defect this entry's own C-90 records.)" + ) + mine = targets[0] + + pinned = { + "meta": {"version": "0.0.0-fixture"}, + "connection": {"APPWRITE_ENDPOINT": {"class": "connection", "value": "e"}}, + "target": {mine: {"class": "target", "value": "unchanged"}}, + } + # The same registry, plus one coordinate belonging to another repository. + current = {**pinned, "secret": {"SOMEBODY_ELSES_API_KEY": {"class": "secret"}}} + + here = sys.modules[__name__] + monkeypatch.setattr(here, "require_sibling", lambda name: Path("/nonexistent")) + monkeypatch.setattr(here, "_registry_at", lambda repo, ref: pinned) + monkeypatch.setattr(here, "_registry_current", lambda repo: current) + + test_nothing_this_repo_reads_has_changed_since_the_pin(partner) + + +@pytest.mark.parametrize("partner", _PARTNERS) +def test_the_drift_check_fires_when_a_row_this_partner_reads_rotates(partner, monkeypatch): + """The positive twin of the silence guard, through the same real check. + + Without this, the check's entire ``assert not changed`` could be deleted and the suite + would stay green — measured. Its two apparent proofs are not about it: the rotation + proof above calls ``_describe_changes``, the helper underneath, and the rename proof + calls ``_declared_classes``, which belongs to a different check altogether. So the + silence guard was the only test driving this one, and a guard that can only pass is + the same defect as a guard pointed at the wrong subject. + """ + targets = sorted(n for n, cls in _PARTNER_ENV[partner][2].items() if cls == "target") + assert targets, f"[{partner}] declares no 'target' coordinate to rotate" + mine = targets[0] + + pinned = { + "meta": {"version": "0.0.0-fixture"}, + "connection": {"APPWRITE_ENDPOINT": {"class": "connection", "value": "e"}}, + "target": {mine: {"class": "target", "value": "at-the-pin"}}, + } + rotated = {**pinned, "target": {mine: {"class": "target", "value": "after-rotation"}}} + + here = sys.modules[__name__] + monkeypatch.setattr(here, "require_sibling", lambda name: Path("/nonexistent")) + monkeypatch.setattr(here, "_registry_at", lambda repo, ref: pinned) + monkeypatch.setattr(here, "_registry_current", lambda repo: rotated) + + with pytest.raises(AssertionError, match=mine): + test_nothing_this_repo_reads_has_changed_since_the_pin(partner) + + @pytest.mark.parametrize("partner", _PARTNERS) def test_the_drift_check_would_catch_a_rename(partner): """A gated test that cannot fail is decoration — so prove this one bites in CI.