Skip to content

Coding standards

The canonical coding-standards doc — assembled from the rules already in force across .github/copilot-instructions.md, CLAUDE.md, GLOSSARY.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 (Microsoft's npm package, npx --yes --package=pyright@1.1.414 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 on .github/workflows/** changes and on every scripts/check.sh full, so on every local-gate run; with no Go on PATH, pre-commit downloads its own toolchain (go.dev, dl.google.com) and the module dependencies (proxy.golang.org). 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 (category and retryable by classify_d365_error in crm/utils/d365_backend.py); 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 the shipped skills (crm/skills/, plus crm/migration-skill/ when the change touches the migration workflow) 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.

Conventions a reviewer should know

Ship's review conventions, true in every repo that installs it. The repo's own conventions live in the sections above.

  • PR body: seven sections, in order. ## Why the change, ## Change outline, ## Special things to note, ## Needs attention, ## Verification, ## Review, ## Attribution. Every body carries all seven, template or not, because ship writes the headings it does not find; a missing one is a finding, and ## Attribution last is what keeps a section rewrite from swallowing the footer.
  • PR body: ## Change outline carries a Shape. A diff fence over a call tree, control flow, pseudocode or component tree, under ## Change outline, which every body carries because ship writes the heading where no template gives it. Text forms only; mermaid and HTML are out. One behavioural fence per PR, about 15 lines or fewer, with a carrier file tree after it only where the same edit lands in more than two files. Every node is a real symbol, each tree's root node carries its file path, and no line carries a line number. Shape: none, mechanical (<kind>). replaces the fence only where the reviewer's question is "did the text change correctly", never where it is "what does X now do"; silent absence is a finding either way.

Depth checks

  • A vocabulary the change extends is swept across the whole tree, sibling spellings included. Grep the new term and the ones it sits beside, across every file rather than the ones the diff already opened; a stale spelling left in the copy nobody grepped reads as the current rule to the agent that finds it first.
  • History keeps the old word through a rename. The sweep above stops at history: a changelog entry and an ADR keep the word as it was, since each records what was true when it was written.
  • A rule-shaped prose change reaches every item it governs, one outcome each. Enumerate the items the rule names and check the change lands on each exactly once; an item the rewrite skipped, or one left carrying two answers, is where a reviewer finds five rounds of work.
  • Self-review edits reach every item. The one-outcome-per-item rule above binds a fix a finding proposes as it binds any rule-shaped change: the finding names one item, and the next round finds its siblings still carrying the old answer.
  • A new test is run once with the fix reverted, and confirmed red. A test written to prove a fix proves nothing until it has failed for the reason it exists: revert the hunk, watch the case fail, restore it, watch it pass. A vacuous assertion reads from a diff exactly like a sound one, so no reviewer catches this and the proof is the author's, before the push.
  • A negated fixture accompanies a test that asserts on prose. Assert on a word the change introduced, and run the assertion once against the sentence with its meaning negated: it must fail. An assertion on a word the old text already held stays green whatever the change did.
  • New pattern-matching code is tested against adversarial inputs. Delimiters inside the field, option groups, field-versus-line anchoring, and the path where the tooling itself fails; ten lines of regex read as correct and answer wrong on the input nobody wrote a case for.
  • A fix landed after review has its hunk re-read before the push. Read the changed lines back out of the file, not out of the reply you are about to post; a fix applied to the wrong copy or applied by half costs a whole round to discover.

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.