From eeb1888aa38d0b37f871f315bb7f504d47dc067d Mon Sep 17 00:00:00 2001 From: Jim Lancaster <40281233+zoltan57@users.noreply.github.com> Date: Wed, 2 Sep 2026 16:48:00 -0500 Subject: [PATCH] Update instructions - epilog (Code review, remediation now complete) --- .github/instructions/tests.instructions.md | 119 +++++++++++++++++++++ .github/workflows/quality-gate.yml | 5 + AGENTS.md | 6 +- tests/test_meta_contract_guards.py | 10 ++ 4 files changed, 139 insertions(+), 1 deletion(-) create mode 100644 .github/instructions/tests.instructions.md diff --git a/.github/instructions/tests.instructions.md b/.github/instructions/tests.instructions.md new file mode 100644 index 0000000..72113d9 --- /dev/null +++ b/.github/instructions/tests.instructions.md @@ -0,0 +1,119 @@ +--- +description: Authoring rules for the test suite, including markers, async discipline, and guard-test design. +applyTo: 'tests/**/*.py' +--- + +# Tests + +Primary references: + +- `AGENTS.md` (Change Protocol — failing test first) +- `docs/index.md` and `docs/invariant/*` +- `.github/skills/test-effectiveness-auditor/skill.md` (periodic audit of this suite) + +The suite is not only regression protection here — it is where several architectural rules are +*defined*. `tests/test_service_boundaries.py`, `tests/test_ui_boundaries.py`, +`tests/test_provider_boundaries.py`, `tests/test_model_contract_guards.py`, and +`tests/test_meta_contract_guards.py` are the enforcement layer named in the `AGENTS.md` authority +order. A weak test in this repository does not merely fail to catch a bug; it can silently repeal a +documented invariant. + +The baseline is green. `uv run pytest -q -m "not external"` must report zero failures and zero +errors, and there is no tolerated set of known-failing tests. + +## Write the Failing Test First + +For any behavioral fix, write the test before the fix and confirm it fails *for the reason you +expect*. A test that passes against the broken code proves nothing, and several defects in this +repository were subtle enough that a test written afterward would have done exactly that. If the +new test passes immediately, you have not reproduced the defect yet. + +## Runner Configuration + +Configured in `pyproject.toml`; do not work around these: + +- `--strict-markers` — an unregistered marker is an error. Register new markers in + `[tool.pytest.ini_options] markers` with a description rather than inventing one at the call site. +- `asyncio_mode = "strict"` — every async test needs an explicit `@pytest.mark.asyncio`, and async + fixtures use `@pytest_asyncio.fixture`. There is no implicit promotion. +- `filterwarnings = ["error:coroutine .* was never awaited:RuntimeWarning"]` — an un-awaited + coroutine is an error, not a warning. This usually means a mock replaced an async callable with a + sync one, or an `await` was dropped. Fix the call; never silence the warning. + +## Markers and Layout + +- `unit` — pure logic, no framework or database. +- `integration` — touches framework, database, or multi-component contracts. +- `external` — calls live services; slow and credential-dependent. + +`external` tests must also carry their own `skipif` so the suite stays green without credentials +(see `tests/services/test_transcription_external.py`). Local and documented runs use +`-m "not external"`; CI intentionally runs unfiltered, which is equivalent because those tests skip +themselves. Never let an unmarked test reach the network. + +Place tests by the layer under test: `tests/services/`, `tests/ui/`, `tests/api/`, +`tests/providers/`, `tests/integration/`, with cross-cutting guards at the top level. + +## Fixtures and Isolation + +- Prefer the shared fixtures in `tests/conftest.py` (`default_settings`, `async_session`, + `default_session_factory`, and the per-aggregate service fixtures) over building settings or + engines by hand. +- `Settings` is isolated suite-wide by the session-scoped autouse fixture in `conftest.py`, because + `env_file` resolves against the working directory. Tests that need env-file loading pass + `_env_file=` explicitly; tests asserting declared defaults need nothing. Do not reintroduce + reliance on a developer's local env file. Guarded by `tests/test_config_isolation.py`. +- Database fixtures refuse to run against anything but the per-test path, and that refusal is + deliberate. Never relax it to point a destructive fixture at a real database. +- Tests must not leave artifacts outside `tmp_path`. + +## Assertion Strength + +Assert on the domain effect, not on the fact that code ran. + +- Prefer persisted state, status transitions, error categories, and evidence records over + "no exception raised", "not None", or a bare status code. +- **Read committed state through a separate session.** Asserting against the same session that + performed the write can pass on unflushed in-memory state and prove nothing about durability. + This is how the atomicity guarantees in `tests/services/test_workflows_reliability.py` and + `tests/integration/test_pipeline_atomicity.py` are made real. +- Critical paths need negative-path coverage — timeouts, provider failures, validation errors, + cancellation. Happy-path-only coverage of a critical module is a gap, not a suite. +- Avoid count-threshold assertions as a proxy for correctness. A test asserting "at least N items + were discovered" passes indefinitely while the thing it was meant to protect rots; assert on a + specific known member instead. + +## Guard Tests + +Structural guards carry extra obligations, because they are cited as proof that a rule holds. + +- **Guard the guard.** Every scanning guard needs a companion assertion that the scan actually found + something, following the existing `test_*_are_discovered` pattern. A guard that silently scans an + empty set passes forever. +- **Scope must match the claim.** A guard's name and docstring must describe only what it actually + verifies. A test covering one function while appearing to enforce a repo-wide rule is worse than + no test, because it stops anyone from writing the real one. +- **Prove non-vacuity by injected fault.** Temporarily introduce the violation, confirm the guard + fails with a comprehensible message, then revert. Do this whenever you add or materially change a + guard. Revert with an explicit edit if the file has uncommitted changes — `git checkout --` will + discard them. +- **Prefer structural analysis to substring matching.** AST inspection of imports and definitions is + resistant to false negatives; a bare-name search across the repository is not, since an unrelated + mention anywhere makes dead code look reachable. +- Failure messages should name the offending file, symbol, and the remedy. These fire for people who + did not write the guard. +- Any new file under `.github/**` must be added to `ACTIVE_CONTRACT_FILES` in + `tests/test_meta_contract_guards.py`, or the completeness guard fails by design. + +## Redundancy + +Duplicate coverage across layers costs runtime and dilutes signal. Pick the canonical layer for a +behavior — unit for logic, integration for wiring — and let the other layer assert only what is +unique to it. Retire tests superseded by a stronger guard instead of accumulating both, and record +deliberate retentions with a rationale rather than leaving them unexplained. + +## Contract Sync Rule + +When a test encodes or relaxes a documented rule, update the corresponding instruction file or +`docs/*` page in the same change. When a guard test is the enforcement for a rule stated in +`AGENTS.md` or an instruction file, cite the test by name there so the link survives refactoring. diff --git a/.github/workflows/quality-gate.yml b/.github/workflows/quality-gate.yml index c2f494d..639112e 100644 --- a/.github/workflows/quality-gate.yml +++ b/.github/workflows/quality-gate.yml @@ -42,4 +42,9 @@ jobs: run: uv run pre-commit run --all-files --show-diff-on-failure - name: Tests + # Deliberately unfiltered, unlike the "-m 'not external'" form the guidance files + # use for local runs. Tests marked "external" skip themselves when live-service + # credentials are absent, so CI gets the same effective set plus a real run of any + # external test whose credentials are configured. Not drift -- do not "fix" this to + # match the local command without also giving those tests a way to run. run: uv run pytest diff --git a/AGENTS.md b/AGENTS.md index ff228c8..c3d08a4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -39,12 +39,16 @@ Resolve every question in this order, and stop at the first that answers it: 1. **`docs/*`** — canonical. Start at [`docs/index.md`](docs/index.md), which defines the reading order. `docs/invariant/*` holds cross-version rules that outlive any release. 2. **`.github/instructions/*.md`** — active steering, auto-attached when you edit matching - paths. Covers services, UI, error handling, and documentation sync. + paths. Covers services, UI, providers, tests, error handling, and documentation sync. 3. **`.github/skills/*`** — periodic audit procedures (code review, provenance, test effectiveness). 4. **`tests/`** — deterministic enforcement. A guard test is the ground truth for whatever rule it encodes. +`.github/agents/` and `.github/prompts/` hold named workflows that are loaded only when +invoked explicitly, so they never override the order above. They are how a review or audit +is *started*, not a source of rules. + `docs/reviews/**` is **not** canonical. Those are dated, opinionated snapshots that were accurate when written and may since have been fixed, superseded, or found wrong. diff --git a/tests/test_meta_contract_guards.py b/tests/test_meta_contract_guards.py index 3974f6b..c3a4239 100644 --- a/tests/test_meta_contract_guards.py +++ b/tests/test_meta_contract_guards.py @@ -19,6 +19,7 @@ ACTIVE_CONTRACT_FILES = ( ".github/instructions/ui.instructions.md", ".github/instructions/error-handling.instructions.md", ".github/instructions/providers.instructions.md", + ".github/instructions/tests.instructions.md", ".github/skills/python-code-reviewer/skill.md", ".github/skills/evidence-provenance-auditor/skill.md", ".github/skills/test-effectiveness-auditor/skill.md", @@ -213,6 +214,8 @@ def test_canonical_authority_references_are_present(): "docs/index.md", "docs/invariant/*", "uv run", + ".github/agents/", + ".github/prompts/", ), ".github/instructions/documentation-sync.instructions.md": ( "docs/index.md", @@ -242,6 +245,13 @@ def test_canonical_authority_references_are_present(): "tests/test_provider_boundaries.py", "SAFE_RESPONSE_HEADERS", ), + ".github/instructions/tests.instructions.md": ( + "AGENTS.md", + "docs/invariant/*", + "tests/test_meta_contract_guards.py", + "tests/test_config_isolation.py", + "asyncio_mode", + ), ".github/skills/python-code-reviewer/skill.md": ( "docs/*", "docs/schema.md",