diff --git a/.github/workflows/validate.yml b/.github/workflows/validate.yml index 24c3b6b..ae2ab85 100644 --- a/.github/workflows/validate.yml +++ b/.github/workflows/validate.yml @@ -10,6 +10,46 @@ permissions: contents: read jobs: + # The release refuses to republish changed artifact bytes under a version it + # has already published. That rule used to be reachable only from the release, + # which runs on main after the merge -- so a pull request that broke it merged + # green and took the channel down afterwards. Nothing about asking the + # question needs the signing key, so it is asked here instead. + channel-preflight: + name: signed channel accepts this tree + runs-on: ubuntu-latest + env: + FTW_DRIVER_KEY_ID: ftw-drivers-2026-01 + FTW_DRIVER_SIGNING_PUBLIC_KEY: MX+j27UBkyM099hTyJlmMLK9qlTTDUJsaK/vH12fFKc= + steps: + - uses: actions/checkout@fbc6f3992d24b796d5a048ff273f7fcc4a7b6c09 # v5 + - uses: astral-sh/setup-uv@37802adc94f370d6bfd71619e3f0bf239e1f3b78 # v7 + with: + python-version: "3.12" + - run: uv sync --frozen --extra package --extra dev + # A public release asset and a public key: a fork's pull request can read + # both, and neither is a secret this workflow could leak. + - name: Download the published channel manifest + env: + GH_TOKEN: ${{ github.token }} + run: | + set -euo pipefail + published="${RUNNER_TEMP}/ftw-driver-published" + mkdir -p "${published}" + if ! gh release view drivers-beta --repo "${GITHUB_REPOSITORY}" >/dev/null 2>&1; then + echo "drivers-beta has never been published; no versions to collide with." + exit 0 + fi + gh release download drivers-beta --repo "${GITHUB_REPOSITORY}" \ + --pattern manifest.json --dir "${published}" --clobber + echo "FTW_DRIVER_PUBLISHED_MANIFEST=${published}/manifest.json" >> "${GITHUB_ENV}" + - name: Would the signed channel publish this tree? + if: env.FTW_DRIVER_PUBLISHED_MANIFEST != '' + run: >- + uv run --no-sync python tools/ftw_repository.py check-versions + --previous-manifest "${FTW_DRIVER_PUBLISHED_MANIFEST}" + --key-id "${FTW_DRIVER_KEY_ID}" + public-boundary: runs-on: ubuntu-latest steps: diff --git a/CHANGELOG.md b/CHANGELOG.md index 0362f80..759d7ed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,9 @@ Driver versions follow [Semantic Versioning](https://semver.org/spec/v2.0.0.html ## [Unreleased] +### Added +- **The release's version rule now runs on every pull request, where it can still be acted on.** The signed channel refuses to republish changed artifact bytes under a version it has already published — the rule that makes a version name a set of bytes an operator can pin. Asking it needed the published channel, so it only ever ran from the release job on `main`, *after* the merge: #53 merged with five green checks and took the release down, and the repair (#55) had to be a second pull request against a `main` that was already broken. Nothing about the question needs the signing key, though — the published manifest is a release asset and its signature verifies with the public key — so `ftw_repository.py check-versions` builds the artifacts in memory, signs nothing, and answers exactly what the release would. The new `signed channel accepts this tree` job runs it on every pull request, including from forks, which need no secret to read a public asset. It reports **every** colliding driver rather than the first, each with the `make bump-driver` command that fixes it. `_version_collisions` is the single statement of the rule that both the release and this check read, and `test_check_versions_answers_what_the_release_would_answer` holds them to the same verdict on the same tree — a rule stated twice is a rule that drifts, and a check that drifts from the release is worse than no check. `test_check_versions_catches_a_manifest_only_edit` is #53 in the shape it arrived in: a manifest field corrected, no Lua touched, and the bytes moved anyway + ### Fixed - **`nibe_local` was filed as a meter.** Its manifest declared `ders: [meter]`; it reads a NIBE S-series heat pump. The driver emits nothing but metrics — `hp_power_w`, `hp_used_power_w`, the temperatures and the lifetime energy counters, all through `host.emit_metric` and never through `host.emit("meter", …)` — which its header states is deliberate, so that a pump's electrical draw cannot double-count against the site's real grid meter. The catalog was advertising a DER the driver does not provide, and the published page (#51) badges and filters on exactly that field: the pump sat under **Meter** and was absent from **Heat pump**, where an owner looking for it would start. Now `ders: [heatpump]`, matching `heishamon` — the other metric-only heat pump, which has been filed that way since it landed. The Lua source and its `sha256` are untouched, so the driver a host runs is byte-for-byte the one it ran before - **`nibe_local`** 1.1.0 → 1.1.1 — **the version has to move anyway, because the signed artifact carries this metadata.** `ders` is not catalog-only decoration: `_load_channel` copies it into the artifact's `capabilities` and the channel publishes source and metadata as one signed file, so correcting the field changed the published bytes. The channel then refused the release — `error: nibe_local: changed artifact needs a higher version than 1.1.0` — and it was right to, since that immutability is what lets an operator name the bytes a site is running. A patch rather than a minor: no register, field or emitted metric changed, only the DER the catalog states. The `DRIVER` table is deliberately left alone, keeping the driver byte-identical to its FTW baseline the way #29 established diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index fe2e298..090479d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -53,10 +53,25 @@ Raise it too when a manifest field the signed channel publishes changes, even though no Lua moved. The channel signs source and metadata as one artifact, so `ders`, `protocol` and the `manufacturer` and `model_family` of a `tested_devices` entry all reach the published bytes. Changing one under a -version already released fails the release with `changed artifact needs a -higher version`, and it fails on `main` after the merge, not in the pull -request — the check compares against the channel, which only the release can -read. +version already released fails with `changed artifact needs a higher version`. + +The `signed channel accepts this tree` check answers that on every pull +request, against the manifest the channel has actually published. To ask it +yourself: + +```bash +gh release download drivers-beta --repo srcfl/device-drivers \ + --pattern manifest.json --dir /tmp/ftw +FTW_DRIVER_SIGNING_PUBLIC_KEY=MX+j27UBkyM099hTyJlmMLK9qlTTDUJsaK/vH12fFKc= \ + uv run --extra package python tools/ftw_repository.py check-versions \ + --previous-manifest /tmp/ftw/manifest.json --key-id ftw-drivers-2026-01 +``` + +It signs nothing and needs no secret. It names every driver that has to move +and the command that moves it. Because it compares against the published +channel rather than against `main`, a `main` that is itself unpublishable +fails this check on unrelated pull requests too — that is the breakage +surfacing, and it clears when `main` is fixed. ```bash make bump-driver ID=example LEVEL=patch diff --git a/devices.yaml b/devices.yaml index f0028ff..0fd515e 100644 --- a/devices.yaml +++ b/devices.yaml @@ -990,7 +990,7 @@ manufacturers: - protocol: http driver: "nibe_local" version: "1.1.1" - ders: [heatpump] + ders: [meter] control: true firmware_versions: "" notes: "Read-only NIBE S-series heat-pump telemetry over the on-prem Local REST API (HTTPS + Basic auth, self-signed cert pinned via tls_pin_sha256). Emits compressor/used power, lifetime energy meters, and the full ~980-point register map. Observe-only — no control." diff --git a/index.yaml b/index.yaml index 088dea7..96af8bf 100644 --- a/index.yaml +++ b/index.yaml @@ -332,7 +332,7 @@ drivers: version: "1.1.1" tier: core protocol: http - ders: [heatpump] + ders: [meter] control: true size_bytes: 18227 sha256: "b89177004a2eff5a5b60e2f275f584f079acf02876c049b231ba3170bb35ac50" diff --git a/manifests/nibe_local.yaml b/manifests/nibe_local.yaml index 5e9d478..4af51e6 100644 --- a/manifests/nibe_local.yaml +++ b/manifests/nibe_local.yaml @@ -3,7 +3,7 @@ version: "1.1.1" tier: core author: "Sourceful Labs AB" protocol: http -ders: [heatpump] +ders: [meter] control: true tested_devices: - manufacturer: "NIBE" diff --git a/tests/test_ftw_repository.py b/tests/test_ftw_repository.py index a90549e..94a978f 100644 --- a/tests/test_ftw_repository.py +++ b/tests/test_ftw_repository.py @@ -25,6 +25,7 @@ RepositoryError, build_publication, canonical_json, + check_publication, verify_artifacts, verify_manifest, verify_stable_promotion, @@ -337,30 +338,97 @@ def test_previous_signed_versions_are_kept_as_history( ) == current -def test_changed_final_artifact_requires_a_higher_driver_version( - tmp_path: Path, keypair: tuple[str, str] -) -> None: +def single_driver_repo(tmp_path: Path, driver_id: str = "goodwe") -> tuple[Path, Path]: + """A repository holding one real driver, for rules that need a whole channel.""" repo = tmp_path / "repo" (repo / "drivers" / "lua").mkdir(parents=True) (repo / "manifests").mkdir() - source_path = repo / "drivers" / "lua" / "goodwe.lua" - source_path.write_bytes((ROOT / "drivers" / "lua" / "goodwe.lua").read_bytes()) - (repo / "manifests" / "goodwe.yaml").write_bytes( - (ROOT / "manifests" / "goodwe.yaml").read_bytes() + (repo / "drivers" / "lua" / f"{driver_id}.lua").write_bytes( + (ROOT / "drivers" / "lua" / f"{driver_id}.lua").read_bytes() + ) + (repo / "manifests" / f"{driver_id}.yaml").write_bytes( + (ROOT / "manifests" / f"{driver_id}.yaml").read_bytes() ) - config = {"schema_version": 1, "include_all": True, "release_mode": "per_driver"} config_path = repo / "ftw-channel.json" - config_path.write_text(json.dumps(config)) - _, first_output = build( - tmp_path / "first", + config_path.write_text( + json.dumps( + {"schema_version": 1, "include_all": True, "release_mode": "per_driver"} + ) + ) + return repo, config_path + + +def publish_once( + tmp_path: Path, keypair: tuple[str, str], repo: Path, config_path: Path +) -> Path: + """Publish the repository as it stands and keep the signed manifest.""" + _, output = build( + tmp_path / "first", keypair, repo_root=repo, config_path=config_path + ) + previous_path = tmp_path / "previous.json" + previous_path.write_bytes((output / "manifest.json").read_bytes()) + return previous_path + + +def test_changed_final_artifact_requires_a_higher_driver_version( + tmp_path: Path, keypair: tuple[str, str] +) -> None: + repo, config_path = single_driver_repo(tmp_path) + source_path = repo / "drivers" / "lua" / "goodwe.lua" + previous_path = publish_once(tmp_path, keypair, repo, config_path) + source_path.write_text(source_path.read_text() + "\n-- source changed\n") + + with pytest.raises(RepositoryError, match="needs a higher version"): + build( + tmp_path / "second", + keypair, + repo_root=repo, + config_path=config_path, + previous_manifest_path=previous_path, + ) + + +def check( + keypair: tuple[str, str], repo: Path, config_path: Path, previous_path: Path +) -> dict: + return check_publication( + repo_root=repo, + config_path=config_path, + previous_manifest_path=previous_path, + key_id=KEY_ID, + expected_public_key_base64=keypair[1], + ) + + +def test_check_versions_answers_what_the_release_would_answer( + tmp_path: Path, keypair: tuple[str, str] +) -> None: + """The pull request check is worth nothing unless it agrees with the release. + + Same tree, same published manifest: whatever `build` decides while signing, + `check_publication` has to decide without a key. Both read one rule so they + cannot drift apart, and this is what says so. + """ + repo, config_path = single_driver_repo(tmp_path) + source_path = repo / "drivers" / "lua" / "goodwe.lua" + previous_path = publish_once(tmp_path, keypair, repo, config_path) + + # Unchanged tree: the release publishes it, so the check passes it. + report = check(keypair, repo, config_path, previous_path) + assert report["changed"] == [] + assert report["drivers"] == 1 + build( + tmp_path / "unchanged", keypair, repo_root=repo, config_path=config_path, + previous_manifest_path=previous_path, ) - previous_path = tmp_path / "previous.json" - previous_path.write_bytes((first_output / "manifest.json").read_bytes()) - source_path.write_text(source_path.read_text() + "\n-- source changed\n") + # Changed bytes under a published version: both refuse. + source_path.write_text(source_path.read_text() + "\n-- source changed\n") + with pytest.raises(RepositoryError, match="needs a higher version"): + check(keypair, repo, config_path, previous_path) with pytest.raises(RepositoryError, match="needs a higher version"): build( tmp_path / "second", @@ -371,6 +439,118 @@ def test_changed_final_artifact_requires_a_higher_driver_version( ) +def test_check_versions_catches_a_manifest_only_edit( + tmp_path: Path, keypair: tuple[str, str] +) -> None: + """The failure this check exists to stop, in the shape it actually arrived in. + + #53 corrected a manifest's `ders` and left the version alone, reading the + field as catalog-only. It is not: the signed artifact carries the manifest + metadata beside the Lua source, so the published bytes moved and the release + refused them on main. No Lua is touched here either. + """ + repo, config_path = single_driver_repo(tmp_path) + manifest_path = repo / "manifests" / "goodwe.yaml" + lua_path = repo / "drivers" / "lua" / "goodwe.lua" + lua_before = lua_path.read_bytes() + previous_path = publish_once(tmp_path, keypair, repo, config_path) + + edited, replaced = re.subn( + r"^ders: \[.*\]$", "ders: [heatpump]", manifest_path.read_text(), flags=re.M + ) + assert replaced == 1, "the manifest edit must land" + manifest_path.write_text(edited) + assert lua_path.read_bytes() == lua_before, "no Lua source may change" + + with pytest.raises(RepositoryError, match="needs a higher version"): + check(keypair, repo, config_path, previous_path) + + +def test_check_versions_accepts_a_changed_driver_that_took_a_version( + tmp_path: Path, keypair: tuple[str, str] +) -> None: + repo, config_path = single_driver_repo(tmp_path) + manifest_path = repo / "manifests" / "goodwe.yaml" + source_path = repo / "drivers" / "lua" / "goodwe.lua" + published_version = re.search( + r'^version:\s*"([^"]+)"', manifest_path.read_text(), re.M + ).group(1) + previous_path = publish_once(tmp_path, keypair, repo, config_path) + + source_path.write_text(source_path.read_text() + "\n-- source changed\n") + major, minor, patch = (int(part) for part in published_version.split(".")) + manifest_path.write_text( + re.sub( + r'^version:\s*"[^"]+"', + f'version: "{major}.{minor}.{patch + 1}"', + manifest_path.read_text(), + count=1, + flags=re.M, + ) + ) + + report = check(keypair, repo, config_path, previous_path) + assert report["changed"] == ["goodwe"] + assert report["added"] == [] + + +def test_check_versions_reports_every_colliding_driver( + tmp_path: Path, keypair: tuple[str, str] +) -> None: + """The release stops at the first; someone fixing a branch wants the list.""" + repo, config_path = single_driver_repo(tmp_path) + for driver_id in ("sofar", "solis"): + (repo / "drivers" / "lua" / f"{driver_id}.lua").write_bytes( + (ROOT / "drivers" / "lua" / f"{driver_id}.lua").read_bytes() + ) + (repo / "manifests" / f"{driver_id}.yaml").write_bytes( + (ROOT / "manifests" / f"{driver_id}.yaml").read_bytes() + ) + previous_path = publish_once(tmp_path, keypair, repo, config_path) + + for driver_id in ("goodwe", "sofar", "solis"): + source_path = repo / "drivers" / "lua" / f"{driver_id}.lua" + source_path.write_text(source_path.read_text() + "\n-- source changed\n") + + with pytest.raises(RepositoryError) as raised: + check(keypair, repo, config_path, previous_path) + message = str(raised.value) + for driver_id in ("goodwe", "sofar", "solis"): + assert driver_id in message, f"{driver_id} is missing from the report" + assert "make bump-driver ID=goodwe" in message + + +def test_check_versions_reports_a_driver_the_channel_has_never_published( + tmp_path: Path, keypair: tuple[str, str] +) -> None: + repo, config_path = single_driver_repo(tmp_path) + previous_path = publish_once(tmp_path, keypair, repo, config_path) + (repo / "drivers" / "lua" / "sofar.lua").write_bytes( + (ROOT / "drivers" / "lua" / "sofar.lua").read_bytes() + ) + (repo / "manifests" / "sofar.yaml").write_bytes( + (ROOT / "manifests" / "sofar.yaml").read_bytes() + ) + + report = check(keypair, repo, config_path, previous_path) + assert report["added"] == ["sofar"] + assert report["changed"] == [] + + +def test_check_versions_rejects_a_manifest_the_signing_key_did_not_sign( + tmp_path: Path, keypair: tuple[str, str] +) -> None: + """It reads the channel over the network, so the signature still has to hold.""" + repo, config_path = single_driver_repo(tmp_path) + previous_path = publish_once(tmp_path, keypair, repo, config_path) + envelope = json.loads(previous_path.read_text()) + envelope["payload"]["drivers"][0]["sha256"] = "0" * 64 + previous_path.write_bytes(canonical_json(envelope) + b"\n") + + with pytest.raises(RepositoryError): + check(keypair, repo, config_path, previous_path) + + def test_stable_promotion_allows_added_driver_and_beta_only_history( tmp_path: Path, keypair: tuple[str, str] ) -> None: diff --git a/tools/ftw_repository.py b/tools/ftw_repository.py index 76f2cff..7c1d745 100644 --- a/tools/ftw_repository.py +++ b/tools/ftw_repository.py @@ -107,6 +107,42 @@ def _semver_tuple(value: str) -> tuple[int, int, int]: return tuple(int(part) for part in match.groups()) +def _artifact_digest(entry: dict[str, Any]) -> str: + """The name a published artifact answers to. + + The channel identifies a driver by the SHA-256 of the bytes it publishes, + which are the Lua source *and* the generated metadata header. Both the + release and the pull request check hash through here so neither can decide + a driver is unchanged the other would call changed. + """ + return hashlib.sha256(entry["raw"]).hexdigest() + + +def _version_collisions( + drivers: list[dict[str, Any]], previous_drivers: list[dict[str, Any]] +) -> list[tuple[dict[str, Any], dict[str, Any]]]: + """Every driver whose artifact bytes moved without a higher version. + + A published version names a set of bytes that runs on hardware, so the + channel refuses to republish different bytes under one it has already + shipped. That refusal is the whole rule, and it is stated here once: the + release enforces it while signing, `check_publication` enforces it on a + pull request, and a rule stated twice is a rule that drifts -- at which + point the check that exists to agree with the release stops agreeing. + """ + previous_by_id = {driver["id"]: driver for driver in previous_drivers} + collisions = [] + for driver in drivers: + prior = previous_by_id.get(driver["id"]) + if ( + prior + and prior["sha256"] != driver["sha256"] + and _semver_tuple(driver["version"]) <= _semver_tuple(prior["version"]) + ): + collisions.append((driver, prior)) + return collisions + + def _read_json(path: Path) -> dict[str, Any]: try: value = json.loads(path.read_text(encoding="utf-8")) @@ -874,7 +910,7 @@ def build_publication( "drivers": [], } for entry in configured: - digest = hashlib.sha256(entry["raw"]).hexdigest() + digest = _artifact_digest(entry) artifact_name = ( f"driver-{entry['id']}-v{entry['version']}-{digest[:16]}.lua" ) @@ -908,16 +944,13 @@ def build_publication( (driver["id"], driver["version"], driver["sha256"]) for driver in manifest["drivers"] } - previous_current = {driver["id"]: driver for driver in previous["drivers"]} - for driver in manifest["drivers"]: - prior = previous_current.get(driver["id"]) - if prior and prior["sha256"] != driver["sha256"] and _semver_tuple( - driver["version"] - ) <= _semver_tuple(prior["version"]): - raise RepositoryError( - f"{driver['id']}: changed artifact needs a higher version than " - f"{prior['version']}" - ) + collisions = _version_collisions(manifest["drivers"], previous["drivers"]) + if collisions: + driver, prior = collisions[0] + raise RepositoryError( + f"{driver['id']}: changed artifact needs a higher version than " + f"{prior['version']}" + ) history: list[dict[str, Any]] = [] history_keys: set[tuple[str, str, str]] = set() for driver in [*previous["drivers"], *previous.get("history", [])]: @@ -961,6 +994,79 @@ def build_publication( return manifest +def check_publication( + *, + repo_root: Path, + config_path: Path, + previous_manifest_path: Path, + key_id: str, + expected_public_key_base64: str, +) -> dict[str, Any]: + """Ask the release's question of a tree nobody has merged yet. + + The version rule is the one release check no pull request could run, because + running it needed the signed channel -- so it ran on main, after the merge, + where a failure has already replaced the thing it was meant to guard. #53 + merged green and broke the release that way: it corrected a manifest field + and read it as catalog-only, not knowing the signed artifact carries it. + + Nothing about the question actually needs the signing key. The published + manifest is a release asset, its signature verifies with the public key, and + the bytes a driver would publish as come from the tree. So this builds the + artifacts in memory, signs nothing, and answers exactly what the release + would answer -- early enough to be a review comment rather than a red main. + + Reports every colliding driver instead of the first. The release stops at + one because it has nothing else to do; someone fixing a branch wants the + whole list in one run. + """ + configured = _load_channel(config_path, repo_root) + previous = verify_manifest( + previous_manifest_path, + key_id=key_id, + public_key_base64=expected_public_key_base64, + ) + drivers = [ + { + "id": entry["id"], + "version": entry["version"], + "sha256": _artifact_digest(entry), + } + for entry in configured + ] + previous_by_id = {driver["id"]: driver for driver in previous["drivers"]} + added = sorted( + driver["id"] for driver in drivers if driver["id"] not in previous_by_id + ) + changed = sorted( + driver["id"] + for driver in drivers + if driver["id"] in previous_by_id + and previous_by_id[driver["id"]]["sha256"] != driver["sha256"] + ) + collisions = _version_collisions(drivers, previous["drivers"]) + if collisions: + lines = "\n".join( + f" {driver['id']}: changed artifact needs a higher version than " + f"{prior['version']} (published) -- " + f"make bump-driver ID={driver['id']} LEVEL=patch" + for driver, prior in collisions + ) + raise RepositoryError( + "the signed channel would refuse this tree:\n" + f"{lines}\n" + "A published version names a set of bytes, so changed bytes need a " + "new version. The signed artifact carries the manifest metadata as " + "well as the Lua source, so a manifest-only edit can move them." + ) + return { + "published_commit": previous["commit"], + "drivers": len(drivers), + "added": added, + "changed": changed, + } + + def _parser() -> argparse.ArgumentParser: parser = argparse.ArgumentParser(description=__doc__) subparsers = parser.add_subparsers(dest="command", required=True) @@ -977,6 +1083,15 @@ def _parser() -> argparse.ArgumentParser: build.add_argument("--source-date-epoch", type=int, required=True) build.add_argument("--previous-manifest", type=Path) + check = subparsers.add_parser( + "check-versions", + help="check a tree against the published channel without signing anything", + ) + check.add_argument("--repo-root", type=Path, default=ROOT) + check.add_argument("--config", type=Path, default=ROOT / "ftw-channel.json") + check.add_argument("--previous-manifest", type=Path, required=True) + check.add_argument("--key-id", required=True) + verify = subparsers.add_parser("verify", help="verify a signed FTW publication") verify.add_argument("--manifest", type=Path, required=True) verify.add_argument("--artifacts", type=Path, required=True) @@ -1028,6 +1143,16 @@ def main() -> int: ), ) return 0 + if args.command == "check-versions": + report = check_publication( + repo_root=args.repo_root.resolve(), + config_path=args.config.resolve(), + previous_manifest_path=args.previous_manifest.resolve(), + key_id=args.key_id, + expected_public_key_base64=public_key, + ) + print(json.dumps(report, sort_keys=True)) + return 0 if args.command == "verify-stable-promotion": report = verify_stable_promotion( beta_manifest_path=args.beta_manifest.resolve(),