ref-python-coding-practices

Python Coding Practices

1. A Python file must not access private variables, functions, or methods in another file or package. It is ok for the test file for a package to access and use the package being tested, even if it is private, and it is ok for the test file to access private variables, functions, and methods in the package.
2. All MCP tools (async functions registered via server.tool()(fn) inside register_tools(server: FastMCP)) have specific docstring requirements — apply the pydocs-improve skill for the full rules, including required "Terminology Note" and "Format Accuracy for AI Agents" sections.
3. f-strings are preferred over % and .format() in format statements.
4. File moves and renames preserve history — use git mv and git rm, never delete-plus-create. Canonical source: AGENTS.md Version Control.
5. A Python file named <file>.py should have a single test file named test_<file>.py. An exception is made for integration tests which are named test_<file>_integration.py. Both the unit test and the integration test live in the same tests/<package>/ directory that mirrors the source's package — not in a separate top-level directory like tests/<package>_integration/. The correct shape is tests/test__logging.py and tests/test__logging_integration.py as siblings; the wrong shape is a parallel tests/integration/ tree.
   • The mirror root follows the source root. src/deephaven_mcp/cli/_help.py → tests/cli/test__help.py; the top-level scripts/convert_config_v1_to_v2.py → tests/scripts/test_convert_config_v1_to_v2.py. A directory that is not a Python package of the shipped library still gets its mirror.
   • test__<name>.py (double underscore) is reserved for mirroring a private module or package named _<name>. tests/cli/test__daemon_integration.py is correct because cli/_daemon/ exists. Applying that form to the config integration test would have been wrong — there is no cli/_config module or package — which is why it lives at tests/cli/_commands/test_config_integration.py, mirroring cli/_commands/config.py. Using the double-underscore form for anything else advertises a source file that was never written, the single most common way test layout drifts.
   • A test whose subject is broader than one source file — a guardrail/contract test, or a package-scope integration test — is named for its subject or invariant (never for a module it does not mirror) and lives in the directory mirroring the narrowest scope containing everything it validates. A guardrail test also carries @pytest.mark.guardrail, so the project's convention-enforcement suite is selectable with uv run pytest -m guardrail. Guardrails are deliberately not collected into one directory: each sits with the code it guards, and the marker — not a shared path — is what makes the family selectable.
     • Scope means the declarations a guardrail audits, not every package it imports. A drift check reads one side's declarations and compares them against the other side as reference data; it belongs with the declaring side. tests/cli/test_tool_wrapper_drift.py imports from both cli/ and mcp_systems_server/_tools/, but the wraps_tool declarations it audits exist only in cli/, so it lives in tests/cli/ rather than at the tests/ root.
     • Canonical implementations: tests/test_field_docs_contract.py (walks the whole package, so the tests/ root is the only scope wide enough), tests/cli/test_help_contract.py (every command's help satisfies the help contract), tests/cli/test_tool_wrapper_drift.py (every CLI wrapper matches its MCP tool), tests/agents/test_skills_catalog.py (mirrors .agents/, not a src/ package at all), tests/mcp_systems_server/_tools/test_tool_module_inventory.py (the tool-module set matches its documented inventory).
6. Prefer specific type hints over Any. Using Any requires an inline justification comment naming the external constraint (third-party stub gap, dynamic plugin interface) that forces it. Without the comment, mypy and reviewers cannot distinguish a deliberate Any from a lazy one.
7. Do not use hasattr or getattr for type narrowing or feature detection. They silently mask AttributeErrors and bypass mypy. Prefer isinstance, structural typing (typing.Protocol), or restructuring the API so the attribute is always present. When getattr is genuinely required (reflection over a closed set of names), pair it with an inline comment naming the constraint.
8. Use American English spelling throughout all code, comments, docstrings, and documentation. For example: "initialized" not "initialised", "recognized" not "recognised", "color" not "colour". Mechanically enforced by uv run codespell (part of ./bin/precommit.sh) using the en-GB-to-en-US dictionary; silence an intentional non-American form (e.g. a quoted counterexample) with an inline codespell:ignore comment on the same line, followed by the comma-separated words to ignore.
9. Unused function parameters should be indicated by prefixing the parameter name with a single underscore (e.g., _request, _host, *_args, **_kwargs). This is the convention ruff/pyright/pylint recognize out of the box and derives from PEP 8's throwaway-variable convention. Do not use del param at the top of a function body to silence unused-argument warnings. The leading-underscore prefix is preferred over del param for all new code.
   • Exception: when callers of the function pass the argument by keyword (and changing the public name would be a breaking change), keep the original name. In that case, either suppress the lint warning locally or use *_args / **_kwargs for generic stubs.
   • Framework/dispatcher-driven handlers (Starlette route handlers, protocol-dispatch callbacks in _run_server, etc.) receive their arguments positionally — the framework does not pass them by keyword name — so the previous exception does not apply and you should use the _ prefix on unused parameters there.
10. In tests, use AsyncMock for async functions/coroutines and MagicMock for synchronous ones. Using MagicMock where AsyncMock is needed is a common mistake — it causes tests to pass or fail misleadingly because the mock does not properly handle await.
11. Do not use assert for invariants or defensive checks in production code under src/. Python's -O flag strips assertions, and the project's lint config (ruff rule S101) flags them — adding # noqa: S101 to bypass that rule is not acceptable. For internal invariant violations, raise deephaven_mcp._exceptions.InternalError (or a more specific subclass) with a descriptive message. Every defensive raise must have a unit test that triggers it — if the path is "unreachable in normal use," construct the test fixture that proves the guard fires when bypassed. assert is fine in test files (tests/) where it is the standard expression of test expectations.
12. Docstrings describe what the function, method, class, value, module, or package is and does — never why it exists. Design rationale, API-symmetry justifications, cross-call-site narratives, and meta-commentary about how the code fits with other code belong in commit messages, PR descriptions, or — when truly load-bearing — a brief comment near the relevant line. Deletion test: if a docstring's body would not make sense to a caller trying to use the function (it explains the author's reasoning rather than the contract), delete that body.
    • Contract-level details are part of the what. Details that affect how a caller uses the function (e.g., "returns None when X so the caller can skip the retry") belong in the docstring.
    • Constants and getters stay minimal. A constant's docstring is one line stating what the value represents. A getter that reads a config key describes the key it reads and the fallback — it does not narrate the larger API's design.
    • Package __init__.py docstrings are the most-violated case. Apply the pydocs-improve "Module and package docstrings" section for the required shape.
    • MCP tool docstrings under rule 2 are a deliberate exception — their extra sections are part of the contract for AI-agent callers.
