Py code style

Python Code-Style Review

Use style findings after correctness findings. Defer to the project's configured formatter and linter when they exist.

Freshness: stable (no external references) — review rules based on core Python conventions, not volatile APIs.

Review Rules

Rule: style-defer-to-tooling

Impact: LOW-MEDIUM Applies when: The project has ruff, black, isort, mypy, or pyright configuration. Skip when: No tooling exists and the issue is purely subjective. Python: any Tools: ruff | mypy | pyright | project-configured Review signal: Review feedback contradicts or duplicates configured automated tooling.

Incorrect:

# Reviewer asks for single quotes while pyproject config uses double quotes.
name = 'Ada'

Correct:

# Follow configured formatter output.
name = "Ada"

Reason: Style review should reinforce automated tools, not create a parallel subjective standard.

Rule: style-import-organization

Impact: LOW-MEDIUM Applies when: Imports are added or moved. Skip when: The project formatter/linter will auto-fix the issue and CI already enforces it. Python: any Tools: ruff | project-configured Review signal: Standard library, third-party, and local imports are mixed, or relative imports are added without need.

Incorrect:

from myapp.models import User
import os
import httpx

Correct:

import os

import httpx

from myapp.models import User

Reason: Predictable import grouping reduces merge churn and makes dependencies easier to scan.

Rule: style-naming-clarity

Impact: LOW-MEDIUM Applies when: New public names, modules, classes, functions, or constants are introduced. Skip when: The name follows a domain convention or matches an external API. Python: any Tools: ruff | project-configured Review signal: Abbreviated or misleading names obscure the domain concept.

Incorrect:

def proc_usr(u: User) -> Result:
    ...

Correct:

def process_user(user: User) -> Result:
    ...

Reason: Clear names reduce the need for comments and make reviewable intent visible.

Rule: style-public-docstring

Impact: LOW-MEDIUM Applies when: A public class, function, or method has non-obvious behavior, side effects, or failure modes. Skip when: The public API is self-evident and fully described by its name and type signature. Python: any Tools: ruff | project-configured Review signal: Public APIs with important arguments, return values, raised exceptions, or examples lack docstrings.

Incorrect:

def process_batch(items: list[Item], max_workers: int = 4) -> BatchResult:
    ...

Correct:

def process_batch(items: list[Item], max_workers: int = 4) -> BatchResult:
    """Process items concurrently and return successes plus per-item failures."""
    ...

Reason: Docstrings are most valuable where types alone do not communicate side effects or failure semantics.

Rule: style-line-length-readability

Impact: LOW Applies when: A changed line is hard to review because it combines multiple calls, conditions, or string fragments. Skip when: The configured formatter keeps the line and readability is acceptable. Python: any Tools: ruff | project-configured Review signal: Long expressions are technically valid but obscure intermediate meaning.

Incorrect:

return client.fetch(user.id, include_orders=True, include_invoices=True, timeout=timeout, retry_policy=retry_policy)

Correct:

return client.fetch(
    user.id,
    include_orders=True,
    include_invoices=True,
    timeout=timeout,
    retry_policy=retry_policy,
)

Reason: Review style should improve scanability where automated formatting alone is not enough.

Rule: style-string-regex-hygiene

Impact: MEDIUM Applies when: Code contains regex patterns, string interpolation, or string construction that could be simplified. Skip when: The regex is inherently complex (e.g., parsing nested structures) and the escaping is unavoidable. Python: any Tools: ruff | project-configured Review signal: Manual escape sequences in character classes, verbose string building, or regex patterns that could use Python convenience features.

Incorrect:

