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.
- Clarified when to use expected-failure vs skip markers for unit tests and how to identify WIP test entries.
- Expanded CI documentation to require workflow scanning and added guidance for updating test files from upstream via the project wiki.

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

📝 Walkthrough

Walkthrough

This PR updates AGENTS.md with three contributor-facing additions: a CRITICAL pre-commit and test-running checklist, a “Choosing the right marker” subsection for RustPython test failures, 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.
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,
Workflows scanned and tests in view,
Small notes to help the keepers true,
Happy tails for contributors too 🥕

🚥 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 describes the main changes: expanding AGENTS.md documentation with test marker guidance and CI audit notes, which directly aligns with the changeset.
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

coderabbitai Bot reviewed May 23, 2026

View reviewed changes

Comment thread AGENTS.md

Comment thread AGENTS.md Outdated

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

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

ShaharNaveh requested changes May 24, 2026

View reviewed changes

Comment thread AGENTS.md Outdated

coderabbitai Bot reviewed May 24, 2026

View reviewed changes

ShaharNaveh approved these changes May 24, 2026

View reviewed changes

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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
JamesClarke7283 wants to merge 2 commits into
RustPython:mainfrom
JamesClarke7283:agents-md-2

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 Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

JamesClarke7283 commented May 23, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 23, 2026 •

edited

Loading