13. Configuration tunables live in the JSON configuration tree as Pydantic-validated fields, not as ad-hoc os.environ reads or as DEFAULT_FOO constants. The only configuration-location environment variable the server itself reads is DH_AI_DATA_DIR; the log level comes from PYTHONLOGLEVEL via setup_logging() (docs/ENV.md is the canonical inventory). Apply the ref-configuration-conventions skill before adding a new tunable, refactoring a config model, or wiring an environment variable into the code.
14. Every field on a field-bearing value class — a Pydantic StrictSchema / RedactableSchema subclass, a @dataclass, or a typing.NamedTuple — carries a PEP 257 trailing docstring, never a class-level Attributes: block. Trailing docstrings sit next to the field, survive refactors, and (for Pydantic) reach runtime consumers (model_fields[name].description, model_json_schema(), MCP tool schemas) that an Attributes: block does not; explicit Field(description="...") works but violates project style. The Pydantic subset is enforced by tests/test_field_docs_contract.py; @dataclass and NamedTuple are convention. Canonical implementations: cli/_runtime.py (Runtime, a dataclass), cli/_help.py (OutputField, OutputSpec); apply the ref-configuration-conventions skill for the Pydantic rule and examples.
    • Plain enums follow the same rule. A plain enum.StrEnum / enum.IntEnum / enum.Enum member carries a PEP 257 trailing docstring next to its value, never a class-level Members: / Values: block. Canonical implementations: SignalOutcome (_processes.py), DaemonState (cli/_commands/daemon.py), InitializationPhase (resource_manager/_registry.py), SystemType / SessionOrigin (_taxonomy.py), ResourceLivenessStatus (resource_manager/_manager.py).
    • Carve-out: metadata-bearing enums that bind per-member attributes via __new__. When an enum stores per-member metadata (help text, exit codes, structured payload) by overriding __new__ and constructing each member with positional metadata args, the metadata strings are the documentation and trailing docstrings would be redundant. Canonical implementations: ExitCode, ErrorCode (cli/_errors.py). This is the only exception to the per-member-docstring rule for enums.
15. dhcli CLI is click + @run_async + CliError. Click commands live under src/deephaven_mcp/cli/_commands/*.py; argparse is not used. Async callbacks must be wrapped with @run_async from cli/_async.py — never call asyncio.run inline. User-facing failures must raise CliError(message, code=ErrorCode.X) from cli/_errors.py — never print(..., file=sys.stderr); return 2. Apply the cli-command-add skill when adding or renaming a CLI command.
    • Surfaced help is plain text, governed by ref-cli-help-standards, not pydocs. Command HelpSpec strings (rendered by build_help), every click.option(help=...), group docstrings used as help, and ErrorCode.help_text are rendered verbatim by click and surfaced in the agents manifest, so they carry no reStructuredText markup (no ``, no :func:/:class:). This is the inverse of rule 12, which governs internal (non-surfaced) docstrings — those keep the RST convention. Apply cli-help-improve / cli-help-accuracy to the surfaced strings.
16. Python version floor. The supported Python floor is authoritative in pyproject.toml under requires-python. Run grep requires-python pyproject.toml before using version-gated syntax — PEP 695 generics (def f[T](...)), tomllib, typing.override, StrEnum, etc. — and re-check whenever you introduce a feature that landed in a recent Python release. Do not duplicate the version number elsewhere in code, docs, or skills; let pyproject.toml be the single source of truth.
17. Named domain exceptions live in src/deephaven_mcp/_exceptions.py. Inline definitions in the raising module are forbidden — the exceptions module is the single source of truth for the project exception hierarchy.
    • Inheritance: every named exception inherits — directly or transitively — from McpError, so unified handling (catch-all middleware, structured logging, the CLI's CliError mapping) sees it.
    • Organization: file is sectioned by domain (# Session Exceptions, # Configuration Exceptions, # Daemon Registry Exceptions, …). New domains add a base class plus specific subclasses, mirroring SessionError → SessionCreationError and DaemonRegistryError → RegistryCorruptError.
    • Exports: every new name appears in _exceptions.__all__ and in the equality check in tests/test__exceptions.py::test_all_exceptions_exported — that test is the enforcement mechanism. The raising module imports from _exceptions; the package __init__.py may re-export for ergonomic call sites.
    • Carve-out: plain RuntimeError / ValueError subclasses are reserved for genuinely-internal helpers that callers never name.
18. Closed-set dispatches must be statically exhaustive. When code dispatches on a value drawn from a closed set — a Literal, an Enum, or a tuple of accepted values — every member must have an explicit case and the fallthrough must call typing.assert_never(value). Adding a new member must surface as a mypy error in every consumer until each branch is written.
    • Canonical match form: