From db565a41ecf635fba9cd3d9bfa0368336171f95f Mon Sep 17 00:00:00 2001 From: James David Clarke Date: Sat, 23 May 2026 20:34:44 +0100 Subject: [PATCH 1/3] docs: expand AGENTS.md with test marker guidance and CI audit notes Document the four-way test marker decision (expectedFailure / expectedFailureIf vs skip / skipIf, with skip reserved for tests that crash the interpreter), add guidance on avoiding zizmor's impostor-commit audit when pinning third-party actions, link the wiki guide for syncing tests from upstream CPython, and note the grep recipe for finding WIP TODO: RUSTPYTHON entries. Co-Authored-By: Claude Opus 4.7 (1M context) --- AGENTS.md | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index b407328cffb..ce1ad4181d8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -81,6 +81,35 @@ The `Lib/` directory contains Python standard library files copied from the CPyt - `unittest.skip("TODO: RustPython ")` - `unittest.expectedFailure` with `# TODO: RUSTPYTHON ` comment +#### Choosing the right marker + +When marking a test that fails on RustPython, prefer one of the following forms: + +```python +@unittest.expectedFailure # TODO: RUSTPYTHON; +# or +@unittest.expectedFailureIf(, "TODO: RUSTPYTHON; ") +``` + +If the test would crash the interpreter (segfault, Rust panic, abort, infinite loop), use `skip` instead so the rest of the suite can still run: + +```python +@unittest.skip("TODO: RUSTPYTHON; ") +# or +@unittest.skipIf(, "TODO: RUSTPYTHON; ") +``` + +**When to use which:** + +- **Prefer `expectedFailure` / `expectedFailureIf`** by default. The test body still runs, so if RustPython is later fixed, the unexpected pass surfaces immediately and the decorator can be removed. Use the conditional `*If` form when the failure is environment-specific (e.g., a platform or build flag). +- **Use `skip` / `skipIf` only when running the test would take down the test process** — segfaults, Rust panics, aborts, or hangs that block subsequent tests. Skipping keeps the suite usable; `expectedFailure` cannot help here, because the test body still executes. + +To find WIP entries that are partly modified and may need follow-up: + +```bash +grep -d recurse 'TODO: RUSTPYTHON' Lib/test/ +``` + ### Clean Build When you modify bytecode instructions, a full clean is required: @@ -258,9 +287,37 @@ See DEVELOPMENT.md "CPython Version Upgrade Checklist" section. - Document that it requires PEP 695 support - Focus on tests that can be fixed through Rust code changes only +## CI Workflows and Security Audits + +PR CI runs [zizmor](https://docs.zizmor.sh/) to audit `.github/workflows/*.yaml` for security issues. The most common failure for contributors editing workflows is the `impostor-commit` audit, which can reject a PR even when the SHA pin "looks correct". + +### Avoiding the `impostor-commit` audit + +GitHub's fork network lets a commit that exists only on a fork be referenced via the parent repo's `owner/repo` slug. A malicious fork could publish a backdoored commit and reference it as if it lived on the canonical upstream — a hash-pinned `uses:` line that visually matches best practice but actually points into a fork. zizmor's [`impostor-commit` audit](https://docs.zizmor.sh/audits/#impostor-commit) detects this by querying whether the commit really exists in the claimed repo. + +**Rule:** when adding or updating a `uses:` line, always pin to a SHA that exists in the **canonical** upstream repo for that action, never one that only exists on a fork. + +Verify a pin before pushing: + +```bash +# For: uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 +gh api repos/actions/checkout/commits/de0fac2e4500dabe0009e67214ff5f5447ce83dd --jq .sha +``` + +If the response is the same SHA, the commit is canonical and the pin is safe. A `404` means the SHA does not exist in `actions/checkout` (it lives only on a fork) — zizmor will flag it as an impostor and CI will fail. + +**Repo convention:** every third-party action is pinned to a full 40-char SHA with the human-readable tag in a trailing comment, e.g. `actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2`. Use the same style when adding new actions. + +To find the canonical SHA for a tag, use the canonical repo's release/tag page on github.com (the SHA shown next to the tag) or: + +```bash +gh api repos///git/refs/tags/ --jq .object.sha +``` + ## Documentation - Check the [architecture document](/architecture/architecture.md) for a high-level overview - Read the [development guide](/DEVELOPMENT.md) for detailed setup instructions - Generate documentation with `cargo doc --no-deps --all` - Online documentation is available at [docs.rs/rustpython](https://docs.rs/rustpython/) +- [How to update test files](https://github.com/RustPython/RustPython/wiki/How-to-update-test-files#checkout-cpython-source-code-initial-setup) — guide for syncing test cases from upstream CPython into the `Lib/` directory From 9ea89fbc3e10e5611710f55f0482ff1c4ad3634c Mon Sep 17 00:00:00 2001 From: James David Clarke Date: Sun, 24 May 2026 12:06:31 +0100 Subject: [PATCH 2/3] docs: add pre-commit checks rule, trim CI section per review Add a "CRITICAL: Pre-commit Checks" subsection requiring prek and the full test suite to pass before any commit. Shrink the CI Workflows section to a single sentence per @ShaharNaveh's review feedback: contributors only need to know that workflow changes must pass a zizmor scan in CI, not the full impostor-commit explainer. Co-Authored-By: Claude Opus 4.7 (1M context) --- AGENTS.md | 33 ++++++++------------------------- 1 file changed, 8 insertions(+), 25 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index ce1ad4181d8..f508d70eda6 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -38,6 +38,12 @@ RustPython is a Python 3 interpreter written in Rust, implementing Python 3.14.0 - Always ask the user before performing any git operations that affect the remote repository - Commits can be created locally when requested, but pushing and PR creation require explicit approval +**CRITICAL: Pre-commit Checks** +- Before creating ANY commit, you MUST run `prek run --all-files` (or `pre-commit run --all-files`) AND the full test suite. Both must pass — do not commit if either fails. +- Test commands are documented in the [Testing](#testing) section below. At minimum run `cargo test --workspace --exclude rustpython_wasm --exclude rustpython-venvlauncher`; if the change touches `extra_tests/snippets/` run `pytest -v` there too, and if it touches `Lib/` or interpreter behavior, run the relevant `cargo run --release -- -m test ` modules. +- If a hook auto-fixes files (e.g. `ruff-format`, `rustfmt`), re-stage the fixes, re-run `prek` until it reports a clean pass, then re-run the tests, then commit. +- NEVER bypass these checks with `--no-verify`, `--no-gpg-sign`, or by skipping tests "because the change is small". If a hook or test fails, fix the underlying issue and create a new commit — do not amend or force the failing commit through. + ## Important Development Notes ### Running Python Code @@ -287,32 +293,9 @@ See DEVELOPMENT.md "CPython Version Upgrade Checklist" section. - Document that it requires PEP 695 support - Focus on tests that can be fixed through Rust code changes only -## CI Workflows and Security Audits - -PR CI runs [zizmor](https://docs.zizmor.sh/) to audit `.github/workflows/*.yaml` for security issues. The most common failure for contributors editing workflows is the `impostor-commit` audit, which can reject a PR even when the SHA pin "looks correct". - -### Avoiding the `impostor-commit` audit - -GitHub's fork network lets a commit that exists only on a fork be referenced via the parent repo's `owner/repo` slug. A malicious fork could publish a backdoored commit and reference it as if it lived on the canonical upstream — a hash-pinned `uses:` line that visually matches best practice but actually points into a fork. zizmor's [`impostor-commit` audit](https://docs.zizmor.sh/audits/#impostor-commit) detects this by querying whether the commit really exists in the claimed repo. - -**Rule:** when adding or updating a `uses:` line, always pin to a SHA that exists in the **canonical** upstream repo for that action, never one that only exists on a fork. - -Verify a pin before pushing: - -```bash -# For: uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 -gh api repos/actions/checkout/commits/de0fac2e4500dabe0009e67214ff5f5447ce83dd --jq .sha -``` - -If the response is the same SHA, the commit is canonical and the pin is safe. A `404` means the SHA does not exist in `actions/checkout` (it lives only on a fork) — zizmor will flag it as an impostor and CI will fail. - -**Repo convention:** every third-party action is pinned to a full 40-char SHA with the human-readable tag in a trailing comment, e.g. `actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2`. Use the same style when adding new actions. +## CI Workflows -To find the canonical SHA for a tag, use the canonical repo's release/tag page on github.com (the SHA shown next to the tag) or: - -```bash -gh api repos///git/refs/tags/ --jq .object.sha -``` +If you modify any file under `.github/workflows/`, the change must pass a [zizmor](https://docs.zizmor.sh/) scan in CI. ## Documentation From 352c5b6c283413096fa99c31d93b71d0bc9440f2 Mon Sep 17 00:00:00 2001 From: James David Clarke Date: Sun, 24 May 2026 21:01:21 +0100 Subject: [PATCH 3/3] docs: warn against doc comments on pyattr/pyclass/pyfunction items - Note that rustpython-doc supplies CPython docstrings via the derive macros - Flag that `///` comments override that source and are dropped on `#[pyattr]` --- AGENTS.md | 1 + 1 file changed, 1 insertion(+) diff --git a/AGENTS.md b/AGENTS.md index f508d70eda6..c89b2a4d3a4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -164,6 +164,7 @@ Run `./scripts/whats_left.py` to get a list of unimplemented methods, which is h - Do not delete or rewrite existing comments unless they are factually wrong or directly contradict the new code. - Do not add decorative section separators (e.g. `// -----------`, `// ===`, `/* *** */`). Use `///` doc-comments or short `//` comments only when they add value. +- Do not put `///` doc comments on items annotated with `#[pyattr]`, `#[pyclass]`, or `#[pyfunction]`. The derive macros pull authoritative docstrings from CPython via the `rustpython-doc` crate; a Rust doc comment overrides that source, and on `#[pyattr]` it is silently dropped. #### Avoid Duplicate Code in Branches