-
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.
-
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.
-
f-strings are preferred over % and .format() in format statements.
-
File moves and renames preserve history — use git mv and git rm, never delete-plus-create. Canonical source: AGENTS.md Version Control.
-
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), (every CLI wrapper matches its MCP tool), (mirrors , not a package at all), (the tool-module set matches its documented inventory).
-
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.
-
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.
-
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.
-
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.
-
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.
-
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.
-
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.
-
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.
-
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.
-
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.
-
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.
-
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.
-
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:
match value:
case "a": ...
case "b": ...
case _ as unexpected:
typing.assert_never(unexpected)
if/elif chains that fall through to a silent default are forbidden under this rule.
-
Pair Literal with a get_args-derived runtime collection. Derive the runtime collection from the Literal via typing.get_args so they cannot drift — the bare tuple when order matters, or wrapped in set / frozenset when the consumer is a membership check:
OutputMode = Literal["human", "json", "json-pretty", "yaml"]
OUTPUT_MODES: tuple[OutputMode, ...] = get_args(OutputMode)
Canonical implementations: cli/_format.py (OutputMode, format_output); VALID_FORMATS = set(get_args(TableFormat)) in formatters/__init__.py.
-
Friendly boundary guard + exhaustive helper. When invalid values must produce a domain error naming the valid options (not an AssertionError), keep the get_args-derived membership guard at the boundary and put the match + assert_never mapping in a separate helper. Fusing guard and match in one function makes the assert_never branch unreachable and untestable. Cover the helper's fallthrough by calling it with an out-of-vocabulary value under (rule 19 shows the justified form). Canonical implementations: / in ; / in .
-
Code-quality tool suppressions are last-resort. # pragma: no cover, # type: ignore[...], # noqa[...], # mypy: ignore-errors, and equivalent in-source escape hatches must not be used to silence a finding that a design fix would eliminate.
-
Try the design move first. Examples in increasing order of effort:
- Factor a helper so a previously-unreachable branch becomes reachable and unit-testable (
_liftable_options(params) in cli/_main.py is the canonical example — extracting the loop made the isinstance(param, click.Option) branch testable, eliminating a # pragma: no cover).
- Narrow with
typing.cast(T, value) instead of # type: ignore.
- Rewrite the API so the tool's complaint becomes correct (e.g., promote a stringly-typed parameter to a
Literal).
-
Coverage suppressions are forbidden. # pragma: no cover is never the fix for an uncovered line. The fix is either a test that exercises the branch or a refactor that deletes the branch when it truly cannot fire.
-
Bare # type: ignore is forbidden. Always include the bracketed error code: # type: ignore[arg-type], # type: ignore[no-untyped-call], etc. Bare ignores silence future errors the author never saw and are a perennial source of regressions.
-
Acceptable suppressions are limited to genuinely external constraints — third-party stub gaps, platform-conditional code on the inactive platform — and must be paired with an inline comment naming what was suppressed and why the design fix is unavailable:
with pytest.raises(AssertionError):
render_error(err, output="rainbow", ...)
-
Exception rendering and inspection live in src/deephaven_mcp/_exception_utils.py — the utility complement to rule 17's exception classes. Never hand-roll f"{type(e).__name__}: {e}" or an ad-hoc BaseExceptionGroup / __cause__ / __context__ walker in a consuming module.
- Render one exception into a user-facing string with
exception_summary(e) — the canonical TypeName: message form (repr fallback for empty messages). This format is a parsed contract (_short_reason in _tools/session_enterprise.py extracts the type name back out), not a style choice.
- Render a whole failure tree (exception groups and cause chains together) with
describe_exception(e); enrich specific leaf types via its render= hook rather than forking the traversal (canonical implementation: describe_exception_chain in client/_base.py, which renders grpc.Call leaves with status details).
- Detect a condition anywhere in a failure tree with
walk_exceptions(e, ...) — e.g. any(isinstance(x, T) for x in walk_exceptions(e, follow_context=True)) (canonical implementation: _is_client_disconnect_error in _monkeypatch.py).
- Boundary with logging: log lines keep the
{e!r} + exc_info=True form per ref-logging-standards; _exception_utils renderers are for user-facing strings (payload error fields, CLI errors, recorded init errors).
- Boundary with translation: the renderers are for embedding an exception whose message is not already a complete user-facing string. A boundary that adopts a message this codebase authored for users — e.g. a CLI wrapper re-raising
McpRequestTimeoutError / McpClientError text as CliError, whose producers already rendered the root cause canonically — embeds it verbatim; adding another TypeName: prefix stacks wrapper noise onto an already-rendered message. Parse feedback embedded into a prompt-shaped error (e.g. a json.JSONDecodeError position in an argument error) is likewise adopted, not re-rendered.
-
A test asserts a contract the project states — never a limit the author invented. A numeric threshold in an assertion must come from the code under test (a constant, an enum, a configured value) or from a documented requirement. A budget the author picked — a byte cap on a string, a maximum number of entries, a line-length ceiling — is a voodoo constant: it fails other people's legitimate work, and its number carries no authority to appeal to when it does. The same rule governs skills (ref-skill-authoring-standards Common failure modes) and user-facing docs (ref-documentation-roles Editing rules).
- Test the property, not a proxy for it. Reach for the threshold only after asking what defect it is standing in for, then assert that defect directly. A size cap on agent-facing guidance text was standing in for "the guidance is true and current" — which is checkable outright: resolve every command path the text names against the live tree, and re-derive every closed set it enumerates from the enum (canonical implementation:
test_agent_conventions_name_only_commands_that_exist and test_agent_conventions_state_the_real_failure_contract in tests/cli/test__manifest.py). The cap could not detect a false claim; the direct assertions cannot miss one.
- If the property genuinely resists a test, do not write one. State the rule where authors read it and let review enforce it. A test that encodes an arbitrary number is worse than no test: it converts a judgment into a false mechanical authority.
- Factual counts are not thresholds.
assert len(written_files) == 2 pins observed behavior and is fine; assert len(text) <= 1200 invents a rule.
