Skip to content

Coding standards

The canonical coding-standards doc — assembled from the rules already in force across .github/copilot-instructions.md, CLAUDE.md, CONTEXT.md, the ADRs, and the actual code; previously-open points (docstrings, formatting/linting, the commands-layer type-checking split) have their decision record in Decide: coding standards for the repo.

This doc is the single source reviewers (the code-review skill, CodeRabbit's path instructions, .github/copilot-instructions.md) derive from — they should shrink to a pointer here rather than repeat these rules.

Architecture & layering

  • crm/core/* — Web API logic, one module per domain (entity, connection, metadata, async_ops, batch, charts, dashboard, dup, …).
  • crm/commands/* — thin Click wrappers, one file per crm <group>, one command group per domain, mirroring the core module it wraps.
  • Credentials come only from a saved profile (crm profile add) or a per-run --password; never environment variables.

Type checking

  • pyrightconfig.json sets strict globally; crm/commands/* and most test files opt back out to basic via a # pyright: basic header comment. This split is permanent by design, not debt: the commands layer is thin, decorator-heavy Click glue where strict mode fights the framework for near-zero bug yield. Don't flag it and don't tighten files opportunistically — when a command file accumulates real logic, the fix is moving that logic into crm/core/* (the strict surface, together with crm/utils/d365_backend.py), not tightening the wrapper.
  • Boundary rule for D365 payloads: dict[str, Any] at the raw OData seam (a TypedDict cast over external JSON is a false contract — the server can add/omit keys the type doesn't know about). Reserve TypedDict for structures the CLI itself constructs and owns (e.g. Reference in crm/core/references.py).
  • Run pyright locally with --pythonpath .venv/bin/python --pythonversion 3.13 — omitting either flag masks import errors or lets 3.10+-only symbols pass that fail at the pinned runtime.

Docstrings

  • Intent rule, not a coverage quota: a public function in crm/core/* gets a docstring when its name and signature don't already say what it does and why. Self-explanatory helpers stay bare — forced docstrings are narrative noise.
  • Sectioned docstrings use Google style (Args: / Returns: / Raises:) — the only style in use today; don't introduce Sphinx/NumPy variants.
  • Lint enforces format only (ruff D2xx/D4xx: quoting, blank lines, imperative mood) on docstrings that exist; there is deliberately no missing-docstring (D1xx) rule.

Formatting & linting

Decision record: Decide: coding standards for the repo; gates landed via Task: adopt ruff — CI enforces ruff check + ruff format --check, and pre-commit install (once per clone) runs both on staged files so the format round-trip never reaches CI.

  • ruff is the single formatter and linter. pyproject.toml is the source of truth for the exact rule codes; the agreed shape:
    • line length 100 (matches the codebase's natural style; not the ruff default 88),
    • rule families E, W, F, I (import order), B (bugbear), UP (pyupgrade, target-version = py313), plus the format-only D subset above,
    • deliberately excluded: S (bandit — rejected in the tooling sweep), C90 complexity, ANN (pyright owns typing), D1xx docstring coverage, and D205 (blank line between docstring summary and description) — D205 treats only physical line 1 as the summary, so it forces multi-line summaries onto one line, which then trips E501 as breakable prose; 482 of 569 sites can't satisfy both without rewording, and the descriptive-first-line convention here is deliberate (#843).
  • ruff format covers the whole tree — crm/ including tests, plus scripts/ — no carve-outs.
  • Noisy rules in tests get a scoped per-file-ignores entry, never a global rule removal.
  • Semgrep CE enforces house-convention rules ruff structurally can't express (see Dry-run contract below). It is not in the [dev] extras — its engine needs Python ≥3.10, so it runs in its own venv in CI and is installed separately locally: pipx install semgrep (or uvx semgrep scan --config ci/semgrep-rules.yml --error --metrics off). Custom rules only; no registry rulesets (they re-tread ruff/CodeQL). Decision: tooling sweep on #831 / adopted in #849.

More tools landed in the tooling sweep (#848):

  • actionlint lints workflow YAML and the inline bash in run: blocks (shellcheck-backed). It runs both as a pre-commit hook and as a step in CI's lint job. The pre-commit hook fires only on .github/workflows/** changes, so a Go toolchain is needed locally only when editing a workflow; CI runs the pinned release binary directly. The rev is pinned in both .pre-commit-config.yaml and ci.yml — bump together.
  • zizmor audits the workflows for security issues (credential persistence, cache poisoning, excessive permissions, unpinned actions). It runs both as a pre-commit hook and as a step in CI's lint job (its own pinned venv, like semgrep), version in lockstep across both. Config is .github/zizmor.yml: a scoped unpinned-uses policy allows ref/tag pins only for the first-party actions/* org (the repo's deliberate tag-pin convention, no dependabot per #819) while third-party actions still require a full SHA pin; every other audit stays armed. Adopted in #858.
  • codespell is a typo checker wired as a pre-commit hook only — deliberately not a CI gate, because a jargon false positive must never block a merge. Its D365 ignore-words list and skip paths live in [tool.codespell] (pyproject.toml); the hook's exclude mirrors that skip (pre-commit passes explicit paths, which bypass codespell's own traversal-level skip).
  • Coverage is report-only (map #819): CI runs pytest --cov and uploads an XML + HTML artifact with no threshold gate and no Codecov. --cov stays out of pytest addopts so local pytest runs stay fast; pytest-cov is in the [dev] extra.

Click command pattern

Every command follows the same shape: @pass_ctx decorator, ctx: CLIContext as the first parameter, ctx.backend() to reach the backend, ctx.json_mode / ctx.dry_run to branch behavior, ctx.emit(...) as the single result seam.

@org_group.command("brief")
@pass_ctx
def org_brief(ctx: CLIContext):
    with d365_errors(ctx):
        brief = org_mod.org_brief(ctx.backend())
    ctx.emit(True, data=brief)

House rules on top of the shape:

  • Wrap file read/write in try/except OSError and return a clean error envelope — never let a raw OSError traceback reach the user.
  • Mutually exclusive flags raise click.UsageError (exit 2); never fake it with ctx.emit(False) (exit 1) — those are different failure classes (see Exit codes below). Validate untrusted input before calling ctx.backend(), not after.
  • CLIContext.emit(meta=...) renders in human mode too, not just --json — gate any JSON-only field on ctx.json_mode before adding it to meta.
  • When serializing a Click option for introspection (crm describe, docs generation), emit opt.secondary_opts (the --no-* forms) alongside opt.opts, not opts alone.
  • crm.cli.cli is a lazy group (_LazyJsonAwareGroup): subcommands are not loaded until requested, so a naive .commands tree walk sees 0 leaves. Any tooling/gate that enumerates the command tree must walk via list_commands(ctx) + get_command(ctx, name) and assert a sanity floor (e.g. len(leaves) > 100) so a lazy-loading regression can't silently empty the set — the e2e coverage gate would otherwise pass while testing nothing.

Error handling & exit codes

  • All D365/backend failures are a D365Error (crm/utils/d365_backend.py) carrying status, code, response_body, and (for multi-stage writes) completed_steps / stage. Commands funnel it through the d365_errors(ctx) context manager (crm/commands/_helpers/errors.py), never a bare try/except D365Error that hand-builds the envelope.
  • Exit-code contract (ADR 0001): 0 success, 1 operational failure (server error, in-command validation, declined confirmation), 2 Click usage error. Granular per-class codes are rejected — failure-class detail lives in the envelope (error, meta.status/code/category/retryable), not the exit code.
  • The envelope's {status, code, category, retryable} is reserved and always derived from the caught D365Error itself; an enrich(exc) callback's extra_meta is strictly additive and raises if it names a reserved key.
  • Never assert for a runtime invariant in shipped code — assert is stripped under python -O, and the frozen PyInstaller build can run optimized. Raise D365Error (or the appropriate domain error) even for a logically-unreachable check. Enforced by ruff S101 (the one bandit rule selected); crm/tests/**, evals/** and scripts/** are exempt — they never ship, and pytest's assertion model is built on assert.

Output contract

  • The data payload is curated and CLI-owned, never a passthrough of the raw D365 Web API response (ADR 0008). List verbs put a bare array in data; OData paging (@odata.nextLink, @odata.count) relocates to meta.next_link / meta.count; per-row @odata.* protocol keys are stripped.
  • _entity_id (+ _entity_id_url) is the one normalized key for an affected record's GUID across create/update/delete/entity get — the leading underscore marks it as CLI-synthesized, distinct from the real PK attribute.
  • Client-side output shaping (--fields, --jq, ADR 0023) is a post-curation, envelope-preserving transform applied once at the CLIContext.emit seam — never re-implemented per command. It only touches data; error envelopes bypass shaping entirely.

Dry-run contract

  • Reads-execute rule: under --dry-run, only mutations are previewed — every GET still runs for real, which is what lets a preview report live facts (_exists, would_skip) instead of guesses.
  • A dry-run mutator returns {_dry_run: true, would_*} (plus meta.dry_run: true), never the bare success key a real write would return (deleted: true, etc.).
  • Machine-checked by a Semgrep CE rule (ci/semgrep-rules.yml, CI lint job): a backend.dry_run short-circuit that directly returns a dict literal missing _dry_run fails the build. The rule catches the direct-return shape only — a preview built into a local variable then returned stays with LLM/human review (documented in the rule header). Iterate new house-convention rules the same way: zero false positives on conforming crm/core/* is the bar before a rule ships.

Encoding

Full policy in docs/contributing/encoding.md; the load-bearing rules:

  • Every file read uses an explicit encoding — never the OS-locale default. utf-8-sig for anything a user or external tool may author (CSV, JSONL, YAML/JSON specs, FetchXML); plain utf-8 (no BOM) for writes.
  • Subprocess captures pin encoding="utf-8", errors="replace" — text=True alone decodes with the locale default and can crash on a stray non-UTF-8 byte.
  • Unix-only imports (fcntl, pwd, termios, …) need try/except ImportError at the import line so the module still loads on Windows.
  • Consult metadata to type a value crossing the file boundary; don't infer type from the value's shape (a string-typed alternate-key column keeps its verbatim text, leading zeros and all).

D365 API conventions

  • Logic enforcing a header invariant ("never emit both X and Y") uses requests' CaseInsensitiveDict, never a plain dict — headers are case-insensitive on the wire.
  • @odata.bind navigation-property names must match $metadata casing exactly; system entities (sdkmessage*, solution, …) use lowercase logical names — flag a guessed PascalCase name.

Docs-in-sync obligation

Every new/changed command, flag, or behavior ships its docs in the same change: docs/how-to/<group>.md, docs/reference/cli.md, README.md if user-facing, and crm/skills/ if the change is workflow-visible. Never hand-edit CHANGELOG.md — python-semantic-release generates it from commit subjects. A new lazily-loaded command group is added to crm.spec hiddenimports (PyInstaller) in the same change.

Public-repo hygiene

The repo is public and must stay generic: flag real org names, internal hostnames, or real-looking GUIDs in tests/docs. Placeholders are Contoso / internalcrm.contoso.local.

Known non-issues — do not flag

  • D365Backend retries only 502/503/504 (+429) for idempotent methods, not all 5xx; a test using a bare 500 to exercise the no-retry path is intentional.
  • Click is pinned >=8.2: CliRunner has no mix_stderr parameter — stdout/stderr are always separate streams.
  • # pyright: basic headers in test files are intentional (see Type checking above).
  • OAuth token acquisition raises D365Error during session.get(), not a requests exception — a caller catching D365Error around a raw session call is correct, not dead code.