chore: import upstream snapshot with attribution

2026-07-13 12:10:27 +08:00
commit 49b9bb6724
992 changed files with 161690 additions and 0 deletions
@@ -0,0 +1,118 @@
+Review the pull request: $ARGUMENTS
+
+Follow these steps carefully. Use the `gh` CLI for all GitHub interactions.
+
+## Step 1: Resolve the PR
+
+Parse `$ARGUMENTS` to determine the PR. It can be:
+
+- A full URL like `https://github.com/owner/repo/pull/123`
+- A `owner/repo#123` reference
+- A bare number like `123` (use the current repo)
+- A description — search for it with `gh pr list --search "<description>" --limit 5` and pick the best match
+
+Once resolved, fetch the PR metadata:
+
+```bash
+gh pr view <PR> --json number,title,body,author,state,baseRefName,headRefName,url,labels,milestone,additions,deletions,changedFiles,createdAt,updatedAt,mergedAt,reviewDecision,reviews,assignees
+```
+
+## Step 2: Gather the diff
+
+Get the full diff of the PR:
+
+```bash
+gh pr diff <PR>
+```
+
+If the diff is very large (>3000 lines), focus on the most important files first and summarize the rest.
+
+## Step 3: Collect PR discussion context
+
+Fetch all comments and review threads:
+
+```bash
+gh api repos/{owner}/{repo}/pulls/{number}/comments --paginate
+gh api repos/{owner}/{repo}/issues/{number}/comments --paginate
+gh api repos/{owner}/{repo}/pulls/{number}/reviews --paginate
+```
+
+Pay attention to:
+
+- Reviewer feedback and requested changes
+- Author responses and explanations
+- Any unresolved conversations
+- Approval or rejection status
+
+## Step 4: Find and read linked issues
+
+Look for issue references in:
+
+- The PR body (patterns like `#123`, `fixes #123`, `closes #123`, `resolves #123`)
+- The PR branch name (patterns like `issue-123`, `fix/123`)
+- Commit messages
+
+For each linked issue, fetch its content:
+
+```bash
+gh issue view <number> --json title,body,comments,labels,state
+```
+
+Read through issue comments to understand the original problem, user reports, and any discussed solutions.
+
+## Step 5: Analyze and validate
+
+With all context gathered, analyze the PR critically:
+
+1. **Intent alignment**: Does the code change actually solve the problem described in the PR and/or linked issues?
+2. **Completeness**: Are there aspects of the issue or requested feature that the PR doesn't address?
+3. **Scope**: Does the PR include changes unrelated to the stated goal? Are there unnecessary modifications?
+4. **Correctness**: Based on the diff, are there obvious bugs, edge cases, or logic errors?
+5. **Testing**: Does the PR include tests? Are they meaningful and do they cover the important cases?
+6. **Breaking changes**: Could this PR break existing functionality or APIs?
+7. **Unresolved feedback**: Are there reviewer comments that haven't been addressed?
+
+## Step 6: Produce the review summary
+
+Present the summary in this format:
+
+---
+
+### PR Review: `<title>` (<url>)
+
+**Author:** <author> | **Status:** <state> | **Review decision:** <decision>
+**Base:** `<base>` ← `<head>` | **Changed files:** <n> | **+<additions> / -<deletions>**
+
+#### Problem
+
+<1-3 sentences describing what problem this PR is trying to solve, based on the PR description and linked issues>
+
+#### Solution
+
+<1-3 sentences describing the approach taken in the code>
+
+#### Key changes
+
+<Bulleted list of the most important changes, grouped by theme. Include file paths.>
+
+#### Linked issues
+
+<List of linked issues with their title, state, and a one-line summary of the discussion>
+
+#### Discussion highlights
+
+<Summary of important comments from reviewers and the author. Flag any unresolved threads.>
+
+#### Concerns
+
+<List any issues found during validation: bugs, missing tests, scope creep, unaddressed feedback, etc. If none, say "No concerns found.">
+
+#### Verdict
+
+<One of: APPROVE / REQUEST CHANGES / NEEDS DISCUSSION, with a brief justification>
+
+#### Suggested action
+
+<Clear recommendation for the reviewer: what to approve, what to push back on, what to ask about>
+
+---
@@ -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.