[Qiming Bao (14H034160212)]

Description

Closes #3722.

The style guide says functions with more than one parameter must put * after self/cls. Today only the constructors of the pluggable base classes enforce this (at class-definition time, through pyrit.common.brick_contract.enforce_keyword_only_init). Nothing checks other functions and methods, so new signatures can drift from the convention.

This PR adds a static check for the rest of pyrit/. It does not change any existing signature. Migrating existing APIs is left to separate PRs.

What changed

• build_scripts/check_keyword_only_params.py: an AST check in the same style as check_async_suffix.py. It runs as the check-keyword-only-params hook in .pre-commit-config.yaml (pass_filenames: false, whole-tree scan), so CI runs it through the existing pre-commit run --all-files step.
• build_scripts/keyword_only_baseline.txt: a baseline of the 291 existing violations, so the hook passes on main and only flags new code.
• doc/contributing/3_style_guide.md: a new "Keyword-Only Parameters" section covering the rule, the exemptions and the baseline workflow. .github/instructions/style-guide.instructions.md gets a short pointer to the hook.

The rule

A def or async def under pyrit/ may declare at most one positional parameter, not counting the implicit self/cls of a method. Positional-only (/) and positional-or-keyword parameters both count. *args and **kwargs do not. @staticmethod functions have no implicit first parameter, and nested functions are not treated as methods. Private helpers, nested functions and all __init__/__new__ are in scope, as the issue asks for internal functions.

Exemptions (narrow, documented)

1. Python data-model dunders other than __init__/__new__ (__exit__, __aexit__, __call__, __eq__, ...). The interpreter or a protocol calls them positionally.
2. @overload stubs. Their signatures mirror the implementation, and the implementation is checked.
3. Three framework method names that a third-party base class calls positionally: dispatch (Starlette middleware), process_bind_param and process_result_value (SQLAlchemy TypeDecorator). This mirrors _FRAMEWORK_EXEMPT_NAMES in check_async_suffix.py.
4. A # pyrit-keyword-only-exempt <reason> marker on any line of the def header, for one-off third-party callbacks such as argparse type= or signal handlers.

@override is not an exemption. In the current tree, no @override method trips the rule.

How the baseline works

• One entry per line in module:qualname form, for example pyrit.auth.azure_auth:get_speech_config or pyrit.x:outer.<locals>.inner. The file has no line numbers and is sorted and de-duplicated, so edits that move code don't touch it and diffs stay reviewable.
• The check fails on new violations (keys not in the baseline). It also fails on stale entries (keys that no longer violate), so a migrated function must be removed from the baseline and the file can only shrink.
• To regenerate it, for example after renaming or moving a baselined function: uv run python -m build_scripts.check_keyword_only_params --write-baseline. The docs say not to add new code to it.
• What the 291 entries contain: 40 in pyrit.backend, 147 private (_-prefixed) functions, 26 nested functions and 18 __init__/__new__. The issue's inventory (about 236 non-backend candidates) used a different scope, which is why the numbers differ. Many of these are public APIs, and changing them would break positional callers, so they should be migrated deliberately in follow-ups.

Open questions for maintainers

• Threshold wording. The style guide says "more than 1 parameter must use *". Read strictly, that also flags def f(self, a, *, b). This check allows that signature and flags only more than one positional parameter, as proposed in the issue. Would you prefer the stricter reading?
• Stale entries fail the check. This keeps the baseline shrinking, but anyone who migrates a signature also has to edit the baseline. Would a warning be enough?
• Scope. Only pyrit/ is scanned. Tests are excluded because pytest injects fixtures as positional parameters, and build_scripts/ is excluded to match check_async_suffix.
• Framework exemptions. Should exemptions stay global by name, as here and in check_async_suffix, or should they use the marker at each site?

Tests and Documentation

New unit tests are in tests/unit/build_scripts/test_check_keyword_only_params.py (49 cases). They cover:

• Signatures that pass: one positional, *, *args, **kwargs, classmethods, keyword-only __init__.
• Signatures that are flagged: two positionals, defaults, /, async, private, __init__/__new__, nested classes and functions, staticmethods, __init__.py module naming.
• Each exemption, plus @override not exempting, and the marker in single-line headers, multi-line headers and function bodies.
• Baseline behaviour: a baselined function passes, keys survive line moves, a new violation next to a baselined one is reported, stale entries fail, --write-baseline output is sorted and de-duplicated with LF endings, a missing baseline file is handled, and syntax errors are reported.

Commands run locally (Linux, Python 3.13, uv sync --frozen):

uv run pytest tests/unit/build_scripts/test_check_keyword_only_params.py -q
# 49 passed in 0.12s

uv run pytest tests/unit/build_scripts -q
# 539 passed, 1 warning in 12.88s

uv run python -m build_scripts.check_keyword_only_params
# exit 0 on this branch (291 baselined, 0 new, 0 stale)

SKIP=ty-check uv run pre-commit run --files <changed files>
# all hooks passed (ruff-format, ruff-check, validate-docs, check-yaml, end-of-file-fixer, check-keyword-only-params, ...)

uv run pre-commit run check-keyword-only-params --all-files
# Passed
uv run pre-commit run check-async-suffix --all-files
# Passed

uv run ty check build_scripts/check_keyword_only_params.py tests/unit/build_scripts/test_check_keyword_only_params.py
# All checks passed!

The ty-check pre-commit hook only covers pyrit/, and this PR does not touch pyrit/. I did not run JupyText because no notebooks or doc code samples changed.

AI assistance: drafted with an AI coding assistant; I reviewed it and tested it as described above.

[Copilot]

🟡 Changes recommended

The exemption scan misidentifies function-header boundaries, allowing body comments to suppress violations while missing valid exemptions on multiline signatures’ closing lines.

1 open issue

• Locate exemption comments using the actual function header boundary · New

What changed in this PR

Adds static enforcement of PyRIT’s keyword-only parameter convention, addressing #3722 without changing existing APIs.

Changes:

• Adds an AST-based checker and pre-commit hook for functions and methods under pyrit/.
• Baselines 291 existing violations and reports new violations or stale entries.
• Documents exemptions and baseline maintenance, with 49 unit-test cases covering checker behavior.

File	Description
tests/​unit/​build_scripts/​test_check_keyword_only_params.py	Tests signature checks, exemptions, baseline handling, and errors.
doc/​contributing/​3_style_guide.md	Documents the rule, exemptions, and migration workflow.
build_scripts/​keyword_only_baseline.txt	Records existing violations to preserve API compatibility.
build_scripts/​check_keyword_only_params.py	Implements AST scanning and baseline validation.
.pre-commit-config.yaml	Registers the whole-tree keyword-only check.
.github/​instructions/​style-guide.instructions.md	Adds guidance pointing to the hook and baseline.

_{Review effort: Balanced}

[Roman Lutz (romanlutz)]

The header-boundary finding in Copilot's review summary was fixed by the author in 1065d6a, and its inline thread is resolved. The regression cases cover both misplaced body comments and exemptions on multiline closing header lines.

I have now pushed the separate legacy-contract exemption guidance and regression coverage, then merged main at a97f359. The updated PR head is beab1f7. Both static-check hooks and documentation sections are preserved. The initial baseline includes three additional signatures already present on main, for 294 total; no existing runtime signatures were changed. The 87 focused static-check tests and applicable commit hooks passed locally, and the refreshed CI runs are approved and running.

_{🤖 AI-authored reply via Agency Finish PR.}