Review: ADR-016 (CI read access to private siblings) — strong accept, 2 actionable findings #233

External review, filed by request. I'm the agent working the views-faoapi / views-crafdapi (consumer) side of this seam; Simon asked me to review ADR-016 and ADR-017 and leave the reviews as issues. I changed nothing in this repo — this is read-only feedback for the maintainer to act on. Line/section refs are to docs/ADRs/016_ci_read_access_to_private_siblings.md and the code it governs (tests/conftest.py, tests/test_ci_sibling_coverage.py, .github/workflows/run_pytest.yml) at fab6456.

Verdict: strong accept

This is one of the better ADRs I've read in the platform. The root-cause narrative is exemplary — a fact about another repo, written in a workflow comment, went stale (private→public on 2026-08-08) and a credential was proposed one day after its justification expired (§3). The fix — "the list of siblings is code, not a comment" (§4), each rule tested against a deliberately broken example (§5), and a removed-design section recording the circular public/date fields (§"What was tried and removed") — is exactly the declarations-over-inference + guards-that-carry-claims discipline (ADR-003 / ADR-014) applied to itself. The findings below are refinements, not objections.

Findings

F1 — the "temporary" views-crafdapi fetch has an unenforced removal trigger (medium)

§5 flags that the views-crafdapi download is temporary — it "buys nothing and goes" once ADR-017 §7 replaces its one test with a registry read. But in tests/conftest.py, views-crafdapi is ci_checkout=True with a note, and rule G4 ("non-coverage is explained") only fires on ci_checkout=False entries (test_ci_sibling_coverage.py::_g4_non_coverage_is_explained). So the note that records "temporary — remove when ADR-017 §7 lands" is enforced by nothing: no rule requires it, checks it names a record, or fires when the trigger is met.

That is precisely the §3 failure this ADR diagnoses — a fact about another repo living in prose that nothing can check — reintroduced for the one download the ADR itself calls temporary. If ADR-017 step 2 stalls, this fetch lingers indefinitely and silently. Suggest: either gate the crafd fetch behind the same trigger so it cannot outlive its one test, or add a rule that a ci_checkout=True entry whose note claims temporariness must name an owning record (ADR-017 §7 / a tracking issue) — G4's discipline, extended to explained inclusions, not just explained exclusions.

F2 — views-appwrite's fetch is load-bearing but its rationale is unrecorded (medium)

Today views-appwrite (ci_checkout=True) is read by the registry-edition check. Once ADR-017 lands, it becomes the authority source for the delivery-label check too — making its fetch permanent and safety-critical. But its Sibling(...) entry carries no note (notes are structurally False-only), so nothing in the list records why it's fetched or that pruning it would silently disable ADR-017's guard. A future contributor trimming "unused" sibling fetches has nothing to stop them — and the resulting green build is the exact invisible-skip §6 exists to prevent. Suggest: permit (and, for authority-source siblings, require) a note on a required ci_checkout=True entry that names the check(s) depending on it, so the dependency is code, not folklore — consistent with §4's own thesis.

F3 — moving-main coupling vs. the platform's pin-by-tag seam discipline (low / clarity)

G7 pins ref: main — a moving branch tip — on every sibling (run_pytest.yml:61,69; _g7_siblings_are_taken_from_main). For a drift tripwire (the "registry is still edition 1.4.4" check) that's correct and intentional: it should go red when the sibling moves. But the rest of the platform's seam discipline is the opposite — views-faoapi/views-crafdapi pin the seam contract by URL@tag, never a branch (joining_the_seam / faoapi ADR-034 / crafd ADR-035) — because a build's colour shouldn't ride another repo's in-flight main. I can see the two coexist here (test_the_pinned_commit_is_reachable_from_the_contract_repos_main implies a pinned commit plus main-reachability). The ADR reads as "always main," while the pin nuance lives only in a test. Suggest: one or two sentences distinguishing checks that read moving-main-as-tripwire (drift is the signal) from checks that verify a pinned edition reachable from main (drift is noise) — so a reader knows which coupling they're accepting where.

F4 — the §7 escape hatch is habit-dependent, the property the ADR faults elsewhere (low)

§7 accepts that a broken sibling blocks merges and offers one mitigation: "the maintainer … can merge over a failing check." That's a single-person, judgment-based override — the same "works exactly as well as one person's habits" property the ADR rightly criticises in the "rely on running the suite by hand" alternative. Not a defect (an override isn't the check), but the asymmetry is worth one honest sentence: what happens when the sole maintainer is unavailable and a release is genuinely urgent? No secondary path is named.

Net

Accept as-is; F1 and F2 are worth a small follow-up PR (they close the same class of hole this ADR is about), F3/F4 are wording. Cross-note for the maintainer: ADR-017's Appendix B step 3 lands in views-faoapi (the consumer-side label check, views-faoapi#379) — that half has a willing owner on my side whenever the registry declaration (views-appwrite#75) lands.