X Tutup
Skip to content

Unify lint CI job#7505

Merged
youknowone merged 19 commits intoRustPython:mainfrom
ShaharNaveh:ci-lint-unify
Mar 26, 2026
Merged

Unify lint CI job#7505
youknowone merged 19 commits intoRustPython:mainfrom
ShaharNaveh:ci-lint-unify

Conversation

@ShaharNaveh
Copy link
Contributor

@ShaharNaveh ShaharNaveh commented Mar 25, 2026

ATM we have multiple places where we define lint steps, like running ruff this PR aims to streamline the process by condensing everything to a single job

Summary by CodeRabbit

  • Chores

    • Streamlined CI: reorganized jobs, adjusted tooling and steps, added doc generation and targeted lint runs, and removed standalone formatting/security jobs
    • Added Dependabot entry to regularly check pre-commit updates
    • Removed the separate PR-formatting workflow
  • Tooling

    • Added repository-wide pre-commit configuration for formatting, linting, metadata generation, and spell checks
    • Enhanced redundant-patch checker to accept file patterns and emit CI-friendly annotations

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 25, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds Dependabot pre-commit updates, introduces a repository-level .pre-commit-config.yaml, refactors CI workflows (consolidates/removes lint/format jobs and steps in .github/workflows/ci.yaml, deletes pr-format.yaml), and enhances scripts/check_redundant_patches.py with glob args, deduplication, and CI-style diagnostics.

Changes

Cohort / File(s) Summary
Dependabot
\.github/dependabot.yml
Added updates entry for the pre-commit ecosystem scoped to / with interval: weekly and default-days: 7.
CI workflows
\.github/workflows/ci.yaml, \.github/workflows/pr-format.yaml
Reworked ci.yaml: added Linux-only cargo doc --locked, reorganized lint job to remove many previous format/lint steps and introduce cargo shear (via cargo-binstall), actionlint, zizmor, prek with cache keyed to .pre-commit-config.yaml, adjusted Rust components and added clippy installs/runs in several jobs, removed standalone cargo-shear/security-lint jobs; deleted pr-format.yaml.
Pre-commit config
.pre-commit-config.yaml
New pre-commit pipeline (fail_fast: false): merge-conflict check, Ruff format/check (I rules) with fixes, local hooks for cargo fmt and opcode metadata generation (serialized), cspell CLI with extra dictionaries, and Prettier limited to wasm/.
Scripts
scripts/check_redundant_patches.py
Added CLI arg parsing (build_argparser()), iter_files() to expand/deduplicate glob patterns, IS_GH_CI flag to emit GitHub Actions ::error annotations, changed main() to main(patterns: list[str]), and adjusted invocation from __main__.

Sequence Diagram(s)

sequenceDiagram
    participant Dev as Developer
    participant GH as GitHub Actions
    participant CI as CI Jobs
    participant PC as Pre-commit Hooks
    participant RD as Reviewdog

    Dev->>GH: Push PR / commit
    GH->>CI: Trigger workflows (ci.yaml)
    CI->>PC: Run pre-commit pipeline (`.pre-commit-config.yaml`)
    PC-->>CI: Return hook results (pass/fail)
    CI->>CI: Run cargo/doc/clippy/shear/actionlint/zizmor/prek steps
    CI-->>GH: Report job status
    GH->>RD: On lint/format failure, run reviewdog suggester
    RD-->>Dev: Post inline suggestions/comments
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested labels

skip:ci

Suggested reviewers

  • youknowone
  • moreal

Poem

🐰 I hopped through hooks and tidy trees,
Ruff brushed my whiskers, Prettier pleased,
Shear snipped edges, actionlint gave a peep,
CI hummed on, reviewdog watched my leap,
A small rabbit cheers the pipeline sweep.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Unify lint CI job' directly and accurately summarizes the main objective of the pull request, which consolidates multiple CI lint steps into a single unified job.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

- name: check compilation without threading
run: cargo check ${{ env.CARGO_ARGS }}

- run: cargo doc --locked
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to here because cargo check and cargo doc uses the same profile

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (3)
.pre-commit-config.yaml (2)

24-31: Consider adding pass_filenames: false for consistency.

The script scripts/check_redundant_patches.py ignores CLI arguments and always scans the entire Lib/test directory (see lines 10-12 of the script). The current types_or: [python] setting passes filenames that are never used.

For clarity, consider:

