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 e45d947..83dcf4d 100644 --- a/docs/ADRs/016_ci_read_access_to_private_siblings.md +++ b/docs/ADRs/016_ci_read_access_to_private_siblings.md @@ -1,206 +1,249 @@ -# ADR-016: Cross-repository checks in CI, and how siblings declare their visibility +# ADR-016: Which sibling repositories CI downloads **Status:** Accepted **Date:** 2026-08-10 **Decider:** Simon Polichinel von der Maase -**Arises from:** register **C-46**, **C-57**, **C-81**, which all carried the same standing -residual: the cross-repository drift detectors ran only on a maintainer's laptop -**Related:** [ADR-003](003_authority_of_declarations_over_inference.md) (declarations over -inference), [ADR-014](014_claims_and_the_guards_that_carry_them.md) §2 (a guard nobody has +**Scope:** repositories CI **can** download — that is, public ones. What to do when a +repository **cannot** be downloaded is a different decision, taken in +[ADR-017](017_facts_across_a_private_boundary.md) and accepted alongside this one. +**Related:** [ADR-014](014_claims_and_the_guards_that_carry_them.md) §2 (a guard nobody has watched fail is decoration) and §4 (a deferral names a trigger and an owner) --- -## Context +## The decision in three sentences -### §1 Some of this repository's tests are about other repositories +A few of this repository's tests read **other** repositories to check that things we say +about them are still true. Those tests only work when the other repository is on disk, so +they ran on a developer's laptop and skipped in CI. **CI now downloads the repositories +those tests need**, and a test fails if CI stops downloading one that the code says it +should. -Most tests here check our own code. A few check something else: whether **claims this code -makes about other repositories are still true**. +*Throughout, a "sibling" is another repository in the views-platform organisation that sits +beside this one in a developer's folder — never a dependency we install.* -The clearest case is the shared Appwrite coordinate registry. Each partner's -`appwrite_env.py` declares, in two constants, which edition of that registry it was written -against. That is a claim about a *different repository*. A test opens that repository, reads -the real edition, and compares. +## What this document does not cover -This is not theoretical maintenance. On 2026-08-05 the check failed: the registry had moved -from v1.4.1 to v1.4.4 while nobody here was looking. Two days earlier, the same family of -checks fired **twice in one day**. A claim about another repository goes stale at *that* -repository's pace, and nothing in this repository's own code can notice. +Downloading a repository requires being able to read it, so **everything here applies only +to repositories that are public.** A private one cannot be downloaded by our CI at all, +and no amount of workflow configuration changes that. -### §2 They only run where the other repository is on disk +That is not a gap in this decision; it is a different problem with a different answer, and +that answer is [ADR-017](017_facts_across_a_private_boundary.md). This document stops at +the boundary and says so, rather than implying a coverage it does not have. -`tests/conftest.py::sibling_repo` resolves a sibling by a declared environment variable and -falls back to the conventional `../` directory. When neither resolves it returns -`None` and the test **skips** — correctly, because on a laptop a missing sibling is normal. +--- -A GitHub Actions job gets a checkout of **one** repository. So in CI they skipped, and -**continuous integration verified strictly less than a laptop did, precisely on the checks -that cross a repository boundary**. +## Context -### §3 Why they stayed dark, which is the more interesting failure +### §1 What these tests actually do -Four siblings, and until now the decision about which ones CI could fetch lived in a -**comment** in the workflow file. That comment said `views-appwrite` was private. +Most tests here check our own code. A few check something different: whether a **claim +this repository makes about a different repository** is still true. -It is not. It was made public on **2026-08-08** — a deliberate, recorded act in that -repository (`views-appwrite@9d80b75`, *"docs: record going public"*). The comment did not -change with it, so seven checks went on skipping in CI for no reason whatsoever. +The clearest example. Each partner's `appwrite_env.py` says, in effect, *"we were written +against edition 1.4.4 of the shared configuration registry."* That registry lives in +another repository. A test opens that repository and checks the edition is still 1.4.4. -The first draft of this very document, written on 2026-08-09, proposed issuing a credential -to reach those seven — **one day after the fact that justified it had ceased to be true**. -That is not an embarrassing footnote to the decision below; it *is* the argument for it. A -fact about another repository, recorded in prose, with no date and nothing able to check it, -will be wrong and nobody will find out. +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 +checks fired **twice in one day**. ---- +### §2 Why they were not running -## Decision +A test like that needs the other repository present. On a laptop all the platform +repositories sit in one folder, so it runs. A GitHub Actions job gets **one** repository, +so it skipped. -### §4 Every sibling is declared, and the declaration separates fact from decision +The result: **CI verified less than a laptop did, exactly on the checks that span two +repositories** — the ones no single repository can replace. -`tests/conftest.py` declares each sibling repository with four fields: +### §3 Why nobody noticed for so long -| field | kind | meaning | -|---|---|---| -| `env` | declaration | the environment variable that overrides its location | -| `public` | **fact** | its visibility on GitHub — not ours to decide | -| `public_checked` | **fact** | the ISO date that visibility was last verified | -| `ci_checkout` | **decision** | whether CI fetches it | -| `note` | reasoning | why not, required whenever `ci_checkout` is false | +Which repositories CI could download was recorded in a **comment** in the workflow file. +That comment said `views-appwrite` was private, so its seven checks were assumed to need +an access credential nobody had issued. -Separating `public` from `ci_checkout` is the point. One is a fact about the world, the -other is a choice we make; §3 is what happens when a single sentence tries to be both. And -`public_checked` is not decoration — every measured claim in this repository carries a date, -and a bare boolean is a fact with no expiry. +It had gone public on **2026-08-08**. The comment had not changed with it. Seven checks +stayed switched off for no reason at all, and the first draft of this very document +proposed issuing a credential to reach them — written one day after the thing that +justified it stopped being true. -### §5 CI fetches exactly what the declaration says, and a test enforces the agreement +**That is the whole lesson.** A fact about another repository, written in prose, with +nothing able to check it, will eventually be wrong and nobody will find out. -`.github/workflows/run_pytest.yml` checks out every sibling declared `ci_checkout=True`, into -`_siblings/`, pointed at by the declared environment variable. - -`tests/test_ci_sibling_coverage.py` fails when the workflow and the declaration disagree **in -either direction** — a declared checkout that is missing, or a checkout nobody declared. Each -rule is a pure function of `(workflow, siblings)` so it can be run against a synthetic broken -world; a rule that can only be demonstrated by editing CI is one nobody ever watches fail. - -Today that means `views-appwrite` and `views-crafdapi` are fetched. `views-datafactory` is -public but is **not** fetched: its checks need raw GAUL parquet files that are not in its git -repository, so checking it out replaces an honest skip with a crash — measured, tried and -reverted. `views-faoapi` is the one genuinely private sibling. - -### §6 A missing sibling that CI declared must turn the build red +--- -Skipping is right on a laptop and wrong in CI once we have declared the repository should be -there. If a checkout silently fails, the checks skip, the build stays green, and we are back -to §2's blindness while looking fixed. +## Decision -The mechanism already existed: -`tests/test_env_declaration.py::test_no_sibling_override_points_at_a_missing_path` fails when -a declared variable is set but resolves to nothing. Because the workflow sets those variables -unconditionally, a failed checkout is a **red build**. Naming it here so nobody deletes it -believing it to be tidiness. +### §4 The list of siblings is code, not a comment -That guard had a hole, closed in the same change: it treated a variable set to the **empty -string** as unset, which is exactly what a YAML interpolation resolving to nothing produces — -so the likeliest CI misconfiguration was the one case it could not see. (`Path("").exists()` -is `True`, so the obvious fix makes it worse.) +`tests/conftest.py` holds one entry per sibling repository, and each entry says: -### §7 `public` is verified by CI doing it, not by a test — and two rules keep that true +- **`env`** — the environment variable that overrides where to find it; +- **`ci_checkout`** — whether CI downloads it; +- **`note`** — why not, when the answer is no. -No test in this suite touches the network. That is a deliberate convention, not an oversight: -what these tests locate is a working copy on disk. +That is all. It replaces a comment with something a test can read. -So `public` is verified by **the checkout itself**. The default `GITHUB_TOKEN` is scoped to -this repository, so a tokenless fetch of a sibling declared public *fails the build* if it is -actually private. +### §5 CI downloads exactly what that list says, and a test enforces it -Two rules exist solely to keep that argument load-bearing, and both look like fussiness until -you see what they protect: +The workflow downloads every sibling marked `ci_checkout=True`. +`tests/test_ci_sibling_coverage.py` fails if the workflow and the list disagree — in +either direction: a repository the list expects and CI does not fetch, or one CI fetches +that the list never mentions. -- a checkout of a **public** sibling must **not** pass a credential — add one to dodge a rate - limit and the field silently becomes an unchecked claim; -- no sibling checkout may carry `continue-on-error` — one key and a failed fetch stops - failing the build. +Six rules, and each exists because of something that has actually gone wrong: -**The reverse case is not detected.** A private sibling that quietly becomes public will go on -being declared private, and nobody here will notice. That is stated rather than papered over; -it costs a stale `note` and an unnecessary skip, not a wrong result. +| | rule | the incident behind it | +|---|---|---| +| G1 | a listed repository is downloaded, and the tests are pointed at it | the tests looked in the wrong place and skipped silently | +| G2 | anything CI downloads appears in the list | the list is the thing people read; CI is not | +| G4 | a repository we skip says why, naming a record | silent non-coverage reads as "nothing to see here" | +| G5 | a download step may not be marked "ignore failures" | one setting and a failed download stops failing the build | +| G6 | downloads land in `_siblings/` | a sibling put elsewhere made the linter report 745 errors in someone else's code | +| G7 | download the sibling's `main` branch | without it you get *their* default branch — which for `views-appwrite` is `development`, not `main` | + +Each rule is a plain function, so each is also run against a deliberately broken example +to prove it objects. A rule only ever tried against a correct file is a rule nobody has +watched fail. + +**One of these downloads is temporary, and it is worth knowing which.** `views-crafdapi` is +fetched for exactly one test. [ADR-017](017_facts_across_a_private_boundary.md) §7 replaces +that test with one that reads a public declaration instead — at which point this download +buys nothing and goes. That has been decided and is not yet done; it waits on the +declaration landing in the registry. That is a decision this document cannot make on its own, which +is why it is recorded there and cross-referenced here. + +### §6 A repository CI expects but cannot find turns the build red + +Skipping is right on a laptop, where a missing sibling is normal. It is wrong in CI once +we have said the repository should be there — a silent skip puts us back in §2 while +looking fixed. So CI names the repositories explicitly, and a missing one is a failure. + +That guard existed already +(`tests/test_env_declaration.py::test_no_sibling_override_points_at_a_missing_path`) and +had a hole, closed here: it treated a variable set to an **empty string** as unset, which +is exactly what a mis-typed CI setting produces. So the likeliest misconfiguration was the +one case it could not see. + +### §7 A broken sibling can block merging here, and that is accepted + +This is the real cost, and it should not be buried. CI now depends on two other +repositories. If one of them changes in a way that fails a check — say the configuration +registry moves again — **this repository's builds go red and merges are blocked until +someone updates the pin.** Since merging to `main` here *is* the release to FAO, that +matters. + +It is accepted for three reasons: + +1. The problem being fixed was that these checks were **invisible**. A check that reports + but cannot block is invisible again, just more politely. +2. A red build in that situation is *correct*. It says "re-pin before you ship", and the + fix is minutes. +3. There is an escape. The maintainer administers this repository and can merge over a + failing check when something genuinely urgent is blocked. + +This overrides an earlier internal recommendation not to couple per-pull-request CI to +another repository at all. That recommendation's stated objection was coupling to another +repository's *default branch* — which G7 removes, by naming `main` explicitly instead of +accepting whatever default the other repository happens to be set to. The coupling that +remains is real, and is the trade described above. + +### §8 One repository stays out, and why that is not this document's problem + +`views-faoapi` is private. Our CI cannot download it, so the one check that reads it does +not run here. + +**No credential is issued to work around that**, and that is a decision rather than an +omission. Where that question *is* decided is +[ADR-017](017_facts_across_a_private_boundary.md): the fact gets declared somewhere public +that both sides can read, so that neither needs access to the other. + +**That decision is accepted but not yet implemented** — it depends on a declaration being +added to the platform registry, which is another repository's work. Until then the affected +check runs on a maintainer's machine and not in CI, and the sibling's `note` says so. + +Until that declaration exists, the affected check runs on a maintainer's machine and not +in CI. That is stated in the sibling's `note`, which rule G4 requires and which must name +the record that owns it. -### §8 The credential for the one private sibling is deferred +--- -`views-faoapi` is private, and reaching it needs a credential. That is **not** done here. +## Consequences -The cost/benefit is thin: it is **one** test. It is, admittedly, the most valuable single one -— it checks that our delivery is filed under the name the consumer actually filters for, and -that failure produces *no error anywhere*: the upload succeeds, storage is paid for, and the -consumer's endpoint is simply empty. But one test does not justify a credential tied to one -person, with an expiry somebody must track, while a better answer is pending. +**What this buys.** Seven cross-repository checks move from *"run when someone happens to +run them"* to *"run on every change"*. Two of them fired in earnest the week this was +written. Separately, the scan that refuses configuration **values** in this public +repository's documentation now runs on every pull request rather than only on a +maintainer's machine — worth having, since the README carried four such values once. -**The better answer being pursued:** asking FAO to consent to that repository being made -public, which removes the need entirely. An audit of its full history found no credentials of -any kind and no partner staff email addresses; it already carries an MIT licence. Two items -remain open — internal storage identifiers appear in 21 tracked files, and whether GAUL 2024's -terms permit redistribution. +**What it costs.** §7: another repository can block merging here. And the list of siblings +is now something a contributor must keep in step with the workflow, enforced by tests they +may not have read. -**Named trigger (ADR-014 §4).** Issue the credential when **either**: FAO declines, or a -**second** private sibling appears. **Owner:** the maintainer. Should it ever be issued, the -scope floor is one repository, read-only, with a deliberately chosen expiry — the default of -30 days would put this repository back in the dark within a month, with a green build -throughout. +**Where this will go wrong first.** Someone debugging a red build marks a download step +"ignore failures", or moves it out of `_siblings/`. Both are rules for that reason. --- -## Consequences +## What was tried and removed + +An earlier version of this design also recorded, for each sibling, whether it was +**public** and the date that was last checked — with a rule pairing visibility against +whether the download used a credential. -**What this buys.** Seven cross-repository checks move from *"run when a maintainer happens to -run them"* to *"run on every change"*. Two of them fired in anger in the week before this was -written. One test also changes character: the scan refusing registry **values** in this public -repository's markdown now runs on every pull request rather than only on a maintainer's -machine — worth having, since README.md carried four such values once already. +A review found the pair circular. The `public` field was read by exactly one rule, and +that rule existed to protect the `public` field's verifiability. -**What it costs.** CI now depends on two other repositories being fetchable, so an outage or a -visibility change there turns this repository's build red. That is the intended trade: a red -build is the honest signal, and §6 exists to make sure it is what happens. +**This is why §5's table runs G1, G2, G4, G5, G6, G7 with two numbers missing.** The +removed rules were **G3** (visibility must match whether a credential was used) and **G8** +(the date must be a plausible one). The remaining rules keep their original names rather +than being renumbered, so that anything written about "G6" still means G6. Nothing else consulted +either. Both were deleted on 2026-08-10 and no behaviour changed. The date field was worse +than useless: nothing could confirm the check had happened, so it manufactured confidence +rather than recording a fact. -**Where this will go wrong first.** Somebody debugging a red CI adds `continue-on-error` to a -sibling checkout, or a `token:` to make a rate limit go away. Either quietly dismantles §7. -Both are rules in `test_ci_sibling_coverage.py` for that reason. +Visibility now lives in a sibling's `note` — prose, where it belongs, because no test here +can verify it in any case. What still holds without the flag is simpler and needs no +field: a repository CI cannot read fails to download, and G5 keeps that failure loud. -**Second most likely.** A sibling checkout is added to a different workflow file and escapes a -rule scoped to `run_pytest.yml`. The declaration check scans every workflow, not one. +Recorded because the removed design is more tempting than it looks, and because this +document once argued for it. --- ## Alternatives considered -**Leave it, and rely on running the suite by hand.** The status quo, and what registers C-46 -and C-57 carried as an open residual for weeks. It works exactly as well as one person's -habits, which is not a property a safety check should have. +**Leave it and rely on running the suite by hand.** The status quo for weeks. It works +exactly as well as one person's habits, which is not a property a safety check should have. -**Vendor the facts instead of reading them.** Copy the registry edition into this repository -and check the copy. Rejected outright: copies of that registry were the platform's original -failure, and the standing rule is that it is referenced by pinned URL and never copied. A test -reading a local copy compares a thing to itself. +**Copy the facts here instead of reading them.** Rejected outright: copies of that +configuration registry were the platform's original failure, and the standing rule is that +it is referenced and never copied. A test reading a local copy compares a thing to itself. -**Check `public` against the GitHub API from a test.** Rejected. It would be the only network -call in the suite, would need a credential to answer for private repositories — the very thing -under discussion — and would be flaky in exactly the conditions where a green build matters. -Verification-by-doing (§7) is weaker but honest, and its limits are written down. +**Ask GitHub whether a repository is public, from a test.** Rejected. It would be the only +network call in the suite, would need a credential to answer for private repositories — +the very thing in question — and would be unreliable exactly when a green build matters. -**Issue the credential now and fetch all four siblings.** Rejected on the arithmetic in §8: it -buys one test, and a decision that may make it unnecessary is outstanding. +**Issue an access credential and download the private one too.** Rejected, and the +reasoning is [ADR-017](017_facts_across_a_private_boundary.md) §9 rather than anything here: +a credential is the wrong shape of answer to a standing category, and there is a route that +needs no credential at all. --- -## Appendix — reproducing the measurement - -Counts drift as tests are added; the command does not. From the repository root: +## Appendix — checking it yourself ``` pytest -q -VIEWS_APPWRITE=/nonexistent pytest -q -rs ``` -The first, with the sibling repositories present, is the full suite. The second is what CI -would see without the checkout — and it must **fail**, not merely skip. That is §6. +with the sibling repositories present is the full suite. Then: + +``` +VIEWS_APPWRITE=/nonexistent pytest -q +``` + +is what CI would see without the download — and it must **fail**, not merely skip. That is +§6. diff --git a/docs/ADRs/017_facts_across_a_private_boundary.md b/docs/ADRs/017_facts_across_a_private_boundary.md new file mode 100644 index 0000000..e7d8bb0 --- /dev/null +++ b/docs/ADRs/017_facts_across_a_private_boundary.md @@ -0,0 +1,294 @@ +# ADR-017: Facts shared with a repository we cannot read + +**Status:** Accepted +**Date:** 2026-08-10 +**Decider:** Simon Polichinel von der Maase +**Scope:** what this repository does when it depends on a fact held in a repository it +cannot download — today because that repository is private, and in principle for any +reason. The companion decision, [ADR-016](016_ci_read_access_to_private_siblings.md), +covers repositories we *can* download. +**Related:** [ADR-003](003_authority_of_declarations_over_inference.md) (declarations over +inference), [ADR-013](013_sampled_forecast_wire_contract.md) §4.1a (the field in question), +[ADR-014](014_claims_and_the_guards_that_carry_them.md) §1 and §4 + +--- + +## The decision in one sentence + +**A fact shared across a boundary we cannot see through is declared in a public place both +sides read, and neither side reads the other's source code.** + +--- + +## Context + +### §1 The failure this is about + +When this repository delivers forecasts, it uploads each file with a label — a store +document field called `name`. The consuming API filters every one of its queries on that +label. + +If our label and their filter ever stop matching: + +- the upload **succeeds**; +- the file is stored, and paid for; +- the consumer's endpoint returns **empty**; +- **nothing, anywhere, raises an error.** + +ADR-013 §4.1a states it plainly: a document uploaded under the wrong name is *"invisible to +the consumer, not merely degraded."* Every system involved reports success while the +partner receives nothing. + +This is the failure mode that most deserves an automatic check, precisely because no human +or machine downstream will notice it. + +### §2 Who owns the label, which is not what you would guess + +ADR-013 §4.1a also says: the name is **"Config-owned by views-faoapi; changing it is a +contract amendment, not a deploy detail."** + +So the consuming API owns the value. This repository holds a **copy** of it — the constant +`CONSUMER_DOCUMENT_NAME` in each partner's `product.py`, which is what we actually write +onto every upload. + +That direction matters for everything below. The question is not *"can we see into their +repository"*. The question is **"they own a fact, we keep a copy, and how do we know the +copy is still right?"** + +**One honest limit on that claim.** ADR-013 states the ownership only for the FAO API — its +table names *"the literal faoapi model name"*, and the CRAF'd partner arrived later and is +not mentioned there at all. So consumer-ownership is written down for one partner and +merely assumed for the other. §7 treats them the same, which is almost certainly right, but +"almost certainly" is not a citation. **Settling that is part of the registry work**: a +declaration naming both partners makes the ownership explicit for both, which is a second, +smaller reason to do it. + +### §3 How we check it today, and why that stops working + +The existing check opens the consumer's source, finds the name their API filters on, and +compares it with our copy. + +That works when the consumer's code is on the same machine — true on a maintainer's laptop, +where all the platform repositories sit in one folder. It is impossible in continuous +integration when the consumer is private, because a CI job cannot download a private +repository without a credential. + +So the check runs when someone happens to run it, and not on the change that would break +it. + +### §4 Why this is a standing category and not a one-off + +Two consuming APIs exist. `views-crafdapi` is public; `views-faoapi` is private. Across the +whole organisation, **15 of 33 repositories are private** — private is ordinary, not +exceptional. + +The maintainer's instruction is explicit: `views-faoapi` may or may not become public one +day, but **there will be one or more private APIs at any given time**. So this document +must decide for the category, not for today's instance. + +--- + +## Decision + +### §5 The rule + +> **A fact shared across a public/private boundary is declared in the public contract +> surface. Each side verifies itself against that declaration. Neither side reads the +> other's source code.** + +Concretely, for the delivery label: + +1. The value is declared once, in the platform's public coordinate registry + (`views-appwrite`, `docs/ADRs/platform/coordinate_registry.toml`). +2. **This repository checks its copy against that declaration.** No access to the consumer + is required, so the check runs in CI on the change that could break it. +3. **The consuming API checks its own code against the same declaration.** It needs no + access to us either — this repository is public — so that half is free for it too. + +**None of those three is in place yet**, and the present tense above describes the decided +end state rather than today's behaviour. Today the check still reads the consumer's source +and still skips in CI. The order the three land in, and what is blocked on what, is +Appendix B. Said here because a decision record that reads as a description of the code is +how this repository has repeatedly ended up believing work was done. + +**"But you rejected copying."** [ADR-016](016_ci_read_access_to_private_siblings.md) turns +down keeping a local copy of the shared registry, on the grounds that a test reading a copy +compares a thing to itself. This is not that, and the difference is the whole point. + +We keep a copy of the label because we have to — it is written onto every upload, so it +must exist in our code. What ADR-016 forbids is a copy that is *also the thing we check +against*. Here the copy is checked against an authority held elsewhere, which is what makes +the check mean something. A copy nobody verifies is the failure; a copy verified against a +declared authority is just a value with a source. + +### §6 Why the coordinate registry, rather than somewhere new + +Four reasons, and the fourth is the one that makes this cheap: + +- **It is the right kind of place.** The registry already holds cross-repository facts + about the delivery seam, already distinguishes secret entries from non-secret ones, and + already records which repository consumes what. +- **It is public**, so a private consumer can read it without anyone granting anything. +- **It has a change process** and an amendment log, so a change to the label is a recorded + contract amendment — which is what ADR-013 §4.1a already says it should be. +- **This repository's CI already downloads and reads that file on every run.** Adding this + check costs no new machinery, no credential, and no new dependency. + +A label is not a secret. Knowing that documents are named `un_fao` grants no access to +anything; access is controlled by keys held elsewhere. So this belongs with the non-secret +coordinates and must never be placed among the secret entries. + +### §7 The rule applies to both partners, not only the private one + +`views-crafdapi` is public, so our check *can* read its source, and does today. It will +move to the registry anyway. + +The reason is not tidiness. If only the private case moves, the platform ends up with two +mechanisms doing one job — one reading a declaration, one reading source code — and a +future contributor has to know which partner uses which. One mechanism, applied uniformly, +is the point of having a rule at all. + +It also means this repository stops reading any consumer's source, which removes a +dependency on another repository's *file layout* — something we were never entitled to +depend on. + +**And it retires a download.** [ADR-016](016_ci_read_access_to_private_siblings.md) has CI +fetch `views-crafdapi`, and measurement shows that fetch serves **exactly one test** — this +one. Once the check reads the registry instead, that download buys nothing, and +`views-crafdapi` should be marked as not fetched, with a note saying why. Stated here +because a download that has quietly stopped earning its place is how the previous version +of all this went wrong, and because ADR-016 cannot know it: the decision that obsoletes it +is this one. + +### §8 What this does **not** verify, stated plainly + +This checks **our copy against the declaration**. It does **not** check the consumer's code +against the declaration. + +So if the registry says `un_fao`, and we write `un_fao`, and the consuming API quietly +starts filtering on something else — our check passes, and the delivery is still invisible. + +That half is genuinely the consumer's to verify, and cheap for it — it is the third item in §5's list, and needs no access to us because this repository is public. But it is +outside this repository's control, and there is a case where it may never exist: **a +private API operated by a third party**, who has no obligation to run any test of ours. + +**In that case the fact is not verifiable from here, and must be recorded as such** — an +accepted blind spot with a named owner, not a check that quietly covers nothing. Pretending +otherwise would be the exact failure ADR-014 §1 exists to prevent. + +### §9 What is deliberately not built + +- **No access credential** — no personal access token, no machine account, no GitHub App. + A credential solves one repository at a time and must be maintained forever, for a + category that grows. It is the wrong shape of answer to a standing problem, and it puts a + rotation obligation on one person. +- **No moving our checks into another repository's test suite.** It looks attractive — + the private side can read us for free — but it fails on its own extension case: a + third-party-operated API will not run our tests, and we would have no lever. A rule that + breaks on its second instance is not a rule. +- **No new file, service, or endpoint.** If a fact does not fit the existing registry, that + is information about the fact, not a reason to build a second surface. +- **No abstraction over "how to reach a private repository."** There is one such + repository, and the decision above is that we do not reach it. + +### §10 When to revisit + +- **A shared fact that genuinely cannot go in the registry** — for instance one that is + itself sensitive. Then this rule has met a case it does not cover, and the case is the + evidence needed to decide the next thing. +- **A third-party-operated private API becomes a real consumer**, making §8's blind spot + concrete rather than hypothetical. +- **The registry stops being public**, which would invalidate §6 entirely. + +**Owner:** the maintainer. None of these is a deadline; each is an event. + +--- + +## Consequences + +**What this buys.** The check that guards the invisible failure runs automatically, on the +change that could cause it, for every partner — with no credential, now or ever, however +many private APIs appear. Adding partner number three costs one registry row. + +**What it costs.** A change to a delivery label now requires editing a file in a third +repository. That is slower than editing a constant here, and deliberately so: ADR-013 +already calls this a contract amendment rather than a deploy detail, and this makes the +process match the words. + +**A dependency moves rather than disappearing.** We stop depending on the consumer's file +layout and start depending on the registry. That is a better dependency — declared, +versioned, public, with a change process — but it is not nothing, and if the registry +becomes unreliable this check inherits that. + +**Where this will go wrong first.** Someone changes the label in the consuming API and not +in the registry. Our check keeps passing, because our copy still matches the declaration, +and the delivery goes invisible exactly as before. **That is §8, and it is the whole +residual risk of this design.** It is why the consumer-side check is part of the rule and +not an optional extra. + +--- + +## Alternatives considered + +### Issue an access credential so our CI can read the private repository + +**Rejected.** It works, and it keeps both halves of the check in one place — genuinely its +strongest property. + +But the brief is a permanent category. A token is tied to a person, expires, and must be +re-scoped for each new private repository; a GitHub App is a substantially larger piece of +infrastructure to own. Either way the cost recurs per repository and per rotation, forever, +to verify a handful of short strings. And when it lapses, the checks skip and the build +stays green — the exact failure the whole of ADR-016 was written to end. + +### Move our check into the private repository's test suite + +**Rejected**, though it was the leading candidate for some time. + +The private repository can read this public one for free, so the check would cost no +credential and would fire in a real pipeline. Attractive, and correct today, when one +person maintains both. + +It fails on the case that defines the category. A private API operated by a **third party** +will not run a conformance test on our behalf, and we would have no way to require it. The +rule would then hold only for repositories we happen to operate — which is not a rule about +private APIs, it is a rule about our own repositories wearing a general-sounding name. + +### Accept the gap and check nothing + +**Rejected as the destination, though it is the honest description of where we are until +the registry entry lands.** The failure it leaves uncovered is the invisible one, and this +repository has already delivered an artifact nobody could find once. + +Worth stating because it is the fallback if the registry route stalls: the position would +then be *"this is not verified, here is the failure mode, here is who owns it"* — an +accepted blind spot, recorded. Never a check that appears to cover it and does not. + +--- + +## Appendix A — the facts behind §4 + +Measured 2026-08-10. Counts drift; the commands do not. + +``` +gh repo list views-platform --limit 60 --json name,visibility +``` +returned 33 repositories, 15 of them private. + +Consuming APIs: `views-crafdapi` public, `views-faoapi` private. `views-productionapi` and +`views-publicapi` are named in platform records but do not exist yet, so no default +visibility can be inferred for future APIs. + +## Appendix B — the work this decision requires, and in what order + +1. **views-appwrite** — declare the label for each partner in the coordinate registry. + Filed as views-appwrite#75. Nothing here can proceed before it. +2. **views-postprocessing** — switch the check to read the registry rather than the + consumer's source, for both partners. Ours, blocked on step 1. **In the same change**, + stop fetching `views-crafdapi` in CI: §7 shows that fetch then serves nothing, and + ADR-016's rules require a sibling that is not fetched to say why. +3. **views-faoapi** — verify its own served label against the declaration; §8's other half. + Filed as views-faoapi#379. + +Step 3 is not a precondition for steps 1 and 2, and the sequence is safe to interrupt: the +existing check keeps running on a maintainer's machine throughout. diff --git a/docs/ADRs/README.md b/docs/ADRs/README.md index d866caf..b8fd52b 100644 --- a/docs/ADRs/README.md +++ b/docs/ADRs/README.md @@ -94,7 +94,7 @@ These ADRs form the architectural constitution of the repository. demand and supply — under which it is revisited. Arises from #146 and the þing-02 ratification, which asked that the reasoning live here rather than in an issue. -- **ADR-016** — Cross-Repository Checks in CI, and How Siblings Declare Their Visibility +- **ADR-016** — Which Sibling Repositories CI Downloads A handful of tests here verify claims this repo makes about *other* repos — chiefly that the coordinate-registry edition we pinned is the one that exists. They need the sibling on disk, so they skipped in CI and ran only on a laptop. Each sibling is now declared with its @@ -104,6 +104,16 @@ These ADRs form the architectural constitution of the repository. for a dated declaration rather than prose. The credential for the one genuinely private sibling is deferred with a named trigger. +- **ADR-017** — Facts Shared With a Repository We Cannot Read + The delivery label this repository writes is owned by the consuming API and mirrored here; + if the two drift the upload succeeds, the file is stored, and the consumer's endpoint is + empty with no error anywhere. Verifying the mirror currently means reading the consumer's + source, which is impossible in CI when that consumer is private — and private consumer APIs + are a standing category, not a one-off. Decides that such a fact is declared in the public + coordinate registry and each side verifies itself against it, so neither reads the other's + source. No credential, for any number of private APIs. States plainly the half it does not + cover: the consumer's own code against its own declaration. + ADRs numbered 010 and above define: - Domain-specific decisions diff --git a/tests/conftest.py b/tests/conftest.py index f9ec51b..b29d9e3 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -65,27 +65,24 @@ class Sibling: """What this repository declares about one views-platform sibling (ADR-016). - Two of these fields are different kinds of thing and the distinction is the point. - ``public`` is a **fact about the world** that this repository does not control. - ``ci_checkout`` is a **decision** this repository makes. Conflating them is how the - workflow ended up asserting, in a comment, that ``views-appwrite`` was private for - days after it went public — and how seven checks stayed dark in CI for no reason. - - ``public_checked`` is the date the fact was last verified, and it is not decoration: - every measured claim in this repository carries one. A bare boolean is a fact with no - expiry, which is precisely what went wrong. - - **Keyword-only and frozen, deliberately.** Two adjacent booleans are a one-token slip - between "public, not checked out" and "private, checked out" — the second being the - combination rule G3 exists to forbid. A plain dict would let a missing ``ci_checkout`` - read as ``None``, silently exempting that sibling from every guard; that is the - failure mode this repository has registered more often than any other (C-47, C-57, - #211). Here the omission is a ``TypeError`` at import. + Two fields, and the second is the one that matters. ``ci_checkout`` says whether CI + fetches this repository; ``note`` says why not, whenever the answer is no. + + **An earlier version also carried ``public`` and ``public_checked``.** They were + removed on 2026-08-10 after a review found them circular: ``public`` was read by + exactly one rule, and that rule existed to protect ``public``'s verifiability. + Nothing else consulted either field, and deleting both changed no behaviour. Whether + a sibling is public is now simply part of ``note`` — prose, where it belongs, because + nothing here could verify it anyway. + + **Keyword-only and frozen.** A plain dict would let a missing ``ci_checkout`` read as + ``None`` and silently exempt that sibling from every rule; that is the failure this + repository has registered more often than any other. Here the omission is a + ``TypeError`` at import. **No validation in ``__post_init__``.** See ``broken_sibling_overrides`` below for what raising at import time costs: one typo became three collection errors and zero - tests run. The rules live in ``tests/test_ci_sibling_coverage.py``, where a violation - is one clean failure and the other four hundred tests still report. + tests run. The rules live in ``tests/test_ci_sibling_coverage.py``. """ #: The environment variable that overrides this sibling's location. Declared, never @@ -93,15 +90,10 @@ class Sibling: #: but a future sibling need not follow the pattern and guessing it would be the #: inference ADR-003 forbids. env: str - #: Visibility on GitHub — a fact about the world, not a decision of ours. - public: bool - #: ISO date ``public`` was last verified. See the class docstring. - public_checked: str - #: Whether CI checks this sibling out. A decision, and the reason for it belongs in - #: ``note`` whenever the answer is no. + #: Whether CI fetches this sibling. ci_checkout: bool - #: Why this sibling is not checked out. Required when ``ci_checkout`` is False, and - #: must name a record, so the non-coverage has an owner rather than a shrug. + #: Why not, when it is not fetched. Required in that case, and must name a record so + #: the non-coverage has an owner rather than a shrug. note: str = "" @@ -114,11 +106,9 @@ class Sibling: SIBLINGS = { "views-datafactory": Sibling( env="VIEWS_DATAFACTORY", - public=True, - public_checked="2026-08-10", ci_checkout=False, note=( - "public, but its checks need the producer's raw GAUL parquets " + "PUBLIC, but its checks need the producer's raw GAUL parquets " "(data/raw/gaul_admin/*.parquet), which are NOT in its git repository. " "Checking it out converts an honest skip into a FileNotFoundError — measured " "2026-08-03, tried and reverted. Closing this needs the data published " @@ -127,26 +117,23 @@ class Sibling: ), "views-appwrite": Sibling( env="VIEWS_APPWRITE", - public=True, - public_checked="2026-08-10", ci_checkout=True, ), "views-faoapi": Sibling( env="VIEWS_FAOAPI", - public=False, - public_checked="2026-08-10", ci_checkout=False, note=( - "the only private sibling. Checking it out needs a credential, which is an " + "PRIVATE — the only one, and that is why it is not fetched. Checking it " + "out needs a credential, which is an " "operator decision deferred pending a request to FAO to make the repository " "public. One check is dark meanwhile — the consumer-name pin, whose failure " - "mode is a delivery nobody can find. See ADR-016 and register C-81." + "mode is a delivery nobody can find. ADR-017 decides what happens instead " + "of a credential: the fact moves to the public coordinate registry, and " + "neither side reads the other. See also register C-81." ), ), "views-crafdapi": Sibling( env="VIEWS_CRAFDAPI", - public=True, - public_checked="2026-08-10", ci_checkout=True, ), } diff --git a/tests/test_ci_sibling_coverage.py b/tests/test_ci_sibling_coverage.py index ce8c31c..10b2785 100644 --- a/tests/test_ci_sibling_coverage.py +++ b/tests/test_ci_sibling_coverage.py @@ -10,14 +10,17 @@ once**, in `tests/conftest.py::SIBLINGS`, and this file fails when the workflow and that declaration disagree — in either direction. -**What verifies the `public` field, since no test here does.** Nothing in this suite -touches the network; that is a hard convention (`conftest`'s docstring: what is located -is a working copy on disk). `public` is verified by CI *doing it*: the default -`GITHUB_TOKEN` is scoped to this repository, so a tokenless checkout of a sibling -declared public fails the build if it is actually private. Rules G3 and G5 exist to keep -that true — one `token:` or one `continue-on-error:` and the claim silently stops being -checked. The reverse case (a private sibling quietly becoming public) is **not** detected -here, and ADR-016 §7 says so rather than implying coverage it does not have. +**Whether a sibling is public is not modelled here, and that is deliberate.** An earlier +version carried a `public` flag and a rule pairing it with the presence of a checkout +token. A review found the pair circular — the field was read by exactly one rule, and +that rule existed to protect the field's verifiability — so both were removed on +2026-08-10 without any change in behaviour. Visibility now lives in a sibling's `note`, +as prose, because nothing here could verify it in any case: no test in this suite touches +the network. + +What still holds without the flag: a checkout of a repository CI cannot read simply fails +the build, because the default `GITHUB_TOKEN` is scoped to this repository. Rule G5 keeps +that failure loud. **Every rule is a pure function of (workflow, siblings)**, so each can be run against a synthetic mutant rather than only against the real file. A guard that can only be @@ -28,7 +31,6 @@ from __future__ import annotations -import datetime as dt from pathlib import Path import pytest @@ -154,39 +156,6 @@ def _g2_every_checkout_is_declared( return problems -def _g3_tokens_match_visibility( - workflow: dict, siblings: dict[str, Sibling] -) -> list[str]: - """Private ⇒ must pass a token. Public ⇒ must NOT. - - The second half is the one that matters and the one that looks pointless. It is what - keeps ``public`` verified-by-doing: a tokenless checkout of a private repo fails the - build. Add a token to a public sibling — to dodge a rate limit, say — and the field - becomes an unchecked claim with nothing anywhere to catch it. - """ - problems = [] - for step in _sibling_steps(workflow): - name = _repo_name(step) - sibling = siblings.get(name) - if sibling is None: - continue # G2's business - has_token = "token" in step["with"] or "ssh-key" in step["with"] - if not sibling.public and not has_token: - problems.append( - f"{name}: declared private but checked out with no credential — the " - "step will fail. Either it is public now (update the declaration and its " - "date) or it needs a token." - ) - if sibling.public and has_token: - problems.append( - f"{name}: declared public but the checkout passes a credential. That " - "removes the only thing verifying the `public` field — a tokenless " - "checkout failing when a repo is private. Drop the token, or the claim " - "is unverified." - ) - return problems - - def _g4_non_coverage_is_explained( workflow: dict, siblings: dict[str, Sibling] ) -> list[str]: @@ -220,9 +189,10 @@ def _g5_no_step_swallows_its_own_failure( ) -> list[str]: """No sibling checkout may carry ``continue-on-error``. - One key, and the entire verification-by-doing argument becomes false: the checkout of - a repo that turned private fails, the build stays green, and the checks it enables - skip. It is also exactly the key someone reaches for while debugging a red CI. + One key and a failed fetch stops failing the build: the checks that sibling enables + go back to skipping, silently, on a green run. That is the state this whole file + exists to leave behind, and it is exactly the key someone reaches for while debugging + a red CI. """ return [ f"{_repo_name(step)}: sibling checkout carries continue-on-error, so a failed " @@ -266,58 +236,13 @@ def _g7_siblings_are_taken_from_main( ] -#: How far ahead of "today" a check date may sit before it is an error rather than a -#: clock difference. See `_g8_the_visibility_fact_carries_a_usable_date`. -_CLOCK_SKEW = dt.timedelta(days=1) - - -def _g8_the_visibility_fact_carries_a_usable_date( - workflow: dict, siblings: dict[str, Sibling] -) -> list[str]: - """``public_checked`` parses, and is not meaningfully in the future. - - **One day of slack, and it is not laziness — this rule failed CI on its first run.** - The dates were stamped from a maintainer's machine in CEST at 01:25 on 2026-08-10; - the runner was in UTC, where it was still 23:25 on 2026-08-09. A date recorded - truthfully today read as tomorrow two hours away, and the guard called it a lie. - - "Today" is not a fact a date alone determines — it depends on where the reader is — - so comparing a bare ISO date against `date.today()` is comparing two different - questions. A full day of tolerance covers every real timezone offset, and a date more - than a day ahead is still what this rule is for: a fact nobody actually checked. - - Recorded here rather than fixed silently, because the alternative repair a hurried - reader would reach for is deleting the rule (ADR-014 §3: when a guard cries wolf, - check the matching before the scope). - """ - problems = [] - horizon = dt.date.today() + _CLOCK_SKEW - for name, sibling in siblings.items(): - try: - checked = dt.date.fromisoformat(sibling.public_checked) - except ValueError: - problems.append( - f"{name}: public_checked={sibling.public_checked!r} is not an ISO date. " - "A fact with an unreadable date is a fact with no date." - ) - continue - if checked > horizon: - problems.append( - f"{name}: public_checked={sibling.public_checked} is more than a day " - "ahead of today — that is not clock skew, it is a date nobody checked." - ) - return problems - - _RULES = { "G1 declared checkouts are present and pointed at": _g1_declared_checkouts_are_present_and_pointed_at, "G2 every checkout is declared": _g2_every_checkout_is_declared, - "G3 tokens match visibility": _g3_tokens_match_visibility, "G4 non-coverage is explained": _g4_non_coverage_is_explained, "G5 no step swallows its own failure": _g5_no_step_swallows_its_own_failure, "G6 siblings land under the excluded path": _g6_siblings_land_under_the_excluded_path, "G7 siblings are taken from main": _g7_siblings_are_taken_from_main, - "G8 the visibility fact carries a usable date": _g8_the_visibility_fact_carries_a_usable_date, } @@ -328,7 +253,7 @@ def test_the_workflow_this_file_scans_actually_parses(): """A YAML restructure would empty every scan below and report success. This is by far the most likely way this file becomes decoration: rename the job, nest - `steps` differently, move the workflow, and eight rules find nothing to complain + `steps` differently, move the workflow, and every rule below finds nothing to complain about. So assert there is something to scan before trusting that there was nothing wrong. """ @@ -437,34 +362,26 @@ def _replace(name: str, **changes) -> dict[str, Sibling]: _MUTANTS = [ - ("G1 declared checkouts are present and pointed at", - {"jobs": {"test": {"steps": [{"run": "poetry run pytest tests/", "env": {}}]}}}, - _ONE, "declared ci_checkout=True but the checkout step is gone"), - ("G2 every checkout is declared", _workflow(), {}, "checked out but undeclared"), - ("G3 tokens match visibility", - _workflow(**{"with": {"token": "${{ secrets.X }}"}}), _ONE, - "public sibling fetched with a credential — kills verification-by-doing"), - ("G4 non-coverage is explained", - _workflow(), _replace("views-appwrite", ci_checkout=False, note=""), - "not checked out and no note"), - ("G4 non-coverage is explained", - _workflow(), _replace("views-appwrite", ci_checkout=False, note="because reasons"), - "note explains but names no record"), - ("G5 no step swallows its own failure", - _workflow(**{"continue-on-error": True}), _ONE, - "a failed fetch would not fail the build"), - ("G6 siblings land under the excluded path", - _workflow(**{"with": {"path": "vendor/views-appwrite"}}), _ONE, - "outside _siblings/, so ruff would lint it"), - ("G7 siblings are taken from main", - _workflow(**{"with": {"ref": "development"}}), _ONE, - "takes a branch that is not the authority"), - ("G8 the visibility fact carries a usable date", - _workflow(), _replace("views-appwrite", public_checked="2099-01-01"), - "the fact was checked far in the future"), - ("G8 the visibility fact carries a usable date", - _workflow(), _replace("views-appwrite", public_checked="last tuesday"), - "unparseable date"), + ("G1 declared checkouts are present and pointed at", + {"jobs": {"test": {"steps": [{"run": "poetry run pytest tests/", "env": {}}]}}}, + _ONE, "declared ci_checkout=True but the checkout step is gone"), + ("G2 every checkout is declared", + _workflow(), {}, "checked out but undeclared"), + ("G4 non-coverage is explained", + _workflow(), _replace("views-appwrite", ci_checkout=False, note=""), + "not checked out and no note"), + ("G4 non-coverage is explained", + _workflow(), _replace("views-appwrite", ci_checkout=False, note="because reasons"), + "note explains but names no record"), + ("G5 no step swallows its own failure", + _workflow(**{"continue-on-error": True}), _ONE, + "a failed fetch would not fail the build"), + ("G6 siblings land under the excluded path", + _workflow(**{"with": {"path": "vendor/views-appwrite"}}), _ONE, + "outside _siblings/, so ruff would lint it"), + ("G7 siblings are taken from main", + _workflow(**{"with": {"ref": "development"}}), _ONE, + "takes a branch that is not the authority"), ] @@ -530,23 +447,3 @@ def test_the_repos_own_checkout_is_not_mistaken_for_a_sibling(): own_checkout_only = {"jobs": {"test": {"steps": [{"uses": "actions/checkout@v3"}]}}} assert not _g2_every_checkout_is_declared(own_checkout_only, SIBLINGS) assert not _sibling_steps(own_checkout_only) - - -def test_g8_tolerates_a_timezone_but_not_a_fiction(): - """The boundary either side of `_CLOCK_SKEW`, pinned. - - A rule with a tolerance needs its tolerance tested, or the number drifts into being - whatever made the last failure go away. Tomorrow is a clock difference; the day after - is a claim about a check that has not happened. - """ - today = dt.date.today() - for offset, should_object in ((0, False), (1, False), (2, True), (400, True)): - siblings = _replace( - "views-appwrite", - public_checked=(today + dt.timedelta(days=offset)).isoformat(), - ) - violations = _g8_the_visibility_fact_carries_a_usable_date(_workflow(), siblings) - assert bool(violations) is should_object, ( - f"a check date {offset} day(s) ahead of today: expected " - f"{'an objection' if should_object else 'no objection'}, got {violations}" - )