# Unnecessary backslash before double-quote in raw string character class
re.search(r'python-version:\s*[\\"\\']{ver}[\\"\\']', text)

# Verbose string construction
expected = "[" + ", ".join(f'"{v}"' for v in VERSIONS) + "]"

# Hardcoded string used in multiple places
if filepath == ".agents/skills/skill-discovery":  # also defined in another file

Correct:

# Switch quote delimiter to avoid escaping inside character class
re.search(rf"python-version:\s*[\"']{ver}[\"']", text)

# Use a magic string constant — readable at a glance
EXPECTED_YAML = '["3.10", "3.14"]'

# Extract to a named constant — single source of truth
SYMLINK_ENTRY = ".agents/skills/skill-discovery"
if filepath == SYMLINK_ENTRY:

Reason: Python provides quote-delimiter switching, f-strings, re.escape(), and named constants to avoid manual escaping. Complex escape sequences are error-prone for both humans and AI agents, and often indicate a simpler approach exists.

Rule: style-regex-escape-strategy

Impact: MEDIUM Applies when: Code builds regex patterns by concatenating user input, configuration values, or dynamic strings. Skip when: The pattern is a static literal with no interpolated values. Python: any Tools: ruff | project-configured Review signal: Manual escaping of regex metacharacters in interpolated values, or missing re.escape() on user-controlled input.

Incorrect:

# Manual escaping — error-prone, misses edge cases
domain = "example.com"
pattern = r"https?://" + domain.replace(".", "\\.") + r"/.*"

# User input not escaped — ReDoS or wrong match
def find_users(query: str) -> list[str]:
    return re.findall(rf"User: {query}", log_text)

Correct:

# re.escape() handles all metacharacters correctly
domain = "example.com"
pattern = rf"https?://{re.escape(domain)}/.*"

# Escape user input before embedding in regex
def find_users(query: str) -> list[str]:
    return re.findall(rf"User: {re.escape(query)}", log_text)

Reason: re.escape() is the canonical way to sanitize strings for regex interpolation. Manual escaping misses characters (e.g., {, }, () and creates maintenance burden when regex syntax evolves.

Rule: style-quote-delimiter-strategy

Impact: LOW Applies when: Raw strings contain quotes that require escaping inside the string delimiter. Skip when: The regex or string is simple enough that escaping is minimal and clear. Python: any Tools: ruff | project-configured Review signal: Unnecessary backslash-escaping of quotes inside raw strings, or mixed delimiter styles within the same module.

Incorrect:

# Double-quoted raw string — " needs escaping, ' doesn't
re.search(r"version:\s*[\"']?[\"']", text)  # confusing

# Single-quoted raw string — ' needs escaping, " doesn't  
re.search(r'version:\s*[\"\\']?[\"\\']', text)  # worse

Correct:

# Choose delimiter so the character class needs no escaping
re.search(r"version:\s*[\"']?[\"']", text)   # " is in class, use " delimiter → ' doesn't escape
re.search(r'version:\s*[\"\'']?[\"\'']', text)  # ' is in class, use ' delimiter → " doesn't escape

# Or use a character class with the delimiter's quote first
re.search(r"[\"']+", text)   # Either quote — no escaping needed

Reason: Consistent delimiter choice eliminates escape noise. If the character class contains ", use ' as the string delimiter (or vice versa). Pick one convention per project and follow it.

Rule: style-constant-placement

Impact: LOW-MEDIUM Applies when: Named values are defined as module-level constants or hardcoded in multiple locations. Skip when: The value is truly local to a single function and used nowhere else. Python: any Tools: ruff | project-configured Review signal: Magic strings/numbers repeated across files, or module-level constants that are only used in one function.

Incorrect:

# Hardcoded in multiple places — no single source of truth
if status == "completed":  # also in validator.py and test_validators.py
    ...

# Module-level constant used only in one function
MAX_RETRIES = 3  # defined at top, used only in fetch_with_retry()

def fetch_with_retry(url: str) -> Response:
    for _ in range(MAX_RETRIES):  # could be local
        ...

Correct:

# Extracted to a shared constant — single source of truth
# In constants.py or at module top
COMPLETED_STATUS = "completed"

if status == COMPLETED_STATUS:
    ...

# Function-local when only used there
def fetch_with_retry(url: str) -> Response:
    max_retries = 3  # local — doesn't need module scope
    for _ in range(max_retries):
        ...

Reason: Constants belong at the narrowest scope that covers all their uses. Module-level is for values shared across functions or files; function-local is for values used in one place. Repeated magic strings should be extracted to a single definition.

Rule: style-deduplicate-reusable-patterns

Impact: MEDIUM Applies when: Code review reveals duplicated logic, repeated boilerplate, or patterns that appear in 2+ places with minor variations. Skip when: The duplication is intentional (e.g., independent implementations that may diverge) or the abstraction would be harder to understand than the repetition. Python: any Tools: ruff | project-configured Review signal: Nearly identical code blocks, repeated setup/teardown sequences, or copy-pasted logic that could be extracted into a shared function, class, or utility.

Incorrect:

# Same validation logic in two places
def validate_user(user: User) -> bool:
    if not user.name:
        return False
    if not user.email or "@" not in user.email:
        return False
    if user.age < 0 or user.age > 150:
        return False
    return True

def validate_admin(admin: Admin) -> bool:
    if not admin.name:
        return False
    if not admin.email or "@" not in admin.email:
        return False
    if admin.age < 0 or admin.age > 150:
        return False
    return True

# Repeated test setup
def test_create():
    db = create_test_db()
    db.connect()
    user = User(name="test", db=db)
    ...
    db.close()

def test_update():
    db = create_test_db()
    db.connect()
    user = User(name="test", db=db)
    ...
    db.close()

Correct:

# Shared validation logic
def validate_person(name: str, email: str, age: int) -> bool:
    if not name:
        return False
    if not email or "@" not in email:
        return False
    if age < 0 or age > 150:
        return False
    return True

def validate_user(user: User) -> bool:
    return validate_person(user.name, user.email, user.age)

def validate_admin(admin: Admin) -> bool:
    return validate_person(admin.name, admin.email, admin.age)

# Shared fixture or helper
@pytest.fixture
def user_with_db():
    user = User(name="test", email="[email protected]")
    db.connect()
    yield user
    db.disconnect()

Reason: Duplicated code drifts — when one copy is updated and the other isn't, bugs follow. Extract shared logic into named functions, fixtures, or utilities. The threshold is 2+ occurrences with minor variations: if the abstraction is clearer than the repetition, extract it.

Rule: style-use-inflection-for-pluralization

Impact: MEDIUM Applies when: Code manually pluralizes or singularizes strings using hand-written rules (e.g., if word.endswith("y"): word[:-1] + "ies"). Domain prefixes in metadata, table-of-contents generators, schema-to-folder mappings, and filename convention checkers are common triggers. Skip when: The project already uses inflection or another pluralization library, or the string transformation is domain-specific and not standard English pluralization. Python: any Tools: inflection Review signal: Manual pluralization patterns: word.endswith("y") + slice arithmetic, word.endswith(("s", "sh", "ch")) + "es" append, or any dictionary of irregular plurals. Fragile edge cases: words like "person"→"people" or "analysis"→"analyses" are almost always missed by hand-written rules. Incorrect:

def pluralize(word: str) -> str:
    """Manually pluralize — misses irregulars and edge cases."""
    if word.endswith("y") and word[-2] not in "aeiou":
        return word[:-1] + "ies"
    if word.endswith(("s", "sh", "ch")):
        return word + "es"
    return word + "s"

# Used in a verifier that checks filename/frontmatter consistency
domain = "skill"
tag = pluralize(domain)  # "skills" — works
domain = "person"
tag = pluralize(domain)  # "persons" — wrong, should be "people"

Correct:

import inflection

domain = "skill"
tag = inflection.pluralize(domain)  # "skills"
domain = "person"
tag = inflection.pluralize(domain)  # "people"
domain = "analysis"
tag = inflection.pluralize(domain)  # "analyses"

# Singularize works the other direction
filename_tag = "personalities"
domain = inflection.singularize(filename_tag)  # "personality"

Reason: English pluralization has dozens of irregular cases (person→people, mouse→mice, datum→data). Hand-written endswith("y") rules silently fail on these. inflection (a pure-Python library, ported from Ruby's ActiveSupport) handles all standard English pluralization in a single function call. It is the standard solution — do not reinvent it.

Sensitive Evidence Safety

If changed code or tool output reveals a suspected credential, token, private key, secret-bearing URL, or other sensitive value, do not quote or reproduce the value. Report only its existence and location. Treat filename and pattern checks as heuristic evidence, not proof that a repository is secret-free.

If the exposure appears credible, make it the first finding, stop lower-priority review, and recommend revocation or rotation. Never place sensitive values in reports, generated examples, or commit subjects or bodies.