♻️ Suggested improvement
       - id: redundant-test-patches
         name: Ensure no redundant test patches
         entry: python scripts/check_redundant_patches.py
         language: system
-        types_or: [python]
+        files: '^Lib/test/.*\.py$'
+        pass_filenames: false
         priority: 0

This makes the trigger more explicit (only run when test files change) and documents that filenames aren't used.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.pre-commit-config.yaml around lines 24 - 31, The hook
redundant-test-patches uses scripts/check_redundant_patches.py which ignores
passed filenames, so update the hook definition for id "redundant-test-patches"
to explicitly set pass_filenames: false and keep the existing entry ("python
scripts/check_redundant_patches.py") so pre-commit won't pass unused file args
to that script; this makes the hook intent explicit and avoids misleading
invocation semantics.

43-49: Add file filter to only trigger on relevant changes.

This hook runs on every commit since there's no files or types filter. The script only needs to run when crates/compiler-core/src/bytecode/instruction.rs changes.

♻️ Suggested improvement
       - id: generate-opcode-metadata
         name: Generate opcode metadata
         entry: python scripts/generate_opcode_metadata.py
         pass_filenames: false
         language: system
+        files: 'crates/compiler-core/src/bytecode/instruction\.rs$'
         require_serial: true # so rustfmt runs first
         priority: 1

This avoids unnecessary script execution and potential git status changes when the bytecode file hasn't been modified.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.pre-commit-config.yaml around lines 43 - 49, The pre-commit hook with id
"generate-opcode-metadata" currently runs on every commit; update its
configuration in .pre-commit-config.yaml to add a files filter that matches the
bytecode source file (crates/compiler-core/src/bytecode/instruction.rs) so the
hook only triggers when that file changes; locate the hook by the id/name
("generate-opcode-metadata" / "Generate opcode metadata") and add an appropriate
files entry (or types entry) to restrict execution to that path.
.github/workflows/ci.yaml (1)

355-358: Consider pinning cargo-shear version for reproducibility.

The cargo binstall --no-confirm cargo-shear command installs the latest version, which could cause unexpected CI failures if a new version introduces breaking changes or new lints.

♻️ Suggested improvement
       - name: cargo shear
         run: |
-          cargo binstall --no-confirm cargo-shear
+          cargo binstall --no-confirm cargo-shear@1.1.3
           cargo shear

Alternatively, Dependabot could manage this if cargo-shear is added as a dev-dependency.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/ci.yaml around lines 355 - 358, Pin the cargo-shear
installation in the "cargo shear" workflow step instead of installing the
latest: replace the current installer invocation (cargo binstall --no-confirm
cargo-shear) with a pinned installer command that specifies a known-good version
(e.g., cargo-shear@<version>), or add cargo-shear as a dev-dependency and let
Dependabot manage updates; update the workflow step that runs "cargo shear" to
rely on that pinned installation so CI remains reproducible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In @.github/workflows/ci.yaml:
- Around line 355-358: Pin the cargo-shear installation in the "cargo shear"
workflow step instead of installing the latest: replace the current installer
invocation (cargo binstall --no-confirm cargo-shear) with a pinned installer
command that specifies a known-good version (e.g., cargo-shear@<version>), or
add cargo-shear as a dev-dependency and let Dependabot manage updates; update
the workflow step that runs "cargo shear" to rely on that pinned installation so
CI remains reproducible.

In @.pre-commit-config.yaml:
- Around line 24-31: The hook redundant-test-patches uses
scripts/check_redundant_patches.py which ignores passed filenames, so update the
hook definition for id "redundant-test-patches" to explicitly set
pass_filenames: false and keep the existing entry ("python
scripts/check_redundant_patches.py") so pre-commit won't pass unused file args
to that script; this makes the hook intent explicit and avoids misleading
invocation semantics.
- Around line 43-49: The pre-commit hook with id "generate-opcode-metadata"
currently runs on every commit; update its configuration in
.pre-commit-config.yaml to add a files filter that matches the bytecode source
file (crates/compiler-core/src/bytecode/instruction.rs) so the hook only
triggers when that file changes; locate the hook by the id/name
("generate-opcode-metadata" / "Generate opcode metadata") and add an appropriate
files entry (or types entry) to restrict execution to that path.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 98cc673b-3373-4fa2-aea3-bdbbc11edfd3

📥 Commits

Reviewing files that changed from the base of the PR and between c9cfb3d and a8a9358.

📒 Files selected for processing (4)
  • .github/dependabot.yml
  • .github/workflows/ci.yaml
  • .github/workflows/pr-format.yaml
  • .pre-commit-config.yaml
💤 Files with no reviewable changes (1)
  • .github/workflows/pr-format.yaml

@github-actions
Copy link
Contributor

github-actions bot commented Mar 25, 2026

📦 Library Dependencies

The following Lib/ modules were modified. Here are their dependencies:

[x] lib: cpython/Lib/operator.py
[ ] test: cpython/Lib/test/test_operator.py

dependencies:

  • operator

dependent tests: (43 tests)

  • operator: test_abstract_numbers test_argparse test_array test_bigaddrspace test_bigmem test_binop test_bool test_builtin test_bytes test_collections test_complex test_copy test_ctypes test_dbm_dumb test_decimal test_enumerate test_float test_fractions test_frame test_functools test_heapq test_index test_inspect test_ipaddress test_iter test_iterlen test_itertools test_kqueue test_math test_monitoring test_numeric_tower test_ordered_dict test_plistlib test_richcmp test_set test_slice test_sort test_str test_struct test_sys test_typing test_weakref test_xml_etree

Legend:

  • [+] path exists in CPython
  • [x] up-to-date, [ ] outdated

@ShaharNaveh ShaharNaveh force-pushed the ci-lint-unify branch 2 times, most recently from 41c8e3a to 1796ab9 Compare March 25, 2026 18:25
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@scripts/check_redundant_patches.py`:
- Line 16: The function build_argparser has an incorrect return type annotation
that calls the constructor; change the annotation from argparse.ArgumentParser()
to the class type argparse.ArgumentParser (no parentheses) and keep the function
body returning a new instance as before; update the signature for
build_argparser accordingly so the type hint refers to the class, not an
instantiated object.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 54e4bb07-26cf-48f8-994e-80f69b4f2b7d

📥 Commits

Reviewing files that changed from the base of the PR and between a8a9358 and 41c8e3a.

⛔ Files ignored due to path filters (1)
  • Lib/test/test_operator.py is excluded by !Lib/**
📒 Files selected for processing (5)
  • .github/dependabot.yml
  • .github/workflows/ci.yaml
  • .github/workflows/pr-format.yaml
  • .pre-commit-config.yaml
  • scripts/check_redundant_patches.py
💤 Files with no reviewable changes (1)
  • .github/workflows/pr-format.yaml
✅ Files skipped from review due to trivial changes (2)
  • .github/dependabot.yml
  • .pre-commit-config.yaml

@ShaharNaveh ShaharNaveh force-pushed the ci-lint-unify branch 4 times, most recently from a0a0172 to 14e04ed Compare March 25, 2026 18:40
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/workflows/ci.yaml:
- Around line 355-358: The workflow currently installs the latest cargo-shear
which can break CI; update the step that runs "cargo binstall --no-confirm
cargo-shear" to pin cargo-shear to an exact version using the cargo-binstall
syntax (e.g., "cargo-shear@=x.y.z"); modify the step named "cargo shear" so the
binstall command references the pinned package name and leave the subsequent
"cargo shear" invocation unchanged.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: c3be1671-bb45-4158-82ed-2ae52e09aaa3

📥 Commits

Reviewing files that changed from the base of the PR and between 76e9460 and a0a0172.

⛔ Files ignored due to path filters (1)
  • Lib/test/test_operator.py is excluded by !Lib/**
📒 Files selected for processing (3)
  • .github/workflows/ci.yaml
  • .pre-commit-config.yaml
  • scripts/check_redundant_patches.py
✅ Files skipped from review due to trivial changes (2)
  • .pre-commit-config.yaml
  • scripts/check_redundant_patches.py

Comment on lines +355 to +358
- name: cargo shear
run: |
python scripts/generate_opcode_metadata.py
if [ -n "$(git status --porcelain)" ]; then
exit 1
fi
cargo binstall --no-confirm cargo-shear
cargo shear
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Checking current cargo-shear install command:"
rg -n 'cargo binstall --no-confirm .*cargo-shear' .github/workflows/ci.yaml

Repository: RustPython/RustPython

Length of output: 164


🏁 Script executed:

# Check the context around the cargo-shear installation and other tool installations
head -360 .github/workflows/ci.yaml | tail -30

Repository: RustPython/RustPython

Length of output: 919


🏁 Script executed:

# Look for other cargo binstall or tool installation commands to see if versions are pinned elsewhere
rg -n 'cargo binstall|curl.*install|wget' .github/workflows/ci.yaml -A 1 | head -50

Repository: RustPython/RustPython

Length of output: 426


🏁 Script executed:

# Check if there's a Cargo.lock or similar that might indicate version management strategy
git ls-files | grep -E '(Cargo|package.*lock|requirements)'

Repository: RustPython/RustPython

Length of output: 822


🏁 Script executed:

# Check if there are any examples of pinned versions with cargo binstall in the repo
rg -n 'cargo binstall.*@' .

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Look at cargo-binstall documentation - check if the repo has any docs or comments about it
rg -n 'cargo-binstall' .github/workflows/ci.yaml -B 2 -A 2

Repository: RustPython/RustPython

Length of output: 233


🏁 Script executed:

# Search for any references to cargo-shear version pinning or issues
rg -n 'cargo-shear' .

Repository: RustPython/RustPython

Length of output: 123


🌐 Web query:

cargo binstall version pinning syntax @

💡 Result:

The version pinning syntax in cargo-binstall uses @ followed by a Cargo version requirement string. For exact versions, use @=VERSION, such as cargo binstall radio-sx128x@=0.14.1-alpha.5. This resolves using Cargo's semver parser, where =0.14.1-alpha.5 pins to exactly that version, and underspecified forms like =1.2 are equivalent to >=1.2.0, <1.3.0. Examples: - Exact version: cargo binstall crate@=1.2.3 - Pre-release: cargo binstall crate@=1.0.0-alpha.1 - Range (compatible updates): cargo binstall crate@^1.2 Alternatively, use the --version VERSION flag: cargo binstall --version 1.2.3 crate. This syntax is shown in the official README and logs, where radio-sx128x@0.14.1-alpha.5 resolves as 'radio-sx128x@=0.14.1-alpha.5'. It leverages Cargo's version req parser (clap-cargo dependency) for flexibility matching Cargo.toml dependencies.

Citations:


Pin cargo-shear to a fixed version for reproducible CI.

Line 357 installs the latest cargo-shear on every run, which can cause sudden CI breakages from upstream releases. The cargo-binstall tool itself is pinned to v1.17.8, so cargo-shear should be pinned as well.

♻️ Proposed fix
       - name: cargo shear
         run: |
-          cargo binstall --no-confirm cargo-shear
+          cargo binstall --no-confirm cargo-shear@=<pinned-version>
           cargo shear

Use the @=<version> syntax to pin to an exact version.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/ci.yaml around lines 355 - 358, The workflow currently
installs the latest cargo-shear which can break CI; update the step that runs
"cargo binstall --no-confirm cargo-shear" to pin cargo-shear to an exact version
using the cargo-binstall syntax (e.g., "cargo-shear@=x.y.z"); modify the step
named "cargo shear" so the binstall command references the pinned package name
and leave the subsequent "cargo shear" invocation unchanged.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

♻️ Duplicate comments (1)
.github/workflows/ci.yaml (1)

355-358: ⚠️ Potential issue | 🟠 Major

Line 357 still leaves cargo-shear floating.

Installing the current cargo-shear release on every run makes the unified lint job vulnerable to unrelated upstream breakage. cargo-binstall supports selecting an explicit package version, so this should be pinned too. (github.com)

♻️ Suggested fix
       - name: cargo shear
         run: |
-          cargo binstall --no-confirm cargo-shear
+          cargo binstall --no-confirm cargo-shear@<tested-version>
           cargo shear
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/ci.yaml around lines 355 - 358, The CI step named "cargo
shear" currently installs the latest cargo-shear with "cargo binstall
--no-confirm cargo-shear" which risks upstream breakage; change the install to
pin a specific release by using the cargo-binstall version syntax (e.g., replace
that line with "cargo binstall --no-confirm cargo-shear@<VERSION>") and keep
invoking the tool via "cargo shear" so the job installs a fixed cargo-shear
release instead of the floating latest.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/workflows/ci.yaml:
- Around line 332-338: The unified lint job ("name: Lint") is missing the
skip:ci short-circuit used elsewhere; update the Lint job to include the same
conditional guard used by other jobs so the workflow is skipped when the skip:ci
label is present on a PR (i.e., add the same "if" expression/guard that checks
for the skip:ci label to the Lint job), leaving the existing permissions block
intact.
- Around line 334-338: Add the missing permission and guard the reviewdog action
to PRs only: under the workflow permissions block add "issues: write" alongside
the existing "checks: write" and "pull-requests: write", and update the
reviewdog/action-suggester step's if condition so it only runs in a pull request
context (e.g. change the current if: failure() to an expression that also checks
the event is a pull_request, like github.event_name == 'pull_request' &&
failure()).
- Line 353: The workflow step that uses the action
"cargo-bins/cargo-binstall@113a77a4ce971c41332f2129c3d995df993cf746" should
explicitly pin the cargo-binstall binary by adding an inputs block (with:) and
setting the version field to a specific release (e.g., version: "v1.17.8" or the
exact binary tag you want), so update the step that contains the uses:
cargo-bins/cargo-binstall... line to include a with: version: "<pinned-version>"
entry to ensure reproducible installs.

---

Duplicate comments:
In @.github/workflows/ci.yaml:
- Around line 355-358: The CI step named "cargo shear" currently installs the
latest cargo-shear with "cargo binstall --no-confirm cargo-shear" which risks
upstream breakage; change the install to pin a specific release by using the
cargo-binstall version syntax (e.g., replace that line with "cargo binstall
--no-confirm cargo-shear@<VERSION>") and keep invoking the tool via "cargo
shear" so the job installs a fixed cargo-shear release instead of the floating
latest.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 51feb7dc-e3f3-4f03-9aa0-ad86e0d5ea14

📥 Commits

Reviewing files that changed from the base of the PR and between a0a0172 and 14e04ed.

⛔ Files ignored due to path filters (1)
  • Lib/test/test_operator.py is excluded by !Lib/**
📒 Files selected for processing (3)
  • .github/workflows/ci.yaml
  • .pre-commit-config.yaml
  • scripts/check_redundant_patches.py
✅ Files skipped from review due to trivial changes (1)
  • .pre-commit-config.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
  • scripts/check_redundant_patches.py

Comment on lines +332 to +338
name: Lint
runs-on: ubuntu-latest
permissions:
contents: read
checks: write
pull-requests: write
security-events: write # for zizmor
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Restore the skip:ci guard on lint if that label is still meant to suppress the whole workflow.

Every other job in this workflow still short-circuits on skip:ci, but the unified lint job no longer does. Applying that label will therefore still run this path.

♻️ Suggested fix
   lint:
+    if: ${{ !contains(github.event.pull_request.labels.*.name, 'skip:ci') }}
     name: Lint
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/ci.yaml around lines 332 - 338, The unified lint job
("name: Lint") is missing the skip:ci short-circuit used elsewhere; update the
Lint job to include the same conditional guard used by other jobs so the
workflow is skipped when the skip:ci label is present on a PR (i.e., add the
same "if" expression/guard that checks for the skip:ci label to the Lint job),
leaving the existing permissions block intact.

Comment on lines +334 to +338
permissions:
contents: read
checks: write
pull-requests: write
security-events: write # for zizmor
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's find and examine the workflow file
find . -name "ci.yaml" -o -name "ci.yml" | head -20

Repository: RustPython/RustPython

Length of output: 92


🌐 Web query:

reviewdog/action-suggester permissions requirements documentation

💡 Result:

The reviewdog/action-suggester GitHub Action requires the following permissions in your workflow YAML: permissions: contents: read checks: write issues: write pull-requests: write These are specified in the official README to enable posting code suggestions via GitHub's multi-line suggestions feature on pull requests. See GitHub docs on Assigning permissions to jobs for details. The action supports pull_request events primarily.

Citations:


🏁 Script executed:

# Read the workflow file around the mentioned lines
fd "ci.yaml" --type f --exec wc -l {} \; 2>/dev/null | head -5

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Read the workflow file to check permissions and action usage
cat -n .github/workflows/ci.yaml | sed -n '320,400p'

Repository: RustPython/RustPython

Length of output: 3245


🏁 Script executed:

# Also check the workflow triggers at the top
cat -n .github/workflows/ci.yaml | head -20

Repository: RustPython/RustPython

Length of output: 915


Add missing issues: write permission and condition the reviewdog/action-suggester to pull requests only.

The action's documentation specifies it requires issues: write in addition to checks: write and pull-requests: write. Since the workflow runs on push, merge_group, and workflow_dispatch events, the current if: failure() condition can invoke the action outside pull request context where it cannot function. Add the required permission and limit execution to pull request events.

Suggested fix
     permissions:
       contents: read
       checks: write
+      issues: write
       pull-requests: write
       security-events: write # for zizmor
       - name: reviewdog
-        if: failure()
+        if: failure() && github.event_name == 'pull_request'
         uses: reviewdog/action-suggester@aa38384ceb608d00f84b4690cacc83a5aba307ff # 1.24.0
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/ci.yaml around lines 334 - 338, Add the missing permission
and guard the reviewdog action to PRs only: under the workflow permissions block
add "issues: write" alongside the existing "checks: write" and "pull-requests:
write", and update the reviewdog/action-suggester step's if condition so it only
runs in a pull request context (e.g. change the current if: failure() to an
expression that also checks the event is a pull_request, like github.event_name
== 'pull_request' && failure()).


- name: Ensure docs generate no warnings
run: cargo doc --locked
- uses: cargo-bins/cargo-binstall@113a77a4ce971c41332f2129c3d995df993cf746 # v1.17.8
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, check the content around line 353 in the workflow file
cat -n .github/workflows/ci.yaml | sed -n '350,360p'

Repository: RustPython/RustPython

Length of output: 431


🌐 Web query:

cargo-binstall action version input default behavior

💡 Result:

The cargo-binstall GitHub Action is cargo-bins/cargo-binstall@main. It installs the cargo-binstall tool (a faster alternative to cargo install that downloads prebuilt binaries). The version input specifies the cargo-binstall version to install (e.g., version: "1.2.3"). If not provided, it defaults to the latest version available from the cargo-binstall GitHub releases. Default behavior: Downloads and installs the latest cargo-binstall binary for the current platform into ~/.cargo/bin, making cargo binstall available for subsequent steps. It supports unattended CI use and works across platforms (Linux, macOS, Windows). For specific versions, provide the version input; otherwise, latest is used automatically.

Citations:


🏁 Script executed:

# Check if the specific action SHA mentioned has version input support
# We'll search for action.yml metadata in the cargo-binstall repo history
curl -s "https://raw.githubusercontent.com/cargo-bins/cargo-binstall/113a77a4ce971c41332f2129c3d995df993cf746/action.yml" 2>/dev/null | head -50

Repository: RustPython/RustPython

Length of output: 852


Pin the cargo-binstall binary version explicitly in the action inputs.

The action SHA pins the action code but not the cargo-binstall binary version. The action defaults to installing the latest cargo-binstall release when the version input is not set, so this job will pick up whatever version is current when the workflow runs, affecting reproducibility.

♻️ Suggested fix
-      - uses: cargo-bins/cargo-binstall@113a77a4ce971c41332f2129c3d995df993cf746 # v1.17.8
+      - uses: cargo-bins/cargo-binstall@113a77a4ce971c41332f2129c3d995df993cf746 # v1.17.8
+        with:
+          version: "1.17.8"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- uses: cargo-bins/cargo-binstall@113a77a4ce971c41332f2129c3d995df993cf746 # v1.17.8
- uses: cargo-bins/cargo-binstall@113a77a4ce971c41332f2129c3d995df993cf746 # v1.17.8
with:
version: "1.17.8"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/ci.yaml at line 353, The workflow step that uses the
action "cargo-bins/cargo-binstall@113a77a4ce971c41332f2129c3d995df993cf746"
should explicitly pin the cargo-binstall binary by adding an inputs block
(with:) and setting the version field to a specific release (e.g., version:
"v1.17.8" or the exact binary tag you want), so update the step that contains
the uses: cargo-bins/cargo-binstall... line to include a with: version:
"<pinned-version>" entry to ensure reproducible installs.


def test_a():
return super().test_a()

Copy link
Contributor Author

@ShaharNaveh ShaharNaveh Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

Edit: nvm, it doesn't show the alert in the comment:/ (but it does work)

@ShaharNaveh ShaharNaveh marked this pull request as draft March 26, 2026 04:52
@ShaharNaveh ShaharNaveh marked this pull request as ready for review March 26, 2026 05:16
@ShaharNaveh ShaharNaveh requested a review from youknowone March 26, 2026 06:14
@youknowone youknowone merged commit 9282a87 into RustPython:main Mar 26, 2026
27 of 28 checks passed
@youknowone
Copy link
Member

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

X Tutup