diff --git a/AGENTS.md b/AGENTS.md index 3710594..486dc2c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -101,6 +101,74 @@ transform pipeline sitting in front of `api.anthropic.com`. unit / integration tests alone can't prove behavior under real traffic. +## Anti-Bloat Lens (no-directive PRs) + +The global baseline's bloat bar is *"larger than the directive's +requirements justify."* **Community PRs have no directive**, so that bar +has nothing to anchor to and the finding defaults to "None" — including +on a PR adding thousands of lines. Measured 2026-07-31 across eight open +community PRs: every review reported `Bloat: None`, one of them on 6,630 +lines of new production code. + +For a PR with no directive, anchor to **the defect it claims to fix** +instead. Ask: *what is the smallest change that fixes the stated +problem, and how much larger is this?* + +Report these numbers in the review, always, even when the verdict is +"proportionate" — a stated number is checkable, "None" is not: + +- **production LOC** (excluding `test/`), and **test:production ratio** +- **new files, new exports, new env vars, new on-disk paths** +- **comment:code ratio** on the production diff + +Calibration from merged work in this repo. These are all *proportionate* +— use them as the reference for what normal looks like: + +| PR | prod LOC | test:prod | shape | +|---|---|---|---| +| #274 header propagation | 7 code + 19 comment | 23x | a wire-path defect fix | +| #277 supervised stop | 9 code + 10 comment | 9.0x | one branch + a watchdog | +| #261 absolute-form | 23 code + 31 comment | 8.4x | one parser + 3 call sites | + +Two things that table shows, both deliberate: + +- **A high test:prod ratio is a good sign, not bloat.** Do not flag it. +- **Comment lines exceeding code lines is normal here** and is not + bloat when the comment explains *why* — every one of the above + records a defect's mechanism or a non-obvious constraint. Flag + comments that restate the code, not comments that carry history. + +Raise a **blocking** finding when any of these hold: + +- Production LOC is **an order of magnitude** beyond what the stated + defect requires, and the excess is not itself explained in the PR. +- New **abstraction** with fewer than ~3 call sites and no concrete + near-term reuse case — default is to inline. +- **Dead code**: an export, branch, or option with no call site. Verify + by grepping `origin/main` *after* the merge base, not a local branch — + a stale checkout produces false "unused" findings. +- **Defensive handling for cases that cannot occur** given the + surrounding code's invariants. +- New **env var / on-disk path / config key** that is not required by + the fix. Each one is permanent surface area. + +Do **not** flag: test volume, fixtures, comment density explaining +mechanism, or complexity you cannot show is safe to remove. Per the +global rule, never assert a simplification is behavior-preserving when +you have not verified it — say you could not verify instead. + +### Missing Non-Functional Requirements + +A community PR over roughly **300 production LOC** should carry a +`## Non-Functional Requirements` section (same checklist as a +directive), including the required **Load-bearing?** yes/no. If it is +absent, say so as an attention item and ask for it — do not block on it +alone, and do not treat its absence as licence to skip the size +question. Independently assess load-bearing status yourself: an author's +"no" on something touching a wire contract, shared abstraction, or +security-relevant path is a blocking finding, because that +classification decides whether a human reviews before merge. + ## Repo-Specific Labels In addition to the global review-outcome labels (`reviewed-by-codex-agent`, diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md new file mode 100644 index 0000000..4b1f28d --- /dev/null +++ b/CONTRIBUTING.md @@ -0,0 +1,83 @@ +# Contributing + +Thanks for working on this. A proxy that sits in front of everyone's API +traffic has an unusually low tolerance for surprises, so a few things +here are stricter than a typical repo. + +## Using an AI agent? Read this part. + +Most contributions here — ours included — are written with AI assistance. +That is fine and welcome. But agents reliably satisfy the *functional* +requirement and neglect the non-functional ones: size, complexity, new +surface area. A change that works and is four times larger than it needs +to be still costs every future reader. + +If you point an agent at this repo, point it at these files too: + +- **`AGENTS.md`** — what our reviewer checks, including the anti-bloat + lens and how we size a change against the defect it fixes. +- **`CLAUDE.md`** — workflow, labels, and the non-functional checklist. + +Both are committed at the repo root. They are written for our own agents, +so some sections are internal (bot identities, label ownership) — ignore +those. The review standards apply to every PR regardless of who or what +wrote it. + +Two habits that make an agent-written PR much easier to accept: + +- **Have it justify the size.** "What is the smallest change that fixes + this?" is a question worth asking before you open the PR, because it is + the first one we ask. +- **Have it check its own claims.** If the PR body says a function is + only called in two places, or a value is bounded, verify it. We + cite-check load-bearing claims against the source, and a confident + wrong claim costs a review round. + +## What we check + +- **Does it fix what it says it fixes?** Red-first evidence is + persuasive: show the test failing on the merge base and passing at your + head. Several recent PRs did this and it shortened review considerably. +- **Is it proportionate?** See the anti-bloat lens in `AGENTS.md`. A high + test-to-code ratio is a *good* sign — we do not count tests against + you. Comments that explain *why* are likewise welcome. +- **Is it load-bearing?** Anything touching a wire contract, a shared + abstraction, TLS trust, credentials, or the on-disk format needs human + review before merge, not just agent review. Say so in the PR if you + think it applies. +- **Does the test cover the changed path?** Not an adjacent one. + +## PRs over ~300 lines of production code + +Include a `## Non-Functional Requirements` section in the PR body. Short +answers are fine — a line or two each, `n/a` where it genuinely does not +apply: + +- **Size/complexity budget** — roughly how big should this be, and is it? +- **Threat model** — inputs, trust boundaries, what must not leak. This + proxy handles API keys and full request/response bodies; be specific. +- **Maintainability** — new abstractions need a reason (≈3+ call sites or + concrete near-term reuse). Otherwise inline it. +- **Performance/reliability** — only where it applies. +- **Load-bearing?** — required yes/no. + +## Practical notes + +- **Fork PRs do not get CI.** GitHub gates workflow runs on forks behind + maintainer approval, and our review bot cannot clear that gate. We run + the full suite locally at the merge commit instead and report the + count on the PR. Nothing is required from you — just know that a + missing green check is not a problem with your PR. +- **Rebase rather than merge** when your branch goes stale. We merge one + PR at a time and test between merges, so a clean fast-forward matters. +- **We will not push to your branch.** If something needs changing we + will ask. (We got this wrong once and reverted it — your branch is + yours.) +- **Draft means draft.** We will not flip a PR out of draft for you. + +## Reporting a bug you have not fixed + +An issue with a reproduction is worth as much as a PR. Wire captures, +`--output-format json` output, and "works with the proxy bypassed" +comparisons are all especially useful — several of the subtlest bugs +here were diagnosed from exactly that.