From 40a88ff0f2e2ac7956602367c3960d1cd6bb8786 Mon Sep 17 00:00:00 2001 From: Craig McChesney Date: Thu, 24 Sep 2026 15:01:44 -0600 Subject: [PATCH 1/4] plan: #56 + #30 -- release body as the notes file verbatim, and a mypy CI gate Triage found #56's root cause was wrong: the workflow created the rel-1.16.0 release (release created_at is the tagged commit's date, not the release's), and the body was replaced afterwards by a notes-file edit. The plan makes the notes file the whole release body, as the Java repos do, and checks it at PR time and after publishing. For #30, a trialled mypy config brings `mypy src/` from 426 errors to the 3 real ones; the typed-stub half moved to osprey-dcs/dp-grpc#158. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01Bqq8uGF2zNbHW9FU3vfj7Q --- plan/tickets/56/plan.md | 234 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 234 insertions(+) create mode 100644 plan/tickets/56/plan.md diff --git a/plan/tickets/56/plan.md b/plan/tickets/56/plan.md new file mode 100644 index 0000000..1117e89 --- /dev/null +++ b/plan/tickets/56/plan.md @@ -0,0 +1,234 @@ +# #56 + #30 — Release body as the notes file verbatim, and a mypy CI gate + +**Status:** decisions settled 2026-09-24; open questions resolved in place. + +## Overview + +One change closing two tickets, both about CI and release tooling: + +- **#56** — the rel-1.16.0 release page lost its artifact verification and install instructions, + and nothing noticed. This makes the reviewed notes file the *entire* release body, the same + arrangement the three Java repos use (dp-grpc#137, plan D9), so no path to the release page can + drop content the file contains, and checks that the published page matches the file. +- **#30** (near-term scope only) — makes `mypy src/` clean and enforces it in the CI `quality` job. + The typed-stub half moved to osprey-dcs/dp-grpc#158. + +For: release consumers (verification instructions that actually reach the page), and maintainers +(a type-check gate on hand-written client logic). + +## Background / triage findings + +### #56: the ticket's root cause is wrong + +#56 attributes the missing content to the release **already existing** when the workflow ran, +so that `action-gh-release` kept the old body. Its evidence is a timeline: release "created" +17:49:05Z, workflow started 18:02:17Z. Both halves of that are contradicted: + +- **GitHub's release `created_at` is the date of the tagged commit**, not when the release object + was created (documented API behavior). The rel-1.16.0 tag commit `17d2cdb` is dated + `2026-09-16T11:49:05-06:00` — 17:49:05Z exactly. rel-1.15.0 shows the same 35-minute "gap" + and its body is correct. +- **The workflow created the release.** The publish step's log (run 35131880732) reads + `Creating new GitHub release for tag rel-1.16.0...`, and the release's author is + `github-actions[bot]`. It received the full 27,145-byte `RELEASE_BODY.md` and passed it as + `body_path`. + +So the body was right when published and was **replaced afterwards**. The replacement is the +notes file alone: no verification section and no generated commit list, both of which only the +workflow adds. That fits `gh release edit rel-1.16.0 --notes-file doc/release-notes/rel-1.16.0.md`, +run while fixing the dp-grpc link. #56's own comment notes that the link was fixed on the page +first and only backported later (aa994d5, PR #53). **Confirmed (Q1):** a notes-file edit made +in another session. + +The page has since been corrected in place and now carries the verification and install +sections. It does **not** carry the generated commit list (rel-1.15.0's does). + +What this changes: the ticket's options A (`append_body`) and B (don't pre-create the release) +address something that didn't happen. Its underlying concern still stands, and is sharper than +it said: **the page and the file are two copies of the same content, and the obvious way to +republish from the file loses whatever the workflow added.** Option C removes the difference +between them. + +The issue description gets a triage note saying this. + +### #30: re-measured, and a config that reaches zero + +Numbers and scope in #30 were updated 2026-09-24 (426 errors / 27 files). Trialled locally: + +| Configuration | Result | +|---|---| +| none | 426 errors, 27 files | +| + `types-PyYAML`, `types-protobuf`, `pandas-stubs`; `ignore_missing_imports` for `grpc` | import-untyped cleared | +| + `follow_imports = "skip"` for `dp_python_lib.grpc.*` **alone** | still 391 — `src/` passes the generated files on the command line, so they are checked regardless | +| + `exclude = ["^src/dp_python_lib/grpc/"]` | **7 errors, 3 files** | +| + `TimestampInput: TypeAlias = ...` | **3 errors** — the real ones | + +The remaining 4 came from `TimestampInput`: once `common_pb2.Timestamp` resolves to `Any`, mypy +can no longer infer the union as an alias. An explicit `TypeAlias` annotation fixes it and is +correct regardless. + +**None of the 3 "real" errors is a runtime defect** (#30 calls them genuine; they are genuine +*type* errors): + +- `mldp_client.py:120,130` — `self.annotation` / `self.query` are legitimately `None`; the + annotations don't say so. +- `data_frame_conversions.py:209` — `data_frame_image_descriptors()` iterates + `frame.imageColumns`, so `image_descriptor_dict()` can never return `None` there, but mypy can't + prove it from a `column: Any` parameter. + +Two constraints found in the trial: + +- **Do not set `python_version = "3.10"`.** numpy's bundled stubs use the 3.12 `type` statement, + and mypy then refuses to check anything. mypy checks against the running interpreter (3.12 in + the `quality` job). The 3.10 unit-test leg and ruff's `target-version = "py310"` already guard + 3.10 syntax. +- **The cookbook checker reads `[tool.mypy]` too** (it runs mypy with `cwd=REPO_ROOT`). Verified + with the config in `pyproject.toml`: `OK: 107 snippets in 8 files (5 skipped)`, unchanged. + +## Design decisions + +**D1 — The notes file is the release body, verbatim.** `body_path` points at +`doc/release-notes/.md`, carried through the `dist/` upload as `RELEASE_NOTES.md`. The +assembly step and its heredoc go. Each release's notes carry a hand-written +"Verifying these artifacts" section, reviewed in the notes PR. Then the page equals the file, and +`gh release edit --notes-file` is a lossless republish rather than a destructive one. +*Rejected:* `append_body: true` (#56 option A), which fixes a pre-existing-release case that +didn't occur and would duplicate the notes if it did; keeping assembly plus a post-publish check +(the check would catch a bad publish, but not a later hand edit, which is what happened). + +**D2 — Add `README.env`, matching the Java repos.** A version-controlled +"Release Artifacts and Verification" reference: asset list, `sha256sum -c` (with the +`--ignore-missing` caveat), and `sigstore verify identity`. The per-release notes section stays +short and points to it. The name is odd for a Python repo, but dp-grpc D9 treats it as the +convention across all three Java repos, and four repos agreeing is worth more than a better name. +No Python convention competes with it: PyPA defines no file for release-verification +instructions, and projects put them in their README or docs. (Q3.) + +**D3 — Drop `generate_release_notes`.** It's the one part of the body not in the file, so it's +exactly what a republish from the file loses. The notes are organized by ticket and link their +PRs, and the Java repos don't set it. A hand-written `**Full Changelog**: …/compare/rel-A...rel-B` +line in the notes gives most of the value. (Q2: agreed.) + +**D4 — Check the notes content at PR time, not only at tag time.** A new +`.dev/tools/check-release-notes.py` (sibling of the cookbook checker) runs in the `quality` job +over every `doc/release-notes/rel-*.md`. It asserts a `## Verifying these artifacts` heading and +a `sigstore verify identity` command whose `--cert-identity` ends +`release.yml@refs/tags/`. The last check matters: the identity is now hand-written per +release, and a copy-pasted previous tag would verify nothing. release.yml's existing +"notes file must exist" step calls the same script on the tagged file. *Rejected:* tag-time only, +because by then the notes are merged and the tag is public, so a failure costs a re-tag. + +**D5 — After publishing, compare the page to the file.** A step after `action-gh-release` +fetches the body (`gh release view "$TAG" --json body`) and diffs it against `RELEASE_NOTES.md`, +ignoring trailing whitespace. With D3 the comparison is exact. This is the "nothing compares +assembled against published" gap #56 names, closed for the one path the workflow controls. +Later hand edits are covered by D6. + +**D6 — Rule: release pages are never hand-edited.** Fix the notes file by PR, then republish +with `gh release edit --notes-file doc/release-notes/.md`. Recorded in CLAUDE.md. +Lossless under D1 + D3. + +**D7 — Backport the verification section into `rel-1.16.0.md`.** The live page has it, and the +file doesn't, so D4 would fail on it and D6's republish would strip it again. After the backport, +file and page match (apart from the trailing newline). + +**D8 — mypy config in `pyproject.toml`**, exactly as trialled: +```toml +[tool.mypy] +exclude = ["^src/dp_python_lib/grpc/"] + +[[tool.mypy.overrides]] +module = ["dp_python_lib.grpc.*"] +follow_imports = "skip" + +[[tool.mypy.overrides]] +module = ["grpc", "grpc.*"] +ignore_missing_imports = true +``` +Each part carries a comment giving its reason, and a pointer to dp-grpc#158 for when the generated +package suppression can go. No `python_version` (see Background). *Rejected:* global +`ignore_missing_imports`, which would also hide a genuinely missing dependency. + +**D9 — Fix `data_frame_conversions.py:209` by restructuring, not by casting.** Split out a +private `_image_descriptor(column) -> dict[str, Any]` that assumes an `ImageColumn`. +`image_descriptor_dict()` keeps its `isinstance` guard and calls it, and +`data_frame_image_descriptors()` calls it directly. *Rejected:* `cast` or `# type: ignore`, which +would hide the claim instead of making it true by construction. + +## Implementation tasks + +### #56 + +- `.github/workflows/release.yml` + - Build job: replace "Assemble the release body" with copying the notes file to + `dist/RELEASE_NOTES.md`. Run `check-release-notes.py` on the tag's file in the existing + notes-presence step. Update the comment block that explains assembly and `body_path` + precedence. + - Publish job: `body_path: dist/RELEASE_NOTES.md`; remove `generate_release_notes` (D3); add + the D5 compare step (`GH_TOKEN: ${{ github.token }}`, `set -euo pipefail`). + - Sigstore signing: no change. +- `.dev/tools/check-release-notes.py` — new (D4). Stdlib only; exits non-zero with a message + naming the file and the missing or incorrect item. Takes optional paths (default: all + `doc/release-notes/rel-*.md`). +- `.github/workflows/ci.yml` — `quality` job: add a "Check release notes" step. +- `README.env` — new (D2). Assets: wheel, sdist, `SHA256SUMS`, and a `.sigstore.json` bundle for + each of the three. Verify all three in one `sigstore verify identity` call, not only the wheel + as today's body does. +- `doc/release-notes/rel-1.16.0.md` — append the verification and install sections exactly as on + the live page (D7). +- `CLAUDE.md` — rewrite the "Release notes" paragraph (no assembly; `body_path` is the file; D6 + rule; `README.env`). +- `README.md` — check for any mention of the release body or verification and align it. + +### #30 + +- `pyproject.toml` — D8 config; add `types-PyYAML`, `types-protobuf`, `pandas-stubs` to `[dev]`. +- `src/dp_python_lib/client/time_conversions.py` — `TimestampInput: TypeAlias = ...`. +- `src/dp_python_lib/client/mldp_client.py` — `AnnotationClient | None` / `QueryClient | None` + on the attribute declarations. +- `src/dp_python_lib/client/data_frame_conversions.py` — D9. +- `.github/workflows/ci.yml` — `quality` job: `mypy src/` step, after ruff. +- `.dev/tools/check-cookbook-snippets.py` — update the stale "~145 of its own mypy errors" + comment. `--follow-imports=silent` stays, since the snippets should not re-report library + errors either way. +- `CLAUDE.md` — CI description (quality job now runs mypy; mention the release-notes check); + `[dev]` extra contents. + +### Verification + +- `mypy src/` → `Success`; cookbook checker unchanged; `pytest tests/unit` green; ruff clean. +- `check-release-notes.py` passes on `rel-1.16.0.md` after D7, and fails on a copy with the tag in + the cert identity changed. +- `workflow_dispatch` rehearsal of release.yml on the branch: build and sign succeed. Publish and + the D5 compare are tag-gated, so they aren't exercised until rel-1.17.0. The compare step can + be dry-checked locally against rel-1.16.0 after D7 (`gh release view rel-1.16.0 --json body`). + +## Out of scope + +- Typed protobuf stubs and removing the D8 suppression — osprey-dcs/dp-grpc#158. +- `py.typed` marker — follow-up to dp-grpc#158 (typing the public surface is only useful once the + protos are typed). +- Env-var vs YAML precedence bug — #19, independent. +- Publishing to PyPI (the disabled job) — unchanged. + +## Dependencies and sequencing + +- **Must merge before the `rel-1.17.0` tag**, or that release publishes through the old assembly + path. +- Not blocked by dp-grpc#158, and doesn't block it. +- Not related to #19. +- D7 (the 1.16.0 backport) must land in the same PR as D4, or the new CI check fails on `main`. + +## Open questions + +All resolved 2026-09-24. + +**Q1 — Was rel-1.16.0's body replaced with `gh release edit --notes-file` (or equivalent)?** +*Resolved:* yes, confirmed by the maintainer. It was done in another session while fixing the +dp-grpc link. The Background section and the #56 triage note state it as the cause. + +**Q2 — Drop the generated commit list (D3)?** *Resolved:* drop it, and add a hand-written Full +Changelog compare link to each release's notes. + +**Q3 — `README.env` name (D2)?** *Resolved:* `README.env`. A Python-specific name would have +been fine, but none is conventional, so consistency with the Java repos wins. From 2d6ba5dfb84db8f858f844f36c3f4a1c0ec46f9a Mon Sep 17 00:00:00 2001 From: Craig McChesney Date: Thu, 24 Sep 2026 15:09:24 -0600 Subject: [PATCH 2/4] mypy src/ clean: config for the untyped gRPC package, and the 3 real errors (#30) - [tool.mypy]: exclude the generated grpc package and skip following imports into it (both needed), ignore_missing_imports scoped to grpc; no python_version, since numpy's stubs need 3.12 syntax. types-PyYAML, types-protobuf, pandas-stubs join [dev]. - TimestampInput gets an explicit TypeAlias; timestamp_list() takes a Sequence. - MldpClient.annotation / .query are typed X | None, which they are. - data_frame_image_descriptors() uses a private _image_descriptor(), non-optional by construction, instead of image_descriptor_dict()'s None arm. - Cookbook checker preamble narrows client.annotation/.query once. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01Bqq8uGF2zNbHW9FU3vfj7Q --- .dev/tools/check-cookbook-snippets.py | 12 +++++-- pyproject.toml | 33 +++++++++++++++++-- src/dp_python_lib/client/data_frame.py | 3 +- .../client/data_frame_conversions.py | 12 ++++++- src/dp_python_lib/client/mldp_client.py | 4 +-- src/dp_python_lib/client/time_conversions.py | 3 +- 6 files changed, 56 insertions(+), 11 deletions(-) diff --git a/.dev/tools/check-cookbook-snippets.py b/.dev/tools/check-cookbook-snippets.py index ca173a0..59924be 100755 --- a/.dev/tools/check-cookbook-snippets.py +++ b/.dev/tools/check-cookbook-snippets.py @@ -126,6 +126,12 @@ from dp_python_lib.client import data_frame_conversions as dfc client: MldpClient = MldpClient() +# client.annotation and client.query are typed `X | None`, which is honest: they are None when MldpClient is given +# only an ingestion channel (connecting.md, "Sub-clients can be None"). A client built from configuration, as every +# recipe's is, always has both, so narrow once here rather than in every recipe. Do NOT instead disable union-attr: +# on an `X | None` receiver that code also carries the "no such attribute on X" error, so the self-test canary stops +# firing and every misspelled name under client.annotation passes. +assert client.annotation is not None and client.query is not None begin: datetime = datetime(2024, 1, 1, tzinfo=timezone.utc) end: datetime = datetime(2024, 1, 2, tzinfo=timezone.utc) @@ -266,9 +272,9 @@ def check_types(snippets: list[Snippet], verbose: bool) -> list[str]: *MYPY_CMD, # The generated gRPC stubs are untyped; without this every `import ..._pb2` is an error. "--ignore-missing-imports", - # Type-check the snippets but NOT the library itself. dp_python_lib currently has - # ~145 of its own mypy errors (mostly untyped protobuf stubs); those are not this - # tool's business and would bury real snippet errors. + # Type-check the snippets but NOT the library itself. The library is checked by + # `mypy src/` in CI's quality job; re-reporting its errors here would bury real + # snippet errors. (The [tool.mypy] config in pyproject.toml applies here too.) "--follow-imports=silent", # Snippets are illustrative; unreachable/redundant warnings are noise here. "--no-warn-unused-ignores", diff --git a/pyproject.toml b/pyproject.toml index d4356fb..8740739 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -55,11 +55,17 @@ analysis = [ "openpyxl", ] -# Development tooling. Not required to use the library. mypy backs the cookbook snippet -# checker (.dev/tools/check-cookbook-snippets.py), which type-checks every example in -# doc/cookbook against this package to catch wrong attribute/method names in the docs. +# Development tooling. Not required to use the library. mypy runs in CI as `mypy src/` +# (config under [tool.mypy]) and also backs the cookbook snippet checker +# (.dev/tools/check-cookbook-snippets.py), which type-checks every example in doc/cookbook +# against this package to catch wrong attribute/method names in the docs. The types-* and +# pandas-stubs packages are the third-party stubs mypy needs to check against real +# signatures rather than treating those imports as untyped. dev = [ "mypy", + "types-PyYAML", + "types-protobuf", + "pandas-stubs", "pytest", "ruff", "build", @@ -92,6 +98,27 @@ markers = [ "integration: requires a live MLDP ecosystem on localhost:50051-50053 (deselect with '-m \"not integration\"')", ] +[tool.mypy] +# No python_version: numpy's bundled stubs use the 3.12 `type` statement, and pinning 3.10 +# makes mypy refuse to check anything. mypy checks against the running interpreter (3.12 in +# CI's quality job); the 3.10 unit-test leg and ruff's target-version guard 3.10 syntax. +# +# The generated gRPC package is untyped (no .pyi stubs; osprey-dcs/dp-grpc#158), so it is +# excluded from checking and its imports are resolved to Any. Both are needed: `exclude` +# alone still follows imports into it, and `follow_imports = "skip"` alone does nothing for +# files `mypy src/` names on the command line. Remove both once typed stubs are synced. +exclude = ["^src/dp_python_lib/grpc/"] + +[[tool.mypy.overrides]] +module = ["dp_python_lib.grpc.*"] +follow_imports = "skip" + +# grpcio ships no type information and there is no maintained stub package for it. Scoped +# to grpc rather than set globally, which would also hide a genuinely missing dependency. +[[tool.mypy.overrides]] +module = ["grpc", "grpc.*"] +ignore_missing_imports = true + [tool.ruff] # 120 matches the margin this codebase was already written to: at line-length 100 there were # 307 violations, nearly all docstrings and long string literals that the formatter will not diff --git a/src/dp_python_lib/client/data_frame.py b/src/dp_python_lib/client/data_frame.py index 9ffb87b..5d9ae7c 100644 --- a/src/dp_python_lib/client/data_frame.py +++ b/src/dp_python_lib/client/data_frame.py @@ -22,6 +22,7 @@ the ones its builders return. """ +from collections.abc import Sequence from numbers import Integral, Real from typing import Any @@ -110,7 +111,7 @@ def sampling_clock( return timestamps -def timestamp_list(values: list[TimestampInput]) -> common_pb2.DataTimestamps: +def timestamp_list(values: Sequence[TimestampInput]) -> common_pb2.DataTimestamps: """ Builds a DataTimestamps with an explicit TimestampList time axis, the form for irregularly-spaced samples -- and, for sample status, for sparse labeling that names only the samples being labeled. diff --git a/src/dp_python_lib/client/data_frame_conversions.py b/src/dp_python_lib/client/data_frame_conversions.py index e921a9f..50e875e 100644 --- a/src/dp_python_lib/client/data_frame_conversions.py +++ b/src/dp_python_lib/client/data_frame_conversions.py @@ -173,6 +173,16 @@ def image_descriptor_dict(column: Any) -> dict[str, Any] | None: """ if not isinstance(column, common_pb2.ImageColumn): return None + return _image_descriptor(column) + + +def _image_descriptor(column: common_pb2.ImageColumn) -> dict[str, Any]: + """ + The descriptor dict for a column already known to be an ImageColumn. + + Split out so data_frame_image_descriptors(), which iterates imageColumns and so never sees another kind, gets a + non-optional dict by construction rather than by a cast over image_descriptor_dict()'s None arm. + """ descriptor = column.imageDescriptor return { "width": descriptor.width, @@ -206,7 +216,7 @@ def data_frame_image_descriptors(frame: common_pb2.DataFrame) -> dict[str, dict[ :param frame: The frame to inspect. :return: A dict of column name -> descriptor dict; empty when the frame has no image columns. """ - return {column.name: image_descriptor_dict(column) for column in frame.imageColumns} + return {column.name: _image_descriptor(column) for column in frame.imageColumns} def data_frame_schema_ids(frame: common_pb2.DataFrame) -> dict[str, str]: diff --git a/src/dp_python_lib/client/mldp_client.py b/src/dp_python_lib/client/mldp_client.py index e10c2bc..fa00033 100644 --- a/src/dp_python_lib/client/mldp_client.py +++ b/src/dp_python_lib/client/mldp_client.py @@ -114,7 +114,7 @@ def __init__( # Annotation service facade (exposes .pv_metadata, etc.); only created if a channel is available if self._annotation_channel is not None: self.logger.info("Initializing annotation client") - self.annotation = AnnotationClient(self._annotation_channel) + self.annotation: AnnotationClient | None = AnnotationClient(self._annotation_channel) else: self.logger.debug("No annotation channel provided - annotation client will be None") self.annotation = None @@ -124,7 +124,7 @@ def __init__( # the .annotation facade. if self._query_channel is not None: self.logger.info("Initializing query client") - self.query = QueryClient(self._query_channel) + self.query: QueryClient | None = QueryClient(self._query_channel) else: self.logger.debug("No query channel provided - query client will be None") self.query = None diff --git a/src/dp_python_lib/client/time_conversions.py b/src/dp_python_lib/client/time_conversions.py index 249ed75..32cfb64 100644 --- a/src/dp_python_lib/client/time_conversions.py +++ b/src/dp_python_lib/client/time_conversions.py @@ -20,12 +20,13 @@ import math from datetime import datetime, timezone +from typing import TypeAlias from dp_python_lib.grpc import common_pb2 # Accepted input types for API parameters that map to a common.Timestamp: # a timezone-aware datetime, epoch seconds (int or float), or an already-built Timestamp. -TimestampInput = datetime | int | float | common_pb2.Timestamp +TimestampInput: TypeAlias = datetime | int | float | common_pb2.Timestamp NANOS_PER_SECOND = 1_000_000_000 From 700d2edf44776ad86ee220836db1facf340d4629 Mon Sep 17 00:00:00 2001 From: Craig McChesney Date: Thu, 24 Sep 2026 15:09:24 -0600 Subject: [PATCH 3/4] Release body is the notes file verbatim; check it at PR time and after publishing (#56, #30) - release.yml: stage doc/release-notes/.md as the body with nothing appended; drop generate_release_notes; diff the published body against the file after publishing. - .dev/tools/check-release-notes.py: every notes file needs a verification section whose --cert-identity names its own tag. Runs in CI's quality job and at tag time. - ci.yml quality job: add mypy src/ and the release-notes check. - README.env: release artifacts and verification reference, as in the Java repos. - rel-1.16.0.md: backport the verification section the live page carries. - CLAUDE.md / README.md: the new release-notes contract, the no-hand-edits rule, mypy. - Plan: implementation notes. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01Bqq8uGF2zNbHW9FU3vfj7Q --- .dev/tools/check-release-notes.py | 105 +++++++++++++++++++++++++ .github/workflows/ci.yml | 13 +++- .github/workflows/release.yml | 96 ++++++++++++----------- CLAUDE.md | 50 ++++++++++-- README.env | 123 ++++++++++++++++++++++++++++++ README.md | 9 ++- doc/release-notes/rel-1.16.0.md | 23 ++++++ plan/tickets/56/plan.md | 23 ++++++ 8 files changed, 381 insertions(+), 61 deletions(-) create mode 100755 .dev/tools/check-release-notes.py create mode 100644 README.env diff --git a/.dev/tools/check-release-notes.py b/.dev/tools/check-release-notes.py new file mode 100755 index 0000000..8c49195 --- /dev/null +++ b/.dev/tools/check-release-notes.py @@ -0,0 +1,105 @@ +#!/usr/bin/env python3 +"""Verify that every doc/release-notes/rel-X.Y.Z.md carries a correct verification section. + +The notes file is published verbatim as the GitHub release body (release.yml; plan/tickets/56/plan.md D1), so +the artifact verification instructions exist only if the file contains them. This checks, per file: + + - the file name is rel-X.Y.Z.md (release.yml derives the name from the tag, so nothing else is ever published); + - there is a `## Verifying these artifacts` heading; + - there is at least one `sigstore verify identity` command, and every `--cert-identity` in the file is exactly + this repository's release.yml at *this file's* tag, with the GitHub Actions OIDC issuer. + +The identity check is the one that earns its keep. The section is hand-written per release, usually by copying +the previous one, and a stale tag in the identity makes `sigstore verify` reject every genuine artifact of the +release ("Certificate's SANs do not match"). Readers would reasonably conclude the release is forged. + +Runs in CI's quality job over every notes file, so a mistake fails the notes PR rather than the tag push; and in +release.yml on the tagged file, as a backstop. + +Usage: + python .dev/tools/check-release-notes.py [FILE ...] + +With no arguments, checks every doc/release-notes/rel-*.md. Stdlib only. Exits 0 if all pass, 1 otherwise. +""" + +from __future__ import annotations + +import re +import sys +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parents[2] +NOTES_DIR = REPO_ROOT / "doc" / "release-notes" + +REPOSITORY = "osprey-dcs/dp-python-lib" +OIDC_ISSUER = "https://token.actions.githubusercontent.com" + +STEM_RE = re.compile(r"^rel-\d+\.\d+\.\d+$") +HEADING_RE = re.compile(r"^## Verifying these artifacts\s*$", re.MULTILINE) +VERIFY_RE = re.compile(r"\bsigstore\s+verify\s+identity\b") +IDENTITY_RE = re.compile(r"--cert-identity[ =]\"?([^\"\s]+)\"?") +ISSUER_RE = re.compile(r"--cert-oidc-issuer[ =]\"?([^\"\s]+)\"?") + + +def expected_identity(tag: str) -> str: + return f"https://github.com/{REPOSITORY}/.github/workflows/release.yml@refs/tags/{tag}" + + +def check_file(path: Path) -> list[str]: + """Returns one message per problem found in `path`; empty when it passes.""" + tag = path.stem + if path.suffix != ".md" or not STEM_RE.match(tag): + return [f"{path}: name must be rel-X.Y.Z.md (release.yml looks the notes up by tag)"] + + text = path.read_text(encoding="utf-8") + problems: list[str] = [] + + if not HEADING_RE.search(text): + problems.append(f"{path}: missing a '## Verifying these artifacts' section") + + if not VERIFY_RE.search(text): + problems.append(f"{path}: missing a 'sigstore verify identity' command") + + identities = IDENTITY_RE.findall(text) + if not identities: + problems.append(f"{path}: missing --cert-identity") + want = expected_identity(tag) + for identity in identities: + if identity != want: + problems.append(f"{path}: --cert-identity is\n {identity}\n expected\n {want}") + + issuers = ISSUER_RE.findall(text) + if not issuers: + problems.append(f"{path}: missing --cert-oidc-issuer") + for issuer in issuers: + if issuer != OIDC_ISSUER: + problems.append(f"{path}: --cert-oidc-issuer is {issuer}, expected {OIDC_ISSUER}") + + return problems + + +def main(argv: list[str]) -> int: + paths = [Path(arg) for arg in argv] if argv else sorted(NOTES_DIR.glob("rel-*.md")) + if not paths: + print(f"FAIL: no release notes found under {NOTES_DIR}") + return 1 + + problems: list[str] = [] + for path in paths: + if not path.is_file(): + problems.append(f"{path}: not found") + continue + problems.extend(check_file(path)) + + if problems: + print(f"FAIL: {len(problems)} problem(s) in {len(paths)} release notes file(s)\n") + for problem in problems: + print(f" {problem}") + return 1 + + print(f"OK: {len(paths)} release notes file(s)") + return 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv[1:])) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 40bcb18..914c721 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -57,7 +57,7 @@ jobs: run: pytest tests/unit -v -m "not integration" quality: - name: Lint, format, and docs + name: Lint, type-check, and docs runs-on: ubuntu-latest # These checks are interpreter-independent, so they run once rather than on all four # matrix legs -- no added signal, 4x the runtime. @@ -85,11 +85,22 @@ jobs: - name: Ruff format check run: ruff format --check . + - name: Type check + # Config under [tool.mypy] in pyproject.toml. The generated gRPC package is excluded + # until it ships typed stubs (osprey-dcs/dp-grpc#158); everything hand-written is checked. + run: mypy src/ + - name: Check cookbook snippets # Type-checks every Python example in doc/cookbook against this package, so a recipe # referencing a renamed attribute or method fails here rather than in a user's hands. run: python .dev/tools/check-cookbook-snippets.py + - name: Check release notes + # Each doc/release-notes/rel-X.Y.Z.md is published verbatim as its release body, so it + # must carry a verification section whose signing identity names its own tag. Checked + # here so a mistake fails the notes PR, not the tag push. + run: python .dev/tools/check-release-notes.py + build: name: Build distributions runs-on: ubuntu-latest diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index b9e7208..bd7782f 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -73,13 +73,17 @@ jobs: echo "Manual dispatch; version comes from setuptools-scm." fi - - name: Verify release notes exist + - name: Verify release notes id: notes # Checked here, before the build, rather than left to action-gh-release: that action # fails on a missing body_path only after everything has been built, signed, and # uploaded, which reads as a late and unrelated failure. A rehearsal (workflow_dispatch) # has no rel- tag and so no notes to look for, hence the push-only guard. # Every release from 1.16.0 on ships notes; see CLAUDE.md "Cutting a release". + # + # The notes file is the entire release body, so it must carry its own verification + # section with this tag's signing identity. CI's quality job runs the same checker + # on every notes file at PR time; this is the backstop for a tag pushed anyway. if: github.event_name == 'push' run: | set -euo pipefail @@ -89,6 +93,7 @@ jobs: echo "::error::Add the notes for ${{ steps.version.outputs.version }} and retag once they are on the tagged commit." exit 1 fi + python .dev/tools/check-release-notes.py "$NOTES" echo "notes=$NOTES" >> "$GITHUB_OUTPUT" - name: Build wheel and sdist @@ -144,48 +149,19 @@ jobs: with: inputs: ./dist/*.whl ./dist/*.tar.gz ./dist/SHA256SUMS - - name: Assemble the release body - # The publish job downloads artifacts but never checks out the repo, so the notes - # have to travel with the dist/ upload. They are concatenated with the verification - # and install instructions here rather than passed to action-gh-release as `body`, - # because `body_path` takes precedence over `body` outright (it is a fallback, not a - # companion) -- setting both would silently drop these instructions from the release. - # action-gh-release appends its generated commit list after whatever body it resolves, - # so generate_release_notes still composes on top of this. + - name: Stage the release notes + # The notes file is the release body, verbatim -- nothing is added to it here. That is + # what makes `gh release edit --notes-file doc/release-notes/.md` a lossless + # republish: anything a workflow appended would be exactly what such an edit drops, which + # is how rel-1.16.0's page lost its verification instructions (#56). The verification + # section is therefore hand-written in each notes file and checked above. + # + # The publish job downloads artifacts but never checks out the repo, so the notes travel + # with the dist/ upload. Staged after signing, so they are not signed as an artifact. if: github.event_name == 'push' env: NOTES: ${{ steps.notes.outputs.notes }} - run: | - set -euo pipefail - { - cat "$NOTES" - cat <<'BODY' - - ## Verifying these artifacts - - Checksums: - - ```bash - sha256sum -c SHA256SUMS - ``` - - Signatures (keyless Sigstore; `pip install sigstore`): - - ```bash - sigstore verify identity \ - --cert-identity "https://github.com/${{ github.repository }}/.github/workflows/release.yml@${{ github.ref }}" \ - --cert-oidc-issuer "https://token.actions.githubusercontent.com" \ - dp_python_lib-*.whl - ``` - - ## Installing - - ```bash - pip install dp_python_lib-*.whl - ``` - BODY - } > dist/RELEASE_BODY.md - cat dist/RELEASE_BODY.md + run: cp "$NOTES" dist/RELEASE_NOTES.md - name: Upload build outputs uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 @@ -222,13 +198,35 @@ jobs: dist/*.tar.gz dist/SHA256SUMS dist/*.sigstore.json - generate_release_notes: true fail_on_unmatched_files: true - # Assembled in the build job: the hand-written doc/release-notes/rel-X.Y.Z.md - # followed by the verification and install instructions. RELEASE_BODY.md is - # deliberately absent from `files` above -- it is the release body, not an - # artifact to download. - body_path: dist/RELEASE_BODY.md + # doc/release-notes/.md, verbatim. RELEASE_NOTES.md is deliberately absent + # from `files` above -- it is the release body, not an artifact to download. + # + # generate_release_notes is deliberately NOT set: GitHub's commit list would be the + # one part of the body not in the file, and so the part a republish from the file + # silently drops. The notes carry a hand-written Full Changelog link instead. + body_path: dist/RELEASE_NOTES.md + + - name: Verify the published body matches the notes + # Closes the loop for the one path this workflow controls: if the action ever resolves + # a different body (a changed default, a pre-existing release it declines to update), + # the release is flagged here instead of being noticed by a reader. Later hand edits of + # the page are out of reach of any workflow; CLAUDE.md forbids them instead. + env: + GH_TOKEN: ${{ github.token }} + GH_REPO: ${{ github.repository }} + TAG: ${{ github.ref_name }} + run: | + set -euo pipefail + gh release view "$TAG" --json body --jq .body > published.md + # GitHub does not preserve the file's trailing newline, so both sides go through + # "$(cat ...)", which strips trailing newlines; CRLF line ends are normalized too. + if ! diff -u --strip-trailing-cr <(printf '%s\n' "$(cat dist/RELEASE_NOTES.md)") \ + <(printf '%s\n' "$(cat published.md)"); then + echo "::error::The published release body differs from doc/release-notes/${TAG}.md." + exit 1 + fi + echo "Published body matches doc/release-notes/${TAG}.md." # --------------------------------------------------------------------------------------- # PyPI publishing -- WIRED UP BUT INTENTIONALLY DISABLED. @@ -264,9 +262,9 @@ jobs: path: dist - name: Remove non-distribution files - # Only sdists and wheels may be uploaded; the checksums file, the assembled release - # body, and the Sigstore bundles would all be rejected. - run: rm -f dist/SHA256SUMS dist/RELEASE_BODY.md dist/*.sigstore.json + # Only sdists and wheels may be uploaded; the checksums file, the release notes, and + # the Sigstore bundles would all be rejected. + run: rm -f dist/SHA256SUMS dist/RELEASE_NOTES.md dist/*.sigstore.json - name: Publish to PyPI uses: pypa/gh-action-pypi-publish@dc37677b2e1c63e2034f94d8a5b11f265b73ba33 # v1.14.2 diff --git a/CLAUDE.md b/CLAUDE.md index fb52c25..470653e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -49,12 +49,30 @@ a comment saying why, rather than reshaping correct code to satisfy the linter. examples: the naive-datetime test inputs (`DTZ001`) that exist precisely to assert a `ValueError`, and the numpy import that doubles as the `[analysis]` availability probe (`F401`). +### Type Checking +```bash +mypy src/ # what CI runs; must report "Success" +``` +Configured under `[tool.mypy]` in `pyproject.toml`. The generated `src/dp_python_lib/grpc/` +package has no type information, so it is excluded and its imports resolve to `Any` -- both an +`exclude` and a `follow_imports = "skip"` override are needed, and the comment there says why. +They go once dp-grpc ships typed stubs (osprey-dcs/dp-grpc#158), at which point a `py.typed` +marker becomes worth adding. There is deliberately no `python_version`: numpy's stubs use 3.12 +syntax, and pinning 3.10 stops mypy checking anything. Two consequences worth knowing: + +- An alias whose union includes a proto type needs an explicit `TypeAlias` annotation + (`TimestampInput: TypeAlias = ...`); with the proto resolving to `Any`, mypy no longer infers it. +- `client.annotation` / `client.query` are typed `X | None`, because they are. The cookbook + checker's preamble narrows them once; do **not** switch the checker to + `--disable-error-code=union-attr` instead, which also hides misspelled names reached through + them (the checker's self-test fails if you try). + ### Continuous Integration and Releases GitHub Actions workflows live in `.github/workflows/`: - **`ci.yml`** — runs on PRs targeting `main` and on pushes to `main`. Three jobs: unit tests across Python 3.10–3.13; a single-interpreter quality job (ruff lint, - ruff format check, cookbook snippet checker); and a build job that produces the + ruff format check, `mypy src/`, cookbook snippet checker, release-notes checker); and a build job that produces the wheel/sdist, runs `twine check --strict`, and verifies the wheel imports in a clean venv. Integration tests are deselected with `-m "not integration"`. - **`release.yml`** — runs on `rel-X.Y.Z` tag pushes. Builds the wheel and sdist, @@ -91,12 +109,27 @@ a note by issue ticket rather than by PR, since a ticket often spans several PRs a breaking release with an "Upgrading from " checklist that separates silent behavior changes from outright errors. -`release.yml` publishes the document as the GitHub release body: the build job concatenates -it with the artifact verification and install instructions into `dist/RELEASE_BODY.md`, -which the publish job passes as `body_path` (that job never checks out the repo, so the -notes travel with the `dist/` upload). Note `body_path` *overrides* `body` rather than -complementing it, so those instructions belong in the assembled file, not in a `body:` input. -GitHub's own generated commit list is appended after all of it. +**The notes file is the release body, verbatim** (#56; `plan/tickets/56/plan.md`), the same +arrangement as the Java repos. `release.yml` copies it to `dist/RELEASE_NOTES.md` (the publish job +never checks out the repo, so it travels with the `dist/` upload) and passes that as `body_path`; +nothing is appended, and `generate_release_notes` is deliberately off. A step after publishing +diffs the live body against the file. So each notes file must carry, by hand: + +- a `## Verifying these artifacts` section: `sha256sum -c SHA256SUMS`, then + `sigstore verify identity` over the wheel, sdist, and `SHA256SUMS`, with `--cert-identity` + exactly `https://github.com/osprey-dcs/dp-python-lib/.github/workflows/release.yml@refs/tags/`, + plus a pointer to `README.env`, the full verification reference; +- a `**Full Changelog**: https://github.com/osprey-dcs/dp-python-lib/compare/rel-...rel-` line, + standing in for GitHub's generated commit list. + +`.dev/tools/check-release-notes.py` enforces the section and, above all, the tag in the identity: +the section is usually copied from the previous release, and a stale tag makes `sigstore verify` +reject every genuine artifact. It runs in CI's quality job and again at tag time. + +**Never edit a release page by hand.** Fix the notes file by PR, then republish with +`gh release edit rel-X.Y.Z --notes-file doc/release-notes/rel-X.Y.Z.md`. With the file as the +whole body that is lossless; when the workflow appended a verification section, exactly this +command silently stripped it from rel-1.16.0's page (#56). The build job fails early — before building — if `doc/release-notes/.md` is missing on the tagged commit, so **write the notes and merge them before pushing the `rel-*` tag**. A @@ -112,7 +145,8 @@ Core dependencies are managed in `pyproject.toml`: Optional extras: - `[analysis]` - `pandas`, `numpy`, `openpyxl` for the query-result conversions -- `[dev]` - `pytest`, `mypy`, `ruff`; install with `pip install -e ".[analysis,dev]"` +- `[dev]` - `pytest`, `mypy` (with `types-PyYAML`, `types-protobuf`, `pandas-stubs`), `ruff`, `build`, `twine`; + install with `pip install -e ".[analysis,dev]"` ## Ticket Planning Workflow diff --git a/README.env b/README.env new file mode 100644 index 0000000..3d49ee8 --- /dev/null +++ b/README.env @@ -0,0 +1,123 @@ +dp-python-lib — Release Artifacts and Verification +================================================== + +This repository publishes versioned dp-python-lib Python distributions via +GitHub Releases. Each release corresponds to a Git tag of the form: + + rel- + +Example: + rel-1.16.0 + +------------------------------------------------------------ +Release Contents +------------------------------------------------------------ + +Each release contains: + + dp_python_lib--py3-none-any.whl + The wheel. This is what you install. + + dp_python_lib-.tar.gz + The source distribution. + + SHA256SUMS + SHA-256 checksums for the two files above, in sha256sum format. + + dp_python_lib--py3-none-any.whl.sigstore.json + dp_python_lib-.tar.gz.sigstore.json + SHA256SUMS.sigstore.json + Keyless Sigstore signature bundles, one per file above. + +Releases before rel-1.16.0 may lack SHA256SUMS or the Sigstore bundles; the +verification steps below do not apply to them. + +------------------------------------------------------------ +Download and Installation +------------------------------------------------------------ + +1. Download the artifacts from the GitHub Release page into a single directory, + with no subdirectories. Both commands below expect to find the files side + by side. With the GitHub CLI: + + gh release download rel- -R osprey-dcs/dp-python-lib + +2. Verify the checksums: + + sha256sum -c SHA256SUMS + + (On macOS without GNU coreutils: shasum -a 256 -c SHA256SUMS) + + The output should indicate: + + dp_python_lib--py3-none-any.whl: OK + dp_python_lib-.tar.gz: OK + + SHA256SUMS lists both distributions, so this fails if you downloaded only + the wheel. To check the files you actually have and ignore the rest: + + sha256sum --ignore-missing -c SHA256SUMS + + Note that --ignore-missing succeeds if it checked nothing at all, so confirm + the file you care about is listed as OK rather than relying on the exit code. + +3. Verify the signatures. + + The checksums establish integrity but not origin: they are published to the + same release page as the artifacts, so anyone able to replace a file could + replace its checksum alongside it. The Sigstore signatures close that gap by + binding each file to the repository, workflow file, and tag that produced + it. There is no public key to fetch or trust: the signing identity is a + short-lived certificate issued to the GitHub Actions run and recorded in the + public Rekor transparency log. + + Install the Sigstore client: + + pip install sigstore + + Then, substituting the release's tag in both places: + + sigstore verify identity \ + --cert-identity "https://github.com/osprey-dcs/dp-python-lib/.github/workflows/release.yml@refs/tags/rel-" \ + --cert-oidc-issuer "https://token.actions.githubusercontent.com" \ + dp_python_lib-*.whl dp_python_lib-*.tar.gz SHA256SUMS + + Each file's bundle is found automatically as .sigstore.json. The + output should be one line per file: + + OK: dp_python_lib--py3-none-any.whl + OK: dp_python_lib-.tar.gz + OK: SHA256SUMS + + The --cert-identity is an exact match, pinned to this repository, this + workflow file, and one tag. A signature made by any other workflow, any + other repository, or the run for a different tag fails with + "Certificate's SANs do not match". Do not loosen it to make a failure go + away; a mismatch is the check doing its job. + +4. Install the wheel: + + pip install dp_python_lib--py3-none-any.whl + + Optional extras are installed the usual way, e.g.: + + pip install "dp_python_lib--py3-none-any.whl[analysis]" + +------------------------------------------------------------ +Release Notes +------------------------------------------------------------ + +The body of each GitHub Release is the version-controlled file +doc/release-notes/rel-.md, published verbatim. Each carries a short +"Verifying these artifacts" section with that release's exact identity; this +file is the fuller reference it points to. + +------------------------------------------------------------ +Notes +------------------------------------------------------------ + +- dp-python-lib is a client library; it does not run as a standalone service. +- It talks to the MLDP services implemented in dp-service, over the gRPC API + defined in dp-grpc. +- Publishing to PyPI is not enabled yet; GitHub Releases are the distribution + channel. diff --git a/README.md b/README.md index e9a4b86..65ee8b6 100644 --- a/README.md +++ b/README.md @@ -129,8 +129,8 @@ older than your `dp_python_lib` will not implement everything listed here. The **Project infrastructure** - Publishing to PyPI. The release workflow has the job wired up but disabled; everything else — - unit tests across Python 3.10-3.13, lint and format checks, the cookbook snippet checker, and - signed release artifacts — runs in CI today. + unit tests across Python 3.10-3.13, lint, format, and type checks, the cookbook snippet checker, + and signed release artifacts — runs in CI today. ## Installation @@ -143,10 +143,13 @@ pip install -e . # with pandas / NumPy / Excel conversions for query results pip install -e .[analysis] -# development tooling (pytest, mypy for the cookbook snippet checker) +# development tooling (pytest, ruff, mypy) pip install -e .[dev] ``` +Released wheels are also attached to each [GitHub release](https://github.com/osprey-dcs/dp-python-lib/releases), +with checksums and Sigstore signatures; [`README.env`](README.env) says how to verify them. + **Upgrading from 1.15.0 or earlier:** 1.16.0 raises the `grpcio` floor to 1.84.0, because the regenerated stubs require it. `pip install` picks that up, but an existing editable install will not upgrade it on its own — the stubs then fail at import with a version mismatch naming the diff --git a/doc/release-notes/rel-1.16.0.md b/doc/release-notes/rel-1.16.0.md index dfda6ab..dbe6f79 100644 --- a/doc/release-notes/rel-1.16.0.md +++ b/doc/release-notes/rel-1.16.0.md @@ -407,3 +407,26 @@ in fact been a no-op since before `rel-1.15.0`. [plan-40]: https://github.com/osprey-dcs/dp-python-lib/blob/rel-1.16.0/plan/tickets/40/plan.md [plan-41]: https://github.com/osprey-dcs/dp-python-lib/blob/rel-1.16.0/plan/tickets/41/plan.md [plan-readme]: https://github.com/osprey-dcs/dp-python-lib/blob/rel-1.16.0/plan/README.md + +## Verifying these artifacts + +Checksums: + +```bash +sha256sum -c SHA256SUMS +``` + +Signatures (keyless Sigstore; `pip install sigstore`): + +```bash +sigstore verify identity \ + --cert-identity "https://github.com/osprey-dcs/dp-python-lib/.github/workflows/release.yml@refs/tags/rel-1.16.0" \ + --cert-oidc-issuer "https://token.actions.githubusercontent.com" \ + dp_python_lib-*.whl +``` + +## Installing + +```bash +pip install dp_python_lib-*.whl +``` diff --git a/plan/tickets/56/plan.md b/plan/tickets/56/plan.md index 1117e89..af5d5ff 100644 --- a/plan/tickets/56/plan.md +++ b/plan/tickets/56/plan.md @@ -232,3 +232,26 @@ Changelog compare link to each release's notes. **Q3 — `README.env` name (D2)?** *Resolved:* `README.env`. A Python-specific name would have been fine, but none is conventional, so consistency with the Java repos wins. + +## Implementation notes + +Found while implementing, 2026-09-24: + +- **The `mldp_client.py` fix moved errors into the cookbook, not out of existence.** Typing + `client.annotation` / `client.query` as `X | None` is correct (they are `None` when only an + ingestion channel is passed; `doc/cookbook/connecting.md`), but it made 83 cookbook snippets + fail `union-attr`. The trial in Background measured the config, not this change. Fixed with a + single narrowing `assert` in the checker's preamble. `--disable-error-code=union-attr` was tried + first and rejected: on an `X | None` receiver that code also carries "no attribute on X", so it + hid misspelled names too, and the checker's self-test canary failed. End users are unaffected + until a `py.typed` marker ships; at that point, revisit whether an unconfigured sub-client should + raise on access instead of being `None`. +- **`timestamp_list()` now takes `Sequence[TimestampInput]`**, not `list[...]`. With the explicit + alias, a `list[datetime]` argument failed on list invariance (2 cookbook errors). +- **D5 diffs with `--strip-trailing-cr`**, not `--ignore-trailing-space`, which BSD `diff` lacks, + so the step can be dry-run on macOS. Trailing newlines are normalized by `"$(cat …)"`. + Dry-run against rel-1.16.0: matches; a one-line change fails. +- **The D4 rationale, corrected:** a stale tag in `--cert-identity` doesn't "verify nothing". + `sigstore verify` *rejects* every genuine artifact ("Certificate's SANs do not match"), which + readers would take as a forged release. Confirmed against the rel-1.16.0 assets, which verify + with the correct identity (wheel, sdist, and `SHA256SUMS` in one call, as `README.env` shows). From 41cb36f4b16cbe2041c8da8beb3320941ae09618 Mon Sep 17 00:00:00 2001 From: Craig McChesney Date: Thu, 24 Sep 2026 15:20:17 -0600 Subject: [PATCH 4/4] Release notes checker covers the changelog link and all three verified files (#56) Review follow-up on PR #57. rel-1.16.0.md lacked the Full Changelog line the plan (Q2) and CLAUDE.md require, and its backported verify command named the wheel only, although CLAUDE.md and README.env both say wheel, sdist, and SHA256SUMS. The checker accepted both. - rel-1.16.0.md: verify all three files, point to README.env, add the rel-1.15.0...rel-1.16.0 Full Changelog link. - check-release-notes.py: every sigstore verify command must name the wheel, sdist, and SHA256SUMS; a Full Changelog compare link must end at the file's own tag and start at an earlier one. A self-test against known-bad samples runs first, so a rule that stops matching fails loudly. - CLAUDE.md, plan: record the rules. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01Bqq8uGF2zNbHW9FU3vfj7Q --- .dev/tools/check-release-notes.py | 163 ++++++++++++++++++++++++++---- CLAUDE.md | 11 +- doc/release-notes/rel-1.16.0.md | 8 +- plan/tickets/56/plan.md | 8 ++ 4 files changed, 165 insertions(+), 25 deletions(-) diff --git a/.dev/tools/check-release-notes.py b/.dev/tools/check-release-notes.py index 8c49195..9b4991f 100755 --- a/.dev/tools/check-release-notes.py +++ b/.dev/tools/check-release-notes.py @@ -1,20 +1,27 @@ #!/usr/bin/env python3 -"""Verify that every doc/release-notes/rel-X.Y.Z.md carries a correct verification section. +"""Verify that every doc/release-notes/rel-X.Y.Z.md carries a correct verification section and changelog link. The notes file is published verbatim as the GitHub release body (release.yml; plan/tickets/56/plan.md D1), so -the artifact verification instructions exist only if the file contains them. This checks, per file: +the artifact verification instructions and the changelog link exist only if the file contains them. This checks, +per file: - the file name is rel-X.Y.Z.md (release.yml derives the name from the tag, so nothing else is ever published); - there is a `## Verifying these artifacts` heading; - - there is at least one `sigstore verify identity` command, and every `--cert-identity` in the file is exactly - this repository's release.yml at *this file's* tag, with the GitHub Actions OIDC issuer. + - there is at least one `sigstore verify identity` command, each naming the wheel, the sdist, and SHA256SUMS, + and every `--cert-identity` in the file is exactly this repository's release.yml at *this file's* tag, with + the GitHub Actions OIDC issuer; + - there is a `**Full Changelog**:` compare link ending at this file's tag and starting at an earlier one. The identity check is the one that earns its keep. The section is hand-written per release, usually by copying the previous one, and a stale tag in the identity makes `sigstore verify` reject every genuine artifact of the -release ("Certificate's SANs do not match"). Readers would reasonably conclude the release is forged. +release ("Certificate's SANs do not match"). Readers would reasonably conclude the release is forged. The +changelog link is copied the same way and goes stale the same way, though less dangerously. Only the `` +end can be checked: which release came before is not knowable from one file, so `` is checked only for +being earlier. Runs in CI's quality job over every notes file, so a mistake fails the notes PR rather than the tag push; and in -release.yml on the tagged file, as a backstop. +release.yml on the tagged file, as a backstop. Each run starts with a self-test that feeds the rules known-bad +notes and fails if any is accepted, so a rule that has quietly stopped matching cannot pass as clean notes. Usage: python .dev/tools/check-release-notes.py [FILE ...] @@ -34,51 +41,165 @@ REPOSITORY = "osprey-dcs/dp-python-lib" OIDC_ISSUER = "https://token.actions.githubusercontent.com" -STEM_RE = re.compile(r"^rel-\d+\.\d+\.\d+$") +STEM_RE = re.compile(r"^rel-(\d+)\.(\d+)\.(\d+)$") HEADING_RE = re.compile(r"^## Verifying these artifacts\s*$", re.MULTILINE) -VERIFY_RE = re.compile(r"\bsigstore\s+verify\s+identity\b") +# The whole command, backslash continuations included, up to the first line that does not continue. +VERIFY_RE = re.compile(r"\bsigstore\s+verify\s+identity\b(?:[^\n]*\\\n)*[^\n]*") IDENTITY_RE = re.compile(r"--cert-identity[ =]\"?([^\"\s]+)\"?") ISSUER_RE = re.compile(r"--cert-oidc-issuer[ =]\"?([^\"\s]+)\"?") +CHANGELOG_RE = re.compile(r"^\*\*Full Changelog\*\*:\s*(\S+)\s*$", re.MULTILINE) +COMPARE_RE = re.compile( + rf"^https://github\.com/{re.escape(REPOSITORY)}/compare/(rel-\d+\.\d+\.\d+)\.\.\.(rel-\d+\.\d+\.\d+)$" +) + +# What each `sigstore verify identity` must name, as (description, predicate over its file operands). Every +# release signs all three, and verifying only the wheel was exactly the shape rel-1.16.0's page first shipped with. +REQUIRED_OPERANDS = [ + ("the wheel", lambda arg: arg.endswith(".whl")), + ("the sdist", lambda arg: arg.endswith(".tar.gz")), + ("SHA256SUMS", lambda arg: arg == "SHA256SUMS"), +] def expected_identity(tag: str) -> str: return f"https://github.com/{REPOSITORY}/.github/workflows/release.yml@refs/tags/{tag}" -def check_file(path: Path) -> list[str]: - """Returns one message per problem found in `path`; empty when it passes.""" - tag = path.stem - if path.suffix != ".md" or not STEM_RE.match(tag): - return [f"{path}: name must be rel-X.Y.Z.md (release.yml looks the notes up by tag)"] +def version_of(tag: str) -> tuple[int, ...]: + match = STEM_RE.match(tag) + assert match is not None + return tuple(int(part) for part in match.groups()) - text = path.read_text(encoding="utf-8") + +def verify_operands(command: str) -> list[str]: + """The file operands of one `sigstore verify identity` command: every token that is not an option or its value.""" + tokens = command.replace("\\\n", " ").split()[3:] + operands: list[str] = [] + skip_value = False + for token in tokens: + if skip_value: + skip_value = False + elif token.startswith("--"): + skip_value = "=" not in token + else: + operands.append(token) + return operands + + +def check_text(name: str, tag: str, text: str) -> list[str]: + """Returns one message per problem found in the notes `text` for `tag`; empty when it passes.""" problems: list[str] = [] if not HEADING_RE.search(text): - problems.append(f"{path}: missing a '## Verifying these artifacts' section") + problems.append(f"{name}: missing a '## Verifying these artifacts' section") - if not VERIFY_RE.search(text): - problems.append(f"{path}: missing a 'sigstore verify identity' command") + commands = VERIFY_RE.findall(text) + if not commands: + problems.append(f"{name}: missing a 'sigstore verify identity' command") + for command in commands: + operands = verify_operands(command) + for description, matches in REQUIRED_OPERANDS: + if not any(matches(arg) for arg in operands): + problems.append(f"{name}: 'sigstore verify identity' does not verify {description}") identities = IDENTITY_RE.findall(text) if not identities: - problems.append(f"{path}: missing --cert-identity") + problems.append(f"{name}: missing --cert-identity") want = expected_identity(tag) for identity in identities: if identity != want: - problems.append(f"{path}: --cert-identity is\n {identity}\n expected\n {want}") + problems.append(f"{name}: --cert-identity is\n {identity}\n expected\n {want}") issuers = ISSUER_RE.findall(text) if not issuers: - problems.append(f"{path}: missing --cert-oidc-issuer") + problems.append(f"{name}: missing --cert-oidc-issuer") for issuer in issuers: if issuer != OIDC_ISSUER: - problems.append(f"{path}: --cert-oidc-issuer is {issuer}, expected {OIDC_ISSUER}") + problems.append(f"{name}: --cert-oidc-issuer is {issuer}, expected {OIDC_ISSUER}") + + changelogs = CHANGELOG_RE.findall(text) + if not changelogs: + problems.append(f"{name}: missing a '**Full Changelog**: .../compare/rel-...{tag}' line") + for url in changelogs: + compare = COMPARE_RE.match(url) + if compare is None: + problems.append( + f"{name}: Full Changelog link is\n {url}\n expected" + f"\n https://github.com/{REPOSITORY}/compare/rel-...{tag}" + ) + continue + prev, this = compare.groups() + if this != tag: + problems.append(f"{name}: Full Changelog link ends at {this}, expected {tag}") + elif version_of(prev) >= version_of(tag): + problems.append(f"{name}: Full Changelog link starts at {prev}, which is not earlier than {tag}") return problems +def check_file(path: Path) -> list[str]: + """Returns one message per problem found in `path`; empty when it passes.""" + tag = path.stem + if path.suffix != ".md" or not STEM_RE.match(tag): + return [f"{path}: name must be rel-X.Y.Z.md (release.yml looks the notes up by tag)"] + return check_text(str(path), tag, path.read_text(encoding="utf-8")) + + +def _sample( + identity_tag: str = "rel-2.1.0", + files: str = "dp_python_lib-*.whl dp_python_lib-*.tar.gz SHA256SUMS", + changelog: str | None = "rel-2.0.0...rel-2.1.0", +) -> str: + notes = f"""# dp-python-lib rel-2.1.0 + +## Verifying these artifacts + +```bash +sigstore verify identity \\ + --cert-identity "{expected_identity(identity_tag)}" \\ + --cert-oidc-issuer "{OIDC_ISSUER}" \\ + {files} +``` +""" + if changelog is not None: + notes += f"\n**Full Changelog**: https://github.com/{REPOSITORY}/compare/{changelog}\n" + return notes + + +def self_test() -> list[str]: + """Confirms a correct sample passes and each known-bad variant is rejected; returns a message per failure.""" + failures: list[str] = [] + good = check_text("", "rel-2.1.0", _sample()) + if good: + failures.append("a correct sample was rejected:\n " + "\n ".join(good)) + + bad_cases = { + "a stale --cert-identity tag": _sample(identity_tag="rel-2.0.0"), + "a verify of the wheel only": _sample(files="dp_python_lib-*.whl"), + "a verify missing SHA256SUMS": _sample(files="dp_python_lib-*.whl dp_python_lib-*.tar.gz"), + "a verify missing the sdist": _sample(files="dp_python_lib-*.whl SHA256SUMS"), + "no Full Changelog line": _sample(changelog=None), + "a Full Changelog link ending at a stale tag": _sample(changelog="rel-1.9.0...rel-2.0.0"), + "a Full Changelog link starting at a later tag": _sample(changelog="rel-2.2.0...rel-2.1.0"), + "a Full Changelog link to another repository": _sample().replace( + f"{REPOSITORY}/compare", "someone-else/dp-python-lib/compare" + ), + "no verification heading": _sample().replace("## Verifying these artifacts", "## Verification"), + } + for description, notes in bad_cases.items(): + if not check_text(f"<{description}>", "rel-2.1.0", notes): + failures.append(f"{description} was accepted") + return failures + + def main(argv: list[str]) -> int: + canary_failures = self_test() + if canary_failures: + print("FAIL: checker self-test failed; its rules no longer catch what they are meant to\n") + for failure in canary_failures: + print(f" {failure}") + return 1 + paths = [Path(arg) for arg in argv] if argv else sorted(NOTES_DIR.glob("rel-*.md")) if not paths: print(f"FAIL: no release notes found under {NOTES_DIR}") diff --git a/CLAUDE.md b/CLAUDE.md index 470653e..4c7abaa 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -122,9 +122,14 @@ diffs the live body against the file. So each notes file must carry, by hand: - a `**Full Changelog**: https://github.com/osprey-dcs/dp-python-lib/compare/rel-...rel-` line, standing in for GitHub's generated commit list. -`.dev/tools/check-release-notes.py` enforces the section and, above all, the tag in the identity: -the section is usually copied from the previous release, and a stale tag makes `sigstore verify` -reject every genuine artifact. It runs in CI's quality job and again at tag time. +`.dev/tools/check-release-notes.py` enforces both, and above all the tag in the identity: the +section is usually copied from the previous release, and a stale tag makes `sigstore verify` +reject every genuine artifact. It also requires each verify command to name all three files (the +backported rel-1.16.0 section first verified the wheel only), and the changelog link to end at +this file's tag and start at an earlier one; which release came before is not knowable from one +file, so `` is checked only for being earlier. Each run starts with a self-test that feeds +the rules known-bad notes, so a rule that stops matching fails loudly instead of passing everything. +It runs in CI's quality job and again at tag time. **Never edit a release page by hand.** Fix the notes file by PR, then republish with `gh release edit rel-X.Y.Z --notes-file doc/release-notes/rel-X.Y.Z.md`. With the file as the diff --git a/doc/release-notes/rel-1.16.0.md b/doc/release-notes/rel-1.16.0.md index dbe6f79..e26e2b9 100644 --- a/doc/release-notes/rel-1.16.0.md +++ b/doc/release-notes/rel-1.16.0.md @@ -407,6 +407,7 @@ in fact been a no-op since before `rel-1.15.0`. [plan-40]: https://github.com/osprey-dcs/dp-python-lib/blob/rel-1.16.0/plan/tickets/40/plan.md [plan-41]: https://github.com/osprey-dcs/dp-python-lib/blob/rel-1.16.0/plan/tickets/41/plan.md [plan-readme]: https://github.com/osprey-dcs/dp-python-lib/blob/rel-1.16.0/plan/README.md +[readme-env]: https://github.com/osprey-dcs/dp-python-lib/blob/main/README.env ## Verifying these artifacts @@ -422,11 +423,16 @@ Signatures (keyless Sigstore; `pip install sigstore`): sigstore verify identity \ --cert-identity "https://github.com/osprey-dcs/dp-python-lib/.github/workflows/release.yml@refs/tags/rel-1.16.0" \ --cert-oidc-issuer "https://token.actions.githubusercontent.com" \ - dp_python_lib-*.whl + dp_python_lib-*.whl dp_python_lib-*.tar.gz SHA256SUMS ``` +Each file's bundle (`.sigstore.json`) is found automatically. [`README.env`][readme-env] is the full +verification reference. + ## Installing ```bash pip install dp_python_lib-*.whl ``` + +**Full Changelog**: https://github.com/osprey-dcs/dp-python-lib/compare/rel-1.15.0...rel-1.16.0 diff --git a/plan/tickets/56/plan.md b/plan/tickets/56/plan.md index af5d5ff..cef5443 100644 --- a/plan/tickets/56/plan.md +++ b/plan/tickets/56/plan.md @@ -255,3 +255,11 @@ Found while implementing, 2026-09-24: `sigstore verify` *rejects* every genuine artifact ("Certificate's SANs do not match"), which readers would take as a forged release. Confirmed against the rel-1.16.0 assets, which verify with the correct identity (wheel, sdist, and `SHA256SUMS` in one call, as `README.env` shows). +- **Review follow-up (PR #57): the checker covers the whole contract, not just the identity.** + The review found `rel-1.16.0.md` still lacked the Full Changelog line that Q2 promised, and its + backported verify command named the wheel only, although CLAUDE.md and `README.env` both say + wheel, sdist, and `SHA256SUMS`. The checker had accepted both. Fixed in the file (it also + gained the `README.env` pointer CLAUDE.md asks for), and the checker now requires every verify + command to name all three files and a Full Changelog compare link ending at the file's own tag + and starting at an earlier one. Following the cookbook checker's canary, it self-tests on each + run against known-bad samples. The rel-1.16.0 page is republished from the file after merge.