Give os.chmod a valid `__text_signature__` and keyword arguments by leehanjeong · Pull Request #8968 · RustPython/RustPython

leehanjeong · 2026-10-05T12:20:46Z

Closes Malformed builtin __text_signature__ makes inspect.signature(len/sorted/open) raise ValueError #8383

One of checkbox below must be checked.

I did not use AI to write the code of this patch.
This PR follows our AI policy

Summary

os.chmod was the last native function whose __text_signature__ was not
valid Python. It also rejected keyword arguments that CPython accepts.

before  ($module, path, *, dir_fd=None, mode, /, follow_symlinks=True)
after   ($module, /, path, mode, *, dir_fd=None, follow_symlinks=(os.name != 'nt'))

The Rust arguments put the keyword-only dir_fd between path and mode.
Since #8807 renders each argument's kind, * came out before mode and /
after it. The same order made path and mode positional-only, so
os.chmod(path="f", mode=0o644) raised TypeError.

Changes

posix.chmod takes a ChmodArgs struct in CPython's order, as nt.chmod
already does. path and mode are positional-or-keyword.
The signature writer asserts that / is not emitted after *, *args or
**kwargs. Signatures are composed in a const context, so a wrong argument
order fails the build.
builtin_signature.py checks os.chmod.

Notes

whats_left.py --signature does not change. CPython's own os.chmod
signature is unparsable (its default is an expression), so both sides
record the same ValueError text.
inspect.signature(os.chmod) still raises, as in CPython.

Assisted-by: Claude Code:claude-opus-5

Summary by CodeRabbit

Bug Fixes
- os.chmod now uses the platform-appropriate default for follow_symlinks, improving consistency across operating systems.
- On platforms that support it, os.chmod correctly applies the requested symlink-following behavior.

posix.chmod took dir_fd between path and mode. Since RustPython#8807 renders each argument's parameter kind, that put `/` after `*`, which inspect cannot parse, and path and mode stayed positional-only, so os.chmod(path=..., mode=...) raised TypeError. A ChmodArgs struct follows CPython's order, as nt.chmod already does. path and mode are positional-or-keyword. follow_symlinks shows CPython's text through py_default, like LinkArgs. Assisted-by: Claude Code:claude-opus-5

The writer counts positional-only parameters up front and emits `/` when they run out, which assumes they come first in the Rust argument list. Nothing enforced that, and posix.chmod violated it. The signature is composed in a const context, so the assert fails the build and names the fix. Assisted-by: Claude Code:claude-opus-5

coderabbitai · 2026-10-05T12:20:54Z

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used

📚 Code guidelines (1)

AGENTS.md — auto-discovered

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: RustPython/RustPython/.coderabbit.yml
Review profile: CHILL
Plan: Advanced
Run ID: 8ddd33ab-4534-4f3f-8d70-423baf71549c

📥 Commits

Reviewing files that changed from the base of the PR and between d3ff1eb and 465e8dc.

📒 Files selected for processing (3)

crates/vm/src/function/signature.rs
crates/vm/src/stdlib/posix.rs
extra_tests/snippets/builtin_signature.py

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

Walkthrough

The signature renderer now tracks emitted **kwargs and rejects positional-only parameters that follow a variadic marker. Both chmod argument structures parse follow_symlinks as a boolean with a platform-dependent displayed default. A builtin-signature test checks the rendered os.chmod prefix.

Changes

Builtin signatures and chmod arguments

Layer / File(s)	Summary
Track and validate signature parameter ordering `crates/vm/src/function/signature.rs`	The renderer tracks when it emits `*kwargs`. When the final positional-only parameter follows a `` marker, `args`, or `*kwargs`, it panics with a message directing Rust arguments to be reordered.
Parse chmod arguments and check its signature `crates/vm/src/stdlib/posix.rs`, `extra_tests/snippets/builtin_signature.py`	Both platform argument structures parse `follow_symlinks` as a boolean, defaulting to `true`, and declare `(os.name != 'nt')` as the Python-side default. The non-Redox path passes the boolean through chmod handling, including platform-specific `lchmod` dispatch. The test checks the `os.chmod` signature prefix.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 465e8

This change fixes the os.chmod signature so keyword arguments such as path and mode work. No concrete merge-blocking risk was identified.

Security Architecture Review

Security architecture risk: ⚪ Minimal · up to 465e8

The change fixes keyword argument acceptance and signature metadata without changing permission-setting authority or filesystem target selection. No introduced or worsened security concern was identified in the reviewed changes.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

inferred — For callers already able to invoke chmod, keyword acceptance adds an invocation form rather than a new class of filesystem targets, descriptor ownership, or privileged mutation sink. Effective permission-setting exposure is unchanged by the inspected contract change.

Trust Boundaries and Controls

observed — The existing no-follow controls remain in place: applicable Unix platforms use fchmodat with AT_SYMLINK_NOFOLLOW, while macOS, FreeBSD, and NetBSD retain their lchmod dispatch. Unsupported no-follow operations continue returning an error rather than falling back to the follow path.

