docs: expand AGENTS.md with test marker guidance and CI audit notes by JamesClarke7283 · Pull Request #7963 · RustPython/RustPython

JamesClarke7283 · 2026-05-23T19:35:03Z

Summary

Four additive updates to AGENTS.md to capture guidance that contributors keep asking about. No existing wording was changed — everything is appended in place.

Test marker decision rule. Documents the four forms (@unittest.expectedFailure / expectedFailureIf / skip / skipIf) with the TODO: RUSTPYTHON; <reason> format, and explains when to use which: prefer expectedFailure so a future fix surfaces an unexpected pass; reach for skip only when running the test would take down the test process (segfault, Rust panic, abort, hang).
Avoiding zizmor's impostor-commit audit. Explains why hash-pinned uses: lines can still be rejected when the SHA only exists on a fork, the canonical-repo verification recipe (gh api repos/<owner>/<repo>/commits/<sha>), and the repo's existing convention of pinning to a full 40-char SHA with a trailing # vX.Y.Z comment.
Wiki link to the "How to update test files" guide for syncing tests from upstream CPython.
Grep recipe for locating partly-modified WIP test entries: grep -d recurse 'TODO: RUSTPYTHON' Lib/test/.

Test plan

Rendered AGENTS.md reads cleanly (headings, code fences, link targets)
Existing sections (lines 1–266 in the prior version) are untouched
No code or behavior changes — documentation only

🤖 Generated with Claude Code

Summary by CodeRabbit

Documentation
- Added contributor guidance to run pre-commit checks and the full test suite before committing, including re-staging after auto-fixes.
- Clarified when to use expected-failure vs skip markers for unit tests and how to find WIP test entries.
- Added a docstring rule for items annotated for Python exposure (avoid /// on those items).
- Expanded CI notes to require workflow scanning and linked wiki guidance for syncing Lib tests from upstream.

$Review Change Stack$

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) <noreply@anthropic.com>

coderabbitai · 2026-05-23T19:35:34Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 1a91b4bb-42bc-4644-8858-6ae1e9dcbc34

📥 Commits

Reviewing files that changed from the base of the PR and between 9ea89fb and 352c5b6.

📒 Files selected for processing (1)

AGENTS.md

✅ Files skipped from review due to trivial changes (1)

AGENTS.md

📝 Walkthrough

Walkthrough

This PR updates AGENTS.md with four contributor-facing additions: a CRITICAL pre-commit and test-running checklist, a “Choosing the right marker” subsection for RustPython test failures, a Rust docstring rule for items annotated with py-attributes, and a “CI Workflows” note plus a documentation link for syncing Lib/ tests from CPython.

Changes

Contributor and CI Documentation

Layer / File(s)	Summary
Pre-commit checks and test run guidance `AGENTS.md`	Adds a “CRITICAL: Pre-commit Checks” section requiring `prek`/`pre-commit run --all-files` and the full test suite before creating commits, including when to run extra snippet tests and how to re-stage after auto-fixes.
Test failure marker selection `AGENTS.md`	Introduces “Choosing the right marker” guidance: prefer `unittest.expectedFailure`/`expectedFailureIf` by default; use `unittest.skip`/`skipIf` for interpreter-crashing or hang-blocking failures; includes a `grep` to locate `TODO: RUSTPYTHON` test entries.
Rust docstring rule for py-annotated items `AGENTS.md`	Adds a rule that Rust `///` doc comments must not be used on items annotated with `#[pyattr]`, `#[pyclass]`, or `#[pyfunction]`.
CI workflows and documentation link `AGENTS.md`	Adds a “CI Workflows” section requiring `zizmor` scans for `.github/workflows/` changes and updates the “Documentation” list with a link for syncing `Lib/` test files from upstream CPython.

🎯 1 (Trivial) | ⏱️ ~3 minutes

🐰 I hopped through docs to make things clear,
A checklist here, a marker near,
Docstring rules and workflows too,
Small notes to guide what contributors do,
Crunchy carrots for reviewers, woo! 🥕

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically summarizes the main additions to AGENTS.md: test marker guidance and CI audit notes.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ 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.}

coderabbitai

Actionable comments posted: 2

🤖 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 `@AGENTS.md`:
- Around line 102-106: Update the “only acceptable modifications” section
(currently restricting changes to adding only expectedFailure) to match the new
“When to use which” guidance: allow contributors to also add skip/skipIf when a
test would crash/hang or otherwise make the test process unusable (segfaults,
Rust panics, aborts, or hangs), and keep expectedFailure/expectedFailureIf as
the default for non-crashing regressions; adjust the text so both sections
consistently state that expectedFailure is preferred but skip/skipIf is
permitted for crash/hang scenarios.
- Line 292: Replace the incorrect capitalization of "Github" with the proper
"GitHub" in the contributor docs; specifically update the occurrences in the
sentences that reference PR CI runs (the line containing "PR CI runs [zizmor] to
audit `.github/workflows/*.yaml`") and the line discussing the `impostor-commit`
audit so both use "GitHub" (and scan the surrounding paragraphs for any other
"Github" instances to ensure consistent capitalization).

🪄 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.yml

Review profile: CHILL

Plan: Pro

Run ID: d6122385-1f87-4008-a04f-27b504d37e52

📥 Commits

Reviewing files that changed from the base of the PR and between d3272e7 and db565a4.

📒 Files selected for processing (1)

AGENTS.md

@ShaharNaveh

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) <noreply@anthropic.com>

