From 158f54e3eeb57fe2383afb75930a524f09e4f71c Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Sun, 16 Aug 2026 18:45:35 -0400 Subject: [PATCH] =?UTF-8?q?docs(contributing):=20state=20the=20bar=20?= =?UTF-8?q?=E2=80=94=20documentation=20standard,=20gates,=20tests-first?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The repo's signature is documentation that argues, and nobody will guess that standard (#282). CONTRIBUTING.md now teaches it from a worked example quoted out of guards.py — pinned by tests/test_contributing.py so the quote cannot drift from the source — beside the dev setup and gates, the tests-first expectation, Conventional Commits, and three-tier scope guidance. Governance and licence sections from Phase 6 kept intact. --- CONTRIBUTING.md | 102 ++++++++++++++++++++++++++ tests/test_contributing.py | 142 +++++++++++++++++++++++++++++++++++++ 2 files changed, 244 insertions(+) create mode 100644 tests/test_contributing.py diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 7d031f51..483260fc 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -1,5 +1,13 @@ # Contributing to keel +Thank you for considering it. One thing before anything else, because it is the single +biggest surprise contributors hit here: **the documentation bar is unusually high, on +purpose.** Decisions carry their reasoning, rejected alternatives stay recorded, and comments +name what was *measured* rather than what was assumed. A stated bar can be met; an unstated +one can only be resented. The standard is spelled out with a worked example under +[The documentation standard](#the-documentation-standard) — read that section before your +first PR and reviews will feel fair instead of heavy. + ## Governance: rulings vs. machinery **keel is not a fatwa engine. It is an enforcement engine for a ruling you supply.** @@ -29,6 +37,100 @@ it — and run the enforcement engine under it. The disagreement then costs nobo upstream stays neutral, your deployment follows your ruling, and the audit trail records exactly who said what. +## Development setup and the gates a PR must pass + +Python **3.14+** is the floor (see `.python-version`; `uv python install 3.14` gets you one). +Then: + +```bash +uv sync --all-extras --dev # everything: the workspace, dev deps, the conformance extra +uv run ruff check # lint — must pass clean +uv run mypy # types — must pass clean +uv run pytest -q # the full suite — must pass (CI runs exactly this) +``` + +All four must be green before you ask for review. CI runs the same commands, so anything red +locally is red everywhere. The suite is fast (tens of seconds) — run it freely. + +## The documentation standard + +A comment or docstring here is acceptable when it does three things: **it says why** (the +constraint the code cannot show), **it names what was measured** (numbers, incidents, the +specific input that broke — not "this is faster"), and **it says what it would take to change +the decision** (which assumption, overturned, reverses it). Code reviews enforce this; the +point of writing it down is that you can enforce it on yourself first. + +A worked example, from `_open_exposure_by_asset` in `keel/execution/guards.py` — the net +at-risk figure that the exposure and concentration caps read: + +> ⚠️ **An unparseable `product_id` is handled by SIDE, and always logged at WARNING.** A +> malformed **BUY** is COUNTED, under whatever key `_asset` gives it; a malformed **SELL** is +> SKIPPED. Both choices are the same choice — never let a row nobody can read make this +> figure SMALLER — and it is the sign of the row, not the fact of the row, that decides which +> action achieves that. + +What makes this acceptable: + +- **It says why, at the level the code cannot.** The code shows a branch on side; the comment + shows the *invariant* the branch serves — the figure feeds caps, and a smaller figure is a + looser cap, so unreadable rows may only ever err towards refusing an order. +- **It names what was measured.** Not "SELLs are risky" but the arithmetic: a counted + `$800` unreadable SELL against a `$900` BUY measures `$100`, and past the BUY total the + `if amt > 0` filter *deletes the bucket entirely*. Someone checked; the comment shows the + check. +- **It records the decision's history and its reversal conditions.** It supersedes an earlier + unconditional rule and its unconditional opposite, names which evidence would flip it again + ("neither survives a SELL"), and stays honest about the impossible case it still guards. + +You do not need to write a essay per function. You need those three properties wherever a +choice was made that a reader could plausibly make differently. + +## Tests come first + +Changes land test-first: write the failing test, run it, watch it fail **for the right +reason** — the assertion you meant to assert, not an import error or a typo — then make it +pass. Your PR should carry that evidence: a test whose failure message is the bug being +described, in the PR description or the diff's story. A test that has never been seen red is +a test that may be green for no reason. + +The suite's own docstrings follow the documentation standard too; `tests/test_packaging.py` +is a good read for how a test argues its own existence. + +## Commit convention + +[Conventional Commits](https://www.conventionalcommits.org/), matching the existing history: +`fix(strategy): fill entries at the next bar's open`, `docs(experiments): ...`, +`chore(release): ...`. The prefix is load-bearing — releases and changelogs are cut from it +(see `docs/RELEASING.md`) — so an untyped commit is not a style nit, it is a missing record. + +## Scope: what is welcome, what needs discussion first, what is out + +**Welcome, PRs directly:** + +- Bug fixes with a failing test that reproduces them. +- Documentation for something that deserves it and lacks it (a rail, a command, a failure + mode) — written to the standard above. +- Tooling, packaging, and test-quality improvements. +- New broker adapters behind the port (`packages/keel-broker-*`), discussed in an issue first + only if they need port changes. + +**Needs an issue and agreement BEFORE the PR:** + +- Anything touching a **rail** (`keel/execution/guards.py`): a rail's semantics are the + product, and a subtle change distributes itself to every operator who upgrades. +- Anything changing a **default classification** — see the governance section above for the + source-and-discussion bar those carry. +- New dependencies, and anything that widens the public surface of `keel-broker-api` (every + adapter codes against it). + +**Out of scope:** + +- Shariah rulings as code defaults — attested locally, never merged (see governance). +- "Make the bot profitable" — the honest measured state of the rules is recorded in + `docs/experiments/`, and that record is the project's posture, not a to-do list. +- Anything that weakens a fails-closed path to make an operational annoyance go away; the + annoyance is the smaller problem. + ## Licence: why Apache-2.0 keel is licensed under [Apache-2.0](LICENSE). That was a decision, not a default, and the diff --git a/tests/test_contributing.py b/tests/test_contributing.py new file mode 100644 index 00000000..4df57df1 --- /dev/null +++ b/tests/test_contributing.py @@ -0,0 +1,142 @@ +"""CONTRIBUTING.md: the bar, stated, so nobody has to guess it. + +This repository's signature is documentation that argues: decisions carry their reasoning, +comments name what was MEASURED rather than what was assumed, and rejected alternatives stay +recorded. It is also the single biggest barrier to contributing, because nobody will guess a +standard they have never seen written down (#282). An unwritten bar does not filter for +quality -- it filters for clairvoyance: PRs arrive at ordinary quality, get heavy review, and +the contributor quietly leaves. That is the most common way a promising project loses its +first ten contributors. + +These tests pin CONTRIBUTING.md to the specific things #282 asks it to state: the gates a PR +must pass, the documentation standard WITH a worked example lifted from the code, the +tests-first expectation, the commit convention, and the scope rules. The governance boundary +and licence rationale already have their own guards in `test_governance.py` and +`test_licensing.py`; this file covers the parts a first-time contributor reads before opening +a PR. +""" + +from __future__ import annotations + +import re +from pathlib import Path + +_ROOT = Path(__file__).resolve().parents[1] + +#: The sentence from `_open_exposure_by_asset`'s docstring (`keel/execution/guards.py`) used as +#: the worked example. Quoted in CONTRIBUTING.md, asserted against the source below, so the +#: example cannot drift from the code it explains. +_WORKED_EXAMPLE_SNIPPET = "never let a row nobody can read make this figure SMALLER" + + +def _contributing() -> str: + return (_ROOT / "CONTRIBUTING.md").read_text() + + +def _unwrapped(text: str) -> str: + """Join markdown wrapping: drop blockquote markers, then collapse all whitespace. + + The worked example is quoted inside a `>` block, where a sentence can break as + `...make this\\n> figure SMALLER...`; a plain whitespace-collapse leaves the `>` in the + way of the match. + """ + return " ".join(re.sub(r"(?m)^\s*>\s?", "", text).split()) + + +def test_the_gates_a_pr_must_pass_are_stated(): + """Dev setup plus the three gates, as runnable commands. + + 'Run the tests' is not an instruction; `uv run pytest -q` is. The commands are pinned + verbatim because a gate nobody can paste is a gate nobody runs. + """ + text = _contributing() + for command in ( + "uv sync --all-extras --dev", + "uv run ruff check", + "uv run mypy", + "uv run pytest -q", + ): + assert command in text, f"CONTRIBUTING.md must state the gate command {command!r}" + + +def test_the_documentation_standard_has_a_worked_example_from_the_code(): + """The standard is taught from a real comment, not described in the abstract. + + The example must be QUOTED from the codebase and must remain there: CONTRIBUTING.md + reproduces a distinctive line from `guards.py`'s `_open_exposure_by_asset` docstring, and + this test asserts both halves -- the quote in the doc, and the quote's continued existence + in the source. If the source comment is ever rewritten, this fails rather than letting the + doc teach from an example that no longer exists. + """ + text = _contributing() + # Wrap-normalised: the quote is markdown, and markdown wraps wherever the line ends. + assert _WORKED_EXAMPLE_SNIPPET in _unwrapped(text), ( + "CONTRIBUTING.md's documentation-standard section must quote the worked example " + f"(from keel/execution/guards.py): expected {_WORKED_EXAMPLE_SNIPPET!r}" + ) + guards = (_ROOT / "keel" / "execution" / "guards.py").read_text() + assert _WORKED_EXAMPLE_SNIPPET in _unwrapped(guards), ( + "the comment CONTRIBUTING.md quotes as its worked example no longer exists in " + "keel/execution/guards.py -- update the doc to quote a comment that does" + ) + + +def test_the_standard_names_what_makes_a_comment_acceptable(): + """The reader must be told the RULE, not only shown the example. + + Three properties, so a contributor can check their own writing before anyone else has to: + it says WHY, it names what was MEASURED, and it says what it would take to change the + decision. + """ + text = _contributing().lower() + assert "why" in text + assert "measured" in text + assert "decision" in text + + +def test_tests_first_is_expected_with_evidence(): + """The TDD expectation, including the evidence that makes it checkable. + + Tests written before the fix, and shown in the PR to have failed FOR THE RIGHT REASON -- + an assertion message, not an import error -- is the house style. Stated, it is a bar + contributors can meet; unstated, it is a review surprise. + """ + text = _unwrapped(_contributing()).lower() + phrases = ("tests first", "test-first", "tests before", "tests come first") + assert any(phrase in text for phrase in phrases), ( + "CONTRIBUTING.md must state that tests are written first" + ) + assert "fail" in text and "right reason" in text, ( + "CONTRIBUTING.md must ask for evidence the failing tests failed for the right reason" + ) + + +def test_the_commit_convention_is_stated(): + """Conventional Commits, because that is what the history already does. + + The point is not aesthetics: `fix(strategy):` vs `feat(engine):` is the changelog, and a + release process that reads it (see docs/RELEASING.md) silently degrades when a commit + arrives untyped. + """ + assert re.search(r"conventional commits", _contributing(), re.IGNORECASE), ( + "CONTRIBUTING.md must name Conventional Commits as the commit convention" + ) + + +def test_scope_guidance_separates_welcome_from_needs_discussion_from_out(): + """Three tiers of scope, with the guarded kinds named. + + Rails and default classifications are the two surfaces where a casual PR does quiet, + distributed harm, so they are the two that must be called out as discuss-first. The + governance section above already carries the fiqh reasoning; this pins that the SCOPE + list exists and points at them. + """ + text = _contributing().lower() + assert "rail" in text, "scope guidance must name rails as discuss-first territory" + assert "classification" in text, ( + "scope guidance must name default classifications as discuss-first territory" + ) + assert "discussion" in text or "discuss" in text, ( + "scope guidance must say these need discussion BEFORE a PR, not review during one" + ) + assert "out of scope" in text, "scope guidance must state what is out of scope entirely"