Resilience and Maintainability Implications

inferred — The metadata-based branch still reads permissions and then mutates the path separately, so concurrent path replacement remains a pre-existing target-identity limitation. The PR preserves this ordering and terminal error mapping and introduces no retries, reservations, rollback, or recovery state.

🚥 Pre-merge checks | ✅ 3 | ❌ 1 | ❓ 1

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 3 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Linked Issues check	❓ Inconclusive	Issue `#8383` requires usable signatures for affected builtins, including `len`, `sorted`, and `open`, or behavior that matches CPython. The current test file confirms that `inspect.signature(len)` succ…	Provide reviewable test or implementation evidence that `inspect.signature(sorted)` and `inspect.signature(open)` return usable signatures or match CPython behavior.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the main change: correcting os.chmod’s text signature and keyword-argument handling.
Out of Scope Changes check	✅ Passed	The `os.chmod` argument changes, signature-rendering assertion, and test update all support signature correctness or CPython-compatible argument handling under issue `#8383`. The reviewed diff shows no …

Full details: Linked Issues check

Explanation

Issue #8383 requires usable signatures for affected builtins, including len, sorted, and open, or behavior that matches CPython. The current test file confirms that inspect.signature(len) succeeds. This PR also fixes os.chmod signature ordering and keyword argument handling. The available evidence does not establish the current sorted and open behavior, so full compliance cannot be confirmed.

Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create a new PR

Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

youknowone

👍

youknowone · 2026-10-05T12:28:04Z

+        dir_fd: DirFd<'static, 0>,
+        // CPython writes the platform expression; on posix it is always true.
+        #[pyarg(named, default = true, py_default = "(os.name != 'nt')")]
+        follow_symlinks: bool,


what's the rationale not to use FollowSymlinks anymore?

I did it to match CPython's __text_signature__. In CPython only os.chmod and os.link show follow_symlinks=(os.name != 'nt'), and the others show True. FollowSymlinks always renders True, and a flattened field can't take py_default, so I used a plain bool, the same way LinkArgs already does for os.link.

Because of that expression, inspect.signature(os.chmod) raises ValueError, same as in CPython. If you prefer to keep FollowSymlinks and show True, I'll switch it back.

probably we can tweak FollowSymlinks to show proper value. but it is good for now

codspeed · 2026-10-05T12:36:17Z

Merging this PR will not alter performance

✅ 62 untouched benchmarks
⏩ 4 skipped benchmarks¹

_{Comparing leehanjeong:8383-chmod-signature (465e8dc) with main (d3ff1eb)}

4 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

chatgpt-codex-connector · 2026-10-05T14:34:26Z

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

youknowone · 2026-10-05T15:14:01Z

Thank you!

leehanjeong added 2 commits October 5, 2026 21:01

github-actions Bot added the z-ca-2026 Tag to track Contribution Academy 2026 label Oct 5, 2026

github-actions Bot assigned leehanjeong Oct 5, 2026

youknowone approved these changes Oct 5, 2026

View reviewed changes

youknowone reviewed Oct 5, 2026

View reviewed changes

leehanjeong marked this pull request as ready for review October 5, 2026 14:34

youknowone merged commit 3126762 into RustPython:main Oct 5, 2026
31 checks passed

youknowdot mentioned this pull request Oct 10, 2026

Support directory descriptors in chmod #9054

Merged

Navigation Menu

Repository navigation

Give os.chmod a valid `__text_signature__` and keyword arguments - #8968

Give os.chmod a valid `__text_signature__` and keyword arguments#8968
youknowone merged 2 commits into
RustPython:mainfrom
leehanjeong:8383-chmod-signature

leehanjeong commented Oct 5, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Oct 5, 2026 •

edited

Loading

Walkthrough

Changes

❌ Failed checks (1 warning, 1 inconclusive)

Uh oh!

youknowone left a comment

Uh oh!

youknowone Oct 5, 2026

Uh oh!

leehanjeong Oct 5, 2026

Uh oh!

youknowone Oct 5, 2026

Uh oh!

codspeed Bot commented Oct 5, 2026

Uh oh!

chatgpt-codex-connector Bot commented Oct 5, 2026

Uh oh!

Uh oh!

youknowone commented Oct 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

leehanjeong commented Oct 5, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Notes

Summary by CodeRabbit

Uh oh!

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

Uh oh!

Walkthrough

Changes

Security Architecture Review

❌ Failed checks (1 warning, 1 inconclusive)

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

youknowone Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

leehanjeong Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

youknowone Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

codspeed Bot commented Oct 5, 2026

Merging this PR will not alter performance

Footnotes

Uh oh!

chatgpt-codex-connector Bot commented Oct 5, 2026

Uh oh!

Uh oh!

youknowone commented Oct 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

leehanjeong commented Oct 5, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Oct 5, 2026 •

edited

Loading