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
Conversation
Both values are the ones a green run actually resolved, and that run is identified. The file is documentation, not a mechanism: GitHub cannot read it into a workflow's env: block, so each Python workflow repeats the values it needs and keeping them in step is a convention and a review item. Records three caveats the pin does not close, including that naming python-version also exports UV_PYTHON job-wide.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Python Preview PackagesVersion
Install with uvAdd the TestPyPI index to your [[tool.uv.index]]
name = "testpypi"
url = "https://test.pypi.org/simple/"
explicit = trueThen install the packages you need: # Core SDK
uv add 'ag-ui-protocol==0.0.0.dev1786027393' --index testpypi
# Integrations (each already depends on the matching ag-ui-protocol preview)
uv add 'ag-ui-langgraph==0.0.0.dev1786027393' --index testpypi
uv add 'ag-ui-crewai==0.0.0.dev1786027393' --index testpypi
# NOTE: ag-ui-agent-spec depends on pyagentspec (git-only, not on PyPI).
# You will need to install pyagentspec separately from its git repo.
uv add 'ag-ui-agent-spec==0.0.0.dev1786027393' --index testpypi
uv add 'ag_ui_adk==0.0.0.dev1786027393' --index testpypi
uv add 'ag_ui_strands==0.0.0.dev1786027393' --index testpypiInstall with pippip install \
--index-url https://test.pypi.org/simple/ \
--extra-index-url https://pypi.org/simple/ \
ag-ui-protocol==0.0.0.dev1786027393
Commit: 5a21107 |
@ag-ui/a2a-middleware
@ag-ui/a2ui-middleware
@ag-ui/event-throttle-middleware
@ag-ui/mcp-apps-middleware
@ag-ui/mcp-middleware
@ag-ui/a2a
@ag-ui/adk
@ag-ui/ag2
@ag-ui/agno
@ag-ui/aws-strands
@ag-ui/claude-agent-sdk
@ag-ui/claude-managed-agents
@ag-ui/crewai
@ag-ui/langchain
@ag-ui/langgraph
@ag-ui/llamaindex
@ag-ui/mastra
@ag-ui/pydantic-ai
@ag-ui/vercel-ai-sdk
@ag-ui/watsonx
@ag-ui/a2ui-toolkit
create-ag-ui-app
@ag-ui/client
@ag-ui/core
@ag-ui/encoder
@ag-ui/proto
commit: |
All 18 astral-sh/setup-uv invocations resolved to a floating '>=0.8.0' or named no version at all, so any job could silently get a uv no green run had used. Each now takes the exact pin from .github/python-toolchain.env. Venv cache keys gain the same two versions, so an environment built by one toolchain is never restored into a job expecting another — the failure mode that hides drift rather than surfacing it. The pin matters most in prepare-release.yml: since #2314 that is the uv whose output lands in a committed lockfile. Also declares the depot self-hosted runner label so the widened actionlint scope resolves it.
Adds the 'uv lock --check' gate #2314 named as its companion. It runs over every first-party lock, which widens coverage past the nine packages that have a test lane: agent-spec and claude-agent-sdk ship committed locks with no lane at all, which is how the drift #2313 repaired went unnoticed for several releases. It also asserts no first-party package is missing a lock, since a package with none is invisible to a find-based check and passes silently. Each Python job that installs dependencies now ends with an assertion that it left no committed lockfile modified. That is measured by outcome — git status over every tracked lockfile — so nothing has to infer which commands a job ran. It runs with if: always(), so a job that rewrote a lock and then failed still reports it.
Separates the two halves by how they are enforced: matching the pin is a convention reviewers check, while the lockfile rules fail the build. States the examples/ exemption that reconciles the plain uv sync used there with the frozen Python jobs.
fd7ee69 to
b66a49a
Compare
contextablemark
left a comment
contextablemark
left a comment
There was a problem hiding this comment.
Careful, well-documented change, and the core of it is clearly right — pinning the tool that builds the environments, and gating the two packages that had committed locks but no test lane, is exactly the gap #2313 exposed. I verified the mechanics locally against b66a49a rather than taking the table on faith:
| claim | checked |
|---|---|
18 setup-uv invocations, all pinned |
confirmed (19 grep hits; one is prose in zizmor.yml:14) |
| 17 pin declarations agreeing | confirmed — only "0.12.1" and "3.12" appear across all 8 workflows |
no hardcoded python-version stragglers |
confirmed, none left in .github/workflows |
lockfiles discovery steps |
ran both — 10/10 locks found, sdks/python/a2ui_toolkit the sole miss |
assert-lockfiles-unchanged pathspec safety |
.gitignore:28 covers every .venv these jobs create |
| release path won't spring a surprise lock | prepare-release.ts:358 early-returns for lock-less packages |
Requesting changes on one item; the rest are inline and mostly cheap.
The blocking one: the pin's own invariant is the only thing here left unenforced, and the stated reason doesn't survive contact. The removed ~1,400-line checker is cited as evidence that checking this is expensive, but comparing 17 literals against two values never needs to parse a run: block — it's about six lines of grep, and the lockfiles job that would host it already exists and already triggers on .github/**. Details and a working snippet inline on python-toolchain.env. I'd like that in before merge, because the alternative is a file that documents a convention nothing can hold anyone to, two files away from a job that enforces the same class of invariant properly.
Non-blocking, but please address or reply:
if: always()→if: '!cancelled()'on all eleven assert steps.always()also fires on cancellation and after a failed checkout, where the composite action can't resolve and produces a misleading second error.a2ui_toolkitis a published package (ag-ui-a2ui-toolkit,buildSystem: uv), not an internal one. Theknown_missingcomment should say so, and it wants a follow-up ticket — otherwise "gate committed lockfiles" ships with the released exception permanently waived.- The
publish-python-preview.ymlhardening deserves its own PR or at least its own line in the title. It's a genuine fork-artifact →pull-requests: writefix and it will not be findable under a uv-pinning changelog entry. lint-release-workflows.yml: triggers widen to.github/workflows/**+scripts/**while the lint scope stays hand-listed, so the extra runs lint nothing new — and withscripts/**included, fork PRs now reach agithub-checkreporter that a read-only fork token can't satisfy.- The composite action is linted by neither the shellcheck job nor actionlint. The table's "composite action — clean" is a local result, not a CI one.
Two smaller things, no reply needed:
dojo-e2e.ymladds.github/python-toolchain.envtopaths:, but that file has no effect on the workflow — the values are inlined — so a no-op edit triggers the full e2e matrix.**/pyproject.tomland**/uv.lockonunit-python-sdk.ymlmean editing anexamples/lockfile runs all nine test lanes, which are the lanes that deliberately excludeexamples/. Worth narrowing if the matrix cost shows up.
Also worth flagging to whoever merges: every venv cache key and dojo-e2e's restore-keys change at once, so the first run after merge is a cold miss across ~10 caches. Expected, but it'll look like a regression if nobody's told.
Generated by Claude Code
| # 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 |
There was a problem hiding this comment.
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 run: blocks, resolve expressions, or model job structure. It's a grep, and the lockfiles job already exists and already runs on .github/** changes:
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 env:" framing rules out too quickly: a small setup job that reads the file into job outputs. ${{ needs.setup.outputs.uv_version }} is valid in with: and in cache keys, which is where both values are actually consumed — the env: limitation is real but not binding. I'd still take the grep over nine extra needs: edges, but the file shouldn't record the stronger claim.
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
There was a problem hiding this comment.
Done — you were right, and the strawman diagnosis was correct. scripts/release/verify-python-toolchain-pins.sh, running as the python-toolchain-pins job.
Two differences from your snippet, both from testing it:
It also checks pass 2. Comparing declarations only sees pins that exist. A newly added setup-uv that names no version declares nothing to compare and passes silently — which is precisely the state all 18 invocations were in before this PR. So it also asserts uses: astral-sh/setup-uv@ count equals version: ${{ env.UV_VERSION }} count. Three lines, catches the regression that matters most.
Quote-tolerant, and it reports file:line. UV_VERSION: 0.12.1 and UV_VERSION: "0.12.1" are the same pin; a check that understood one spelling would read the other as absent and pass over the drift. On failure it names each file and line rather than diffing two sorted blobs.
Hosted in lint-release-workflows.yml, not the lockfiles job. This is the one place your comment was off: unit-python-sdk.yml doesn't trigger on .github/**, it lists three specific paths (python-toolchain.env, its own file, .github/actions/**). A pin drifted in zizmor.yml would never have fired it. All nine pin-carrying workflows sit inside lint-release-workflows.yml's lint scope, and it already hosts three sibling guards of exactly this class — hand-maintained list vs. the file it mirrors. It also needs no uv.
Proved it goes red, not just green: drifted literal, unquoted-but-wrong pin, unpinned setup-uv, and renamed pins — 4/4 fail correctly, and a correct-but-single-quoted pin stays green. Both passes refuse to pass vacuously.
Writing it also caught a bug in my own first version: grep -r over .github/workflows with no --include picked up the .yml.bak files my tests left behind and reported 19/19 against a tree with 17 declarations and 18 invocations. A stray .yml.orig from a bad merge would have done the same in CI. Now filtered, and the reason is in the comment so nobody removes it.
On the setup-job point — you're right and the file overstated it. Job outputs are valid in with: and cache keys, which is where both values are consumed, so "GitHub cannot read a file into env:" is true but implies more than it should. The header now records that alternative and why the grep won: nine extra needs: edges, including onto the nine-lane matrix, to remove a duplication six lines already hold in place. If the needs: graph gets rearranged for other reasons, it says to revisit.
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: .node-version from #2325 has the identical gap, and CONTRIBUTING says so in as many words ("nothing enforces that automatically yet"). Filed as PNI-280 and cross-linked, since the mechanism now exists.
| - name: Assert no lockfile was rewritten | ||
| # always(): a job that rewrote a lockfile AND then failed its tests would | ||
| # otherwise skip this step and report only the test failure, losing the signal. | ||
| if: always() |
There was a problem hiding this comment.
if: always() should be if: '!cancelled()' — here and on the other ten copies (lines 154, 202, 249, 318, 367, 415, 463, 514).
The comment justifies always() by the test-failure case, which !cancelled() covers identically. What always() adds is two cases you don't want:
- Cancellation. Concurrency cancels supersede runs constantly on this repo; each one now also runs this step.
- Checkout failure.
uses: ./.github/actions/assert-lockfiles-unchangedneeds the repo on disk. IfCheckout codefails, this step still fires and dies withCan't find 'action.yml' under '.../assert-lockfiles-unchanged'— which lands after the real error and reads like the guard itself is broken. That's precisely the misleading-signal failure mode the action's own header comment is arguing against.
Generated by Claude Code
There was a problem hiding this comment.
Switched all of them to !cancelled() — the cancellation argument is right, and concurrency cancels are frequent enough here that it matters.
The checkout half doesn't work as stated, though. !cancelled() is true when an earlier step failed — it only excludes cancellation. So on a failed Checkout code the job is failure, not cancelled, !cancelled() evaluates true, the step still runs, and it still dies with Can't find 'action.yml' under .../assert-lockfiles-unchanged. Swapping always() for !cancelled() doesn't fix that case at all.
Fixing it needs the outcome gate explicitly, so each checkout now carries an id and the assertion is:
if: ${{ !cancelled() && steps.checkout.outcome == 'success' }}actionlint validates steps.<id>.outcome references, so the wiring is checked rather than assumed. The step comment now separates the two clauses and says what each one buys, including that the second is not redundant with the first.
Minor count correction: there are nine assert steps, not eleven. Your line list has nine entries so the substance was right. The other two always() in that file are unrelated pre-existing steps — Report google-adk 2.x result (from 177348a, reporting a continue-on-error leg) and one in publish-python-preview.yml. Left alone as out of scope.
| set -euo pipefail | ||
| # Packages knowingly without a uv.lock. Adding one here is a decision, not | ||
| # a workaround: it means nothing verifies that package's dependency graph. | ||
| known_missing=( | ||
| "./sdks/python/a2ui_toolkit" |
There was a problem hiding this comment.
Worth being explicit that this waiver covers a published package, not an internal one.
sdks/python/a2ui_toolkit is ag-ui-a2ui-toolkit with "buildSystem": "uv" in scripts/release/release.config.json:45 — it ships to PyPI. And relockPythonPackage (scripts/release/prepare-release.ts:358) returns early when there's no uv.lock, so the release path won't grow one either. Net effect: the one package this new gate can't cover is one that users install.
The step comment says "it means nothing verifies that package's dependency graph" but doesn't say the package is released, which is the part that decides whether this is a shrug or a follow-up. Please note it here and open a ticket to uv lock it — a PR titled "gate committed lockfiles" landing with the published exception waived is the kind of thing that stays waived.
Generated by Claude Code
There was a problem hiding this comment.
Noted in the comment and filed as PNI-279. Verified your facts first — ag-ui-a2ui-toolkit, "buildSystem": "uv" at release.config.json:45, and relockPythonPackage early-returning at prepare-release.ts:358 all check out, so the release path genuinely will not grow one.
One thing that changes the severity, worth recording: that pyproject.toml declares dependencies = []. uv lock there resolves exactly one package — itself — so the committed lock would be near-empty and there is no dependency graph going unverified today. The real exposure is forward-looking: with no lock, the first dependency anyone adds arrives unlocked and invisible to the step above. That's what the ticket closes, and both facts are now in the comment so the next reader doesn't have to re-derive either.
I considered just locking it here, since at zero dependencies it's nearly free. Decided against it for the reason you applied to the security fix: it changes that package's release behaviour (relockPythonPackage would start relocking it on every bump) and it empties known_missing, leaving the stale_exception branch and "${known_missing[@]}" under set -u with no exercised path. Both fine changes; neither belongs as a rider on a CI-pinning PR. The ticket's acceptance criteria name both so it doesn't get done carelessly.
Also confirmed the residual surface a lockfile wouldn't fix either way: [build-system] requires uv_build>=0.8.0,<0.9, which is floating. In the ticket.
| 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/**" |
There was a problem hiding this comment.
Two problems with widening the triggers while the lint scope stays explicit.
The widening buys nothing on its own. actionlint_flags below is still a hand-listed set of workflows, and shellcheck still globs scripts/release/*.sh. So a PR touching scripts/foo.ts or an unlisted workflow now spins up both jobs to lint exactly the same files as before. Either widen the lint scope to match the triggers, or keep the triggers at the files that actually affect the result plus the six new workflows.
It changes the fork-PR story. With reporter: github-check, reviewdog creates a check run — and on pull_request from a fork the GITHUB_TOKEN is read-only no matter what the new checks: write says, so that call fails. Previously this workflow almost never fired on fork PRs because the paths were five internal release files. With scripts/** in the list, an external contributor touching scripts/ now trips it. Worth either gating the job on github.event.pull_request.head.repo.full_name == github.repository or switching the reporter for the fork case.
Generated by Claude Code
There was a problem hiding this comment.
Narrowed the triggers to match the lint scope, which fixes both halves at once.
paths: is now the ten linted workflows + .github/actions/assert-lockfiles-unchanged/action.yml + .github/actionlint.yaml + .github/python-toolchain.env + scripts/release/** + nx.json. No more runs that lint nothing new, and scripts/** is gone so an external contributor touching scripts/ no longer reaches the github-check reporter.
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 paths: blocks plus actionlint_flags); that's stated in the header, along with why a generated fourth list would be the wrong trade here.
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 prepare-release.yml, publish-release.yml and canary.yml before this PR widened anything, so it's pre-existing rather than introduced here. Happy to add the head.repo.full_name == github.repository gate if you'd rather close it properly, but it felt like a separate change from undoing my own widening.
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 .github/actions/**" note as a # comment inside the actionlint_flags: >- folded block scalar. # isn't a comment there — it's literal text. The flags string parsed to 73 arguments, 63 of them prose fragments being passed to actionlint. actionlint on the workflow was clean because the YAML is valid; only parsing the value exposed it. Comment moved above the key, with a line warning that nothing below it may be a comment, and I now assert the value parses to exactly 10 existing paths.
| - name: Read PR metadata | ||
| id: meta | ||
| # These three files come from a workflow_run artifact, which on a fork PR is | ||
| # produced by the fork's own run — so they are untrusted input reaching a job | ||
| # that holds `pull-requests: write`. Validate the shape before it becomes a step | ||
| # output: `echo "k=$(cat f)"` exits 0 even when `cat` fails, so a missing file | ||
| # silently yielded an empty value, and a value containing a newline would set | ||
| # arbitrary extra outputs. A single grouped redirect also satisfies SC2129, | ||
| # which reviewdog reports at error level now that this file is in the | ||
| # actionlint scope. | ||
| run: | |
There was a problem hiding this comment.
This is a real security fix and it shouldn't be buried in a uv-pinning PR.
Fork-produced artifact contents were reaching a job holding pull-requests: write; echo "k=$(cat f)" exits 0 when cat fails, and a newline in any of the three values sets arbitrary extra step outputs. The replacement — set -euo pipefail, shape validation, grouped redirect — is right, and I checked the version regex matches what build-python-preview.yml actually produces (0.0.0.dev${git log -1 --format=%ct}).
The problem is discoverability: nobody auditing this repo's fork-PR surface later will find it under chore(ci): pin one uv and CPython version. Please split it into its own PR, or at minimum call it out in the title and description so it shows up in the changelog as a security change rather than as toolchain housekeeping.
Generated by Claude Code
There was a problem hiding this comment.
Kept it here, but the discoverability complaint is fully taken: it's now named in the PR title and has its own section in the description, above the fold, labelled as a security change rather than housekeeping.
The reason for not splitting is mechanical. Adding publish-python-preview.yml to the actionlint scope is what surfaced this — SC2129 at error level on the old echo "k=$(cat f)" block. So a split gives two bad options: land the security PR first and this one rebases onto it (a two-PR ordering dependency on a fix you'd want in fast), or drop the file from the actionlint scope here so the PRs are independent, which ships the new lint coverage deliberately incomplete and leaves a one-line re-add for someone to forget.
Naming it in the title gets the changelog outcome you're after without either. If you'd still rather have it separate I'll do it — say which of the two orderings you prefer and I'll cut it.
Thanks for checking the version regex against build-python-preview.yml's actual output rather than just reading it; that's the part I'd most likely have got wrong.
| 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 ' ') |
There was a problem hiding this comment.
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:
- the
shellcheckjob globsscripts/release/*.sh, which doesn't reach.github/actions/** - reviewdog's actionlint gets an explicit workflow file list via
actionlint_flags, so composite action definitions are out of scope there too
So 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.yml to the actionlint_flags list, or .github/actions/**/*.yml to 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 .venv these jobs create, so the git status pathspec 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.
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:
action.yml:1:1: "jobs" section is missing in workflow [syntax-check]
action.yml:1:1: "on" section is missing in workflow [syntax-check]
action.yml:2:1: unexpected key "description" for "workflow" section [syntax-check]
action.yml:23:1: unexpected key "runs" for "workflow" section [syntax-check]
So adding it to actionlint_flags would 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.sh and action.yml invokes it via $GITHUB_ACTION_PATH; the shellcheck job globs .github/actions alongside scripts/release. Logic unchanged. 12 files, clean.
Two things that fell out of doing it:
- The job used
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. - It also exited 0 when the glob matched nothing. A lint job that passes by checking nothing is the same vacuous-pass failure you credited the action for guarding against, so that's now a hard failure with an error message.
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:28 finding independently — **/python/**/.venv/ does cover every .venv these jobs create, so the pathspec stays clean.
The pin was the one invariant in this change left to convention, and the
reason recorded for that did not survive review: the ~1,400-line workflow
analyser that was built and removed is not what checking this needs.
Comparing 17 literals against two recorded values never has to parse a
`run:` block, resolve an expression, or model job structure.
scripts/release/verify-python-toolchain-pins.sh does it in two passes:
1. every UV_VERSION / PYTHON_VERSION literal under .github/workflows/
equals .github/python-toolchain.env, reported as file:line on failure
and quote-tolerant so an unquoted pin is not read as absent
2. every `uses: astral-sh/setup-uv@` has a matching
`version: ${{ env.UV_VERSION }}`, which pass 1 cannot see — a new
unpinned invocation declares nothing to compare and would pass silently
Both refuse to pass vacuously: a renamed or reindented pin fails loudly
rather than inspecting nothing.
It runs as the python-toolchain-pins job in lint-release-workflows.yml
rather than unit-python-sdk.yml's `lockfiles` job. That job watches three
specific .github paths, not .github/workflows/**, so a pin drifted in
zizmor.yml would never have triggered it. All nine pin-carrying workflows
are inside this workflow's lint scope, and it already hosts three sibling
guards of exactly this class.
Two knock-on fixes in the same file:
- `paths:` now mirrors the lint scope instead of watching
.github/workflows/** + scripts/**. The wide triggers spent two runners
to lint the same hand-listed files as before, and `scripts/**` meant a
fork PR touching scripts/ reached reviewdog's github-check reporter,
which a read-only fork token cannot satisfy.
- shellcheck now covers .github/actions via find rather than a
scripts/release/*.sh glob, and fails instead of exiting 0 when the
search matches nothing.
`.github/python-toolchain.env` and eight workflow headers all asserted the
pin was unchecked; every one of those claims is now false, so all are
rewritten. The pins file also over-claimed the `env:`-cannot-read-a-file
limitation — a setup job exposing job outputs would work in both `with:`
and cache keys — so it now records that alternative and why the grep was
preferred over nine extra `needs:` edges.
Verified: 17/17 declarations agree, 18/18 invocations pinned, and the check
goes red on a drifted literal, an unquoted-but-wrong pin, an unpinned
setup-uv, and renamed pins.
Refs PNI-187
Review pointed out that the file carrying the most new shell in this change
is the one file nothing checks, and suggested adding it to actionlint_flags.
That does not work: actionlint 1.7.x has no composite-action mode. Pointed
at an action.yml it parses it as a workflow and reports four bogus
syntax-check errors ("jobs" section is missing, unexpected key "runs", ...),
so adding it there would have turned the actionlint job red rather than
linting anything.
Shell embedded in a composite action is therefore unlintable by either tool
in this repo. Extracting it to assert-lockfiles-unchanged.sh makes it an
ordinary script that the shellcheck job covers — the previous commit widened
that job to .github/actions for this reason. action.yml now invokes it via
$GITHUB_ACTION_PATH; the logic is unchanged.
Verified: 12 files shellchecked clean, including this one, under bash 3.2.
Refs PNI-187
Four review findings on the gate itself.
`if: always()` -> `if: !cancelled() && steps.checkout.outcome == 'success'`
on all nine assert steps. always() also fires on cancellation, and
concurrency cancels supersede runs constantly here.
The checkout half needs the explicit gate, though: !cancelled() is TRUE when
an earlier step failed, including `Checkout code`. Since the assert step is a
local `uses:`, a failed checkout would still run it and die with "Can't find
'action.yml'" — landing after the real error and reading as if the guard were
broken, which is the misleading signal action.yml's header argues against.
!cancelled() alone does not fix that, so each checkout now carries an id and
the assertion gates on its outcome.
The known_missing waiver now says its single entry is a PUBLISHED package:
sdks/python/a2ui_toolkit ships as ag-ui-a2ui-toolkit with
"buildSystem": "uv", and relockPythonPackage early-returns for lock-less
packages, so the release path will not grow one either. It also records that
the package declares `dependencies = []` — so the lock would be near-empty
and nothing is unverified today; the exposure is the FIRST dependency anyone
adds arriving invisible to the gate. Tracked in PNI-279, which writes down
the release-behaviour and empty-array consequences of closing it.
Two trigger fixes:
- `!examples/**` added after the **/uv.lock and **/pyproject.toml globs.
Those globs stay — unit-python-sdk.yml:19 documents them as load-bearing
for the repo-wide lockfiles job — but an examples-only lockfile edit was
running all nine test lanes, which deliberately exclude examples/.
- dojo-e2e.yml drops .github/python-toolchain.env from `paths:`. Its values
are inlined, so editing the record triggered the full e2e matrix on a run
that used the OLD values: misleading, not merely wasteful. The path stays
on unit-python-sdk.yml, where it is real.
Refs PNI-187, PNI-279
The merge from main brought in PNI-186's Toolchain section, which documents `.node-version` and states that nothing enforces it yet. That is now the only unenforced pin record in the repo, sitting two sections away from one that is enforced, so the two read as unrelated when they are the same pattern. Cross-links them and points at PNI-280, which tracks closing the same gap for Node and pnpm using the script this PR adds. pnpm needs a decision first — it is pinned inline in 9 of 11 steps and via package.json#packageManager in 2. Refs PNI-187, PNI-280
mme
changed the title
|
All nine items addressed, pushed as four commits on top of the
Two places I pushed back rather than complied, both in-thread:
On the zizmor bot finding ( Two bugs the review process caught in my own fixes, both of which lint clean and are worth knowing about:
One thing outside this PR that your blocking argument applies to unchanged: Verification table in the description is refreshed against Outstanding question for you: item 3. If you still want the security fix split out, tell me which ordering you prefer — land it first and rebase this, or drop |
contextablemark
left a comment
contextablemark
left a comment
There was a problem hiding this comment.
Approving. The blocking item is genuinely resolved, and you were right on all three points where you pushed back.
I mutation-tested the new check rather than reading it, against 325907e:
| mutation | result |
|---|---|
| baseline | 17/17 declarations, 18/18 invocations, exit 0 |
zizmor.yml UV_VERSION → 0.12.2 |
correct file:line failure |
one version: line dropped |
18 invocation(s) but 17 pinned, names zizmor.yml |
all version: lines stripped |
exit 1, no output — see inline |
all setup-uv removed |
exit 1, no output — see inline |
| cross-file count balance | passes green with an unpinned setup-uv live — see inline |
Also confirmed: 9 checkout ids against 9 steps.checkout.outcome conditions, 12 files in the shellcheck glob, the python-toolchain-pins paths: covering all nine pin-carrying workflows, and both new scripts clean under bash -n. CI is green across all 56 checks.
Three corrections of mine that you got right, worth recording since two of them would have made things worse:
!cancelled()does not cover the failed-checkout case. A failed checkout isfailure, notcancelled, so the step would still have run and still died on the missingaction.yml.!cancelled() && steps.checkout.outcome == 'success'is the correct fix and I was wrong to conflate the two.- Nine assert steps, not eleven. I counted two unrelated pre-existing
always()steps into the total; you were right to leave both alone. - actionlint 1.7.x cannot lint composite actions. Following my suggestion would have turned the actionlint job red with four bogus
syntax-checkerrors while linting no shell at all. Extracting to a.shand pointing the shellcheck job at.github/actionsis the right shape.
Your own findings in that round are the kind I'd have missed: the # inside the folded block scalar passing 63 prose fragments to actionlint, globstar/mapfile being bash 4 against a repo developed on bash 3.2, the old nullglob branch exiting 0 on an empty match, and dependencies = [] on a2ui_toolkit — that last one genuinely changes the severity of the waiver and I'd have left it overstated.
Two things I'd fix before merge, both one-liners, neither worth another round:
!examples/**matches nothing. There is no top-levelexamples/directory — all 13 example lockfiles are nested underintegrations/*/python/examples/andmiddlewares/a2a-middleware/examples/, and GitHubpathspatterns are root-anchored unless they start with**. An examples-only lock edit still runs all nine lanes, which is what the comment and the PR description say it prevents."!**/examples/**"works, with one scoping consequence noted inline.- The pin verifier goes silent on total regressions. All pins stripped → exit 1 with zero output, because
grep -cexits 1 on no match andpipefail+set -ekill the script before the diagnostic. Fails closed, so nothing unsafe, but line 109's vacuous-pass guard is unreachable in exactly the scenario it names.
Two smaller ones, take or leave: the cross-file counting hole is closable with the per-file loop already in the failure branch (~5 lines, no YAML modelling), and the CONTRIBUTING checklist claims the script verifies venv cache keys, which it doesn't — cache keys remain the one convention-only part of this change.
Nothing here blocks. Good change, and the response round was better than the ask.
Generated by Claude Code
| - "**/uv.lock" | ||
| - "**/pyproject.toml" | ||
| # ...but not the examples/ apps. Their locks are deliberately out of scope for | ||
| # the lockfiles job (prep-dojo-everything.js relocks them on purpose), so an | ||
| # examples-only lockfile change would spin up all nine test lanes to verify | ||
| # nothing. Negation must follow the two globs above to override them. | ||
| - "!examples/**" |
There was a problem hiding this comment.
This matches nothing — there is no top-level examples/ in this repo.
GitHub paths patterns are anchored at the repository root unless they begin with ** (their own docs give docs/** as "any file in the docs directory at the root of the repository"). All 13 example lockfiles are nested:
integrations/adk-middleware/python/examples integrations/langroid/python/examples
integrations/ag2/python/examples integrations/llama-index/python/examples
integrations/agno/python/examples integrations/microsoft-agent-framework/python/examples
integrations/claude-agent-sdk/python/examples integrations/pydantic-ai/python/examples
integrations/claude-managed-agents/python/examples integrations/server-starter-all-features/python/examples
integrations/langgraph/python/examples integrations/server-starter/python/examples
middlewares/a2a-middleware/examples
git ls-files | grep -E '^examples/' returns nothing. So the negation is a no-op, and an examples-only lockfile edit still spins up all nine lanes — the exact behaviour the comment above and the PR description say it prevents. Note this is one of the few claims in the table that couldn't be verified by running something; it reads as true and isn't.
"!**/examples/**" is the working form. One consequence to decide rather than absorb: five packages have integrations/<pkg>/python/** earlier in this list, and last-match-wins means the fixed negation would also stop integrations/langgraph/python/examples/** (and adk-middleware, langroid, claude-managed-agents, claude-agent-sdk) from triggering their own lane. That's probably what you want — the lanes don't test the examples — but it's a wider change than the comment describes, so it should be stated there.
Generated by Claude Code
| invocations=$(grep -c 'uses: astral-sh/setup-uv@' "${GREP_SCOPE[@]}" | awk -F: '{ n += $2 } END { print n + 0 }') | ||
| pinned=$(grep -c 'version: ${{ env.UV_VERSION }}' "${GREP_SCOPE[@]}" | awk -F: '{ n += $2 } END { print n + 0 }') | ||
|
|
||
| if [ "$invocations" -eq 0 ]; then | ||
| echo "::error::No astral-sh/setup-uv invocations found under $WORKFLOWS/ — the search is wrong, not the repo" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Pass 2 fails silently in the two cases it exists to catch, and the vacuous-pass guard on line 109 is unreachable.
I mutation-tested this rather than reading it. Strip every version: ${{ env.UV_VERSION }} line from .github/workflows/*.yml — 18 invocations, 0 pinned, the headline regression — and you get:
$ bash scripts/release/verify-python-toolchain-pins.sh
$ echo $?
1
No output at all. Same when every uses: astral-sh/setup-uv@ is removed. bash -x shows execution stopping dead at + pinned=0, so neither the -eq 0 guard nor the -ne diagnostic below ever runs.
Cause is on these two lines: grep -c exits 1 when no file matches (confirmed against GNU grep 3.11 — grep -c 'X' -r --include='*.yml' .github/workflows; echo $? → 1), set -o pipefail propagates that through the | awk, and set -e kills the script at the assignment. Minimal repro:
set -euo pipefail
echo before
n=$(grep -c 'NO_SUCH' -r --include='*.yml' .github/workflows | awk -F: '{n+=$2} END{print n+0}')
echo "after: n=$n" # never reached; exit 1It fails closed, so nothing unsafe ships — but a red job with zero explanation is the outcome this script's own header argues against, and line 109's message ("the search is wrong, not the repo") can never print.
invocations=$(... ) || true on both, or { grep -c ... || true; } | awk ..., restores the diagnostics.
Worth noting why the table missed it: all four listed cases are partial mutations, where grep still matches somewhere. I hit it on the total ones. The partial path is genuinely good, for the record — dropping just zizmor.yml's pin gives 18 setup-uv invocation(s) but 17 pinned plus zizmor.yml: 1 invocation(s), 0 pinned.
Generated by Claude Code
| if [ "$invocations" -ne "$pinned" ]; then | ||
| echo "::error::${invocations} setup-uv invocation(s) but ${pinned} pinned to \${{ env.UV_VERSION }}" | ||
| echo "Every setup-uv must take 'version: \${{ env.UV_VERSION }}'. Unpinned invocations:" | ||
| for wf in "$WORKFLOWS"/*.yml "$WORKFLOWS"/*.yaml; do | ||
| [ -e "$wf" ] || continue | ||
| uses=$(grep -c 'uses: astral-sh/setup-uv@' "$wf" || true) | ||
| has=$(grep -c 'version: ${{ env.UV_VERSION }}' "$wf" || true) | ||
| [ "$uses" -ne "$has" ] && echo " ${wf}: ${uses} invocation(s), ${has} pinned" | ||
| done |
There was a problem hiding this comment.
Non-blocking, but the disclosed hole is narrower than the header claims — and the fix is already sitting in this block.
The counts are compared repo-wide, so a cross-file balance passes green. Demonstrated:
- drop
zizmor.yml'sversion: ${{ env.UV_VERSION }}line → 1 invocation, 0 pinned - add a stray
version: ${{ env.UV_VERSION }}to an unrelateduses:intest-release-scripts.yml→ 0 invocations, 1 pinned
All 17 pin declaration(s) match .github/python-toolchain.env (uv 0.12.1, CPython 3.12).
All 18 setup-uv invocation(s) resolve their version from it.
exit=0
…with an unpinned setup-uv live in zizmor.yml.
The header attributes this to needing "the YAML structure this check deliberately does not model," and that's the part I'd push back on: the for wf in loop directly below already does the per-file comparison. Promoting it from the failure branch to the primary check closes the cross-file case entirely and leaves only the same-file variant — no structure modelling, and it removes a caveat rather than documenting one. Roughly:
for wf in "$WORKFLOWS"/*.yml "$WORKFLOWS"/*.yaml; do
[ -e "$wf" ] || continue
uses=$(grep -c 'uses: astral-sh/setup-uv@' "$wf" || true)
has=$(grep -c 'version: ${{ env.UV_VERSION }}' "$wf" || true)
[ "$uses" -eq "$has" ] || { unpinned="${unpinned} ${wf}: ${uses} invocation(s), ${has} pinned"$'\n'; }
doneThe per-file || true here is also why this loop doesn't hit the set -e problem from my other comment — worth keeping that spelling when you fix the totals.
Generated by Claude Code
| - [ ] End-to-end test spec files added for every supported feature | ||
| - [ ] Tests pass locally | ||
| - [ ] CI workflow matrix updated in `.github/workflows/dojo-e2e.yml` (entry name matches `agents.ts`) | ||
| - [ ] **Python only:** new CI steps use `${{ env.UV_VERSION }}` / `${{ env.PYTHON_VERSION }}` matching `.github/python-toolchain.env`, and any venv cache key carries both versions — verify with `bash scripts/release/verify-python-toolchain-pins.sh` (see Step 7) |
There was a problem hiding this comment.
This overstates what the script covers. verify-python-toolchain-pins.sh never looks at cache keys — pass 1 greps env: declarations, pass 2 counts setup-uv invocations against version: ${{ env.UV_VERSION }}. There's no handling of py${{ env.PYTHON_VERSION }}-uv${{ env.UV_VERSION }} anywhere in it.
So a venv cache key that omits the toolchain segments passes this check green, and the checklist now tells contributors it was verified. Cache keys are the one part of this change that's still convention-only, which is worth saying plainly given the round we just had about that distinction.
Either split the line so only the pin half claims verification, or add a third pass. The third pass is tractable if you want it — count actions/cache steps whose path: matches .venv against keys containing both segments — but scoping it correctly is more work than the other two, and I'd take the honest checklist over a rushed check.
Same applies to the "11 / 11" row in the verification table: true, and locally established, not enforced.
Generated by Claude Code
3 participants
Closes PNI-187.
Python CI did not pin the tool that builds its environments, and nothing verified that a committed lockfile still matched its manifest for packages without a test lane.
What changed
One uv, one CPython, everywhere. All 18
astral-sh/setup-uvinvocations resolved to a floating>=0.8.0or named no version at all, so any job could silently get a uv that no green run had used. Each now takes an exact version from.github/python-toolchain.env, which also records the run those values came from — run 30958253208 (unit-python-sdk.yml,main@bfc22e4e) installed uv 0.12.1 in every Python job and built every venv against CPython 3.12.3.That pin is enforced, not just recorded.
scripts/release/verify-python-toolchain-pins.shruns as thepython-toolchain-pinsjob and fails the build when a workflow's literal disagrees with the record, or when asetup-uvinvocation gets its version any other way. Two passes, because the second catches what the first structurally cannot:UV_VERSION/PYTHON_VERSIONliteral under.github/workflows/equals the record — reported asfile:line, and quote-tolerant so an unquoted pin is not misread as absentuses: astral-sh/setup-uv@has a matchingversion: ${{ env.UV_VERSION }}— a newly added invocation that names no version declares nothing for pass 1 to compare, which is exactly the state all 18 were in before this changeBoth refuse to pass vacuously: renamed or reindented pins fail loudly rather than inspecting nothing.
Venv cache keys carry both versions. Previously a cached environment built by one toolchain could be restored into a job expecting another, which hides drift rather than surfacing it.
Lockfile drift fails the build.
uv sync --lockedalready covers the nine packages with a test lane. A newlockfilesjob runsuv lock --checkover every first-party lock, which is what closes the gap:agent-specandclaude-agent-sdkship committed lockfiles with no lane at all, and that is how the drift #2313 had to repair went unnoticed across several releases. A second step asserts no first-party package is missing a lockfile, since a package with none is invisible to afind-based check and passes silently.No job may rewrite a committed lockfile. Each Python job that installs dependencies now ends with
.github/actions/assert-lockfiles-unchanged. It measures the outcome —git statusover every tracked lockfile — rather than trying to infer which commands a job ran.Fork-artifact hardening (
publish-python-preview.yml)Called out separately because it is a security change, not housekeeping, and would not be findable under this PR's title otherwise.
That workflow runs on
workflow_run, so on a fork PR the three metadata files it reads are produced by the fork's run — untrusted input reaching a job that holdspull-requests: write. The previousecho "k=$(cat f)"form had two defects: it exits 0 whencatfails, so a missing file silently yielded an empty value; and a value containing a newline could set arbitrary additional step outputs. Nowset -euo pipefail, shape validation on all three values, and a single grouped redirect. The version regex was checked against whatbuild-python-preview.ymlactually emits (0.0.0.dev${git log -1 --format=%ct}).It stayed in this PR rather than being split because adding the file to the actionlint scope is what surfaced it (SC2129 at error level), so splitting would mean either landing a red lint job or reverting the scope addition. Splitting was considered and rejected on that basis — see the thread.
Design notes for review
prepare-release.ymlis where the pin matters most. Since #2314 that workflow runs uv, and its output lands in a committed lockfile — so an exact version there is load-bearing in a way it is not elsewhere.examples/lockfiles are deliberately out of scope.prep-dojo-everything.jssyncs those non-frozen on purpose (it is shared with local dev, where relocking is wanted), so the checks above exclude them, and!examples/**keeps an examples-only lock edit from running the nine lanes that exclude examples anyway. Documented at thelockfilesjob and in CONTRIBUTING.The composite action's shell lives in a
.shfile. Not cosmetic: actionlint 1.7.x has no composite-action mode, so pointed at anaction.ymlit parses it as a workflow and emits four bogussyntax-checkerrors. Shell embedded in a composite action is therefore linted by nothing. Extracting it makes the shellcheck job cover it — which now globs.github/actionstoo.Third pin caveat worth knowing: naming
python-versiononsetup-uvalso exportsUV_PYTHONfor the whole job, so every later uv call in it is constrained to that interpreter. Everyrequires-pythonin the repo admits 3.12 today; a future example pinned to>=3.13would fail inside dojo-e2e rather than in the package that declared it. Recorded in the pins file.One waiver, deliberately left open.
sdks/python/a2ui_toolkitships asag-ui-a2ui-toolkitand has no lockfile, so it is the one published package the new gate cannot cover. It declaresdependencies = [], so nothing is unverified today and the committed lock would be near-empty; the exposure is that the first dependency anyone adds arrives invisible to the gate. Closing it changes that package's release behaviour and emptiesknown_missing, so it is PNI-279 rather than a rider here.Verification
Run locally against
325907e5:setup-uvinvocations pinnedverify-python-toolchain-pins.shgoes red on driftsetup-uv, renamed pins)python-versionstragglersuv lock --checkacross first-party locksactionlintover the linted scopeshellcheck(scripts/release+.github/actions, 12 files)actionlint_flagsparses to 10 existing paths, no stray argsPython runtime support ranges are unchanged — this is about the tooling that builds and tests, not which Python versions the packages support.
For whoever merges
Expect a one-time cold cache. Every venv cache key and dojo-e2e's
restore-keyschange at once, so the first run after merge misses across ~10 caches. That is expected, not a regression.Commits
Split by area of concern, each independently reviewable: