From 57e85dec390a472930de8e34a59d3d48962b6c16 Mon Sep 17 00:00:00 2001 From: John Cusack Date: Thu, 29 Oct 2026 10:40:18 -0400 Subject: [PATCH] docs(upstream): review-panel process for upstream PRs and replies Rewrite docs/UPSTREAM_PRS.md around the idna#286 and email-validator#182 rejections: why first, smallest diff with per-hunk numbers, import time and documented settings measured, short text, three fresh review agents (maintainer, AI tells, facts) on a packet with only diff, evidence and draft, and human sign-off. Replies go through the same panel. Add reviewer/writer prompts and PR/reply templates under prompts/upstream_pr/, plus scripts/upstream_pr_check.sh (model-free checks) and scripts/upstream_pr_packet.sh (review packet builder). --- README.md | 2 +- docs/UPSTREAM_PRS.md | 837 +++++++++++++---------- prompts/upstream_pr/pr_body_template.md | 63 ++ prompts/upstream_pr/reply_template.md | 48 ++ prompts/upstream_pr/review_ai_tells.md | 70 ++ prompts/upstream_pr/review_facts.md | 67 ++ prompts/upstream_pr/review_maintainer.md | 93 +++ prompts/upstream_pr/writer.md | 63 ++ scripts/upstream_pr_check.sh | 165 +++++ scripts/upstream_pr_packet.sh | 107 +++ 10 files changed, 1163 insertions(+), 352 deletions(-) create mode 100644 prompts/upstream_pr/pr_body_template.md create mode 100644 prompts/upstream_pr/reply_template.md create mode 100644 prompts/upstream_pr/review_ai_tells.md create mode 100644 prompts/upstream_pr/review_facts.md create mode 100644 prompts/upstream_pr/review_maintainer.md create mode 100644 prompts/upstream_pr/writer.md create mode 100755 scripts/upstream_pr_check.sh create mode 100755 scripts/upstream_pr_packet.sh diff --git a/README.md b/README.md index f750341..d3c6a03 100644 --- a/README.md +++ b/README.md @@ -265,7 +265,7 @@ Built milestone by milestone, M0 → M9, each with verbatim acceptance criteria: - `docs/PYTHON_PATTERNS.md` — the living pure-Python speedup guide: what made each tier-1 fix faster (and where it didn't), same evidence discipline, separate from the Rust guide - `docs/PYTHON_SPEEDUPS.md` — the pure-Python speedups tracker: package, downloads/mo, measured speedup + workload, patch location, upstream PR URL + status, Rust-rewrite follow - `docs/PATTERN_REVIEW.md` — the post-port review process + brief checklist that keeps both guides current (`scripts/gather_pattern_review.sh` builds the review bundle) -- `docs/UPSTREAM_PRS.md` — the end-to-end process for offering speedups upstream so they actually get merged (target selection, proof format, humanizer pass, review response, pre-flight checklist, PR-body template) +- `docs/UPSTREAM_PRS.md` — the process for upstream PRs and replies: why first, smallest diff, measured costs (import time included), short text, a three-agent review panel (`prompts/upstream_pr/`), human sign-off; `scripts/upstream_pr_check.sh` and `scripts/upstream_pr_packet.sh` do the mechanical check and build the review packet Pinned fixtures (`config/fixture.toml`): primary `Nicoretti/crc`, secondary `luozhouyang/python-string-similarity` for DAG checks. diff --git a/docs/UPSTREAM_PRS.md b/docs/UPSTREAM_PRS.md index eb67b9b..a71088c 100644 --- a/docs/UPSTREAM_PRS.md +++ b/docs/UPSTREAM_PRS.md @@ -1,355 +1,490 @@ -# Upstream PRs — the end-to-end process for offers that get merged - -How rustsmith offers pure-Python speedups (and, earlier, small Rust -accelerators) back to upstream projects so that maintainers actually -merge them. Docs only: no code, no gate changes, no upstream contact -beyond the PRs themselves. Per the repo rule, per-project patch work -lives in its fork or its own repo; this file holds the generic process -plus the evidence behind each rule. - -Related: [`PYTHON_SPEEDUPS.md`](PYTHON_SPEEDUPS.md) (per-package -tracker — the upstream lane owns its PR URL + status cells), -[`PATTERN_REVIEW.md`](PATTERN_REVIEW.md) (the review pass never touches -those cells), [`PYTHON_PATTERNS.md`](PYTHON_PATTERNS.md) (the mechanisms -worth offering). - -## Evidence base (all states as of 2026-10-29) - -Ten upstream PRs from two generations of lanes. The first generation -offered small Rust accelerators / pure-Python hot-path fixes; the -second (tier-1 lanes, report in -`/home/john/firstmate/data/rs-upstream-tier1/report.md`) offers -measured pure-Python tier-1 patches with before/after wall + peak RSS. -Every rule below cites the row that taught it. - -| PR | Shape | State | Maintainer signal | -|---|---|---|---| -| [Nicoretti/crc#210](https://github.com/Nicoretti/crc/pull/210) | pure-Python fast loop (~18x) | **closed, unmerged** | "AI PR with failing CI." — 12/18 checks red | -| [Nicoretti/crc#211](https://github.com/Nicoretti/crc/pull/211) | resubmit + missing tests | **closed, unmerged** | "code is not dry, test do not use best practices and does not fit existing style" | -| [Nicoretti/crc#212](https://github.com/Nicoretti/crc/pull/212) | single-impl loop, unittest-style tests | open | no maintainer reply yet; fork-CI awaiting approval, local proof posted in a comment instead | -| [JoshData/python-email-validator#182](https://github.com/JoshData/python-email-validator/pull/182) | 4 mechanical wins, 2.97x rep | open | "But why?" — motivation questioned, not mechanics | -| [arrow-py/arrow#1398](https://github.com/arrow-py/arrow/pull/1398) | class-level memo + parser-per-locale, 1.81x pipeline | open, `REVIEW_REQUIRED` | none yet; 24/27 CI legs green, 3 reds triaged in a comment | -| [horejsek/python-fastjsonschema#233](https://github.com/horejsek/python-fastjsonschema/pull/233) | draft04-only codegen shapes, 1.48x rep | open | none yet | -| [yaml/pyyaml#976](https://github.com/yaml/pyyaml/pull/976) | optional Rust scan core | open | none — no comments, no reviews (ignored so far) | -| [dateutil/dateutil#1609](https://github.com/dateutil/dateutil/pull/1609) | tokenizer by index, py2.7-compatible | open | none — 44/45 checks, no comments | -| [executablebooks/markdown-it-py#449](https://github.com/executablebooks/markdown-it-py/pull/449) | per-line markers | open | none — 2/2 checks, no comments | -| [Kludex/python-multipart#337](https://github.com/Kludex/python-multipart/pull/337) | callback hoist, same shape as their #316 | open, 2 reviews | codspeed bot: **+30.55%** on their own benchmark | - -Plus: idna tier-1 (2.60x mixed / 7.52x ascii, note in -`/home/john/firstmate/data/rs-upstream-idna/upstream-patch-note.md`) -was PR-ready but unopened at time of writing — the offer lane was still -running. And the deliberate non-offers: pyasn1 (1.17x, low priority), -mako (1.015x noise — issue-only lead), networkx tier-1 (needs an API -change; the offer was a Rust kernel port instead, ADR-066). - -Two patterns in the table matter most. First, **nothing has been merged -yet** — the process below is built from closes and silences, not from -victories; follow it to avoid the known failure modes, not because it -guarantees a merge. Second, the only maintainer sentences we have are -the two crc closes and one "But why?": every one of them is about -*trust* (red CI, foreign style, unclear motive), never about the -speedup number. The process optimizes for trust. - -## 1. Target selection - -- **Active maintainers first.** Check the commit log and recent merge - rate before investing lane time: PRs into repos that merge weekly get - answers; #976 / #1609 / #449 show that even live PRs with green CI - can sit uncommented for weeks when maintainers are busy. Prefer the - highest-download target whose maintainers merged *something* in the - last month. -- **Read their AI-contribution policy and disclosure requirements - before writing a line.** None of email-validator, arrow, or - fastjsonschema had an AI clause in CONTRIBUTING as of 2026-10-29 — - but crc's maintainer closed #210 as "AI PR with failing CI" with no - written policy to point at. Policies appear without notice and norms - apply without being written: assume every diff will be read as - AI-authored, and pre-empt it (green gates, house style, human body - text — §§4–6). If a repo *does* require disclosure or a sign-off, - that is a hard gate, not guidance. -- **Don't offer what they can't take.** pyasn1 (1.17x) was held as low - priority; mako (1.015x, inside noise) was never offered; networkx T2 - needed an API change so it stayed a note (ADR-066). Below ~1.2x on - the representative track, or anything needing an API change, is not - an offer — record it and walk away. - -## 2. Issue-first versus PR-first - -Discover per repo, every time: CONTRIBUTING, PR template, code of -conduct, dev docs, CI config. No shortcut from the last repo carries -over. - -- None of email-validator / arrow / fastjsonschema required an issue - first — PRs went directly. If the docs *do* require an issue (or a - discussion), open it and stop at needs-decision until maintainers - respond; never open the PR to force the question. -- If the repo has a PR template, it goes first verbatim (checklist - included), evidence after — arrow #1398 kept the template checklist - on top. No template → free-form body (§6). -- Changelog and sign-off rules are per repo and binding: - email-validator and fastjsonschema both expect contributor CHANGELOG - entries under the unreleased section (verified from their git log — - both PRs added one); arrow's changelog is maintainer-curated at - release time, so #1398 added none. Copy what recent merged PRs did, - not what you prefer. - -## 3. Smallest reviewable diff, no behavior change, their style - -The #211 close is the whole section: "code is not dry, test do not use -best practices and does not fit existing style." Concretely: - -- **One concern, minimal files.** fastjsonschema #233 kept to - `draft04.py` even though the same shapes apply to draft06/07 — - "kept this PR to `draft04` to stay reviewable." Offer the follow-ups - in prose, not in the diff. -- **No behavior change, ever.** Same exceptions, messages, normalized - forms, error paths. Prove it with a differential corpus on top of - their suite (email-validator: 468/468 identical stock-vs-patched - including exception type and message; fastjsonschema: sha256 outcome - checksums + error samples agree on all three tracks; arrow: 20/20 - including 5 error-path cases). -- **Match the file's existing idiom, not yours.** #212 exists because - #211's tests weren't in the module's unittest style and the loop was - two near-identical copies. Constraints propagate: dateutil #1609 - kept Python 2.7-compatible syntax because the codebase is; arrow - #1398 added tests because their template and 99%-coverage gate demand - them. -- **New tests where the repo's gates need them, not blanket coverage.** - Arrow got cross-instance memo sharing, locale isolation, eviction, - and factory tests (coverage gate 99% → achieved 99.93%). crc #210 - died on `fail-under=100` at 98% — five uncovered lines — so #211/#212 - added exactly the delegation-guard and bypassed-body tests. Run - *their* coverage gate locally and close the gap before opening. - -## 4. Reproduce their environment, run their gates, rebase, re-measure - -Do this on current upstream main, not on the lane's pinned base — the -rebase itself finds real bugs: - -- **Fresh venv per repo, installed per their docs.** - email-validator: `test_requirements.txt` + `pip install -e .`, then - `make`-equivalent flake8/mypy/pytest. Arrow: `requirements-tests.txt` - + editable install, `pytest` with `--cov-fail-under=99`, `pre-commit - run --all-files`. fastjsonschema: `.[devel]` + the - `JSON-Schema-Test-Suite` submodule init (`make test` needs it). -- **Rebase the patch onto main HEAD and re-measure there.** Stock arm = - pristine `git archive` of HEAD; patched arm = the fork branch; same - frozen workload, fresh subprocesses, min-of-several walls, peak RSS - both arms. Numbers measured against a stale base are not evidence. -- **Run their full suite on both arms.** Twice this caught real patch - bugs, not flakes: arrow's naive class-level key broke - `test_parse_unrecognized_token` (in-place `_BASE_INPUT_RE_MAP` mock - vs stale cache entry) — fixed by keying the memo on a per-instance - input-map fingerprint, no test modifications needed. fastjsonschema's - `_keys`-line skip exposed codegen `IndentationError` on - trivially-checkless schemas (`properties: {}`, solo - `additionalProperties: true/{}`) — fixed by hoisting trivial returns - above the `if`-emitting block. If their suite passes unmodified, say - so; it is the strongest sentence in the body. - -## 5. Proof format (PR body evidence section) - -Every offer carries the same five items, workload-labeled, both sides. -Copy the template in §9. Rules: - -- Before/after **wall time and peak RSS** on the same frozen workload, - both sides, with machine + interpreter versions and exact - reproduction commands. Min-of-N fresh-subprocess walls; VmHWM/`ru_maxrss` - for RSS (note: `ru_maxrss` is constant-broken on some boxes — the tier-1 - lanes verified against VmHWM; say which you used). -- Representative track first, showcase second, near-unchanged tracks - disclosed, not hidden (email-validator idn-1000 1.09x "idn-bound, - near unchanged"; idna idn-4000 1.12x). A 1.00x row is a finding. -- Outcome counts identical both arms (parse counts, ok/err splits). -- Their suite result verbatim, including pre-existing failures with the - stock-arm proof (email-validator IPv6 `::ffff:` rendering fails - identically on unmodified main — py3.12 `ipaddress` drift; arrow's 3 - red legs triaged with master-HEAD evidence; see §7). -- No internal paths, tokens, fleet details, or Rust-port mentions. The - bench script stays available on request ("say the word and I'll - attach it") rather than inflating the diff. - -## 6. Title, body, and the humanizer pass - -Title: what + size + "no behavior change" -(`Leaner generated validation code (~1.5x on object-heavy schemas), no -behavior change`). Body: their template first, then What / Measured / -Test-proof, in that order — see the three tier-1 PR bodies -(`ev_pr_body.md`, `arrow_pr_body.md`, `fjs_pr_body.md` in the tier-1 -lane scratch) as worked examples. - -Then run the body text through the **humanizer skill in Claude Code** -before opening. Why: both crc closes read as reactions to -AI-sounding, process-ignoring output — "AI PR with failing CI" and a -style mismatch. The humanizer pass exists to remove the tells (stiff -diction, over-formatting, hedged throat-clearing) while keeping every -number and claim byte-identical. It edits prose only — never code, -never figures. - -- **The exact skill and how a worker invokes it.** There is no skill - named `humanizer` installed in this environment (checked 2026-10-29: - `~/.claude/skills/` carries only `no-mistakes` + synced entries; - installed plugins are only `rust-analyzer-lsp` from the official - `anthropics/claude-plugins-official` marketplace, which ships no - humanizer). The captain's "humanizer skill in Claude Code" therefore - means a **third-party plugin/skill**, and installing any third-party - plugin needs **captain approval first**. Proposed install once the - captain names the marketplace and plugin: - `claude plugin marketplace add ` then - `claude plugin install humanizer@` (exact names from the - captain — do not guess a repo). After install, the worker invokes it - in Claude Code with the Skill tool (`humanizer`) on the draft - title + body file, then diffs the result to confirm no number, - filename, command, or claim changed. -- **Until it is installed:** write the body from the §9 template in a - plain human register — short sentences, first person where the - template allows, plain acknowledgment of anything red (the crc #212 - body is the model: "Fair catch… Happy to adjust anything — tests, - style, or scope"). The #211→#212 tone shift plus genuine fixes is - what a second chance looks like; no wording pass substitutes for - green gates and house style. - -## 7. Responding to review - -Lane rule, unchanged from the first upstream brief: maintainer -questions come back as needs-decision with their words quoted verbatim -— never paraphrased, never answered unilaterally when they imply a -direction choice. - -- **Latency:** acknowledge within one day (a short comment, even before - the fix), fix within the lane's next run. #182's "But why?" (2026-10-29) - is the live case: it questions *motive*, so the answer is data, not - argument — who pays the cost (per-validation overhead on every - `validate_email` call), the measured size, and the offer to split the - PR (the email-validator body already pre-offers: "If (1) reads as too - clever, (2)–(4) stand alone … happy to split the PR either way"). -- **One concern per commit**, re-run their gates per commit, say what - each commit addresses. #212's body models this: "the byte loop is now - a single implementation … and the new tests are written in the - unittest style the test module already uses" — naming both review - points from #211 explicitly. -- **Never argue benchmarks without data.** If a maintainer doubts a - number, re-measure on their terms (their workload, their box - description) and report both arms again. If they doubt the approach, - offer the split/narrowing first. -- **CI failures that are not ours: document politely, with evidence, in - a comment — not in the body.** Arrow #1398's triage comment is the - template: each red leg named, each shown failing identically on - unmodified master HEAD (commit hash given), closing with "Happy to - rebase/retest if you want a fresh full-green signal." Same for the - email-validator `ipaddress` drift (fails identically on main) and crc - #212's fork-approval wait (`action_required`, plus the local gate - results in the comment). Never call their CI broken; show the - stock-arm proof and offer the re-run. - -## 8. Follow-up and closing the loop - -- **Monitor via contributions records.** Every upstream PR gets a - `contributions.json` entry (`fm-contributions.v1`) in its lane dir; - re-check state, comments, reviews, and checks on a cadence — silence - is also data (#976/#1609/#449: green, uncommented, unmerged for - weeks). Report maintainer replies as needs-decision the same day. -- **Update [`PYTHON_SPEEDUPS.md`](PYTHON_SPEEDUPS.md) when state - changes and nothing else does:** `being offered` → PR URL + `offered` - when it opens → `merged` / `declined` on verdict. The upstream lane - owns those cells; the pattern-review pass never touches them - ([`PATTERN_REVIEW.md`](PATTERN_REVIEW.md) § "Upstream tracker"). -- **A merged pure-Python change can unlock a Rust rewrite re-scope.** - fastjsonschema is the receipt: tier-1 measured 1.48–1.63x, and the - port then measured 2.44x rep (1.69x *over* the tier-1 baseline, - ADR-061) — the merged-shape question and the port ceiling are - re-computed against the new baseline. Conversely arrow and - email-validator tier-1s confirmed no_gain (ceilings 1.78x / 1.66x < - 2x, ADR-059 / ADR-065): no rewrite follows, and the doc says so - honestly. -- **If it stalls, say so in the tracker.** A PR ignored for a month is - not "offered" forever — note the stall date in the lane, keep the - fork branch, and move on. Do not bump the thread, do not close and - re-open to game visibility. - -## 9. Pre-flight checklist - -Copy into the offer lane; every box answered before opening. - -- [ ] Target selected on evidence: downloads, recent maintainer merges - (merged something in the last month), no standing no-offer reason - (saturated hot path, needs-API-change, <1.2x rep). -- [ ] AI-contribution policy + disclosure rules read today; hard gates - (sign-off, issue-first, changelog) listed. -- [ ] Their dev env reproduced fresh (their install docs, their venv); - no lane paths or tokens anywhere near the fork. -- [ ] Diff is minimal, behavior-identical, house-styled; new tests only - where their gates need them; their coverage gate green locally. -- [ ] Rebased onto current upstream main; re-measured there (min-of-N - fresh subprocesses, wall + peak RSS both arms, workload-labeled); - differential equivalence corpus green on top of their suite. -- [ ] Their full suite + lint + typing green on the branch; any red - shown failing identically on pristine main (hash recorded) with - the triage ready as a comment, not in the body. -- [ ] PR body: their template first (if any), then What / Measured / - Test-proof; title carries size + "no behavior change"; bench - script offered on request, not inlined. -- [ ] Body run through the humanizer skill (or hand-written in a plain - human register per §6); numbers/commands/claims diffed unchanged. -- [ ] Fork branch under our account, one branch per package; never - touched their branches; PR opened ready-for-review, never draft. -- [ ] contributions.json entry + tracker cells (`being offered` → PR - URL + `offered`) updated on open. - -## 10. PR-body template - -```md - (~x on ), no behavior change - - - -## What - -. mechanical hot-path wins, each behavior-preserving -(): - -1. (``). . -2. … - - - -## Measured - -, . Frozen workload: , each arm a fresh subprocess, min-of- walls, peak -RSS per arm: - -| Track | Stock (main @ ) | Patched | Speedup | -|---|---|---|---| -| | | | | -| | | | | -| | | | <~1x, cause named> | - -Peak RSS (~ both). Outcome counts identical both -arms (). - -Reproduce (from a checkout at vs this branch): +# Upstream PRs + +How we offer speedups to upstream Python projects, and how we reply on +those threads afterwards. Every PR and every comment goes through the same +steps: a reason a real user needs it, the smallest change that delivers +it, measured costs, short text, three independent reviews, and a person +who signs off before anything is posted. + +Related: [`PYTHON_SPEEDUPS.md`](PYTHON_SPEEDUPS.md) is the per-package +tracker (the offering lane owns its PR URL and status cells), +[`PYTHON_PATTERNS.md`](PYTHON_PATTERNS.md) lists the mechanisms worth +offering, and [`PATTERN_REVIEW.md`](PATTERN_REVIEW.md) never touches the +tracker's PR cells. + +## What maintainers have told us + +As of 2026-10-29 none of our upstream PRs has been merged. Every +maintainer reply so far has been about motive, size, or whether a person +checked the work. None was about the speedup number. + +| PR | What happened | +|---|---| +| [Nicoretti/crc#210](https://github.com/Nicoretti/crc/pull/210) | Closed: "AI PR with failing CI." Their coverage gate (`fail-under=100`) fell to 98%. | +| [Nicoretti/crc#211](https://github.com/Nicoretti/crc/pull/211) | Closed: "code is not dry, test do not use best practices and does not fit existing style". | +| [JoshData/python-email-validator#182](https://github.com/JoshData/python-email-validator/pull/182) | "But why?" The body said what changed and how fast, but not who needs it. | +| [kjd/idna#286](https://github.com/kjd/idna/pull/286) | Closed: "highly complicated for what is a trivial small optimization", and the import move "causes a significant regression by forcing a significant dependency that most users of the library will not use. It looks like it was AI generated and I can't imagine it has had any legitimate human review." The maintainer merged the same idea as [kjd/idna#287](https://github.com/kjd/idna/pull/287): three added lines. | +| [arrow-py/arrow#1398](https://github.com/arrow-py/arrow/pull/1398), [horejsek/python-fastjsonschema#233](https://github.com/horejsek/python-fastjsonschema/pull/233), [yaml/pyyaml#976](https://github.com/yaml/pyyaml/pull/976), [dateutil/dateutil#1609](https://github.com/dateutil/dateutil/pull/1609), [executablebooks/markdown-it-py#449](https://github.com/executablebooks/markdown-it-py/pull/449) | Open, no maintainer reply yet. | +| [Kludex/python-multipart#337](https://github.com/Kludex/python-multipart/pull/337) | Open. Their own benchmark bot reported +30.55%. | + +So the process is built against three complaints: no clear reason, too +much code and text for the gain, and no sign that a person checked it. + +## The steps + +1. Write the why. +2. Make the diff small. +3. Measure the gain and every cost a user could see. +4. Write the text and run the humanizer skill on it. +5. Run the mechanical check. +6. Run the review panel: three fresh agents, one packet. +7. Resolve the verdicts. Shrink or hold when they say so. +8. A person signs off and posts. + +Replies and comments on upstream threads go through steps 4 to 8 as +well (see [Replies and comments](#replies-and-comments)). + +The pieces live here: + +| File | Used in | +|---|---| +| [`prompts/upstream_pr/writer.md`](../prompts/upstream_pr/writer.md) | Steps 1 to 4: brief for whoever prepares the PR | +| [`prompts/upstream_pr/pr_body_template.md`](../prompts/upstream_pr/pr_body_template.md) | Step 4: PR title and body | +| [`prompts/upstream_pr/reply_template.md`](../prompts/upstream_pr/reply_template.md) | Step 4: replies and comments | +| [`scripts/upstream_pr_check.sh`](../scripts/upstream_pr_check.sh) | Step 5: word counts, banned terms, dashes, title, diff warnings | +| [`scripts/upstream_pr_packet.sh`](../scripts/upstream_pr_packet.sh) | Step 6: builds the packet the reviewers see, runs the check | +| [`prompts/upstream_pr/review_maintainer.md`](../prompts/upstream_pr/review_maintainer.md) | Step 6: skeptical maintainer | +| [`prompts/upstream_pr/review_ai_tells.md`](../prompts/upstream_pr/review_ai_tells.md) | Step 6: AI tells, using the humanizer skill's patterns | +| [`prompts/upstream_pr/review_facts.md`](../prompts/upstream_pr/review_facts.md) | Step 6: facts and numbers | + +## 1. The why + +Before touching code, write two sentences: who calls this code, how often, +and what it costs them today. Then check them. + +- Read the project's README for the use it names. email-validator says it + is for registration and login forms: one address per request, often + followed by a DNS check that takes milliseconds. Saving 37 µs per + address means nothing there. A why for that project has to name a + different caller, and one exists: pydantic's `EmailStr` calls + `email_validator.validate_email(..., check_deliverability=False)` for + every value it validates, so bulk imports pay it once per record. +- Name a real caller and link it: a dependent library, a framework + integration, an issue that asks for speed. +- If the only caller you can name is our benchmark loop, there is no PR. + Mark it held in the tracker. + +The older target rules still apply. Prefer projects whose maintainers +merged something in the last month. Read CONTRIBUTING, the PR template, +and any AI or disclosure policy before writing a line; a required issue, +sign-off, or disclosure is a hard gate. Do not offer anything below about +1.2x on the workload that matches the why, or anything that needs an API +change. + +## 2. The diff + +Start from the smallest change that could carry the gain, and add a hunk +only when it earns its place. + +- Measure each hunk on its own (leave it out, or add it alone) on the why + workload, and keep the table in the evidence. A hunk stays if it + carries roughly 10% or more of the total gain, or if another hunk needs + it to be correct. idna#287 kept 89% of the time idna#286 saved on the + mixed workload, with 3 added lines against 11. The import move in #286 + bought nothing we could measure (our own body said it "barely changes + the numbers") and cost every user at import. +- Write it the way the maintainer would: their names, their comment + density, no new module state unless it pays for itself. Keep our terms + out of the code. #182 shipped a comment that began "Tier-1 (pure-Python) + fast paths". +- If you would offer to split the PR, split it first and post the smaller + part. An offer to split in the body tells the maintainer the PR is + bigger than it needs to be. +- No behavior change: same return values, exceptions, and messages. Match + the file's idiom (crc#211 failed on style). Add tests only where their + gates need them, and run their coverage gate locally (crc#210 failed on + it). + +## 3. Measure + +Reproduce their environment from their own docs in a fresh venv, rebase +onto current upstream main, and measure there. Both arms (main and the +branch) run in fresh processes, arms interleaved, min of several runs. +Use VmHWM for memory; `ru_maxrss` reads constant on some of our machines. + +The evidence directory holds raw output, both arms, for: + +- the speed number on the why workload, and the per-hunk table; +- `python -X importtime -c "import "` and VmHWM after import, + whenever the diff touches imports or module-level code; +- their full test suite, lint, and type checks (a failure that also + happens on main gets the main-branch proof, with the commit); +- a differential run over generated inputs comparing return values and + exception type and message; +- every documented runtime setting, changed after the first call, still + taking effect. + +The last item is new because #182 broke it. The PR cached the module +defaults on the first call, so `email_validator.CHECK_DELIVERABILITY = +False`, which the README tells users to set, is ignored on the PR branch +once one address has been validated. Its 468-case differential corpus +passed because no case changed a setting. + +## 4. The text + +Use [`pr_body_template.md`](../prompts/upstream_pr/pr_body_template.md). +In short: + +- The title says what the change does, like a commit subject. No speedup + multiplier and no "no behavior change". +- The body leads with the why, then what changed, then one number, then + one line on tests. Under 200 visible words, and aim for about 100. + Benchmark details, scripts, and any table go in a collapsed `
` + block. +- Write to the maintainer. "The test suite passes", not "their gates are + green". +- No offers to attach more, no pre-offered splits, no internal terms, no + dashes, no bold. +- Their PR template goes first if they have one. Add a changelog entry + only if recent outside PRs did. + +Then run the humanizer skill on the draft (in Claude Code, the Skill tool +with `humanizer:humanizer`, embedded mode) and diff the result against +your draft. No number, name, command, or claim may change. + +On AI assistance: follow the project's policy, disclose when it asks, and +never deny it when someone asks. The humanizer pass removes padding and +habits that generated text carries; it does not change how the work was +done or who is answerable for it. Step 8 is what answers "has a person +reviewed this". + +## 5. Mechanical check + +`scripts/upstream_pr_check.sh --body [--title ] [--diff +] [--kind pr|reply]` runs without any model. It fails on a visible +body over 200 words (120 for a reply), internal terms in the text or the +added lines, dashes in prose, a title over 90 characters or one carrying +a multiplier or "no behavior change", and headings or tables in a reply. +It warns on chat residue and pre-offers, stock words, bold, new +module-level imports, regexes and `global` statements, and comments that +narrate the optimization. Fix every FAIL before the panel runs. The +packet builder runs it and saves the output as `checks.txt`. + +Against our own past PRs: idna#286 fails four ways (656 visible words for +a 13-line diff, title length, multiplier and "no behavior change" in the +title) and email-validator#182 fails twelve. kjd's #287 passes. + +## 6. Review panel + +Build the packet: ```sh - - +scripts/upstream_pr_packet.sh --repo --base \ + --head --evidence --title --body \ + --out ``` -(Bench script with the frozen available on request — -say the word and I'll attach it.) - -## Test / lint proof - -Their own gates, run in a venv built exactly per : - -* ``: **** on both stock and patched; fails identically on unmodified main — , unrelated. -* ``: clean. -* Differential equivalence: -case corpus () dumped - under stock main and under this branch — **/ identical**. - -" | No changelog entry: .> -``` +For a reply, add `--thread ` with the conversation so far, and +drop `--title`. The packet holds the diff, the raw evidence, the draft, +the thread, the check output, and a `MANIFEST.md` with an id hashed from +all of them. It does not hold the writer's notes, plans, earlier drafts, +or earlier verdicts, so the reviewers judge only what the maintainer +will see. + +Run three reviewers in parallel, each a fresh agent. In Claude Code that +is the Agent tool with a general-purpose agent, never a fork: a fork +inherits the writer's context. The whole prompt is: "Read your role +instructions in `prompts/upstream_pr/review_.md` and follow them. +Packet: ``." Add the read-only and no-posting constraints, and +nothing else. No summary of the change and no hint of what worries you. + +| Reviewer | Asks | Verdicts | +|---|---|---| +| Maintainer | Would I merge this? Is there a why in the first two sentences? Is it too big for the gain? What cost is unmeasured? | PASS, SHRINK, HOLD, FAIL | +| AI tells | Where would a maintainer first think "generated"? Uses the humanizer skill's numbered patterns plus PR-specific ones (length, method narration, title taglines, pre-offers, our jargon in code comments). | PASS, FAIL | +| Facts | Does every number, claim, and "no behavior change" trace to evidence or a public source? Are derived numbers right? Is any required cost measurement missing? For a reply, does the live thread still match? | PASS, FAIL | + +Each returns a fixed block with the packet id, the verdict, and numbered +findings. Save them as `reviews/.txt` in the packet. + +## 7. Resolving verdicts + +- Post only when all three say PASS on the same packet id. +- The strictest verdict wins: HOLD, then SHRINK, then FAIL, then PASS. +- Any edit to the diff or the text makes a new packet, and all three + reviewers run again on it. A rewrite for tone can break a fact, and a + fact fix can add a tell. +- Facts FAIL: correct the claim, delete it, or measure what is missing. + It is never argued. +- AI-tells FAIL: the writer rewrites the flagged parts (the reviewer only + flags), then runs the humanizer again. +- SHRINK: cut what the maintainer reviewer named, re-measure, rebuild. + If the smaller version falls under the gain bar, it becomes HOLD. +- HOLD: do not post. Record the reason in the tracker. If the only gap is + the why and the project welcomes questions, the person signing off may + open a short issue asking whether they would want it. +- When reviewers pull in different directions (the maintainer wants a + sentence of context, the AI-tells reviewer wants it shorter), the + shorter text wins and the extra goes into `
` or is dropped. + Numbers always follow the facts reviewer. +- The writer may contest one finding per round, in writing, citing the + packet. A new agent of the same role gets the packet, the finding, and + the rebuttal, and rules UPHOLD or DROP; that ruling stands. A facts + finding can only be answered with evidence. +- Three rounds at most. If the packet still has no unanimous PASS, it is + held and goes to a person with the packet and the last verdicts. + +## 8. Sign-off and posting + +A person reads the final diff, the text, and the three verdicts, and +posts only what they would defend line by line in the thread. This is the +review kjd said was missing. Before posting, re-fetch the thread: if +anything changed since the packet was built, rebuild it and review again. + +Post from a fork branch under our account, one branch per package, ready +for review, not as a draft. Update the tracker cells (`being offered`, +then the PR URL and `offered`) and the lane's contributions record. + +## Replies and comments + +Every comment we post on an upstream PR or issue goes through steps 4 +to 8: answering a question, replying to a review, closing our own PR, a +CI triage note, a follow-up. Use +[`reply_template.md`](../prompts/upstream_pr/reply_template.md) and build +the packet with `--thread`. The same three reviewers run on it; the +maintainer reviewer asks whether the first sentence answers what was +asked, and the facts reviewer checks the live thread. + +Rules for replies: + +- The first sentence answers the question or says what you are doing. +- Under 120 words, and most should be under 60. No headings, no tables. +- Every number is already in the thread or in the evidence. +- Offer only a version you have measured, with its number. +- When the maintainer found a real problem, say so once and fix it. +- When the maintainer has already done it themselves, thank them in a + clause and stop. + +The older reply rules still hold. A maintainer's comment goes to the +person signing off the same day, quoted word for word, before anyone +drafts an answer. Acknowledge within a day. Push one concern per commit +and say what each commit addresses. Never argue a benchmark without new +data; re-measure on their terms instead. A CI failure that also happens +on main goes in a comment with the main-branch proof (arrow#1398 has the +model), never in the body. + +### First use: the two pending replies + +Both drafts below went through the full pipeline on 2026-10-29. Neither +has been posted. + +idna#286, draft as written: + +> Thanks for looking at it, and for #287, which gets the same win in far +> less code. You're right about the import move too. I didn't measure +> import time, and it costs everyone who never touches uts46. Closing this +> in favour of #287. + +Round 1: maintainer PASS, AI tells PASS, facts FAIL. #287 recovers 89% +of the time #286 saved on the mixed workload and 95% on plain ASCII, so +"the same win" overstates it. The facts reviewer also checked the live +thread: kjd had closed #286 and merged #287 a few minutes after our +snapshot, so "Closing this" was stale. + +Round 2, after re-fetching the thread: all three PASS. Final text: + +> Thanks for #287, which gets most of the win in far less code. You're +> right about the import move too. I didn't measure import time, and it +> costs everyone who never touches uts46. + +The maintainer reviewer advised against adding the measured cost (about +2 ms and 1 MiB per import), because numbers would read as arguing with +"significant", and against offering the `ulabel()` hunk as a follow-up. + +email-validator#182, draft as written: + +> Fair question. The main reason is that pydantic's EmailStr calls +> validate_email for every value, so it runs on every request or record +> with an email field, for example API servers and bulk imports. In the +> mixed benchmark above it takes about 55 µs per address today, so a +> million addresses is roughly a minute of CPU. With this change it's +> about 18 seconds. Nobody filed an issue asking for this, so if the +> extra regex code isn't worth it to you, that's fine. Changes 2-4 are +> the simplest part, and I can cut the PR down to just those. + +The pydantic claim checks out: `EmailStr` calls +`pydantic.networks.validate_email`, which calls +`email_validator.validate_email(email, check_deliverability=False)`, the +same setting as the benchmark. The rest did not survive. We measured each +change on its own for this review: + +| Version | Lines | Time per address (mixed-2000) | +|---|---|---| +| main | | 56 µs | +| change 1 only (skip the IDNA round trip for plain ASCII domains) | +9 | 35 µs (1.63x) | +| changes 2 to 4 | +73 -27 | 38 µs (1.47x) | +| changes 2 to 4 without the module-level caches | +60 -23 | 40 µs (1.41x) | +| whole PR | +83 -27 | 19 µs (2.93x) | + +Round 1: all three FAIL. The offer kept the wrong part: changes 2 to 4 +are the regex code the sentence offers to drop, carry less of the gain +than the 9-line change 1, and include the defaults cache that breaks +documented settings. The facts reviewer found that reassigning +`email_validator.SPECIAL_USE_DOMAIN_NAMES` after the first call is +ignored too, and that the new regexes add about 11 ms to `import +email_validator` (23.1 ms to 33.9 ms median, mostly compiling the +character classes in `syntax.py`; change 1 alone adds nothing). The +18-second figure describes code that cannot ship. "Fair question." and +the closing offer are chat habits, and the reply ran to 99 words. API +servers were the wrong example: 37 µs per request does not show there. + +Round 2 led with pydantic, disclosed the bug, and offered change 1 with +its own number. Maintainer PASS and AI tells PASS. Facts FAIL: "56 µs per +address" was stated as a general cost when it holds for one synthetic +workload, the reply offered the cut without saying the title and +changelog ("~3x", "no behavior change") would be corrected, and the +evidence had no test, differential, or memory result for the change-1-only +version. We ran them: the test suite gives the same result on main and on +change 1 alone, 10,044 differential cases are identical, and memory after +import is flat. + +Round 3, with that evidence in the packet: all three PASS. Final text: + +> pydantic's `EmailStr` calls `validate_email()` for each address it +> validates, so bulk imports pay it once per record: about 56 µs per +> address on our mixed benchmark, or a minute of CPU per million. +> +> This PR has a bug: the cached defaults ignore settings such as +> `email_validator.CHECK_DELIVERABILITY` when they change after the first +> call. I'll cut it down to the 9-line change that skips the IDNA round +> trip for plain ASCII domains, which on its own brings the benchmark to +> about 35 µs per address, and correct the title and changelog to match. + +It cannot be posted as is: it promises a change, so the cut-down branch +(change 1 only, its comment shortened to one line, title and changelog +corrected) goes through the PR pipeline first. The reviewers' minor notes +for whoever signs off: say "my" benchmark rather than "our", consider +stating the saving (about 20 µs per address) instead of today's cost, and +link the pydantic source line. Any of those edits means another packet +and another review. + +## Follow-up + +- Re-check every open PR on a cadence: state, comments, reviews, checks. + Record a lack of replies too: pyyaml#976, dateutil#1609 and + markdown-it-py#449 have been green and uncommented for weeks. +- Update [`PYTHON_SPEEDUPS.md`](PYTHON_SPEEDUPS.md) when state changes: + `being offered`, then PR URL and `offered`, then `merged` or `declined`. + Add `held` with the reason when the panel holds an offer. +- A PR ignored for a month gets its stall date noted in the tracker. Do + not bump the thread, and do not close and reopen it for visibility. +- A merged pure-Python change can change the case for a Rust port; redo + the port's ceiling against the new baseline (fastjsonschema went from + 1.48x pure Python to a 2.44x port, ADR-061). + +## Retrospective: idna#286 and email-validator#182 + +What each reviewer would have said if the panel had existed, and what the +PR should have been. Numbers below are from re-running on 2026-10-29: a +shared Ryzen 9 5950X, CPython 3.12.3, the benchmark script from the #286 +body, and `python -X importtime` over 41 interleaved runs. + +### kjd/idna#286 + +| | master | #286 | #287 | +|---|---|---|---| +| Lines changed | | +11 -2 | +3 -0 | +| mixed-8000 encode+decode | 0.228 s | 0.096 s | 0.110 s | +| ascii-16000 | 0.416 s | 0.087 s | 0.102 s | +| `import idna`, median | 10.0 ms | 11.8 ms | 11.0 ms | +| VmHWM after import | 11.5 MiB | 12.5 MiB | 11.7 MiB | +| `uts46data` loaded by `import idna` | no | yes | no | + +Maintainer reviewer: SHRINK. The import move has no measured gain of its +own (the body says so) and loads `uts46data`, a 237 KB module, into every +process that imports idna, including the many that never use UTS 46 +remapping. The `ulabel()` shortcut is a second branch for about 10% of +the gain. The paragraphs on how it fits with #279 add length and decide +nothing for the maintainer. Smallest version: the `check_label()` early return, which is +what kjd wrote. + +AI-tells reviewer: FAIL. 656 visible words (1,514 with the script) for a +13-line diff. Title tagline "(~2.3x on ordinary hostnames), no behavior +change". A numbered list that explains every hunk; method narration ("best +of 7 passes; each cell's figure is the best of 3 such runs"); two tables; +"It goes with #279 rather than competing with it" (humanizer §1); "I can +attach that script too if it's useful" (§22). Each one is minor alone. +Together they read as generated, and length was the first thing kjd +named. + +Facts reviewer: FAIL. The diff adds a module-level import and the +evidence has no import-time or import-memory measurement for either arm. +"Barely changes the numbers" has no per-hunk table behind it. + +What it should have been: kjd's #287 diff, with a body like this (numbers +from the table above): + +> Title: Skip full validation for plain lowercase LDH labels in +> check_label +> +> Most labels that reach `check_label()` are plain lowercase LDH +> (`mail`, `example`, `com`), and those pass every check after the empty +> test. This returns early for them; `xn--` labels and anything else take +> the full path as before. encode+decode over 8,000 mixed hostnames goes +> from 0.228 s to 0.110 s. The test suite passes. + +That is 53 words. It still lacks a named caller, which the maintainer +reviewer would push on; for idna the honest answer is callers like +email-validator that run every domain through `idna.encode()` and +`idna.decode()`. + +### JoshData/python-email-validator#182 + +Maintainer reviewer: HOLD. The README names registration forms as the +use, where 37 µs per address is invisible, and the body names no other +caller. Four changes, +84 -27 across three files, nine new module-level +regexes, two module-level caches, and no per-change numbers, so nobody +could say which change carried the gain. The defaults cache breaks +`CHECK_DELIVERABILITY` and the other documented settings once the first +address has been validated, and a reassigned `SPECIAL_USE_DOMAIN_NAMES` +is ignored the same way. The pre-offered split ("If (1) reads as too +clever") says the PR is too big. + +AI-tells reviewer: FAIL. Title tagline "(~3x mixed, ~5x valid-ASCII), no +behavior change", repeated as the first line of the body. What / Measured +/ Test proof headings and a table for a small change. Bold test counts. +"Their own gates", which addresses the maintainer in the third person. +"Say the word and I'll attach it". Seven dashes. "Tier-1" in a code +comment, and four comments that start "Fast path:". + +Facts reviewer: FAIL. "No behavior change" is false (the settings test +above). The changelog line says "typical ASCII email addresses" about a +seeded synthetic generator. Import time was not measured although the +diff adds nine regexes compiled at import. Measured now, they add about +11 ms to `import email_validator`, which is more than the whole PR saves +for any process that validates fewer than about 300 addresses. That +covers the registration-form use the README names. + +What it should have been: change 1 alone, led by the pydantic why. + +> Title: Skip the IDNA round trip for plain ASCII domains +> +> pydantic's `EmailStr` calls `validate_email()` (with +> `check_deliverability=False`) for every value it validates, so bulk +> imports run it once per record. For a domain that is plain ASCII with no +> `xn--` label, the `idna.decode()`/`idna.encode()` round trip at the end +> of `validate_email_domain_name()` only lowercases it. This returns the +> lowercased domain directly in that case. On 2,000 generated addresses it +> takes validation from 56 µs to 35 µs per address. + +The test and differential sentence would need a fresh run on that diff; +we have not run it, so it is not written here. + +### The other open PRs + +Not changed by this work. For comparison, the mechanical check on each: + +| PR | Visible words | Diff | Why in the first two sentences | Import time measured | Check | +|---|---|---|---|---|---| +| arrow#1398 | 456 | +106 -5 | partly (every `arrow.get` rebuilds a parser; no caller named) | no (adds `import threading`) | FAIL: length, dashes | +| fastjsonschema#233 | 342 | +42 -15 | no | not applicable (no new imports) | FAIL: length, dashes, title | diff --git a/prompts/upstream_pr/pr_body_template.md b/prompts/upstream_pr/pr_body_template.md new file mode 100644 index 0000000..99abfcf --- /dev/null +++ b/prompts/upstream_pr/pr_body_template.md @@ -0,0 +1,63 @@ +# PR title and body template + +Fill this in, then delete every line in angle brackets. If the project has +a PR template, put it first, filled in, and fit the paragraphs below under +its headings. + +The visible body (everything outside code blocks, tables and `
`) +stays under 200 words; aim for about 100. `scripts/upstream_pr_check.sh` +counts it. + +--- + +Title: + +``` + +``` + +Body: + +```md + + + + + + + + +
+Benchmark + + + +
+``` + + + +## Example + +From kjd/idna#287, which the maintainer wrote for the same +optimization we had proposed: + +> Title: Skip full validation for plain lowercase LDH labels in check_label +> +> Addresses the underlying optimization sought in #279 and #286, i.e. +> short-circuiting assessment if the label is LDH. + +The diff is three lines. An outside contributor needs a little more than +that (the why and one number), but not much more. diff --git a/prompts/upstream_pr/reply_template.md b/prompts/upstream_pr/reply_template.md new file mode 100644 index 0000000..f38940e --- /dev/null +++ b/prompts/upstream_pr/reply_template.md @@ -0,0 +1,48 @@ +# Reply template + +For any comment we post on an upstream PR or issue: answering a question, +replying to a review, closing our own PR, or a follow-up. The visible +reply stays under 120 words; most should be under 60. + +```md + + + + + +``` + +Rules: + +- Quote nothing back at them; they know what they wrote. +- No promise you have not measured ("I can cut it to X" needs the number + for X in `evidence/`). +- If a maintainer found a real problem, say it is a problem and fix it. + Do not explain how it happened. +- When they have already done the work themselves (kjd/idna#287), thank + them in one clause and close. Do not compare the two PRs. +- Closing our own PR needs the same human sign-off as opening one. + +Examples, both passed by all three reviewers (see `docs/UPSTREAM_PRS.md`, +"First use"). On kjd/idna#286, after the maintainer closed it and merged +their own three-line version: + +> Thanks for #287, which gets most of the win in far less code. You're +> right about the import move too. I didn't measure import time, and it +> costs everyone who never touches uts46. + +On JoshData/python-email-validator#182, answering "But why?": + +> pydantic's `EmailStr` calls `validate_email()` for each address it +> validates, so bulk imports pay it once per record: about 56 µs per +> address on our mixed benchmark, or a minute of CPU per million. +> +> This PR has a bug: the cached defaults ignore settings such as +> `email_validator.CHECK_DELIVERABILITY` when they change after the first +> call. I'll cut it down to the 9-line change that skips the IDNA round +> trip for plain ASCII domains, which on its own brings the benchmark to +> about 35 µs per address, and correct the title and changelog to match. diff --git a/prompts/upstream_pr/review_ai_tells.md b/prompts/upstream_pr/review_ai_tells.md new file mode 100644 index 0000000..e5576c5 --- /dev/null +++ b/prompts/upstream_pr/review_ai_tells.md @@ -0,0 +1,70 @@ +# Reviewer: AI tells + +A maintainer will read this text and the code comments in the diff. Find +everything that makes it read as generated, so the writer can fix it +before it is posted. You only flag; you do not rewrite. + +## What you get + +The packet directory given below: `title.txt`, `draft.md` (and +`thread.md` for a reply), `diff.patch`, `checks.txt`. You do not need +`evidence/`. Read nothing outside the packet. + +Never start a shell command with `cd`; use absolute paths. + +## How to review + +1. Load the `humanizer` skill (in Claude Code, the Skill tool with + `humanizer:humanizer`) and use its pattern list as the reference. Do not + apply its rewrite steps; use it to mark tells, citing the pattern number + (for example "§1 not X but Y", "§8 dashes", "§22 chatbot residue"). + If the skill is not available, use the list below. +2. Read the draft once as the maintainer would: someone who gets many + drive-by PRs and has seen a lot of generated ones. Note where you + started to suspect a tool. +3. Check the added lines in `diff.patch` too: comments, docstrings, + changelog entries, test names. + +PR text has its own tells on top of the humanizer list: + +- Length out of proportion to the diff. A three-line change with a page of + text reads as generated, whatever the sentences are like. +- Every hunk explained in a numbered list; every claim proved inline. +- Method narration: how the numbers were produced, best-of-N details, + machine noise caveats, all in the visible body. +- Headings (What / Measured / Test proof) and tables on a small change. +- Bold numbers or bold test counts. +- Title taglines: a speedup multiplier or "no behavior change" in the + title. +- Talking about the maintainer in the third person ("their gates", "their + suite"), which shows the text was written for someone else. +- Pre-offering to split, drop or adjust parts ("if (1) reads as too + clever", "happy to drop it"), and offers to attach more ("say the word", + "I can attach that script too"). +- Words from our own tooling: tier-1, lane, oracle, port, representative + track, showcase, rep, fleet, ADR, or any internal project name. +- Code comments that explain the optimization at length, mention + profiling, or say "fast path" on every branch. +- Em dashes and en dashes in prose. + +Apply the humanizer's rule on strength: §1 to §5 and any PR-specific tell +above count on one sighting; a pattern marked "weak alone" needs company. + +## Verdict + +- `PASS`: nothing a maintainer would notice. Minor items may remain. +- `FAIL`: at least one strong tell, or several weak ones together. + +## Output + +Reply with exactly this shape and nothing else: + +``` +ROLE: ai-tells +PACKET: +VERDICT: PASS | FAIL +FIRST_SUSPICION: +FINDINGS: +1. [blocking|minor] : : +2. ... +``` diff --git a/prompts/upstream_pr/review_facts.md b/prompts/upstream_pr/review_facts.md new file mode 100644 index 0000000..ed71447 --- /dev/null +++ b/prompts/upstream_pr/review_facts.md @@ -0,0 +1,67 @@ +# Reviewer: facts and numbers + +Every claim in the text must trace to evidence in the packet or to a +public source it links. Your job is to check each one. You do not judge +style or whether the change is worth it. + +## What you get + +The packet directory given below: `title.txt`, `draft.md` (and +`thread.md` for a reply), `diff.patch`, `evidence/`, `MANIFEST.md`, and the +upstream checkout named in `MANIFEST.md`. Read nothing else. You may fetch +public URLs to check claims about other projects (for example, that some +library calls a function), and you may read or run code in the upstream +checkout or the evidence directory to confirm a claim. + +Never start a shell command with `cd`; use absolute paths or +`git -C `. + +## How to review + +1. List every claim in the title, draft, and added diff lines (comments, + changelog). A claim is any number, version, commit, file name, test + count, comparison, statement about who calls the code or how often, and + any "no behavior change", "unchanged", "covered", "passes", or "merges + cleanly". +2. For each one, find its source: a file and line in `evidence/`, a line + in `diff.patch`, a file in the upstream checkout, or a public URL. +3. Recompute derived numbers from the raw ones: speedups, per-call times, + percentages, "about a minute". Check rounding does not flatter. +4. Check scope. A number from a synthetic workload cannot be called + "typical". A claim about one track cannot be stated as general. +5. Check absence claims hard. "No behavior change" needs their test suite + on both arms and a differential check that covers error paths. If the + project documents settings users change at runtime (module globals, + config objects), the change must keep them working; look at the README + and the diff for module-level caches of such values. +6. Check consistency. Does the text refer to the right parts of the diff + (for example "changes 2 to 4" really being the ones described)? Does an + offer in a reply match what the evidence says those parts deliver? +7. For a reply, fetch the live thread (for example `gh pr view -R + --comments`) and compare it with `thread.md`. If the state + changed (closed, merged) or new comments arrived, every sentence that + depends on the old state is INCONSISTENT. +8. Check costs. If the diff adds or moves module-level imports or + module-level work, `evidence/` must hold import-time and memory numbers + for both arms. If it is missing, that is a finding, even when the text + makes no claim about it. + +## Verdict + +- `PASS`: every claim is sourced and correct. +- `FAIL`: any claim is unsourced, wrong, overstated, or inconsistent, or a + required cost measurement is missing. + +## Output + +Reply with exactly this shape and nothing else: + +``` +ROLE: facts +PACKET: +VERDICT: PASS | FAIL +CLAIMS: +1. "" : : OK | WRONG | UNSOURCED | OVERSTATED | INCONSISTENT : +2. ... +MISSING_EVIDENCE: +``` diff --git a/prompts/upstream_pr/review_maintainer.md b/prompts/upstream_pr/review_maintainer.md new file mode 100644 index 0000000..28e2c88 --- /dev/null +++ b/prompts/upstream_pr/review_maintainer.md @@ -0,0 +1,93 @@ +# Reviewer: skeptical maintainer + +You maintain the upstream project this packet targets. You did not ask for +this change, you have limited time, and you will live with the code after +the author is gone. Decide whether you would merge it as posted. + +## What you get + +A packet directory (path given below) holding: + +- `MANIFEST.md`: upstream repo, base and head commits, file list. +- `diff.patch`: the change against upstream at the base commit. +- `title.txt` and `draft.md`: the text that would be posted. For a reply, + `thread.md` holds the conversation so far and `draft.md` is the reply. +- `evidence/`: raw measurement and test output. +- `checks.txt`: output of the mechanical checks. +- `upstream/` (or a path in `MANIFEST.md`): a checkout of the project at + the base commit. Read its README, CONTRIBUTING, and `git log` to learn + what the project is for and what merged PRs look like. + +Read nothing outside the packet and the upstream checkout. You may fetch +public URLs that the draft or thread cites. Do not ask the author for +their reasoning; judge what a maintainer would see. + +Never start a shell command with `cd`; use absolute paths or +`git -C `. + +## Questions to answer + +1. Why. After the first two sentences of the draft, do you know who calls + this code, how often, and why it matters to them? Does that match the + use the README names? A speedup that only matters in a benchmark loop + is not a reason to take on code. +2. Size. Is the diff the size you would accept for this gain? For each + hunk, check `evidence/` for what it buys on its own. A hunk with no + measured gain of its own, or one the text says "barely changes the + numbers", should go. Could you get most of the gain in a few lines? + If so, say which lines. +3. Your idiom. Would you have written it this way? Look for new module + globals, caches, regexes, comments that explain the optimization at + length, defensive branches, names or terms the project never uses. +4. Unmeasured costs. Check `evidence/` for each that applies, and treat a + missing one as unmeasured: + - import time and memory after import, if the diff touches module-level + code or imports (`python -X importtime`, VmHWM); + - work moved to import time that users who never call the fast path + now pay for; + - memory growth from caches, and whether they are bounded; + - module-level state that breaks documented configuration (settings + users assign on the module after import), monkeypatching in tests, + or thread safety; + - supported Python versions and platforms; + - error paths: same exception types and messages. +5. Behavior. Is "no behavior change" backed by their suite plus a + differential check that covers error paths and the documented + configuration knobs? +6. Review cost. Would reviewing and maintaining this cost you more time + than it saves your users? + +For a reply draft, also ask: does it answer what the maintainer actually +asked, in the first sentence? Does it promise anything the author has not +measured or cannot deliver? Does it argue where it should concede? + +## Verdict + +Pick one: + +- `PASS`: you would merge (or, for a reply, you would be glad to read it). +- `SHRINK`: worth taking only smaller. Name the hunks or sentences to cut. +- `HOLD`: do not post. Use this when there is no credible why, the gain is + not visible in the use the README names, a cost is unmeasured and + plausibly larger than the gain, or you would rather write it yourself. +- `FAIL`: fixable problems that are not about size (style, missing test, + wrong claim about the project). + +## Output + +Reply with exactly this shape and nothing else: + +``` +ROLE: maintainer +PACKET: +VERDICT: PASS | SHRINK | HOLD | FAIL +WOULD_MERGE: yes | no | only if +FINDINGS: +1. [blocking|minor] : . Fix: . +2. ... +UNMEASURED_COSTS: +SMALLEST_VERSION: +``` + +Blocking findings make the verdict anything but PASS. Keep each finding to +one or two sentences. diff --git a/prompts/upstream_pr/writer.md b/prompts/upstream_pr/writer.md new file mode 100644 index 0000000..96020c1 --- /dev/null +++ b/prompts/upstream_pr/writer.md @@ -0,0 +1,63 @@ +# Writer brief: upstream PR or reply + +You prepare one upstream PR (or one reply on an upstream thread) for +review. You do not post anything. The process is in +`docs/UPSTREAM_PRS.md`; this brief is the part you act on. + +Never start a shell command with `cd`; use absolute paths or +`git -C `. + +## 1. Write the why first + +Before touching the diff, write two sentences: who calls this code, how +often, and what it costs them today. Use the project's README for its +stated use, and find a real caller (a dependent library, an issue, a +framework integration) with a link. If you cannot write these sentences, +stop and report that the offer has no why. Do not continue to a PR. + +## 2. Make the diff small + +- Start from the smallest change that could carry the gain, then add + hunks only when they earn it. +- Measure each hunk on its own (leave one out, or add one at a time) on + the workload that matches the why. Keep a hunk only if it carries at + least about 10% of the total gain or is needed for correctness. Record + the table in `evidence/`. +- Write it the way the maintainer would: their names, their comment + density, no new module state unless it earns its place, no terms from + our own tooling. +- If you would offer to split it, split it now and post the smaller part. + +## 3. Measure the costs + +Put raw output for each of these in `evidence/`, both arms: + +- the speed number on the why workload (fresh processes, min of several + runs, arms interleaved); +- `python -X importtime -c "import "` and memory after import + (VmHWM), if the diff touches imports or module-level code; +- their full test suite, lint and type checks; +- a differential run over generated inputs that compares return values + and exception type and message; +- every documented runtime setting (module globals, config objects) still + takes effect when changed after the first call. + +## 4. Draft the text + +Use `prompts/upstream_pr/pr_body_template.md` or +`prompts/upstream_pr/reply_template.md`. Then run the humanizer skill on +the draft in embedded mode (Skill tool, `humanizer:humanizer`) and diff +the result against your draft: no number, name, command, or claim may +change. + +## 5. Build the packet and hand off + +```sh +scripts/upstream_pr_packet.sh --repo --base \ + --head --evidence --title --body \ + [--thread ] --out +``` + +Put only raw measurements and test output in `evidence/`. Your notes, +plans, earlier drafts, and this conversation stay out: the reviewers must +judge what the maintainer will see. diff --git a/scripts/upstream_pr_check.sh b/scripts/upstream_pr_check.sh new file mode 100755 index 0000000..28758de --- /dev/null +++ b/scripts/upstream_pr_check.sh @@ -0,0 +1,165 @@ +#!/usr/bin/env bash +# upstream_pr_check.sh — mechanical checks on an upstream PR draft or reply. +# +# Runs before the review agents (docs/UPSTREAM_PRS.md, "Mechanical +# check"), so they spend their time on judgment, not on counting words. +# No LLM calls; every rule is a regex or a count. +# +# Usage: +# scripts/upstream_pr_check.sh --body [--title ] \ +# [--diff ] [--kind pr|reply] +# +# Prints one line per finding (FAIL or WARN) and a final RESULT line. +# Exit codes: 0 no FAIL, 1 at least one FAIL, 2 usage/input error. + +set -euo pipefail + +usage() { + echo "usage: $0 --body [--title ] [--diff ] [--kind pr|reply]" >&2 + exit 2 +} + +BODY=""; TITLE=""; DIFF=""; KIND="pr" +while [ $# -gt 0 ]; do + case $1 in + --body) [ $# -ge 2 ] || usage; BODY=$2; shift 2 ;; + --title) [ $# -ge 2 ] || usage; TITLE=$2; shift 2 ;; + --diff) [ $# -ge 2 ] || usage; DIFF=$2; shift 2 ;; + --kind) [ $# -ge 2 ] || usage; KIND=$2; shift 2 ;; + *) echo "unknown argument: $1" >&2; usage ;; + esac +done +[ -n "$BODY" ] || usage +case $KIND in pr|reply) ;; *) usage ;; esac +for f in "$BODY" "$TITLE" "$DIFF"; do + [ -z "$f" ] || [ -f "$f" ] || { echo "not a file: $f" >&2; exit 2; } +done + +python3 -I - "$KIND" "$BODY" "$TITLE" "$DIFF" <<'PY' +import re +import sys + +kind, body_path, title_path, diff_path = sys.argv[1:5] +fails, warns = [], [] + +# Words from our own tooling that must never reach an upstream thread. +INTERNAL = re.compile( + r"\b(rustsmith|firstmate|crewmate|captain|fleet|lanes?|tier[- ]?1|oracle|" + r"rust[- ]port|representative track|showcase track|ADR-?\d*)\b", re.I) +# Humanizer §22 chat residue and the offer habits our old bodies had. +RESIDUE = re.compile( + r"(happy to|let me know|hope this helps|feel free|say the word|" + r"if (it'?s|that'?s) useful|i can attach|great question)", re.I) +# Humanizer §12 stock words (warn only: weak alone). +STOCK = re.compile( + r"\b(additionally|crucial|delve|enhance[sd]?|pivotal|robust|seamless(ly)?|" + r"showcas\w*|leverag\w*|meticulous\w*|comprehensive|significantly)\b", re.I) +# An en dash between digits is a range (1.6–1.7x), not a connector. +DASH = re.compile("—|(? blocks, tables and inline code.""" + out, fence, details = [], False, 0 + for line in text.splitlines(): + s = line.strip() + if s.startswith("```") or s.startswith("~~~"): + fence = not fence + continue + if fence: + continue + details += s.count("") + if details > 0 or "
" in s: + continue + if s.startswith("|"): + continue + out.append(re.sub(r"`[^`]*`", "", line)) + return out + + +body = open(body_path, encoding="utf-8").read() +prose = visible_lines(body) +words = sum(len(l.split()) for l in prose) +limit = 200 if kind == "pr" else 120 +target = 150 if kind == "pr" else 60 +if words > limit: + fails.append(f"visible body is {words} words; limit {limit} for a {kind}") +elif words > target: + warns.append(f"visible body is {words} words; aim under {target}") + +for i, l in enumerate(prose, 1): + for m in INTERNAL.finditer(l): + fails.append(f"internal term {m.group(0)!r} in body: {l.strip()[:80]}") + if DASH.search(l): + fails.append(f"dash in prose (humanizer §8): {l.strip()[:80]}") + for m in RESIDUE.finditer(l): + warns.append(f"chat residue or pre-offer {m.group(0)!r} (humanizer §22): {l.strip()[:80]}") + for m in STOCK.finditer(l): + warns.append(f"stock word {m.group(0)!r} (humanizer §12): {l.strip()[:80]}") + +headings = sum(1 for l in prose if l.lstrip().startswith("#")) +bold = sum(len(re.findall(r"\*\*[^*]+\*\*", l)) for l in prose) +tables = len(re.findall(r"^\s*\|.*\|\s*$\n^\s*\|[\s:|-]+\|\s*$", body, re.M)) +if kind == "reply" and (headings or tables): + fails.append(f"reply has {headings} heading(s) and {tables} table(s); a reply has neither") +if kind == "pr" and headings > 3: + warns.append(f"{headings} headings; a small PR needs none unless the repo template has them") +if kind == "pr" and tables > 1: + warns.append(f"{tables} tables in the body; keep at most one, in
if possible") +if bold > 1: + warns.append(f"{bold} bold spans (humanizer §19)") + +if title_path: + title = open(title_path, encoding="utf-8").read().strip() + if len(title) > 90: + fails.append(f"title is {len(title)} characters; limit 90") + elif len(title) > 72: + warns.append(f"title is {len(title)} characters; aim for 72") + if MULTIPLIER.search(title): + fails.append(f"speedup multiplier in title: {title!r}") + if re.search(r"no behaviou?r change", title, re.I): + fails.append("'no behavior change' in title") + if INTERNAL.search(title): + fails.append(f"internal term in title: {title!r}") + if DASH.search(title): + fails.append(f"dash in title: {title!r}") + +if diff_path: + # On a reply the diff is already posted: report its problems, but they + # are not the reply's to fail. + diff_fails = fails if kind == "pr" else warns + added, removed, files = 0, 0, set() + for line in open(diff_path, encoding="utf-8", errors="replace"): + if line.startswith("+++ b/"): + files.add(line[6:].strip()) + continue + if line.startswith("---") or line.startswith("+++"): + continue + if line.startswith("+"): + added += 1 + text = line[1:] + for m in INTERNAL.finditer(text): + diff_fails.append(f"internal term {m.group(0)!r} in diff: {text.strip()[:80]}") + if re.match(r"(import|from)\s+\S+", text): + warns.append(f"new module-level import; measure import time and memory: {text.strip()[:80]}") + if re.match(r"\s+global\s+\w", text): + warns.append(f"new 'global' (module state); check documented settings still apply after the first call: {text.strip()[:80]}") + if re.match(r"_?[A-Za-z_]\w*\s*(:[^=]+)?=\s*re\.compile", text): + warns.append(f"new module-level regex (import-time compile): {text.strip()[:80]}") + if re.search(r"#.*\b(fast path|hot path|profil\w*|speedup)\b", text, re.I): + warns.append(f"comment narrates the optimization: {text.strip()[:80]}") + elif line.startswith("-"): + removed += 1 + print(f"INFO: diff +{added} -{removed} in {len(files)} file(s)") + if kind == "pr" and added + removed > 0 and words > 15 * (added + removed): + warns.append(f"{words} body words for a {added + removed}-line diff; the text is long for the change") + +print(f"INFO: visible body {words} words ({kind})") +for w in warns: + print(f"WARN: {w}") +for f in fails: + print(f"FAIL: {f}") +print(f"RESULT: {'FAIL' if fails else 'PASS'} ({len(fails)} fail, {len(warns)} warn)") +sys.exit(1 if fails else 0) +PY diff --git a/scripts/upstream_pr_packet.sh b/scripts/upstream_pr_packet.sh new file mode 100755 index 0000000..853a73f --- /dev/null +++ b/scripts/upstream_pr_packet.sh @@ -0,0 +1,107 @@ +#!/usr/bin/env bash +# upstream_pr_packet.sh — build the review packet for an upstream PR or reply. +# +# The packet is the only input the review agents get +# (docs/UPSTREAM_PRS.md, "Review panel"): the diff, the raw evidence, the +# draft text, and for a reply the thread so far. The writer's notes and +# reasoning stay out, so the reviewers judge what the maintainer will see. +# Every file is hashed into a packet id; verdicts quote it, and a verdict +# only counts for the packet it names. +# +# Usage: +# scripts/upstream_pr_packet.sh --repo --evidence \ +# --body --out [--title ] \ +# [--base --head ] [--thread ] +# +# --repo upstream checkout; the packet links it at the base commit +# --base/--head refs in --repo; diff.patch is base...head (needed for a PR) +# --evidence directory of raw measurements and test output only +# --body the draft PR body or reply +# --title the draft PR title (PRs only) +# --thread the thread so far, oldest first (replies only) +# --out packet directory (created; must not exist or be empty) +# +# Exit codes: 0 packet written (checks may still FAIL: see checks.txt), +# 2 usage/input error. + +set -euo pipefail + +usage() { + echo "usage: $0 --repo --evidence --body --out [--title ] [--base --head ] [--thread ]" >&2 + exit 2 +} + +REPO=""; EVIDENCE=""; BODY=""; OUT=""; TITLE=""; BASE=""; HEAD=""; THREAD="" +while [ $# -gt 0 ]; do + case $1 in + --repo) [ $# -ge 2 ] || usage; REPO=$2; shift 2 ;; + --evidence) [ $# -ge 2 ] || usage; EVIDENCE=$2; shift 2 ;; + --body) [ $# -ge 2 ] || usage; BODY=$2; shift 2 ;; + --out) [ $# -ge 2 ] || usage; OUT=$2; shift 2 ;; + --title) [ $# -ge 2 ] || usage; TITLE=$2; shift 2 ;; + --base) [ $# -ge 2 ] || usage; BASE=$2; shift 2 ;; + --head) [ $# -ge 2 ] || usage; HEAD=$2; shift 2 ;; + --thread) [ $# -ge 2 ] || usage; THREAD=$2; shift 2 ;; + *) echo "unknown argument: $1" >&2; usage ;; + esac +done +if [ -z "$REPO" ] || [ -z "$EVIDENCE" ] || [ -z "$BODY" ] || [ -z "$OUT" ]; then usage; fi +if { [ -n "$BASE" ] && [ -z "$HEAD" ]; } || { [ -z "$BASE" ] && [ -n "$HEAD" ]; }; then + echo "--base and --head go together" >&2; exit 2 +fi +if [ -z "$THREAD" ] && [ -z "$BASE" ]; then + echo "a PR packet needs --base and --head; a reply packet needs --thread" >&2; exit 2 +fi +git -C "$REPO" rev-parse --git-dir >/dev/null 2>&1 || { echo "not a git checkout: $REPO" >&2; exit 2; } +[ -d "$EVIDENCE" ] || { echo "not a directory: $EVIDENCE" >&2; exit 2; } +for f in "$BODY" "$TITLE" "$THREAD"; do + [ -z "$f" ] || [ -f "$f" ] || { echo "not a file: $f" >&2; exit 2; } +done +if [ -e "$OUT" ] && [ -n "$(ls -A "$OUT")" ]; then + echo "out-dir exists and is not empty: $OUT" >&2; exit 2 +fi + +KIND="pr"; [ -z "$THREAD" ] || KIND="reply" +SCRIPT_DIR=$(cd "$(dirname "$0")" && pwd) +mkdir -p "$OUT/evidence" +cp "$BODY" "$OUT/draft.md" +[ -z "$TITLE" ] || cp "$TITLE" "$OUT/title.txt" +[ -z "$THREAD" ] || cp "$THREAD" "$OUT/thread.md" +cp -R "$EVIDENCE"/. "$OUT/evidence/" + +BASE_SHA=""; HEAD_SHA="" +if [ -n "$BASE" ]; then + BASE_SHA=$(git -C "$REPO" rev-parse "$BASE^{commit}") + HEAD_SHA=$(git -C "$REPO" rev-parse "$HEAD^{commit}") + git -C "$REPO" diff "$BASE_SHA...$HEAD_SHA" > "$OUT/diff.patch" +fi + +CHECK_ARGS=(--body "$OUT/draft.md" --kind "$KIND") +[ ! -f "$OUT/title.txt" ] || CHECK_ARGS+=(--title "$OUT/title.txt") +[ ! -f "$OUT/diff.patch" ] || CHECK_ARGS+=(--diff "$OUT/diff.patch") +"$SCRIPT_DIR/upstream_pr_check.sh" "${CHECK_ARGS[@]}" > "$OUT/checks.txt" || true + +PACKET_ID=$( (cd "$OUT" && find . -type f ! -name MANIFEST.md -print0 | LC_ALL=C sort -z \ + | xargs -0 sha256sum) | sha256sum | cut -c1-12) + +{ + echo "# Review packet $PACKET_ID" + echo + echo "- kind: $KIND" + echo "- upstream: $(git -C "$REPO" remote get-url origin 2>/dev/null || echo unknown)" + echo "- upstream checkout: $(cd "$REPO" && pwd)" + [ -z "$BASE_SHA" ] || echo "- base: $BASE_SHA" + [ -z "$HEAD_SHA" ] || echo "- head: $HEAD_SHA" + [ -z "$THREAD" ] || echo "- thread captured: $(date -u -r "$THREAD" +%Y-%m-%dT%H:%MZ) (re-fetch before posting)" + echo "- checks: $(tail -n 1 "$OUT/checks.txt")" + echo + echo "Read the upstream checkout for context (README, CONTRIBUTING, git log);" + echo "for a PR, read it as of the base commit (git -C show :)." + echo "Do not read anything outside this packet and that checkout." + echo + echo "## Files" + echo + (cd "$OUT" && find . -type f ! -name MANIFEST.md | LC_ALL=C sort | sed 's|^\./|- |') +} > "$OUT/MANIFEST.md" + +echo "packet $PACKET_ID written to $OUT ($(tail -n 1 "$OUT/checks.txt"))"