Add agent skill for adding c-api functions#8001
Merged
Merged
Conversation
bschoenmaeckers
changed the title
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR adds a skill guide documentation file that defines the workflow and rules for implementing missing RustPython C-API functions, specifying source-of-truth mappings from pyo3-ffi headers to capi modules, implementation procedures, testing expectations, and completion criteria. ChangesRustPython C-API Expansion Skill Documentation
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. 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
🧹 Nitpick comments (2)
.agents/skills/rustpython-capi-expansion/SKILL.md (2)
28-28: ⚡ Quick winClarify "direct contract assumptions" terminology.
The phrase "direct contract assumptions" is ambiguous in the context of FFI safety. For AI agents implementing unsafe FFI code, clearer guidance would help prevent safety issues.
Consider rephrasing to be more explicit:
-7. Prefer direct contract assumptions over defensive null checks unless required by the established local style. +7. Follow the C-API contract directly (e.g., assume non-null where documented) rather than adding defensive checks, unless existing capi modules show a different pattern.This makes it clear that the guidance is about following documented C-API contracts, not about skipping safety checks arbitrarily.
🤖 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/skills/rustpython-capi-expansion/SKILL.md at line 28, Replace the ambiguous phrase "direct contract assumptions" in SKILL.md with an explicit wording that directs authors to follow the documented C-API/FFI contract precisely (e.g., "follow documented C-API contracts and preconditions exactly") and clarify that this guidance means trusting declared non-null/non-ownership guarantees from the foreign API rather than omitting safety checks arbitrarily; update the surrounding sentence to mention "FFI/C-API documented preconditions (e.g., non-null pointers, lifetime/ownership rules)" so readers know the scope and exceptions.
3-3: 💤 Low valueConsider shortening the description field.
The description spans 277 characters and mixes multiple concerns (what, where, when, and examples). Some YAML parsers or metadata systems may have length constraints. Consider moving detailed examples to the skill body and keeping the description focused on when to trigger this skill.
Example refactor:
-description: Implement missing RustPython C-API functions in crates/capi using the pyo3-ffi header split mapping (`pyo3-ffi/src/*.rs`, mirroring CPython C API headers). Use this whenever the user asks to add or port C-API functions (for example from setobject.h, dictobject.h, unicodeobject.h) or add capi tests. +description: Implement missing RustPython C-API functions using pyo3-ffi header mappings. Triggered when user requests C-API function additions or capi tests.🤖 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/skills/rustpython-capi-expansion/SKILL.md at line 3, The description field is too long and mixes examples and implementation details; shorten it to a single focused trigger statement (e.g., "Trigger when implementing RustPython C-API functions in crates/capi using pyo3-ffi header mappings") and move examples and file/path details (like "pyo3-ffi/src/*.rs", "crates/capi", and example headers such as setobject.h, dictobject.h, unicodeobject.h) into the SKILL.md body or examples section so the description stays concise and under length limits.
🤖 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/skills/rustpython-capi-expansion/SKILL.md:
- Line 12: The SKILL.md reference to pyo3-ffi/src/*.rs is incorrect because
pyo3-ffi is a crate dependency not vendored here; update the SKILL.md
instruction (line referencing `pyo3-ffi/src/*.rs`) to tell agents how to obtain
the exact pyo3-ffi sources that match the repo’s Cargo.lock (e.g., instruct to
fetch the crate from the Cargo registry with the version from Cargo.lock, use
`cargo vendor` to vendor pyo3-ffi into the workspace, or point to the matching
docs.rs/crates.io release), and include which file pattern to look for
(`pyo3-ffi/src/*.rs`) once the crate sources are fetched so the CPython
header-split mapping becomes accessible.
---
Nitpick comments:
In @.agents/skills/rustpython-capi-expansion/SKILL.md:
- Line 28: Replace the ambiguous phrase "direct contract assumptions" in
SKILL.md with an explicit wording that directs authors to follow the documented
C-API/FFI contract precisely (e.g., "follow documented C-API contracts and
preconditions exactly") and clarify that this guidance means trusting declared
non-null/non-ownership guarantees from the foreign API rather than omitting
safety checks arbitrarily; update the surrounding sentence to mention "FFI/C-API
documented preconditions (e.g., non-null pointers, lifetime/ownership rules)" so
readers know the scope and exceptions.
- Line 3: The description field is too long and mixes examples and
implementation details; shorten it to a single focused trigger statement (e.g.,
"Trigger when implementing RustPython C-API functions in crates/capi using
pyo3-ffi header mappings") and move examples and file/path details (like
"pyo3-ffi/src/*.rs", "crates/capi", and example headers such as setobject.h,
dictobject.h, unicodeobject.h) into the SKILL.md body or examples section so the
description stays concise and under length limits.
🪄 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: c58da508-756c-4584-8320-cba5335fabb4
📒 Files selected for processing (1)
.agents/skills/rustpython-capi-expansion/SKILL.md
|
|
||
| ## Source of truth for target files | ||
|
|
||
| - Use this mapping source: `pyo3-ffi/src/*.rs`, which mirrors the CPython header split used by the C API. |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if pyo3-ffi exists in the repository or is a known external dependency
# Check if pyo3-ffi is a local path in the repository
if fd -t d 'pyo3-ffi' .; then
echo "Found pyo3-ffi directory in repository"
fd -t f -e rs . pyo3-ffi/src/ | head -5
else
echo "pyo3-ffi not found as local directory"
fi
# Check if pyo3-ffi is referenced in Cargo.toml files
rg -n 'pyo3-ffi' -g 'Cargo.toml' -C 2Repository: RustPython/RustPython
Length of output: 223
Fix pyo3-ffi/src/*.rs reference in SKILL.md (line 12)
pyo3-ffi/src/*.rsis not reachable via a localpyo3-ffi/srcdirectory in this repo checkout (the follow-up search pathpyo3-ffi/src/is missing).- Since
pyo3-ffiis a crate dependency rather than vendored here, update the skill to tell agents where to obtain thepyo3-ffisources that correspond to theCargo.lockversion (e.g., the Cargo registry checkout /cargo vendor/docs.rs), so the CPython header-split mapping is actually accessible.
🤖 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/skills/rustpython-capi-expansion/SKILL.md at line 12, The SKILL.md
reference to pyo3-ffi/src/*.rs is incorrect because pyo3-ffi is a crate
dependency not vendored here; update the SKILL.md instruction (line referencing
`pyo3-ffi/src/*.rs`) to tell agents how to obtain the exact pyo3-ffi sources
that match the repo’s Cargo.lock (e.g., instruct to fetch the crate from the
Cargo registry with the version from Cargo.lock, use `cargo vendor` to vendor
pyo3-ffi into the workspace, or point to the matching docs.rs/crates.io
release), and include which file pattern to look for (`pyo3-ffi/src/*.rs`) once
the crate sources are fetched so the CPython header-split mapping becomes
accessible.
youknowone
approved these changes
Jun 1, 2026
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
3 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.
Summary
Summary by CodeRabbit