-
Notifications
You must be signed in to change notification settings - Fork 1.4k
chore(ci): pin and enforce one uv/CPython version across Python CI, gate committed lockfiles, and harden fork-artifact handling in publish-python-preview #2338
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 5 commits
b0aa5d3
dee06a7
725ceba
b66a49a
cefb780
7a8999c
93b477e
d60bd58
325907e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| # Runner labels actionlint cannot know about. Without this, every workflow using a | ||
| # Depot runner is an `unknown label` error — which matters now that | ||
| # lint-release-workflows.yml lints dojo-e2e.yml at `fail_level: error`. | ||
| self-hosted-runner: | ||
| labels: | ||
| - depot-ubuntu-24.04 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| name: Assert lockfiles unchanged | ||
| description: >- | ||
| Fails the job if any step modified a committed lockfile. This is the direct check | ||
| that CI never silently repairs lockfile drift instead of reporting it. | ||
|
|
||
| # Why this is an outcome check rather than a rule about which commands ran: | ||
| # an earlier version of this change had a CI script that read the shell inside every | ||
| # `run:` block looking for a uv command that might rewrite a lockfile. Three review | ||
| # rounds found new shell shapes that slipped past its regexes — `$(uv sync)`, | ||
| # `$((1<<n))` read as a heredoc opener, `shell: python` bodies scanned as bash — so | ||
| # that approach was abandoned in favour of measuring the thing itself. A lockfile | ||
| # that changed during the job is the property it was trying to infer, and git | ||
| # answers that directly. | ||
| # | ||
| # Run this LAST in a job. `uv sync --locked` refuses to rewrite in the first place, | ||
| # so on a healthy job this asserts what already holds; it earns its keep when a step | ||
| # is added that syncs without `--locked`. | ||
| # | ||
| # NOT used by jobs that sync the `examples/` apps: five of those lockfiles are | ||
| # knowingly stale and dojo-e2e rewrites them on purpose. See the `lockfiles` job | ||
| # comment in .github/workflows/unit-python-sdk.yml. | ||
|
|
||
| runs: | ||
| using: composite | ||
| steps: | ||
| - name: Assert no committed lockfile was modified | ||
| shell: bash | ||
| run: | | ||
| set -euo pipefail | ||
|
|
||
| # Refuse to pass vacuously. If the pathspec matches nothing the assertion | ||
| # below is trivially true, which is the one outcome a guard must never | ||
| # silently produce. | ||
| tracked=$(git ls-files -- '*uv.lock' '*poetry.lock' | wc -l | tr -d ' ') | ||
| if [ "$tracked" -eq 0 ]; then | ||
| echo "::error::No lockfiles are tracked at $(pwd) — this check would pass by" \ | ||
| "inspecting nothing. Is the repository checked out?" | ||
| exit 1 | ||
| fi | ||
|
|
||
| # git status, NOT git diff. `git diff` compares worktree against the INDEX and | ||
| # only reports tracked files, so it missed the two likeliest ways a lockfile | ||
| # changes: a step that staged it (`git add`), and a NEW lock created where none | ||
| # existed — which is exactly what `uv sync` in sdks/python/a2ui_toolkit, the one | ||
| # first-party package with no committed lock, would produce. Both printed | ||
| # "unchanged". `--porcelain` reports modified, staged and untracked in one pass. | ||
| changed=$(git status --porcelain -- '*uv.lock' '*poetry.lock') | ||
| if [ -n "$changed" ]; then | ||
| echo "::error::A step in this job modified a committed lockfile:" | ||
| echo "$changed" | sed 's/^/ /' | ||
| echo | ||
| echo "CI must not repair lockfile drift. Run the equivalent command locally," \ | ||
| "commit the updated lockfile, and push it." | ||
| git --no-pager diff --stat -- '*uv.lock' '*poetry.lock' | ||
| exit 1 | ||
| fi | ||
|
|
||
| echo "All $tracked tracked lockfile(s) unchanged." | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| # Pinned Python build toolchain for CI. | ||
| # | ||
| # This file records the uv and CPython versions every Python job in .github/workflows/ | ||
| # is expected to use, and the green run they came from. | ||
| # | ||
| # It is documentation, not a mechanism. GitHub cannot read a file into a workflow's | ||
| # `env:` block, so each Python workflow repeats whichever of these two values it | ||
| # actually uses, and NOTHING IN CI CHECKS THAT THEY STILL MATCH — keeping them in step | ||
| # is on whoever edits a workflow, and on review. An earlier version of this change | ||
| # shipped a checker for it; it cost far more than the drift it prevented and was | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the main blocker for me. The removed checker is cited as the reason no check exists, but it's a strawman for what's actually needed here. Verifying that 17 literals equal two values is not a workflow analyser — it never has to parse set -euo pipefail
. .github/python-toolchain.env
found=$(grep -rhoE '^ (UV_VERSION|PYTHON_VERSION): "[^"]+"' .github/workflows | sort -u)
expected=$(printf ' PYTHON_VERSION: "%s"\n UV_VERSION: "%s"' "$PYTHON_VERSION" "$UV_VERSION")
if [ "$found" != "$expected" ]; then
echo "::error::workflow pins disagree with .github/python-toolchain.env"
diff <(echo "$expected") <(echo "$found") || true
exit 1
fiI ran the equivalent against this branch — 17/17 agree today, so this lands green. That's the moment to add it; it only ever goes red on the drift this file exists to prevent. There's also a genuine single-source-of-truth option the "GitHub cannot read a file into As it stands the PR applies a weaker standard to its own invariant than the one it imposes on lockfiles two files over, and the reasoning for that asymmetry is recorded in a way that discourages revisiting it. Generated by Claude Code
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done — you were right, and the strawman diagnosis was correct. Two differences from your snippet, both from testing it: It also checks pass 2. Comparing declarations only sees pins that exist. A newly added Quote-tolerant, and it reports Hosted in Proved it goes red, not just green: drifted literal, unquoted-but-wrong pin, unpinned Writing it also caught a bug in my own first version: On the The "do not re-add a checker" framing is gone from the file. It was doing what you describe — discouraging revisiting a decision that deserved it. One thing your comment prompted that's outside this PR: |
||
| # removed deliberately, so please do not read the absence as an oversight. | ||
| # | ||
| # A workflow that needs only one value declares only that one — prepare-release.yml and | ||
| # lint-release-workflows.yml install no uv, so they carry PYTHON_VERSION alone; an | ||
| # unused copy of a pin is only somewhere for it to drift. | ||
| # | ||
| # Provenance: both versions are the ones a *green* run actually resolved, not a | ||
| # guess. Run https://github.com/ag-ui-protocol/ag-ui/actions/runs/30958253208 | ||
| # (unit-python-sdk.yml, main @ bfc22e4e, 2026-08-04 — several workflows share the | ||
| # display name "unit", so the file name is what identifies it) installed uv 0.12.1 in | ||
| # every Python job ("Successfully installed uv version 0.12.1") and built every venv | ||
| # against "CPython 3.12.3 interpreter at: /usr/bin/python3". | ||
| # | ||
| # Three honest caveats about what this pin does and does not freeze: | ||
| # - Naming python-version makes setup-uv select a uv-MANAGED CPython rather than | ||
| # the runner's /usr/bin/python3. Same minor version as the run above, different | ||
| # provenance. That is the intended trade: a declared interpreter beats whatever | ||
| # the runner image ships. | ||
| # - PYTHON_VERSION is minor-precision, so 3.12.x patch releases still resolve at | ||
| # run time and share one cache key. uv recreates a venv whose interpreter moved, | ||
| # so the cost is a cache HIT whose contents are then discarded and rebuilt — | ||
| # slower than a plain miss, but not a red build. Set a full patch version here to | ||
| # close it; either precision works. | ||
| # - Naming python-version also exports UV_PYTHON for the whole job, so EVERY later uv | ||
| # invocation in it is constrained to this interpreter — including the example-app | ||
| # syncs prep-dojo-everything.js performs during dojo-e2e, which previously resolved | ||
| # per project. Every requires-python in the repo admits 3.12 today, so nothing | ||
| # breaks; a future example pinned to >=3.13 would fail inside dojo-e2e rather than | ||
| # in the package that declared it. | ||
| # | ||
| # To move the pin: pick the versions from a newer green run, update this file, update | ||
| # the `env:` block of every workflow that declares them (grep for UV_VERSION under | ||
| # .github/workflows/), and record the new run id above. | ||
| UV_VERSION=0.12.1 | ||
| PYTHON_VERSION=3.12 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,39 +1,52 @@ | ||
| name: Lint Release Workflows | ||
|
|
||
| # Runs actionlint + shellcheck against the release / create-pr, release / | ||
| # publish, and canary / publish pipelines and the scripts they call. Keeps | ||
| # these critical, retry-sensitive files from silently regressing on shell or | ||
| # action-syntax bugs. | ||
| # Runs actionlint + shellcheck against the release, canary, and Python CI | ||
| # pipelines and the scripts they call. Keeps these critical, retry-sensitive files | ||
| # from silently regressing on shell or action-syntax bugs. | ||
| # | ||
| # Scope is intentionally narrow: only the release workflows and | ||
| # scripts/release/*. Expanding later is cheap; starting narrow avoids | ||
| # drowning unrelated changes in pre-existing lint noise. | ||
| # The actionlint file list below is explicit rather than repo-wide, so that adding | ||
| # a workflow does not drown an unrelated PR in pre-existing lint noise — read that | ||
| # list, not this comment, for the current scope. Everything else under | ||
| # .github/workflows/ is unlinted; widening is a one-line change plus whatever it | ||
| # turns up. shellcheck covers scripts/release/*.sh only. | ||
|
|
||
|
|
||
| on: | ||
| push: | ||
| branches: [main] | ||
| paths: | ||
| - ".github/workflows/prepare-release.yml" | ||
| - ".github/workflows/publish-release.yml" | ||
| - ".github/workflows/canary.yml" | ||
| - ".github/workflows/lint-release-workflows.yml" | ||
| - "scripts/release/**" | ||
| - ".github/workflows/**" | ||
| - ".github/actionlint.yaml" | ||
| - "scripts/**" | ||
| - "nx.json" | ||
| pull_request: | ||
| paths: | ||
| - ".github/workflows/prepare-release.yml" | ||
| - ".github/workflows/publish-release.yml" | ||
| - ".github/workflows/canary.yml" | ||
| - ".github/workflows/lint-release-workflows.yml" | ||
| - "scripts/release/**" | ||
| - ".github/workflows/**" | ||
| - ".github/actionlint.yaml" | ||
| - "scripts/**" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Two problems with widening the triggers while the lint scope stays explicit. The widening buys nothing on its own. It changes the fork-PR story. With Generated by Claude Code
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Narrowed the triggers to match the lint scope, which fixes both halves at once.
Chose narrowing over widening the lint scope because widening is the "one-line change plus whatever it turns up" the header already anticipates, and what it turns up is pre-existing noise across every unlinted workflow — which is what the narrow scope was protecting against. The cost is that the file list now lives in three places (two On the fork case: narrowing makes it rare, not impossible. A fork PR editing one of the ten linted workflows still trips the reporter. I left that alone deliberately — it was already true for Writing this caught a bug I'd introduced in the fix itself, worth flagging since it's the kind that lints clean: I first put the "not |
||
| - "nx.json" | ||
|
|
||
| permissions: | ||
| contents: read | ||
|
|
||
| # Pinned CPython — see .github/python-toolchain.env, which records this value and the | ||
| # green run it came from. Keeping it equal across workflows is a convention, not an | ||
| # enforced check. | ||
| # | ||
| # No UV_VERSION here: this workflow installs no uv, and an unused copy of the pin is | ||
| # only somewhere for it to drift. | ||
| env: | ||
| PYTHON_VERSION: "3.12" | ||
|
|
||
| jobs: | ||
| actionlint: | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| contents: read | ||
| # reviewdog's github-check reporter creates the check run that carries the | ||
| # annotations; without it, findings are enforced by exit code alone and nothing | ||
| # visible says why the job failed. | ||
| checks: write # create the check run reviewdog reports annotations through | ||
| steps: | ||
| - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 | ||
| with: | ||
|
|
@@ -49,6 +62,12 @@ jobs: | |
| .github/workflows/publish-release.yml | ||
| .github/workflows/canary.yml | ||
| .github/workflows/lint-release-workflows.yml | ||
| .github/workflows/unit-python-sdk.yml | ||
| .github/workflows/dojo-e2e.yml | ||
| .github/workflows/build-python-preview.yml | ||
| .github/workflows/publish-python-preview.yml | ||
| .github/workflows/zizmor.yml | ||
| .github/workflows/test-release-scripts.yml | ||
|
|
||
| shellcheck: | ||
| runs-on: ubuntu-latest | ||
|
|
@@ -110,6 +129,6 @@ jobs: | |
| - name: Setup Python | ||
| uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 | ||
| with: | ||
| python-version: "3.12" | ||
| python-version: ${{ env.PYTHON_VERSION }} | ||
| - name: Verify config package names match manifests | ||
| run: bash scripts/release/verify-config-manifest-names.sh | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The verification table lists "shellcheck (
scripts/release/*.sh, composite action) — clean". That's true of your machine, but CI lints neither half of this file:shellcheckjob globsscripts/release/*.sh, which doesn't reach.github/actions/**actionlint_flags, so composite action definitions are out of scope there tooSo the file carrying the most new shell in this PR is the one file nothing checks. actionlint does lint composite actions when pointed at them — adding
.github/actions/assert-lockfiles-unchanged/action.ymlto theactionlint_flagslist, or.github/actions/**/*.ymlto the shellcheck job, closes it.The logic itself looks right to me, for what it's worth — I confirmed
.gitignore:28(**/python/**/.venv/) covers every.venvthese jobs create, so thegit statuspathspec won't pick up venv noise, and the vacuous-pass guard is a good call.Generated by Claude Code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, but not the way you suggested — actionlint can't lint composite actions. 1.7.x has no composite-action mode. Pointed at this file it parses it as a workflow:
So adding it to
actionlint_flagswould have turned the actionlint job red with four bogus errors and linted nothing. I tried it with a deliberately injected SC2086 to check whether the shell got scanned at all — it doesn't.Your other suggestion is the workable one, and it needs the shell to be in a file first. The check now lives in
assert-lockfiles-unchanged.shandaction.ymlinvokes it via$GITHUB_ACTION_PATH; the shellcheck job globs.github/actionsalongsidescripts/release. Logic unchanged. 12 files, clean.Two things that fell out of doing it:
shopt -s nullglob+ ascripts/release/*.sharray. I first reached for.github/actions/**/*.sh, then foundglobstaris bash 4 and this repo gets developed on macOS where/bin/bashis 3.2 — the glob silently matches nothing there.mapfilehas the same problem. It'sfind+xargs -0now, which runs identically in 3.2 and 5, so the job can actually be verified locally rather than only in CI — which was your underlying point about my table.You're right that "composite action — clean" in the table was a local result. The table now marks what was verified how, and this file is genuinely in CI's shellcheck scope rather than only on my machine.
Also confirmed your
.gitignore:28finding independently —**/python/**/.venv/does cover every.venvthese jobs create, so the pathspec stays clean.