diff --git a/.forgejo/workflows/ci.yml b/.forgejo/workflows/ci.yml index 2d21737..acca729 100644 --- a/.forgejo/workflows/ci.yml +++ b/.forgejo/workflows/ci.yml @@ -68,7 +68,23 @@ jobs: echo "::warning::This gate does not cover the sandbox. Run 'pytest' on a host with" echo "::warning::Docker before trusting a green tick here — see CONTRIBUTING.md." fi + # **The registry check runs here and nowhere else** (item 192). `test_the_surface_is_not_behind + # _the_registry` asks ghcr.io whether an image exists for the version this tree claims to be, + # and skips without this variable so a developer with no network still gets a green suite. + # + # The question cannot be answered inside the tree: `version=0.1.0a8, surface=0.1.0a7` is a + # correct publication whose post-release steps are not done *yet*, and also post-release steps + # done *wrong*. Same bytes. Only the registry separates them, and the pull is anonymous — a + # public package, no credential, nothing to leak or rotate. + # + # It is expected to be **red on `main` for about half an hour** after each release, between the + # image being published and the post-release commit landing. That is accurate rather than + # unfortunate: the published image really is newer than the documented one, and an alarm that + # stays quiet during the bad state to avoid being inconvenient is the defect this repository + # keeps rediscovering. - run: uv run pytest -ra + env: + ASK_THE_REGISTRY: "1" # DR-0001: the DCO is our contributor agreement, so it gets checked, not just documented. # Deliberately our own five lines of shell: several published DCO actions call the GitHub API to diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2d21737..acca729 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -68,7 +68,23 @@ jobs: echo "::warning::This gate does not cover the sandbox. Run 'pytest' on a host with" echo "::warning::Docker before trusting a green tick here — see CONTRIBUTING.md." fi + # **The registry check runs here and nowhere else** (item 192). `test_the_surface_is_not_behind + # _the_registry` asks ghcr.io whether an image exists for the version this tree claims to be, + # and skips without this variable so a developer with no network still gets a green suite. + # + # The question cannot be answered inside the tree: `version=0.1.0a8, surface=0.1.0a7` is a + # correct publication whose post-release steps are not done *yet*, and also post-release steps + # done *wrong*. Same bytes. Only the registry separates them, and the pull is anonymous — a + # public package, no credential, nothing to leak or rotate. + # + # It is expected to be **red on `main` for about half an hour** after each release, between the + # image being published and the post-release commit landing. That is accurate rather than + # unfortunate: the published image really is newer than the documented one, and an alarm that + # stays quiet during the bad state to avoid being inconvenient is the defect this repository + # keeps rediscovering. - run: uv run pytest -ra + env: + ASK_THE_REGISTRY: "1" # DR-0001: the DCO is our contributor agreement, so it gets checked, not just documented. # Deliberately our own five lines of shell: several published DCO actions call the GitHub API to diff --git a/docs/releasing.md b/docs/releasing.md index 4c749de..f432624 100644 --- a/docs/releasing.md +++ b/docs/releasing.md @@ -163,17 +163,38 @@ anybody pinning either, and the bug report arrives months later from somebody wh **Then, once the image is public**, and in this order, because each step needs the one before it: ```bash -./scripts/record-the-published-surface.py # asks the new image what it accepts +./scripts/record-the-published-surface.py 0.1.0aN # the tag you just released — pass it # move the pins: README.md, docker-compose.yml, docs/install.md, PRIVACY.md ``` +**Pass the tag.** With no argument the script reads the pin from `docker-compose.yml`, and at this +point in the order that pin is still the *previous* release — so the bare command asks the old image +and re-records the surface it already had. It says which tag it asked, and that line is the only +thing that distinguishes the no-op from the work. + That is what lets the documentation describe the new release — including anything held back for it, per [CONTRIBUTING](../CONTRIBUTING.md#documentation-describes-the-released-artefact-not-this-checkout). -Skipping either half is not quiet: re-record without moving the pins and the pins disagree with the -recording; move the pins without re-recording and they disagree the other way. Both fail + +Skipping either half is usually not quiet: re-record without moving the pins and the pins disagree +with the recording; move the pins without re-recording and they disagree the other way. Both fail `tests/test_the_documentation_describes_the_published_artefact.py`, which is the whole point of storing the recording rather than trusting the sequence. +> **One combination used to escape both**, found cutting `0.1.0a8`: run the recorder with no argument +> *and* forget the pins, and everything still names the previous release, consistently. The gate +> compared the two halves to each other, so agreeing on the wrong answer passed, and the release went +> undocumented while every check stayed green. +> +> `tests/test_the_surface_is_not_behind_the_registry.py` closes it by asking the one question the +> tree cannot answer about itself: **is an image published for the version this tree claims to be?** +> If it is, the surface has to have been recorded from it. If it is not, the surface is allowed to +> lag — which exempts the whole bump-to-release window by a fact rather than by a flag somebody has +> to remember to clear. +> +> **So `main` goes red for the half hour between the release and the post-release commit.** That is +> the intended behaviour and it is the to-do list: the published image really is newer than the +> documented one until the two steps above are done. + ## The private forge Development happens on a Forgejo instance, and this repository is a **derivation** of it: the publish diff --git a/hullwork/cli.py b/hullwork/cli.py index ba0351a..7ec926c 100644 --- a/hullwork/cli.py +++ b/hullwork/cli.py @@ -2059,6 +2059,10 @@ def _cmd_status( session, code_token_configured=settings.forge_code_token is not None, forge=_status_forge, + # The dispatcher skips an item whose enrichment has not come back, and only when there + # is a tracker for it to come back from. Counting without this reports work as ready + # that the dispatcher would deliberately leave alone. + tracker_configured=bool(settings.tracker_url and settings.tracker_token), ) # **Asked, not assumed** (item 129). `readiness._forge_state` is module state in whichever # process last spoke to the forge; this is a different process from the receiver, so diff --git a/hullwork/work.py b/hullwork/work.py index 384ddb9..c2cb4b4 100644 --- a/hullwork/work.py +++ b/hullwork/work.py @@ -49,6 +49,14 @@ #: A clone that has not finished by now is a repository too large for one attempt to be worth it. CLONE_TIMEOUT_SECONDS = 600 +#: Counting is not dispatching. `eligible` takes a limit because it hands work out one item at a +#: time; a count that stopped at the first would report `1 item(s) are ready` for ever. +_EVERY_READY_ITEM = 10_000 + +#: Where the credential question can actually be answered, in the deployment shape this ships. +#: Deliberately the same sentence `doctor` gives, because it is the same wrong place to be standing. +WHERE_TO_ASK_THE_DISPATCHER = "`docker compose exec dispatcher hullwork doctor`." + #: Attributed to the tool, not to a person. A plausible human identity on these commits would #: imply somebody reviewed them, which is the claim the whole product exists not to make. #: `.invalid` is reserved by RFC 6761 precisely so an address can be honest about not being one. @@ -793,8 +801,45 @@ def give_up_publishing(session: Session, attempt: Attempt, *, why: str) -> None: ) +def _a_dispatcher_is_alive(session: Session) -> bool: + """Whether some other process is holding the lease and renewing it. + + The same test `doctor.not_from_here` makes, and it has to fail the same way: asking the lease is + asking the database, and this runs where the database may have no tables. Cannot tell → not + alive → nothing is downgraded, because a false silence is worse than a false alarm. + """ + from hullwork import lease + + try: + state, _ = lease.state(session) + except Exception: # any failure here means "cannot tell", whatever its class + return False + return state == "alive" + + +def ready_for_the_dispatcher( + session: Session, *, tracker_configured: bool = False +) -> list[Eligible]: + """Everything the dispatcher would attempt, rather than everything that looks ready. + + **One question, one answer** (item 193). `readiness_notes` used to select on state and lane, + and `eligible` applies six conditions; the two were written apart and drifted by four of them. + On the live instance that made `status` count an item belonging to an **inactive** project, and + then name a missing credential as the reason nobody had picked it up. + + Adding the missing condition to the second query would have re-created the defect with a longer + fuse, so there is no second query. The limit exists only because `eligible` takes one — counting + is not dispatching, and a count that stops at the first item is not a count. + """ + return eligible(session, limit=_EVERY_READY_ITEM, tracker_configured=tracker_configured) + + def readiness_notes( - session: Session, *, code_token_configured: bool, forge: object | None = None + session: Session, + *, + code_token_configured: bool, + forge: object | None = None, + tracker_configured: bool = False, ) -> list[Note]: """What `hullwork status` should say about the dispatcher half. @@ -810,17 +855,37 @@ def readiness_notes( against what the models declare. """ notes: list[Note] = [] - ready = session.execute( - select(Item).where(Item.state == ItemState.READY, Item.lane != Lane.RED) - ).scalars().all() + ready = ready_for_the_dispatcher(session, tracker_configured=tracker_configured) in_progress = session.execute( select(Item).where(Item.state == ItemState.IN_PROGRESS) ).scalars().all() - if ready and not code_token_configured: + if ready and not code_token_configured and _a_dispatcher_is_alive(session): + # **Whose resource is it** (item 193). This process is the receiver, and DR-0005 requires + # the receiver not to hold the code token — so its own environment says nothing about the + # process that does. Measured live: this printed `nothing will ever pick them up` while the + # dispatcher's own doctor printed `ok code token → hullwork`. + # + # Not silence either. The question is real and the operator is the one who can go and ask + # it, so it is reported as unanswerable from here, with where to ask — which is what + # `doctor` has done since item 105 and this sentence never learnt. + notes.append( + Note( + f"{len(ready)} item(s) are ready to attempt. Whether the dispatcher can publish " + f"them cannot be answered from here: HULLWORK_FORGE_CODE_TOKEN is not set in this " + f"process, and a dispatcher is running, so this is a resource it holds rather than " + f"one this process does. Ask it: {WHERE_TO_ASK_THE_DISPATCHER}" + ) + ) + elif ready and not code_token_configured: # A degradation, not a note. Items only reach `ready` when a manifest names an agent, so # this state means somebody configured one and never gave the dispatcher a credential — # work that will sit there for ever while everything reports fine. + # + # **With no dispatcher alive nothing is downgraded**, which is `doctor.py`'s rule word + # for word: the absence of one is exactly when somebody needs to know what is missing. It + # is also the shape `hullwork work` itself runs in, where the credential is genuinely this + # process's business and the exit code has to stay 1. notes.append( Note( f"{len(ready)} item(s) are ready to attempt and HULLWORK_FORGE_CODE_TOKEN is not " @@ -866,9 +931,9 @@ def readiness_notes( # before the dispatcher does, which is the difference between a warning and a post-mortem. if forge is not None and ready: stranded = [ - item.forge_issue_ref - for item in ready - if item.forge_issue_ref and not _issue_resolves(forge, item) + candidate.item.forge_issue_ref + for candidate in ready + if candidate.item.forge_issue_ref and not _issue_resolves(forge, candidate.item) ] if stranded: notes.append( diff --git a/tests/test_the_alarm_the_architecture_guarantees.py b/tests/test_the_alarm_the_architecture_guarantees.py new file mode 100644 index 0000000..1e84341 --- /dev/null +++ b/tests/test_the_alarm_the_architecture_guarantees.py @@ -0,0 +1,131 @@ +"""What `status` may claim about a credential it cannot see. Item 193. + +Found on the live instance: a healthy deployment reporting a permanent degradation, in a sentence +where every clause was false. `hullwork status` runs in the receiver, DR-0005 requires the receiver +not to hold `HULLWORK_FORGE_CODE_TOKEN`, and the note read that absence as *nothing will ever pick +them up* — a claim about a **different process**, which had the credential all along. + +`doctor.py` had solved this and `work.py` never learnt it. Item 073, in `doctor`'s own words: *a +signal that is permanently on is not a signal, and the day the credential really expires that line +is already red.* + +Every test here was verified by reintroducing the defect it covers. +""" + +from __future__ import annotations + +from sqlalchemy.orm import Session +from test_work import _item, _project + +from hullwork import lease, work +from hullwork.models import ItemState + + +def _a_dispatcher_is_alive(session: Session) -> None: + """The state the receiver is in whenever the deployment is working.""" + lease.acquire(session, lease.new_holder()) + + +# --- the claim that could not be true ---------------------------------------------------------- + + +def test_a_live_dispatcher_means_the_missing_credential_is_not_this_process_business( + session: Session, +) -> None: + """The whole finding. The receiver never holds this credential **by design**, so reading its own + environment and announcing a consequence for another process is not a diagnosis. + + Measured on `atlas`, 2026-08-09: this note said nothing would ever pick the item up while + `docker exec hullwork-dispatcher-1 hullwork doctor` said `ok code token → hullwork`. + """ + _item(session, _project(session)) + _a_dispatcher_is_alive(session) + + notes = work.readiness_notes(session, code_token_configured=False) + + assert not any(n.degraded for n in notes), ( + "a healthy instance reported a degradation, which is how this was found" + ) + said = " ".join(n.text for n in notes) + assert "nothing will ever pick them up" not in said + + +def test_it_says_where_the_question_can_be_answered_rather_than_going_quiet( + session: Session, +) -> None: + """Not silence. Downgrading a false alarm into nothing loses a real question — *does the + dispatcher have it?* — and the operator is the one who can go and ask.""" + _item(session, _project(session)) + _a_dispatcher_is_alive(session) + + said = " ".join(n.text for n in work.readiness_notes(session, code_token_configured=False)) + + assert "dispatcher" in said + assert "cannot" in said or "not from here" in said + + +def test_with_no_dispatcher_alive_the_degradation_is_kept_exactly(session: Session) -> None: + """The true positive, and the reason the branch cannot simply be deleted. + + `doctor.py:956`: *with no dispatcher alive, nothing is downgraded — the absence of one is + exactly when somebody needs to know what is missing.* This is also the shape `hullwork work` + runs in, where the credential question is genuinely local and the answer must still exit 1. + """ + _item(session, _project(session)) + + notes = work.readiness_notes(session, code_token_configured=False) + + assert any("nothing will ever pick them up" in n.text for n in notes) + assert any(n.degraded for n in notes) + + +def test_a_stale_lease_is_not_a_live_dispatcher(session: Session) -> None: + """A dispatcher that died holding the lease must not silence the alarm for ever — that would + turn one permanently-on signal into one permanently-off, which is worse.""" + _item(session, _project(session)) + _a_dispatcher_is_alive(session) + lease.release(session, lease.holder_of(session) or "") + + notes = work.readiness_notes(session, code_token_configured=False) + + assert any(n.degraded for n in notes) + + +# --- the second defect in the same sentence ----------------------------------------------------- + + +def test_an_item_of_an_inactive_project_is_not_counted_as_ready(session: Session) -> None: + """The exact shape that produced the alarm: project `simplecheck`, `active = 0`, holding one + item in `ready` that had already opened a pull request on 2026-07-30. + + A count that includes work nobody intends to do turns every other sentence built on it into a + guess. + """ + _item(session, _project(session, active=False, slug="parked")) + + said = " ".join(n.text for n in work.readiness_notes(session, code_token_configured=True)) + + assert "ready for the dispatcher" not in said + + +def test_status_counts_exactly_what_the_dispatcher_would_attempt(session: Session) -> None: + """**Asserted by construction**, because two lists of conditions kept equal by hand is what + produced a four-condition drift in the first place. + + `eligible()` applies six conditions; this note applied two. Any future condition added to the + dispatcher has to reach this sentence without anybody remembering to come here. + + **Two eligible items, not one**, and that is deliberate. `eligible` hands work out one at a time + and defaults to `limit=1`; with a single eligible item this test passes against a count that + stops at the first, which would report `1 item(s) are ready` on an instance with fifty. + """ + live = _project(session, slug="live") + first = _item(session, live) + second = _item(session, live, fingerprint="fp2") + _item(session, _project(session, active=False, slug="parked"), fingerprint="fp3") + _item(session, _project(session, slug="done"), state=ItemState.DONE, fingerprint="fp4") + + counted = work.ready_for_the_dispatcher(session) + + assert [e.item.id for e in counted] == [first.id, second.id] + assert [e.item.id for e in counted] == [e.item.id for e in work.eligible(session, limit=1000)] diff --git a/tests/test_the_surface_is_not_behind_the_registry.py b/tests/test_the_surface_is_not_behind_the_registry.py new file mode 100644 index 0000000..fd919a3 --- /dev/null +++ b/tests/test_the_surface_is_not_behind_the_registry.py @@ -0,0 +1,119 @@ +"""Whether the documented release is the published one. Item 192. + +`test_the_documentation_describes_the_published_artefact` compares the recorded surface to the +pins and catches both of the ordinary mistakes. It compares the two halves **to each other**, and +nothing compares either to the tag that was actually pushed — so the two of them agreeing on the +*previous* release is green, and that state is reachable by following `docs/releasing.md` in the +order it is written, with no step skipped. + +Found cutting `0.1.0a8`: the recorder, given no argument, reads the tag pinned in +`docker-compose.yml` — which at that point in the order is still the previous release. The step is a +no-op, and if the pins are then forgotten too, everything agrees on `0.1.0a7` and the suite is green +while the registry serves `0.1.0a8`. + +**The tree cannot tell the two states apart**, which is the whole reason this file needs a fact from +outside it. `version=0.1.0a8, surface=0.1.0a7` is a correct publication whose post-release steps are +not done *yet*, and it is also post-release steps done *wrong*. Same bytes. The one thing that +separates them is whether an image exists for the version this tree claims to be. + +Every test here was verified by reintroducing the defect it covers. +""" + +from __future__ import annotations + +import json +import os +from pathlib import Path + +import pytest +from the_registry import behind_the_registry, published_tags + +ROOT = Path(__file__).resolve().parent.parent +SURFACE = json.loads((ROOT / "docs/published-surface.json").read_text(encoding="utf-8")) + + +def _version() -> str: + """What this tree claims to be, read from the package rather than from a document.""" + from hullwork import __version__ + + return __version__ + + +# --- the rule, decided without a network --------------------------------------------------------- + + +def test_a_published_image_for_this_version_requires_the_surface_to_record_it() -> None: + """The failure this file exists for, and the state that is green in every other test. + + Reported with the published tag in it, because *the surface and the pins disagree* sends a + reader to reconcile two things that are already equal. + """ + said = behind_the_registry("0.1.0a8", surface="0.1.0a7", published=("0.1.0a7", "0.1.0a8")) + + assert said is not None + assert "0.1.0a8" in said and "0.1.0a7" in said + + +def test_no_published_image_for_this_version_means_the_surface_may_lag() -> None: + """**The bump-to-release window, exempted by a fact rather than by a flag.** + + The version is raised before the release exists — the publication pull request carries the bump, + and the image is built from the tag that merge produces. For all of that time the surface + honestly records the previous release, because that is the artefact that exists. + """ + assert behind_the_registry("0.1.0a8", surface="0.1.0a7", published=("0.1.0a7",)) is None + + +def test_a_surface_recorded_from_this_version_is_the_finished_state() -> None: + said = behind_the_registry("0.1.0a8", surface="0.1.0a8", published=("0.1.0a7", "0.1.0a8")) + + assert said is None + + +def test_it_replays_the_sequence_that_produced_the_defect() -> None: + """`0.1.0a8`'s own release, state by state, which is what item 192's gate asks for. + + The third row is the one that is green today and must not be: the image is on the registry and + every document in the tree describes the release before it. + """ + steps = [ + ("before the bump", "0.1.0a7", "0.1.0a7", ("0.1.0a7",), False), + ("version bumped, nothing published", "0.1.0a8", "0.1.0a7", ("0.1.0a7",), False), + ("image public, post-release not done", "0.1.0a8", "0.1.0a7", ("0.1.0a7", "0.1.0a8"), True), + ("surface re-recorded", "0.1.0a8", "0.1.0a8", ("0.1.0a7", "0.1.0a8"), False), + ] + for name, version, surface, published, should_fail in steps: + said = behind_the_registry(version, surface=surface, published=published) + assert (said is not None) is should_fail, name + + +def test_a_registry_that_cannot_be_reached_is_not_a_pass() -> None: + """**`None` means checked and fine**, so an unreachable registry must not borrow that word. + + A check that reports success when it could not run is the failure this repository has now found + three times: the permanently-on signal (item 073), the two halves agreeing wrongly (this item), + and a mutation harness that called six caught defects uncaught (item 193). + """ + with pytest.raises(LookupError): + behind_the_registry("0.1.0a8", surface="0.1.0a7", published=None) + + +# --- the same rule, against the real registry ---------------------------------------------------- + + +@pytest.mark.skipif( + not os.environ.get("ASK_THE_REGISTRY"), + reason="asks ghcr.io; set ASK_THE_REGISTRY=1 to run it (CI does)", +) +def test_this_tree_is_not_describing_a_release_the_registry_has_moved_past() -> None: + """The one that runs for real, and the only one here that can fail because of a person. + + Anonymous: the token endpoint issues a pull scope for a public package with no account. If this + is red, either the surface was recorded from the wrong tag or the post-release commit has not + landed yet — `docs/releasing.md` has both, in order. + """ + said = behind_the_registry( + _version(), surface=SURFACE["version"], published=published_tags() + ) + + assert said is None, said diff --git a/tests/the_registry.py b/tests/the_registry.py new file mode 100644 index 0000000..c9f21e0 --- /dev/null +++ b/tests/the_registry.py @@ -0,0 +1,88 @@ +"""Whether an image exists for the version this tree claims to be. Item 192. + +Split from the test that uses it so the rule can be decided with no network at all: every state of a +release is a tuple of three strings, and `behind_the_registry` is a function of them. Only +`published_tags` touches the wire, and only the one test that asks for it calls it. + +**This lives in `tests/` on purpose.** `scripts/` is withheld from publication, so a check that +lived there would be absent from the public tree — which is the one that gets pulled, and therefore +the one whose documentation being a release behind actually costs somebody something. +""" + +from __future__ import annotations + +import json +import urllib.error +import urllib.request +from collections.abc import Iterable + +#: The published package. Anonymous pulls are allowed on it, which is what makes this check runnable +#: from any checkout and any CI job without a credential to leak or rotate. +IMAGE = "easybytehub/hullwork" + +_TOKEN = f"https://ghcr.io/token?scope=repository:{IMAGE}:pull&service=ghcr.io" +_TAGS = f"https://ghcr.io/v2/{IMAGE}/tags/list" + +TIMEOUT_SECONDS = 20 + + +def behind_the_registry( + version: str, *, surface: str, published: Iterable[str] | None +) -> str | None: + """The failure, or `None` when there is nothing to report. + + Three lines, and the middle one is the whole reason this is not a tree-local rule: + + * the surface already records this version → nothing to say; + * **no image is published for this version** → nothing to say either, and this exempts the + whole window between the bump and the release without a flag anybody has to clear; + * an image is published for this version and the surface records something else → say so, naming + the published tag. + + `published=None` is not "no images", it is "could not ask", and it raises. A check that reports + success when it could not run is worse than no check: `docs/releasing.md` would carry a promise + with nothing behind it, which is precisely the shape item 192 was opened about. + """ + if published is None: + msg = ( + "could not ask ghcr.io which versions are published, so whether this tree documents " + "the current release is unknown — not confirmed" + ) + raise LookupError(msg) + if surface == version: + return None + if version not in set(published): + return None + return ( + f"ghcr.io/{IMAGE}:{version} is published and docs/published-surface.json was recorded from " + f"{surface}, so every document here describes the release before this one. Run " + f"`./scripts/record-the-published-surface.py {version}` and move the pins — " + f"docs/releasing.md has both, in that order." + ) + + +def published_tags() -> tuple[str, ...] | None: + """Every tag on the package, or `None` when the registry could not be asked. + + `None` rather than an exception so a caller can tell *offline* from *nothing published*, which + are the two answers this must never blur. + """ + try: + token = _get(_TOKEN)["token"] + tags = _get(_TAGS, {"Authorization": f"Bearer {token}"})["tags"] + except (urllib.error.URLError, TimeoutError, KeyError, ValueError, OSError): + return None + if not isinstance(tags, list): + # A registry answering something else is a registry that was not asked what we think. + return None + return tuple(str(tag) for tag in tags) + + +def _get(url: str, headers: dict[str, str] | None = None) -> dict[str, object]: + request = urllib.request.Request(url, headers=headers or {}) # noqa: S310 - literal https + with urllib.request.urlopen(request, timeout=TIMEOUT_SECONDS) as response: # noqa: S310 + loaded = json.load(response) + if not isinstance(loaded, dict): + msg = f"{url} did not answer with an object" + raise ValueError(msg) + return loaded