coderabbitai

♻️ Duplicate comments (1)

AGENTS.md (1)
108-111: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Resolve policy conflict with the "only acceptable modifications" section

This guidance correctly allows skip/skipIf for crash/hang cases, but it conflicts with lines 281-283, which currently state "The only acceptable modifications to test files are: [...] Adding @unittest.expectedFailure decorators when tests cannot be fixed." Please update that section to also allow adding skip/skipIf when a test would crash the interpreter or hang the test process, so contributors have a single consistent rule set.
📝 Suggested fix for lines 281-283
 - The only acceptable modifications to test files are:
   1. Removing `@unittest.expectedFailure` decorators and the upper TODO comments when tests actually pass
-  2. Adding `@unittest.expectedFailure` decorators when tests cannot be fixed
+  2. Adding `@unittest.expectedFailure` decorators when tests cannot be fixed (prefer this by default)
+  3. Adding `@unittest.skip` or `@unittest.skipIf` decorators when running the test would crash the interpreter or hang the test process (segfault, Rust panic, abort, or hang)
🤖 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 `@AGENTS.md` around lines 108 - 111, Update the "The only acceptable
modifications to test files are:" guidance to resolve the conflict by explicitly
allowing adding skip/skipIf in addition to `@unittest.expectedFailure` when a test
would crash or hang the test process; locate the section that currently
references "Adding `@unittest.expectedFailure` decorators when tests cannot be
fixed" and expand that clause to permit adding skip/skipIf (with the same
restriction that these are only for tests that would segfault, panic, abort, or
hang the test process), and ensure you reference both decorators by name
(`@unittest.expectedFailure`, skip, skipIf) so the policy is unambiguous.

🤖 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.

Duplicate comments:
In `@AGENTS.md`:
- Around line 108-111: Update the "The only acceptable modifications to test
files are:" guidance to resolve the conflict by explicitly allowing adding
skip/skipIf in addition to `@unittest.expectedFailure` when a test would crash or
hang the test process; locate the section that currently references "Adding
`@unittest.expectedFailure` decorators when tests cannot be fixed" and expand that
clause to permit adding skip/skipIf (with the same restriction that these are
only for tests that would segfault, panic, abort, or hang the test process), and
ensure you reference both decorators by name (`@unittest.expectedFailure`, skip,
skipIf) so the policy is unambiguous.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 86a57042-bdf9-4815-8d75-c0e05b770c33

📥 Commits

Reviewing files that changed from the base of the PR and between db565a4 and 9ea89fb.

📒 Files selected for processing (1)

AGENTS.md

- Note that rustpython-doc supplies CPython docstrings via the derive macros - Flag that `///` comments override that source and are dropped on `#[pyattr]`

$coderabbitai[bot]$

coderabbitai Bot reviewed May 23, 2026

View reviewed changes

Comment thread AGENTS.md

Comment thread AGENTS.md Outdated

$@ShaharNaveh$ ShaharNaveh added the skip:ci Skip running the ci label May 23, 2026

$@youknowone$

youknowone requested a review from ShaharNaveh May 24, 2026 04:45

$ShaharNaveh$

ShaharNaveh requested changes May 24, 2026

View reviewed changes

Comment thread AGENTS.md Outdated

$coderabbitai[bot]$

coderabbitai Bot reviewed May 24, 2026

View reviewed changes

$ShaharNaveh$

ShaharNaveh approved these changes May 24, 2026

View reviewed changes

$@ShaharNaveh$

ShaharNaveh requested review from fanninpm and youknowone May 24, 2026 18:02

$@JamesClarke7283$

docs: warn against doc comments on pyattr/pyclass/pyfunction items

352c5b6

- Note that rustpython-doc supplies CPython docstrings via the derive macros - Flag that `///` comments override that source and are dropped on `#[pyattr]`

$@youknowone$

youknowone merged commit 021c786 into RustPython:main May 25, 2026
13 checks passed

$@coderabbitai$ coderabbitai Bot mentioned this pull request Aug 8, 2026

Allow automated commits and fix C-API test instructions #8468

Merged

Navigation Menu

Uh oh!

docs: expand AGENTS.md with test marker guidance and CI audit notes - #7963

docs: expand AGENTS.md with test marker guidance and CI audit notes#7963
youknowone merged 3 commits into
RustPython:mainfrom
JamesClarke7283:agents-md-2

$@JamesClarke7283$ JamesClarke7283 commented May 23, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented May 23, 2026 •

edited

Loading

Walkthrough

Changes

Uh oh!

$@coderabbitai$ coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

$@coderabbitai$ coderabbitai Bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

JamesClarke7283 commented May 23, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented May 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

$@JamesClarke7283$ JamesClarke7283 commented May 23, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 23, 2026 •

edited

Loading

$@coderabbitai$ coderabbitai Bot left a comment

$@coderabbitai$ coderabbitai Bot left a comment