Add .claude/skills/test-quality and reference it from AGENTS.md (#2993)

.claude/skills/test-quality/SKILL.md

@@ -0,0 +1,150 @@
+---
+name: test-quality
+description: Test quality bar for this repo. Read when writing, reviewing, or designing tests — covers naming, abstraction level, assertions, determinism, and the process for agent-driven test work.
+---
+
+# Test & code quality guide
+
+What "best practice for new work" means in this repo. Each rule carries its recorded reasoning
+where one exists; a rule with no stated why is a convention — follow it anyway.
+
+## Naming & shape
+
+- **Test names are behaviour sentences** stating the observable outcome, not the feature being
+  poked: `test_elicit_form_decline_returns_no_content`, never `test_elicit_form_decline`.
+- Plain top-level `test_*` functions; no `Test` classes (legacy files have them — don't copy).
+- **Docstrings: 1–2 sentences of behaviour, honest about provenance** — spec-mandated,
+  SDK-defined, or pinning a known gap? Say which: provenance is the triage key when the test
+  later fails. A pinned-gap assertion breaking usually means a change *fixed* the gap; a
+  spec-mandated assertion breaking means a regression.
+- Define things in dependency order; nothing forward-references. For client↔server tests:
+  handlers → server construction → client setup → act → assert — the test reads in the order
+  the conversation happens.
+- Inline the server (or equivalent setup) in the test, so the whole observable behaviour fits
+  on one screen. Lift to a file-level fixture only when several tests in *that file* genuinely
+  share it; never share across files.
+- A big multi-step test is fine when the property is irreducibly multi-step (e.g. resumability).
+  Split when a failure wouldn't tell you which claim broke — not for shortness. Compensate with
+  a numbered "Steps:" docstring so a reader sees the choreography before the body.
+
+## Level of abstraction
+
+- **Drive through the highest-level public API that can observe the property.** Hand-built wire
+  requests are brittle and don't prove the user-facing contract; tests that stay above the
+  internals keep working when the internals change. Drop to raw HTTP only when the assertion is
+  about something the high-level API *cannot* observe (status codes, headers, wire framing).
+- **Scripting a peer over raw streams is a last resort**, reserved for behaviour the typed API
+  cannot *produce* (malformed input, an impossible peer response). First ask what it would take
+  for the public API to express it — often a small helper suffices. Every such test's docstring
+  states why the public API couldn't do it.
+- **In-memory / in-process first.** HTTP-, SSE-, and auth-shaped behaviour can all be driven
+  through an in-process ASGI transport; threads only when necessary, subprocesses only when the
+  process boundary is itself the thing under test. In-process isn't just faster — it surfaces
+  bugs (a real stream leak was found this way) that subprocess indirection masks.
+- **Tests read like real user code**: no aliasing shims in conftest, no walls of suppressions,
+  no private imports unless that is genuinely the documented way to do the thing.
+- The assertion must prove the round trip — no side-channel state. What the server saw comes
+  back through the protocol, or via a closure-captured list asserted after the call. Handlers
+  assert their dispatch identity first (`assert params.name == "add"`), proving the request
+  that arrived is the request the test sent.
+
+## Assertions
+
+- **Transformations** (input → output the SDK produced) → full-object `snapshot(...)` equality,
+  so an added or dropped field fails. Regenerate with `--inline-snapshot=fix` so intentional
+  changes arrive as a reviewable diff; never hand-edit snapshot literals.
+- **Pass-through values** (opaque tokens, `_meta`, cursors) → identity against the same
+  variable you sent. A snapshot of a pass-through value only "matches" because a human checked
+  two literals correspond — it proves nothing.
+- **Errors** → `pytest.raises` + `.code` against the named constant; snapshot SDK-authored
+  messages; never `match=` on message text. Third-party text (pydantic, jsonschema) → stable
+  prefix only — never pin text that changes with a dependency upgrade — with a comment saying so.
+
+## Determinism
+
+- **No sleeps, ever.** A sleep guesses at timing instead of waiting on the condition, so it
+  either flakes or pads the run — sleeps head the list of the older test code's failure modes.
+  Coordinate with `anyio.Event` so the wait ends exactly when the condition holds (that
+  discipline is why 529 e2e tests run in ~10 s). Sole exception: tests *of* time-based
+  features, with a comment.
+- Bound every indefinite wait with `anyio.fail_after(5)`. **5 is the standard** — widening it
+  needs an articulable reason; "10 to be safe" is covering up a flake, not fixing one, and
+  unexplained widenings propagate (one agent used 10 and every later one copied it).
+- **Never assert wall-clock time, even with huge margins.** `elapsed < 0.9` on a ~0.01 s
+  operation — a 90× margin — was still rejected in review. `fail_after` bounding a hang is the
+  only timing primitive allowed.
+- **Concurrency tests must prove genuine interleaving.** Without barriers the scheduler is free
+  to serialize — "a" can start, finish, and return before "b" even begins — so two `start_soon`
+  calls prove nothing. Gate with events so all parties are mid-flight before any proceeds, emit
+  interleaved, then assert the demux.
+- **Don't over-synchronize either.** When delivery ordering is guaranteed (notifications
+  emitted during a request you're awaiting, over a single ordered in-memory stream), a plain
+  collected list asserted after the call is correct; events are for messages not tied to an
+  awaited operation. Verify the ordering guarantee actually holds for your transport first.
+- Async tests use anyio, not asyncio.
+
+## Behaviour philosophy
+
+- **Pin current behaviour; never xfail.** A green suite asserting what actually happens is a
+  regression bar for any refactor; an xfail proves nothing about it. Where current behaviour
+  falls short of spec, pin the divergent output and record the gap as data — a tracking issue,
+  with the docstring naming the known gap (suites with a requirements manifest record it as a
+  divergence entry). Not hidden, not skipped.
+- **Hollow-proof check**: before claiming a test covers a behaviour, re-read the claim and ask
+  "which assertion proves *this*?" A passing test near a behaviour is not proof of it — a full
+  review of the e2e suite found two such cases even under this discipline.
+
+## Hygiene
+
+- No new `# pragma`, `# type: ignore`, `# noqa` by default — restructure first; a suppression
+  usually means a test or a type is missing. The narrow sanctioned escape hatches (and the
+  audit to run before pushing) are in AGENTS.md. In tests, narrow types with `assert
+  isinstance`; never `Any`/`object` when a real type exists.
+- **Warnings raised during tests are findings, not noise** (the repo runs
+  `filterwarnings = error`). Fix the cause; if the fix can't land in the same change, scope the
+  suppression to the one fixture that needs it and track the real fix explicitly.
+- Registered-but-never-invoked handler bodies are `raise NotImplementedError`, so they cannot
+  silently become load-bearing.
+- Comments live next to the line they explain, not in docstrings; single backticks for code
+  refs; match the surrounding comment density (one-liners next to one-liners). No
+  `from __future__ import annotations` (py310+ repo).
+- Test work doesn't change `src/` as a side effect — the one mechanical exception is deleting a
+  pragma a new test now covers. If a test can't be written without a library change, raise it
+  as a finding or defer the test; don't quietly edit `src/`.
+
+## Process (for agent-driven work)
+
+- **Small chunks.** ≤10 tests per human-reviewed batch; when fanning out to multiple agents,
+  ≤5 per agent. Review quality scales inversely with batch size: the observed shortcuts
+  (over-stacked tests, a timing assertion, wrong abstraction level) all surfaced in one
+  oversized 27-test batch.
+- **Design → review → implement, as separate steps.** The design deliverable is not just the
+  plan — it's the judgement calls (abstraction level, deferrals, contested assertions) stated
+  explicitly, so the reviewer vetoes them before implementation rather than discovering them
+  in the diff.
+- **High-stakes areas get fresh adversarial reviewers** on both the design and the
+  implementation — fresh, because whoever designed it (or saw the proposed fix) anchors on the
+  same layer. SERIOUS findings (wrong assertion, missed MUST/SHOULD, an unrecorded known gap)
+  re-loop; style doesn't. Reserve the full panel for areas where being wrong is worse than
+  being slow.
+- **Investigations pair a full-context look with a fresh agent given only the problem** —
+  never the proposed fix — and compare conclusions. The unbiased read regularly catches
+  anchoring on the wrong layer.
+- **When review questions a decision, reconsider genuinely**: re-derive the tradeoff, state the
+  options, recommend with reasons. Defending the original choice is a valid outcome;
+  reflexively agreeing with every challenge is as bad as ignoring it.
+- **Validate the reference artifact before building on it.** Whatever you treat as ground truth
+  (a spec import, a baseline, a generated list), check it *first* — discovering it was
+  incomplete after five batches costs far more than before batch one.
+- **Verify as you go**: per-file `uv run --frozen pytest <file> -q` + pyright + ruff while
+  iterating; full suite, coverage, and `./scripts/test` (when `src/` was touched) at
+  integration.
+- **Every agent writes notes**: what it did, what broke, and — most importantly — what it
+  couldn't decide, so open questions surface instead of being silently resolved by whoever hit
+  them.
+- **Quality over speed is a stated goal, not a preference** ("rushing something out the door is
+  not the goal here, explicitly so"). Don't quietly de-scope agreed work mid-stream;
+  renegotiate scope explicitly.
+- **Don't copy patterns from existing test code by default** — much of the repo's older test
+  code is below the current bar (sleeps, mocks, `Test` classes, raciness). These rules define
+  the bar for new work.

AGENTS.md

@@ -45,6 +45,8 @@
 
 ## Testing
 
+- When writing or reviewing tests, conform to `.claude/skills/test-quality/SKILL.md`
+  — it defines the bar for naming, abstraction level, assertions, and determinism.
 - Framework: `uv run --frozen pytest`
 - Async testing: use anyio, not asyncio
 - Do not use `Test` prefixed classes — write plain top-level `test_*` functions.
@@ -129,6 +131,13 @@ changes softened by a backwards-compatibility shim. Include:
 Search for related sections in the migration guide and group related changes together
 rather than adding new standalone sections.
 
+## Documentation
+
+When a change affects public API or user-visible behaviour, update the relevant
+page(s) under `docs/` in the same PR. Docs are organised by topic
+(`tutorial/`, `client/`, `run/`, `advanced/`) — find the page covering the
+feature you touched rather than adding a new one.
+
 ## Formatting & Type Checking
 
 - Format: `uv run --frozen ruff format .`