Harden the release permissions / inputs - #809
Merged
Merged
Conversation
…shell code Each job in the release now carries only the token permissions it uses, rather than every job inheriting id-token: write, contents: write and pull-requests: write from the workflow. Only the job that creates the draft release can write repository contents; the platform jobs get an OIDC token and read access; the job that just computes outputs gets nothing. Because a called workflow can only reduce what its calling job grants, those per-job grants are also the ceiling for the reusable platform workflows, and the Windows workflow scopes its own three jobs explicitly on top of that. The workflow-level default is now empty, so a job added later without a permissions block fails loudly instead of quietly inheriting write access. Nothing in the release path ever used pull-requests: write, so that grant is gone. Workflow inputs no longer reach a PowerShell body as expanded source. ARCHITECTURE and PYTHON_VERSIONS arrive as environment variables, so their contents cannot be parsed as script. The wheel enumeration stops setting pyo3's interpreter variables. They were added to keep that build a no-op, but maturin resolves no runnable interpreter for an abi3 wheel and sets no such variables itself, so matching them reasoned about a mechanism that was never in play. The v0.52.6 release measured the enumeration at 0.76s with nothing recompiled, and the comment now records that number instead of an argument for it. bin/README.md still pointed patch_dev_version at CI workflows that no longer exist; it is used only by publish_test_pypi for local builds.
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughRelease workflows now apply explicit least-privilege permissions. The Windows workflow updates runner permissions, PowerShell environment handling, multi-version wheel builds, architecture-based archive naming, and dependency-cache refresh behavior. Development-version and Windows release-profile documentation are also clarified. ChangesRelease workflow permissions
Windows release build and packaging
Documentation clarification
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant release_windows
participant uv
participant maturin
participant S3Cache
participant Archives
release_windows->>uv: Install requested Python versions
release_windows->>maturin: Build wheels per interpreter
release_windows->>S3Cache: Refresh exact-hit dependency object
release_windows->>Archives: Create architecture-specific zip files
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/release_windows.yml:
- Around line 196-203: Update the release workflow’s Python version handling
before the uv python install step: split PYTHON_VERSIONS into individual
versions and install/pass each version separately. In the foreach loop around
maturin build, resolve the matching cpython-$version directory to a concrete
python.exe path instead of passing a literal wildcard, then use that resolved
path for --interpreter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b61b4539-1f23-438c-81b5-fc0a8a114722
📒 Files selected for processing (3)
.github/workflows/release.yml.github/workflows/release_windows.ymlbin/README.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/release_windows.yml:
- Around line 203-205: Update the foreach version build loop to resolve the
cpython-$version.*\python.exe wildcard to exactly one matching interpreter path
before invoking maturin. Pass that resolved path to maturin’s --interpreter
argument, and preserve the existing nonzero LASTEXITCODE handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1785bfca-728b-47ef-885b-2d108277b886
📒 Files selected for processing (1)
.github/workflows/release_windows.yml
Comment on lines
203
to
205
| foreach ($version in $versions) { | ||
| uvx maturin build --release --interpreter ($uvPython -f $version) | ||
| uvx maturin build --release --interpreter "C:\Users\Administrator\AppData\Roaming\uv\python\cpython-$version.*\python.exe" | ||
| if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
Resolve the interpreter glob before invoking maturin.
The quoted cpython-$version.*\python.exe argument is passed literally to the native command; PowerShell does not expand this wildcard, and maturin does not resolve it. Resolve exactly one matching python.exe path before calling maturin. This is the same unresolved issue raised in the previous review.
🛠️ Proposed fix
foreach ($version in $versions) {
- uvx maturin build --release --interpreter "C:\Users\Administrator\AppData\Roaming\uv\python\cpython-$version.*\python.exe"
+ $interpreters = @(Get-ChildItem -Path "C:\Users\Administrator\AppData\Roaming\uv\python\cpython-$version.*\python.exe" -File -ErrorAction SilentlyContinue)
+ if ($interpreters.Count -ne 1) {
+ throw "Expected exactly one Python interpreter for $version, found $($interpreters.Count)"
+ }
+ uvx maturin build --release --interpreter $interpreters[0].FullName
if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/release_windows.yml around lines 203 - 205, Update the
foreach version build loop to resolve the cpython-$version.*\python.exe wildcard
to exactly one matching interpreter path before invoking maturin. Pass that
resolved path to maturin’s --interpreter argument, and preserve the existing
nonzero LASTEXITCODE handling.
An exact cache hit re-archives nothing, so the stored tree's age kept climbing while it was still serving every release, and an age-based expiry would eventually delete a tree the next release depends on. The save step now copies the object onto itself on a hit, restarting its clock and leaving the archive, its size and its storage class untouched. That took 30 seconds server-side for the current 7.6 GiB tree, and a failure is reported as a warning rather than failing the release, since a stale timestamp costs one slow build and nothing more. No expiry is configured on the bucket today, so this changes nothing by itself; it is what makes adding one safe. The maturin interpreter argument also picks up a comment. It is a glob that reaches maturin unexpanded and matches nothing, which reads like an oversight and has been raised as one several times. The v0.52.4 and v0.52.6 logs show maturin building the abi3 wheel from bundled sysconfig with no interpreter resolved, and dropping the flag is not equivalent: with none given, maturin falls back to PYO3_PYTHON or whatever python happens to be on PATH.
Fat LTO costs 17m23s to build the CLI on a Windows release runner where thin costs 10m38s, and Windows binary size is not a constraint. Six profile variants were measured back to back on one runner with every build cold, and fat accounted for 405 of the 590 seconds available across all of them, so thin is where nearly all of the win is and the remaining levers grow the binary far faster than they save time. The Linux and macOS releases keep fat LTO and stay as small as they can be. Cargo cannot set a profile per target, so the Windows job overrides the shared profile through CARGO_PROFILE_RELEASE_LTO, which also reaches the cdylib that maturin builds. codegen-units stays at 1 everywhere. Raising it to 16 saves roughly two more minutes but grows the binary by 14 MiB and its effect on runtime is unmeasured; ENG-1459 tracks measuring that.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/release_windows.yml (1)
199-200: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNormalize whitespace before splatting versions.
-split ' 'leaves empty items for repeated/leading/trailing spaces and misses tabs/newlines, souv python installcan receive bad version args. Split on\s+, filter empties, and reject an empty list before invokinguv.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release_windows.yml around lines 199 - 200, Update the PowerShell version parsing around $env:PYTHON_VERSIONS to split on \s+, remove empty entries, and reject an empty resulting list before calling uv python install. Preserve the existing splatting behavior for valid version lists and ensure uv is not invoked when no versions are configured.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/release_windows.yml:
- Around line 199-200: Update the PowerShell version parsing around
$env:PYTHON_VERSIONS to split on \s+, remove empty entries, and reject an empty
resulting list before calling uv python install. Preserve the existing splatting
behavior for valid version lists and ensure uv is not invoked when no versions
are configured.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5b646be9-004f-4c7f-80e0-6cbd938257f6
📒 Files selected for processing (2)
.github/workflows/release_windows.ymlCargo.toml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Each job in the release now carries only the token permissions it uses, rather than every job inheriting broad permissions.
Workflow inputs no longer reach a PowerShell body as expanded source.
ARCHITECTUREandPYTHON_VERSIONSarrive as environment variables, so their contents cannot be parsed as script.The wheel enumeration stops setting pyo3's interpreter variables. They were added to keep that build a no-op, but maturin resolves no runnable interpreter for an abi3 wheel and sets no such variables itself, so matching them reasoned about a mechanism that was never in play. The v0.52.6 release measured the enumeration at 0.76s with nothing recompiled, and the comment now records that number instead of an argument for it.
bin/README.mdstill pointedpatch_dev_versionat CI workflows that no longer exist. It is used only bypublish_test_pypifor local builds.