diff --git a/.agents/skills/tether-worker/SKILL.md b/.agents/skills/tether-worker/SKILL.md index c8070a46..d8584945 100644 --- a/.agents/skills/tether-worker/SKILL.md +++ b/.agents/skills/tether-worker/SKILL.md @@ -82,7 +82,10 @@ gap — that is what invalidated reviews across three PRs at once under the old **Open the PR as a draft** and get the checks green there. Then follow `AGENTS.md` §Review: an external provider reads the final head, you fix what is serious and defer or drop the rest, and -CodeRabbit with no actionable comments is the last gate before merge. +CodeRabbit with no actionable comments is the last metered gate before merge. If its two-review cap +is spent and no finding is left outstanding, a fresh **posted** Codex review of the final head +closes the gate in its place — §Review carries the conditions, and no maintainer is waited on for +it. Posted, because the closer has to name the head it read; a local CLI run satisfies no leg. You do not have to sit and watch it. A review takes as long as it takes, and a short-lived worker that polls is spending tokens to wait — so **write the state into the PR body before you go**: diff --git a/.agents/skills/tether-worker/agents/openai.yaml b/.agents/skills/tether-worker/agents/openai.yaml index 0cf03094..0a5226d2 100644 --- a/.agents/skills/tether-worker/agents/openai.yaml +++ b/.agents/skills/tether-worker/agents/openai.yaml @@ -1,4 +1,4 @@ interface: display_name: "Tether Worker" short_description: "Claim one accepted issue and open its draft PR onto the review lane" - default_prompt: "Use $tether-worker to claim issue #N with .agents/bin/claim.py, implement it in an isolated worktree, open a DRAFT PR, request the first Codex review, write the lane state into the PR body, and exit. Do not arm auto-merge: that happens at the end of the review lane, after the mandatory CodeRabbit gate." + default_prompt: "Use $tether-worker to claim issue #N with .agents/bin/claim.py, implement it in an isolated worktree, open a DRAFT PR, request the first Codex review, write the lane state into the PR body, and exit. Do not arm auto-merge: that happens at the end of the review lane, once the gate is closed — by a clean CodeRabbit review, or by a posted Codex review of the final head when CodeRabbit's two-review cap is spent. AGENTS.md §Review carries the conditions on that second path and this prompt states none of them, so read them there rather than inferring them from this line." diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index e162efc5..ef15ea28 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -34,10 +34,11 @@ linked work maps to one. The checklist, not CI, enforces those fields. - Risk rationale: - Final head SHA: - Codex — first, on the green diff, and not optional (the draft by default; a ready-opened PR whose reason is recorded is asked there, at the same point in the lane): reviewed, nothing blocking outstanding (quote it) | reviewed, findings answered below | not reviewed (say why). Unmetered, so uncapped -- Greptile: reviewed the final head — quote its verdict, not only the spend (spent N credits; a standard review is 1, a TREX review 3) | skipped — no budget this month | skipped (say why). Balance from ` .agents/bin/greptile_usage.py`, where `` is your lane's interpreter -- **CodeRabbit — the last gate**: no actionable comments (quote the review — permalink, the **full 40-hex** `commit_id` it read **which must be the final head**, its `submitted_at`, its state — **`COMMENTED` or `APPROVED`**, since `DISMISSED` is a verdict withdrawn and `PENDING` is unsubmitted — and the opening of its body, which must show that **`Actionable comments posted:` is ABSENT**: zero is written by that line not being there, and a clean body opens straight onto `🧹 Nitpick comments` or `No actionable comments were generated`. A review of an earlier head does not qualify, a `PENDING` one is not submitted, a `DISMISSED` one is a verdict withdrawn, and a green status check with no review body is **not** the gate) | in flight (status check `pending` — never re-request, it aborts the run) | throttled, retrying after the stated interval *and* a non-pending status check (a wait, not a freeze) | unavailable (freezes the PR) +- Codex closing review — required whenever the CodeRabbit line below records a **spent cap**, whatever heads those two reviews read, **including where one came back clean at a head a permitted non-material push has since moved**: that review's evidence still stands, but no metered provider has named the commit the merge binds, and the cap forbids asking for a third to name it: n/a — a clean CodeRabbit review at the head being merged, with nothing since | **closed the gate** — quote it exactly as the CodeRabbit gate is quoted: permalink or run artifact, the **full 40-hex** head it read **which must be the final head**, when, and what it said. A re-quoted earlier Codex pass is **not** a closing review, since the head that pass read is not the head being merged. It must be **posted by the provider and name the commit it read** — asked with the Codex review command named in `AGENTS.md` §Review, written in prose here because a handle in this template would fire a real review on every PR opened from it. Two shapes both count: a run with findings posts a review whose `commit_id` is the full 40-hex, and a **clean** run posts a comment carrying `Reviewed commit: ` — expand that with `git rev-parse` and record both. A local CLI run posts nothing and cannot close. Anything this read surfaces is disposed of above before it closes +- Greptile: reviewed — quote its verdict, not only the spend (spent N credits; a standard review is 1, a TREX review 3), and name the **full 40-hex** head it read, which need not be the final one: Greptile is asked before CodeRabbit, so a later finding-fix legitimately moves the head past it, and only the provider that *closes* the gate must reach the final head. Buying a second review to make a checkbox true is not a reason to spend a credit | skipped — no budget this month | skipped (say why). Balance from ` .agents/bin/greptile_usage.py`, where `` is your lane's interpreter +- **CodeRabbit — the last metered gate**: no actionable comments (quote the review — permalink, the **full 40-hex** `commit_id` it read **which must be the final head when CodeRabbit is what closes the gate**, its `submitted_at`, its state — **`COMMENTED` or `APPROVED`**, since `DISMISSED` is a verdict withdrawn and `PENDING` is unsubmitted — and the opening of its body, which must show that **`Actionable comments posted:` is ABSENT**: zero is written by that line not being there, and a clean body opens straight onto `🧹 Nitpick comments` or `No actionable comments were generated`. A review of an earlier head does not qualify: where every push since it is non-material its evidence still stands and it is one of the two completed reviews, but it is the Codex closing review above that names the merging head, so that case is recorded as **cap spent, closed by Codex** and not here. A `PENDING` one is not submitted, a `DISMISSED` one is a verdict withdrawn, and a green status check with no review body is **not** the gate) | in flight (status check `pending` — never re-request, it aborts the run) | throttled, retrying after the stated interval *and* a non-pending status check (a wait, not a freeze) | unavailable (freezes the PR) | **cap spent, closed by Codex** — two *completed* reviews stand — each **submitted**, in state `COMMENTED` or `APPROVED`, carrying a body or inline findings of its own; a throttle, quota refusal, failed run, `PENDING` or `DISMISSED` review, bare status check or reply on someone else's thread is none of them; the second was asked only after the first one's findings were **disposed of** (by commits that answer them, or by the replies and resolutions recording a deferral or drop — same `commit_id` is fine, since disposal on the record moves no head); no finding from either is left outstanding, with the thread resolved on each — cleared by being fixed, deferred-and-tracked, dropped sub-floor, or **withdrawn by the provider that raised it**; *outstanding* is the test and those are the known ways of clearing one; **nothing but disposal and the non-material exceptions landed after the commit the second review read** — every hunk since answers something already recorded on this PR **that you were required to address** — a review finding from any provider, a CodeQL or `secret-scan` alert, a condition a human sign-off attached, the closing review's own finding; the test is the change, not who raised it — or is a clean `main` merge / formatting / comment or docstring edit / **ADR renumber-only** (a renumber that also edits a word of the decision is material, not an exception), or is the resolution of a conflict in the `main` merge the contract requires, which admits the reconciliation only — so no new scope reached the merge unread by an **external** provider — metered up to the commit the second review read, the closing review after it, which is the guarantee the conditions buy and not a metered read of everything; neither came back clean **at the head being merged with its evidence still standing** — all three, since a clean review that a later material push re-armed is not a gate that already closed, reading it as "neither was ever clean" would strand the case where review 1 was clean at an earlier head and review 2 then found something, and a clean review whose head a *non-material* push has since moved lands **here** rather than on the line above, because its evidence stands but no metered provider has named the commit the merge binds; and the Codex closing review above is quoted. That closing review is then the `` the merge below binds to - Provider that did not review: none | which, and why — a quota refusal means the provider **did not review**, and never counts as a pass -- Findings: `` serious (fixed) | `` below the floor (deferred to #____, or dropped if this is an agent-layer path — ADR-0064). Dropped is not silent: reply on the thread in the wording `AGENTS.md` §Review gives, and resolve it +- Findings: `` serious (fixed) | `` below the floor (deferred to #____, or dropped if this is an agent-layer path — ADR-0064) | `` **withdrawn by the provider that raised them** — quote the withdrawal and resolve the thread, since a finding retracted by its author is cleared but is not something you fixed, deferred or dropped, and the cap-spent line above tests whether anything is left *outstanding* rather than which of the four cleared it. Dropped is not silent: reply on the thread in the wording `AGENTS.md` §Review gives, and resolve it - Human sign-off: n/a | release/tag/signing | new scientific claim **or citation** (reviewer and evidence) ## Type of change @@ -58,7 +59,7 @@ Confirm before requesting review: - [ ] **Data policy respected** — no raw/private/unlicensed data or large data in ordinary Git; issue-authorized redistributable fixtures carry license and provenance in named small or LFS/gated paths. - [ ] **No secrets committed** — no token, key, credential, or private path in code, tests, logs, or fixtures; `secret-scan` and push protection are green. - [ ] **Code scanning clean** — CodeQL (GitHub code-scanning *default setup*, hence no `codeql.yml` workflow) reports no new alerts on this PR. -- [ ] **Review complete** (`AGENTS.md` §Review) — the diff went green before anything was asked to read it, on a draft by default or on a ready-opened PR whose reason is recorded above, and then **Codex on that green diff before any metered provider**; **every provider the lane reached** has a final-head result recorded above — a quoted verdict, or the reason it produced none — and at least one of them is an external provider that reviewed the **final head**, its verdict quoted with all six of **which provider it was** — its name, never its @-handle, since a mention in the PR body fires the bot — permalink, the **full 40-hex** `commit_id` it read, `submitted_at`, a state of **`COMMENTED` or `APPROVED`**, and **what it actually said**: the submitted review body, or enough of it to establish the verdict, since metadata alone records that a provider ran and not what it found; and **CodeRabbit returned no actionable comments at that head**, asked with the **full-review** command. Neither silence nor a green `CodeRabbit` status check is the gate — both are also what a request that reviewed *nothing* leaves behind. A provider that could not act is recorded above with the reason, and a quota refusal means the provider **did not review**, and never counts as a pass. Serious findings fixed; the rest deferred to one follow-up issue, or dropped without one if this is an agent-layer path (ADR-0064) — dropping still owes the thread the reply `AGENTS.md` §Review words, so the decision is on the record rather than inferred from silence. Every conversation resolved. +- [ ] **Review complete** (`AGENTS.md` §Review) — the diff went green before anything was asked to read it, on a draft by default or on a ready-opened PR whose reason is recorded above, and then **Codex on that green diff before any metered provider**; **every provider the lane reached** has a result recorded above — a quoted verdict at the head it read, or the reason it produced none — and **the provider that closed the gate reviewed the final head** — where that is Codex, quote what it emits and skip the fields that exist only on a review — but the **full 40-hex** head comes from the provider's own posted artifact, in either shape `AGENTS.md` §Review describes — a review's `commit_id`, or a clean run's `Reviewed commit: ` expanded with `git rev-parse`, which is mechanical and redoable and so still the provider's head rather than yours. A head asserted by the author is never acceptable, since it could name a commit the provider never saw. Where the closer is CodeRabbit, its verdict quoted with all six of **which provider it was** — its name, never its @-handle, since a mention in the PR body fires the bot — permalink, the **full 40-hex** `commit_id` it read, `submitted_at`, a state of **`COMMENTED` or `APPROVED`**, and **what it actually said**: the submitted review body, or enough of it to establish the verdict, since metadata alone records that a provider ran and not what it found; and **the gate is closed at that head** — either **CodeRabbit returned no actionable comments** there, asked with the **full-review** command, or its two-review cap is spent under the conditions the CodeRabbit line above sets out and a **fresh posted Codex review of that head closed it in their place**. On the cap-spent path CodeRabbit's two reviews are recorded at **whatever heads they read** — earlier ones where a fix moved the head, the same one where the disposal was a deferral or drop — and it is the Codex closing review that names the final head; requiring CodeRabbit itself to reach the final head there would demand the third review the cap forbids. Neither silence nor a green `CodeRabbit` status check is the gate — both are also what a request that reviewed *nothing* leaves behind. A provider that could not act is recorded above with the reason, and a quota refusal means the provider **did not review**, and never counts as a pass. Serious findings fixed; the rest deferred to one follow-up issue, or dropped without one if this is an agent-layer path (ADR-0064) — dropping still owes the thread the reply `AGENTS.md` §Review words, so the decision is on the record rather than inferred from silence. Every conversation resolved. - [ ] **Provenance stamped** — coordinates / corrections / app-version / parameters written into the `.tether` for any new analysis (NFR-REPRO). - [ ] **New tunables registered in PRD §11.2** (single source of truth), not hardcoded. - [ ] **Scientific/statistical claims carry a citation**; **SPDX `GPL-3.0-or-later`** header on every new source file (`reuse lint` green). diff --git a/.greptile/README.md b/.greptile/README.md index 27ca0151..0fb75c6c 100644 --- a/.greptile/README.md +++ b/.greptile/README.md @@ -98,4 +98,6 @@ reads the diff before any metered provider does. **One *review* in practice**, a reviews per metered provider** in `AGENTS.md` §Review applies here as everywhere, but it is a ceiling rather than a second credit to plan on, so ask again only if the first found something blocking and the seat still has budget. A request that produced no review — a throttle, a quota refusal, a failed run — is not an -ask and spends nothing. CodeRabbit, not Greptile, is the gate. +ask and spends nothing. CodeRabbit, not Greptile, is the metered gate — and when CodeRabbit's own +cap is spent with no finding left outstanding, an unmetered **posted** Codex review closes it — §Review carries the +conditions on that, and this page states none of them. Greptile is never the closer either way. diff --git a/AGENTS.md b/AGENTS.md index ea19372d..8814344f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -154,20 +154,34 @@ validity turns on it being the right test — must satisfy both. ## Review - **You are never the only reviewer of your own diff.** Before merge at least one external provider - must have reviewed the final head and reported what it found. Author-side or local output never - satisfies this, and a green status check with no review body is not a review. Quote the provider - and name the 40-hex head it read in the PR body. + must have reviewed **every substantive change reaching the merge**, and reported what it found. + Author-side or local output never satisfies this, and a green status check with no review body is + not a review. **Quote the provider and name the 40-hex head its artifact records.** Providers say + it in two shapes and both count: a review object carries `commit_id`, while Codex's *clean* result + is a posted comment carrying `Reviewed commit: ` — expand that with `git rev-parse` and + record both, the expansion being mechanical and redoable rather than an assertion of yours. This + applies to **every** leg, the ordinary one included; requiring a `commit_id` field would accept + only the runs that found something. + Normally the head it read *is* the head you merge. Where the cap-spent close below applies, the + final head may differ only by changes the fourth condition there admits, and the closing review is + what confirms nothing else crept in. - **Open as a draft and get it green there.** Every required check runs on a draft, so the diff reaches fully green before anyone is asked to read it. Opening ready is **not forbidden** but is never free: it spends a metered provider on a diff no unmetered one has read, so record the reason in the PR. - **The lane is cheapest provider first, and the order is the point.** On the green diff — the draft by default, or the ready PR whose reason is recorded — **Codex** first, unmetered and so - uncapped, until it surfaces nothing blocking. Then **optionally one Greptile review**, if the + uncapped, until it surfaces nothing blocking. Ask the GitHub bot with an `@codex review` comment; + it posts an artifact naming the head it read, in either shape the first bullet describes. It has + refused for quota + before and may again — that is a wait like any other refusal, and **availability is determined by + asking, never inferred from an earlier refusal**. Then **optionally one Greptile review**, if the seat has budget: a *review*, since a standard one costs a credit and a TREX one three. Then - ready-for-review if it is not already, and **CodeRabbit last**. Codex is not optional: it is what - makes the metered providers affordable, and skipping it is the same spend as opening ready. - Record each leg in the PR. + ready-for-review if it is not already, and **CodeRabbit last** — last of the *metered* providers, + which is the spend the order buys. The unmetered one is not confined to the front and may read + again behind it. Codex is not optional: it is what makes the metered providers affordable, and + skipping it is the same spend as opening ready. Record each leg in the PR, including the closing + read. - **Review evidence survives a non-material push, so answering findings does not restart the gate.** **The non-material list is a set of exceptions and it wins**, so a change touching a material path is still non-material when the change itself is one of them: merging `main` in @@ -175,9 +189,9 @@ validity turns on it being the right test — must satisfy both. is why `docs/agents/adr.md` can say a renumber needs no fresh review even though `docs/adr/**` is a material path. A renumber that also edits a word of the decision is not renumber-only. Otherwise: Executable code, scientific claims, data, schema, locks, CI and release - configuration, and **every file that states a rule** — `AGENTS.md`, `CLAUDE.md`, + configuration, and **every file that states a rule** — `AGENTS.md` **anywhere**, `CLAUDE.md`, `CONTRIBUTING.md`, `docs/PRD.md`, `docs/adr/**`, `.agents/**`, `docs/agents/**`, `.claude/**`, - `.github/pull_request_template.md`, `.greptile/**` — are material, and a material push re-arms + `.github/pull_request_template.md`, `.greptile/**`, and `AGENTS.override.md` anywhere — are material, and a material push re-arms the review. The rule-stating files are on that list for a specific reason: a push that changes what the gate requires must not keep evidence gathered under the old requirement. - **Metered credits are the maintainer's money.** Greptile is 50 credits per seat per month shared @@ -185,12 +199,80 @@ validity turns on it being the right test — must satisfy both. ` .agents/bin/greptile_usage.py` before spending one, and if the seat is empty record *"Greptile: no credits this month"* and move on; exhaustion never blocks. **A quota refusal from any provider means the provider did not review, and never counts as a pass.** Copilot is advisory and satisfies nothing. -- **CodeRabbit is the last gate**: at least one review with no actionable comments, asked with the - **full-review** command (the bare incremental one applies only where automatic reviews are - *paused*; they are *disabled* here, so it reviews nothing and says so in words that read like a - clean pass). Read its commit status before every ask — `pending` means one is running and a second - request destroys it. A fair-use refusal naming a retry time is a **wait**, not unavailability; - **never** accept its usage-based-billing offer, which is the maintainer's spending decision. +- **CodeRabbit is the last metered gate**: at least one review with no actionable comments, asked + with the **full-review** command (the bare incremental one applies only where automatic reviews + are *paused*; they are *disabled* here, so it reviews nothing and says so in words that read like + a clean pass). Read its commit status before every ask — `pending` means one is running and a + second request destroys it. A fair-use refusal naming a retry time is a **wait**, not + unavailability, and the time it names is a floor rather than a guarantee; **never** accept its + usage-based-billing offer, which is the maintainer's spending decision. +- **A spent cap closes on Codex rather than on a maintainer.** When two *completed* CodeRabbit + reviews stand on this PR — **submitted**, in state `COMMENTED` or `APPROVED`, carrying a body or + inline findings of its own; a throttle, quota refusal, failed run, `PENDING` or `DISMISSED` + review, bare status check, or a reply on someone else's thread is none of them — and **no finding + they raised is left outstanding**, with the thread resolved on each, then a **fresh Codex review of the final head** closes the gate in their place. + **It must report nothing actionable, which is not the same as nothing *major*.** Codex's clean + result reads *"Didn't find any major issues"* — a run that found something minor says otherwise + while still sounding benign, so read what it reported. A closing review carrying findings of any + severity is disposed of first and re-read. The close substitutes for a *clean* pass; accepting + "no majors" would make it the discount ADR-0065 forbids. + It must be **posted by the provider and name the commit it read**, in either shape the first + bullet describes; a re-quoted earlier pass is not one, its head not being the one merged. *Nothing + left outstanding* is the test — fixed, deferred-and-tracked, dropped sub-floor and withdrawn by the + provider are the ways of clearing a finding, not the test itself. Anything the closing review + surfaces is cleared the same way before it closes: this is a *substitute for the clean pass*, not a + lower bar. That review is then *the clean review* the merge binding below names. ADR-0065 carries + the reasoning and the drafts that were wrong. +- **The Codex CLI satisfies no leg; it is an author-side tool.** Local output satisfies nothing + (first bullet), so a CLI run finds your own defects before a provider is asked — the same category + as running the tests. Run it freely, it is unmetered; just never record it as the Codex leg. + **On a diff that edits agent instructions, do not run it under them.** The CLI takes them from + the checkout by two routes: it discovers `AGENTS.md`, `AGENTS.override.md` anywhere (which takes + precedence) and `CLAUDE.md`, **and** it injects repository skills from `.agents/skills/**`. A diff + touching either route shapes its own reader, so the trigger is both — a skill-only change edits + none of the three files and still qualifies. Run + `codex review --strict-config -c project_doc_max_bytes=0 -c skills.include_instructions=false --base origin/main` + — two switches because the skills one is separate and defaults to on, and `--strict-config` so a + mistyped key fails loudly rather than leaving a read that reports as isolated and is not. This does + not reach the **posted** bot review, whose loading is not ours to configure; **#451** covers that. +- **Four things shut that close, and each is readable off the pull request rather than out of your + own account of why you did something.** A refusal is **not** a spent cap: it reviewed nothing, so + it is a wait. If either completed review came back clean, its evidence still stands under the + non-material rule above, **and it read the head you are merging**, **that** review is the gate and + none of this applies — all three, since a clean review whose head a non-material push has moved + leaves `--match-head-commit` binding a commit no metered provider named, and that case takes the + ordinary close instead of stranding. The second completed review must have been asked **after the + first one's findings were disposed of** — by commits that answer them, or by the replies and + resolutions recording a deferral or drop. **The test is the disposal, not a new commit**: a review + answered wholly on the record moves no head. Asking twice at one head with nothing answered in + between is one review asked twice and buys the close nothing. + And **nothing but disposal may land after the cap is spent**: everything added after the commit + the second completed review actually *read* — its `commit_id`, never its `submitted_at` — must + **answer something already recorded on this pull request that you were required to address** (a + finding from any provider, a CodeQL or `secret-scan` alert, a condition a human sign-off attached, + the closing review's own), or be one of the non-material exceptions above, or be **the resolution + of a conflict in the `main` merge this contract requires**. The first is a test on the change and + **not on its source** — the examples illustrate it rather than bound it, and ADR-0065 records why + every attempt to bound it by source deadlocked. The conflict resolution is not optional: + §Concurrent GitHub Flow orders you to merge a freshly fetched `origin/main` and resolve it here, + while the non-material list covers that merge only when it is *clean*. Both admissions turn on the + same fact — the change answers something already read, or reconciles two things already read — so + neither lets unread scope through, and a resolution carrying new logic of its own is new scope like + any other. Anchor it + at the commit and not the clock, because a material push landing while that review is still + running is a push it never saw. **The unit is the change, not the commit**: a commit that fixes a + recorded finding *and* carries an unrelated hunk would otherwise smuggle exactly the scope this + shuts out, so every hunk traces to one of the three — a judgement you apply and nothing checks for + you, which ADR-0065 states plainly rather than dressing up as mechanical. New scope pushed after the cap has spent it is scope **no + metered provider will ever read**, so the close is entitled to be a further opinion on a diff + **every substantive part of which some external provider has already read** — metered up to the + commit the second review read, the closing review after it, and no stronger than that (ADR-0065). + New scope therefore shuts the close. Motive is not a test and never becomes one; these four are, + and they are also why spending an ask to reach the close buys nothing — it costs the disposal of + every finding and a further review on top. +- **Clearing the gate is not authority to merge.** Different things; the second is per-PR, explicit + and never inferred. Escalate only when the closing review surfaces something blocking that you may + not resolve inside this item's scope. - **Never write a provider's handle in a comment you do not intend as a request.** A mention fires the bot even inside backticks — a code span is not an escape. Describe the command in prose instead. @@ -201,7 +283,8 @@ validity turns on it being the right test — must satisfy both. `Deferred: … Tracked in #N` and resolve the thread. Fixing a non-serious finding in the PR is scope breach, not diligence. - **On agent-layer paths, a sub-floor finding is dropped rather than tracked.** Those paths are - `.agents/`, `docs/agents/`, `AGENTS.md`, `CLAUDE.md` and the agent test modules. Reply + `.agents/`, `docs/agents/`, `AGENTS.md` and `AGENTS.override.md` anywhere, `CLAUDE.md` and the + agent test modules. Reply `Noted; below the floor on an agent-layer path and not tracked (ADR-0064)` and resolve the thread. This inverts the rule above deliberately and only here, because only here does the output feed back into the input — sixteen agent-layer issues came from that loop in ten days. @@ -210,7 +293,8 @@ validity turns on it being the right test — must satisfy both. and may not originate in a review finding. - **Two completed reviews per METERED provider, then stop.** The cap bounds how many times a provider whose reads cost money or quota is made to *read the diff*, so **Codex is - uncapped** — it is unmetered, and throttling it bought nothing but slower convergence. + uncapped** — it is unmetered, and throttling it bought nothing but slower convergence. It has + refused for quota before and may again; that is a wait, not a cap. Otherwise **a request that produced no review is not one of the two** — a throttle, a quota refusal or a failed run reviewed nothing, which is the same rule — a refusal means the provider did not review — @@ -218,12 +302,13 @@ validity turns on it being the right test — must satisfy both. provider is rate-limiting: both asks spent on refusals and no review obtainable. It does **not** license a third review, and it does not license hammering — **honour the retry interval the refusal names**, and never re-request while the status check reads `pending`, which aborts the - run in flight. If a third pass would be needed, hand the PR to the maintainer with a comment - saying why. Nothing counts this for you; the merged history is auditable. + run in flight. **A spent cap is not a stuck PR**: it opens the Codex close above, so a PR whose + findings are all disposed of and whose threads are all resolved finishes on an unmetered read + rather than on a maintainer. Nothing counts this for you; the merged history is auditable. - **Greptile is one *review* in practice, and a review is not always one credit** — a standard review costs one, a TREX review three, so a second ask is a real spend. Two is the ceiling every - provider shares, not a second credit to plan on, so ask again only if the first found something - blocking and the seat still has budget. + *metered* provider shares, not a second credit to plan on, so ask again only if the first found + something blocking and the seat still has budget. - Human sign-off: releases, tags, signing, any new scientific claim or citation. Nothing else waits. - Merge under explicit per-PR authority, with checks green and threads resolved. Then arm and exit — never wait, never poll: diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e4f4dd52..cce2e30a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -12,7 +12,9 @@ review lane as merge gates**: branch protection on `main` requires green required CI plus a self-review checklist on every PR, while `AGENTS.md` requires a substantive final-head review on one fixed lane, cheapest provider first: **Codex on the green diff until nothing blocking remains, then optionally one metered -Greptile review, then CodeRabbit with no actionable comments before merge.** +Greptile review, then CodeRabbit with no actionable comments before merge — or, when +CodeRabbit's two-review cap is spent and every finding is disposed of, a fresh Codex +review of the final head closing the gate in its place.** Copilot is advisory only and never satisfies a leg. The ruleset still requires zero GitHub approval reviews; load-bearing changes additionally need any qualified human/domain judgment specified in `AGENTS.md`. This scales to required human reviews + `CODEOWNERS` if @@ -259,15 +261,28 @@ Before requesting review / merging, confirm: routes nothing; the round; and a result from every provider the lane reached — **either** a substantive review **or** that provider's own quoted "nothing to review" for the head it read, a Codex 👍 included. - **CodeRabbit with no actionable comments is required**, and that is a verdict a completed - review reached rather than an absence of one: record the review itself — permalink, the - `commit_id` it read, **which must be the final head**, `submitted_at` with a state of - **`COMMENTED` or `APPROVED`** (a `PENDING` review has no `submitted_at` and is not a submitted - one; a `DISMISSED` one is a verdict *withdrawn* and proves nothing), and the - opening of its body. **A review of any earlier head does not qualify, however clean it was** — - answering a finding moves the head, so that review is evidence about a diff this one is no - longer. The clean verdict is written by the `Actionable comments posted:` line being - **absent** rather than reading `0`. Neither silence + **CodeRabbit with no actionable comments is required — or, if its two-review cap is spent + and every finding is disposed of, the Codex review of the final head that closed the gate in + its place** — and that is a verdict a completed + review reached rather than an absence of one: record the review itself — permalink, and the + commit it read, **which must be the final head for whichever review closes the gate**. On the + cap-spent path CodeRabbit's two are recorded at whatever heads they read, and it is the posted + Codex review that closes and names the final one — asked with an `@codex review` comment, since + the closer has to be posted and name the head it read — in either shape `AGENTS.md` §Review + describes — and a local CLI run posts nothing. A `PENDING` review has no + `submitted_at` and is not a submitted one; a `DISMISSED` one is a verdict *withdrawn* and + proves nothing. **The `Actionable comments posted:` convention is CodeRabbit's** — its clean + verdict is written by that line being **absent** rather than reading `0`, and a Codex review + has no such line: what stands in its place is the verdict it posted, quoted, against the commit + it names. + **A review of an earlier head does not close the gate**, whether a material push moved the + head or a non-material one did. It still stands as one of the two completed reviews, which is + exactly what the cap-spent path relies on: each recorded at the head it read, with the Codex + closing review covering the final one. The non-material case is worth naming because it looks + like an exception and is not one — a clean review followed by a formatting commit keeps its + evidence, so the cap is spent with nothing outstanding, and the Codex close is what names the + commit `--match-head-commit` binds. That is the ordinary close doing ordinary work, not a + special path, and it is strictly more work than the clean review it follows. Neither silence nor a green `CodeRabbit` status check is the gate; both are also what a request that reviewed **nothing** leaves behind (see the full-review command below). Greptile is optional, and its absence for want of credits is recorded rather than excused as a review. Blocking @@ -285,7 +300,7 @@ Automated agents are peers, not a hierarchy: each claims one issue, opens one ** PR, gets it reviewed, and **hands off or merges** rather than sitting and polling. There is no coordinator. Auto-merge is armed at the **end** of the lane, and completing the lane is **not by itself authority to arm it** — `AGENTS.md` requires explicit per-PR merge authority, which is a separate grant that no amount of green -checks confers. Arming it on a draft would merge the PR past the mandatory CodeRabbit +checks confers. Arming it on a draft would merge the PR past the mandatory review gate, since that gate is not a required check. The merge is bound to the head the review evidence covers with `gh pr merge N --auto --squash --match-head-commit ` — that guard is what stands in for the merge queue, which needs an organization-owned repository and so is unavailable @@ -313,21 +328,47 @@ checks are green and the diff is declared final. **Every PR walks the same lane, cheapest provider first: Codex on the green diff, uncapped — the draft by default, or the ready PR whose reason is recorded; then optionally one metered Greptile **review** if the seat has budget, a review being one credit as a standard and three as a TREX; then -CodeRabbit with no actionable comments, which is the last gate before merge.** **Open as a draft and get it green there** — every +CodeRabbit with no actionable comments, which is the last metered gate before merge — and +when that cap is spent with every finding disposed of, a fresh posted Codex review of the final head +closes the gate in its place rather than a maintainer doing it — under conditions `AGENTS.md` +§Review sets and this summary does not restate.** **Open as a draft and get it green there** — every required check runs on a draft, so the diff reaches fully green before anyone is asked to read it, and that is what makes the sequence affordable rather than a policy nobody keeps. Opening ready is not forbidden, but it spends a metered provider on a diff no cheap one has seen; the old rationale for allowing it turned on the round counter ADR-0064 retires, so -what remains is simply that it costs more for nothing. Record the reason in the PR. Author-side or local review, and status-only -output, do not satisfy it. **Exhaustion is not incapacity** — a provider with no budget +what remains is simply that it costs more for nothing. Record the reason in the PR. Author-side +review and status-only output do not satisfy it — and *author-side* is about whose judgement it +is, not which machine ran it. The verdict has to be the provider's and **posted on the pull +request**, so GitHub attests that it said so and the artifact names the head the merge +binds; a local run quoted by its author is not that, however faithfully it is quoted. What has to +hold is that **every substantive change reaching the merge was externally reviewed**, +which `AGENTS.md` §Review states operatively and which its fourth condition is what enforces on +the cap-spent path. **Exhaustion is not incapacity** — a provider with no budget left has not reviewed: Greptile out of credits is skippable and never blocks, while CodeRabbit unavailable freezes the PR. **No provider auto-reviews this repository; you have to ask.** CodeRabbit replies -to an unrequested PR with *"Auto reviews are disabled on this repository"*, and Codex -reviews only when you open a PR for review, mark a draft ready, or comment -`@codex review`. A provider that was never asked has not declined — so if you are -waiting on a review, check that a request was actually posted. +to an unrequested PR with *"Auto reviews are disabled on this repository"*. A provider +that was never asked has not declined — so if you are waiting on a review, check that a +request was actually posted. + +**Codex has two delivery paths and they are not interchangeable.** The GitHub bot triggers +on opening a PR for review, marking a draft ready, or an `@codex review` comment, and posts an +artifact naming the head it read — a review object, or a comment when the run is clean (§Review). The CLI runs locally and posts nothing. The bot has refused for +quota before — #427 and #428, 2026-08-07 — and a refusal is not a +review. **The Codex leg is the posted review**, asked with an `@codex review` comment, because +`AGENTS.md` §Review requires a posted artifact naming the head, which no local run produces and says in the same breath +that local output satisfies nothing. + +The CLI is still worth running and satisfies **no leg**: it is an author-side tool for finding +your own defects before a provider is asked, in the same category as running the tests. Use it +freely — it costs nothing and it is uncapped — and do not record it as the Codex leg. When you +run it on a diff that touches `AGENTS.md`, `AGENTS.override.md` (anywhere), `CLAUDE.md` **or +`.agents/skills/**`** — two routes, discovered files and injected skills — add +`--strict-config -c project_doc_max_bytes=0 -c skills.include_instructions=false`, so the branch +does not supply the instructions its own reader follows. **#451** is where the exact set the CLI +loads gets established; until then that trigger over-fires deliberately, which costs the reader +`main`'s contract and is the cheaper mistake. One exception, and it has already cost money: `.greptile/config.json` is read from the pull request's **source branch**, so a branch cut before that file landed still @@ -364,12 +405,13 @@ quoting a trigger while describing it spends a real review. Break the handle, or "the full-review command" instead. Review evidence **survives a non-material push**, so addressing findings does not -restart the gate — merging `main` in cleanly, formatting, comment edits and ADR +restart the gate — merging `main` in cleanly, formatting, comment and docstring edits, and ADR renumbering (renumber-only — touching a word of the decision is not) are all non-material, and that exception list WINS over the material paths below, while executable code, scientific claims, data, -schema, locks, CI/release config and the governance text itself (`AGENTS.md`, +schema, locks, CI/release config and the governance text itself (`AGENTS.md` anywhere, `CLAUDE.md`, this file, `docs/PRD.md`, `docs/adr/**`, `.agents/**`, `docs/agents/**`, -`.claude/**`, `.github/pull_request_template.md`, `.greptile/**`) are material — the +`.claude/**`, `.github/pull_request_template.md`, `.greptile/**`, and `AGENTS.override.md` +anywhere) are material — the list is *every file that states a rule*, because a push changing what the gate requires must not keep evidence gathered under the old requirement. A material push re-arms the review but raises no ceiling: there are **at most two completed reviews per @@ -398,12 +440,16 @@ feature-complete** (ADR-0064) — they take bug and safety fixes only, so a capa change needs a maintainer-opened issue and may never originate in a review finding. Dropping the sub-floor finding without that second rule would still leave a reviewer able to commission new agent machinery through the deferral above. If a **selected** provider reports nothing to review at the -head it read — a deletion, a pure rename, or Codex's 👍 reaction, which is its documented +head it read — a deletion, a pure rename, or Codex's clean result, which its own documentation +calls a 👍 reaction and which on this repository is a posted **comment** reading +`Codex Review: Didn't find any major issues.` with `Reviewed commit: ` on its own line +(measured, 2026-08-12). Either way it is its documented "no suggestions" — that satisfies its leg; quote it. A statement from the author, or from any other commenter, never does. **Exhaustion is not incapacity**: a provider with nothing to say has reviewed, a provider with no budget left has not. Greptile out of credits is skippable and never blocks; **CodeRabbit unavailable freezes the PR**, because it is the -gate. Record which and why — and a quota refusal means the provider **did not review**, and never counts as a pass. +metered gate and an unavailable provider is not a spent cap — nothing reviewed, so nothing +opens the Codex close. Record which and why — and a quota refusal means the provider **did not review**, and never counts as a pass. **Two completed reviews per metered provider, then stop.** The cap bounds how many times a provider whose reads cost money or quota is made to *read the diff*, so **Codex @@ -413,11 +459,52 @@ exactly when the provider is rate-limiting. It does **not** license a third revi does not license hammering: honour the retry interval the refusal names, and never re-request while the status check reads `pending`. **Greptile is one *review* in practice**, and a review is not always one credit — a standard review costs one, a TREX review three. Two is the shared ceiling, not a second review to plan on, so ask -again only if the first found something blocking and the seat still has budget. If a third -pass would be needed, hand the pull -request to the maintainer with a comment saying why. Nothing counts this for you; the +again only if the first found something blocking and the seat still has budget. **A spent cap +is not a stuck PR**: when **no finding those two reviews raised is left outstanding** — cleared by +being fixed, deferred-and-tracked, dropped sub-floor, or **withdrawn by the provider that raised +it**, that last quoted on the thread like any other disposition — and every thread resolved, a fresh posted Codex review of the final head closes the gate in their place, so the +PR finishes on an unmetered read rather than on a maintainer. **`AGENTS.md` §Review sets four +conditions on that and is the operative statement of all of them**; the other two are above — +a provider that refused reviewed nothing, so a spent cap is not what an unavailable CodeRabbit +leaves behind, and a review that came back clean with its evidence still standing **and that +read the head being merged** is already the gate. That last qualifier is not decoration: where +a permitted non-material push has since moved the head, no metered provider has named the +commit `--match-head-commit` binds, so the case takes the ordinary Codex close rather than +resting on the earlier review. The two restated here are the two a summary most easily drops, and dropping either +would authorise the close in a case the contract shuts, which a contributor reading the two +documents together would have to stop over. **The cap has to have been spent on two reads with a disposition between +them**: the second review must have been asked only *after* the first one's findings were +disposed of, by commits that answer them or by the replies and resolutions recording a +deferral or a drop. The test is the disposal and **not** a differing diff — a review answered +wholly on the record moves no head, so a second review at that same commit is in order, and +demanding a changed one would re-create the deadlock this removes for every PR whose findings +were all deferred or dropped. What buys the close nothing is asking twice at one head with +nothing answered in between: that is one review asked twice. **And nothing but disposal may land after the cap is spent**: +every hunk added after the commit the second completed review actually read — its +`commit_id`, never its `submitted_at`, since a material push landing while that review is +still running is a push it never saw — must answer a finding **already recorded on the pull +request that you were required to address** (a review finding from any provider, a CodeQL or +`secret-scan` alert, a condition a human sign-off attached, the closing review's own finding — +illustrations, not the rule, which is a test on the change and not on its source), or be one +of the non-material exceptions above, **or be the resolution of a +conflict in the `main` merge the contract requires** — that merge is non-material only when +it is *clean*, so a conflicted one is a material push you were ordered to make, and shutting +the close on it would strand any PR that `main` happened to touch. Only the reconciliation is +admitted; a resolution carrying new logic of its own is new scope like any other. The +provider-neutral wording is load-bearing rather than loose: whoever raised a serious finding, +you are obliged to fix it, so any list of *whose* findings count shuts the close against the +fix it just required — and every list drafted here has omitted somebody, first the closing +read and then Greptile. What follows such a fix is another stamped read of the head it +produced. New scope pushed past that point is scope no metered provider will ever read, and +the close is a further opinion on a diff every substantive part of which **some external provider** +has already read — never a first opinion on an unread one. Not *twice*-read, and not all of it +*metered*: a fix answering review 2 lands after that review's `commit_id` by design, as do a +permitted conflict resolution and anything the closing review raises, and the closing review is the +only provider that reads those. Metered up to the second review's commit, the closing review after +it, external throughout — which is what `AGENTS.md` §Review's first bullet asks for and no more. +Nothing counts this for you; the merged history is auditable and you are trusted with it. On agent-layer paths -(`.agents/`, `docs/agents/`, `AGENTS.md`, `CLAUDE.md` and the agent test modules) a +(`.agents/`, `docs/agents/`, `AGENTS.md` and `AGENTS.override.md` anywhere, `CLAUDE.md` and the agent test modules) a finding below the severity floor is **dropped rather than tracked**, because there the follow-up issue becomes another agent-layer pull request and the loop feeds itself (ADR-0064). Dropped is not silent: reply on the thread in the wording `AGENTS.md` diff --git a/docs/PRD.md b/docs/PRD.md index 8ae57caa..d1a6161f 100644 --- a/docs/PRD.md +++ b/docs/PRD.md @@ -1062,7 +1062,15 @@ has not — until it surfaces nothing blocking; it is the free provider, and throttling it bought nothing but slower convergence. Then **optionally one Greptile review**, if the seat has budget — a *review*, not a credit, since a standard one costs one credit and a TREX one three; then ready-for-review if it is not already; then **CodeRabbit with no actionable comments, -which is the last gate before merge**. The round +which is the last metered gate before merge — and when that two-review cap is spent, a fresh posted Codex review +of the final head closes the gate in its place, so a reviewed PR finishes on an unmetered read rather +than on a maintainer (ADR-0065)**. **`AGENTS.md` §Review is the operative statement of the conditions +that close is subject to, and this document deliberately restates none of them** — `CONTRIBUTING.md` +and the pull-request template do carry working copies, as detail and as evidence fields, and both +defer to §Review where they differ. Four drafts of this +paragraph tried to summarise them and each admitted or refused a case the contract does not — a +restatement that is wrong in either direction is worse than a pointer, because `AGENTS.md` tells a +worker to stop when these two files disagree, so the summary strands the pull request either way. The round ledger that used to count this is gone — ADR-0064 retired it along with the launcher that consumed it, leaving **at most two completed reviews per metered provider** as a convention a worker keeps rather than a counter that publishes labels — **Codex is @@ -1077,11 +1085,15 @@ no single one was sufficient — the lane keeps that property while spending the works in, billed per **completed review** — one credit for a standard review, **three for a TREX** one; Copilot is budgeted the same way and is **advisory only** — it never satisfies a leg, and a quota refusal from it means the provider **did not review**, never a pass. Exhaustion and incapacity differ: Greptile out of credits is skippable, CodeRabbit unavailable **freezes the PR** — though a fair-use refusal that names a retry time -is a **wait**, not unavailability, and a request that produced no review is not one of the two completed reviews. -Author-side/local review and a green or status-only -result do not satisfy the gate. **No provider auto-reviews this repository** — CodeRabbit reports auto reviews -disabled, Greptile is held by `.greptile/config.json`'s `skipReview: "AUTOMATIC"`, and Codex fires only on -open-for-review, draft-ready, or an `@codex review` comment — so a provider that was not asked has not declined. The +is a **wait**, not unavailability — the interval it names being a floor rather than a guarantee — and the retry is a fresh provider request that consumes no completed-review slot, since a request that produced no review is not one of the two. +Author-side review and a green or status-only +result do not satisfy the gate; *author-side* names whose judgement it is rather than which machine ran it, and what +the gate protects is that **every substantive change reaching the merge was externally reviewed** — `AGENTS.md` +§Review is the operative statement of that and of what makes it hold when the reviewed commit is not the merged one. **No provider auto-reviews a pull request opened under the current configuration** — CodeRabbit reports auto reviews +disabled, Greptile is held by `.greptile/config.json`'s `skipReview: "AUTOMATIC"`, and the GitHub Codex bot fires only on +open-for-review, draft-ready, or an `@codex review` comment — so a provider that was not asked has not declined. That +bot posts an artifact naming the head it read, which every leg requires — §Review gives the two shapes; the CLI runs locally, posts nothing and therefore +satisfies no leg — it is an author-side tool, useful before a provider is asked and recorded as none. The one exception is a branch cut **before** that config landed: it is read from the PR's source branch, so such a branch still auto-fires Greptile on open. The credit is spent regardless, so the review is answered and the optional Greptile step recorded as spent, never discarded as unsolicited. @@ -1089,11 +1101,14 @@ step recorded as spent, never discarded as unsolicited. Review evidence **survives a non-material push**, so responding to findings does not restart the gate: merging or rebasing `main` in without conflict resolution, formatting, comment/docstring edits and ADR renumbering are non-material — and those exceptions WIN over the material paths, which is what lets an ADR renumber-only change keep its review evidence — while executable code, scientific claims, data, schema, locks, CI/release configuration and the -governance text itself (`AGENTS.md`, `CLAUDE.md`, `CONTRIBUTING.md`, this document, `docs/adr/**`, +governance text itself (`AGENTS.md` anywhere, `CLAUDE.md`, `CONTRIBUTING.md`, this document, `docs/adr/**`, `.agents/**`, `docs/agents/**`, `.claude/**`, `.github/pull_request_template.md` and -`.greptile/**`) are material — the list is *every file that states a rule*, because a push that +`.greptile/**`, and `AGENTS.override.md` anywhere) are material — the list is *every file that states a rule*, because a push that changes what the gate requires must not keep evidence gathered under the old requirement. A material push re-arms the review, and a PR gets **at most two completed reviews per metered provider**, Codex being unmetered and uncapped — -needing a third means the issue was scoped too large, and the lane stops for the maintainer rather than continuing. +needing a third usually means the issue was scoped too large. The lane does **not** stop for the maintainer: when +**CodeRabbit's** two-review cap specifically is spent — not any other provider's, since Greptile is optional and its +exhaustion never blocks — a fresh posted Codex review of the final head closes the gate instead (ADR-0065), subject to +conditions `AGENTS.md` §Review states and this document, again deliberately, does not. That bound is a **convention a worker keeps, not a counter that publishes labels.** ADR-0064 retired the round ledger, the `agent:round-*` / `agent:review-capped` / `agent:gate-blocked` labels and the launcher that consumed them, after @@ -1120,7 +1135,8 @@ exist. When a **selected** provider reports that a change has nothing to review including Codex's 👍 reaction, its documented form of "no suggestions" — that statement satisfies its leg, quoted and never substituted by the author or any other commenter. **Exhaustion is not incapacity**: a provider with no budget left has not reviewed. Greptile out of credits is skippable and never blocks; **CodeRabbit unavailable freezes the -PR**, because it is the last gate and nothing merges past it. **Throttled is not unavailable**: CodeRabbit's fair-use +PR**, because it is the last metered gate and an unavailable provider is not a spent cap — it reviewed +nothing, so nothing opens the unmetered close. **Throttled is not unavailable**: CodeRabbit's fair-use limit is adaptive, and a refusal that names when the next included review is due is a wait — wait it and ask again, which costs no round and no request, and never accept the usage-based-billing offer that accompanies it, since that is the maintainer's spending decision. The elapsed interval is necessary but **not sufficient**: the `CodeRabbit` commit @@ -1168,8 +1184,11 @@ the ADR-0052 run was a lease that only a sleeping human could renew. Every agent is a peer: it claims one issue, works one isolated worktree/branch/PR, opens the review lane on a draft, and hands off. Auto-merge — bound to the reviewed head with `--match-head-commit` — is armed at the **end** of that -lane by whoever completes it, never on the draft, since the mandatory CodeRabbit gate is not a required check and -nothing else would hold the merge. No agent waits on another, and no agent merges on another's +lane, never on the draft, since the mandatory review gate — CodeRabbit, or the Codex close +when its cap is spent (ADR-0065) — is not a required check and nothing else would hold the merge. Completing the lane +is **not** what authorises the arming: clearing the gate and having authority to merge are separate, the second is +per-PR and explicit and is never inferred from the first, and a worker who finishes the lane without it records that +the gate is satisfied and hands off. No agent waits on another, and no agent merges on another's behalf. ADR-0052's coordinator, leases, run records and guarded-merge monopoly are retired, not merely superseded. **Label taxonomy** (prefixed namespaces, so labels group and filter cleanly): diff --git a/docs/adr/0064-the-agent-layer-coordinates-writers-not-reviews.md b/docs/adr/0064-the-agent-layer-coordinates-writers-not-reviews.md index 2273b0b4..89ac457a 100644 --- a/docs/adr/0064-the-agent-layer-coordinates-writers-not-reviews.md +++ b/docs/adr/0064-the-agent-layer-coordinates-writers-not-reviews.md @@ -5,7 +5,7 @@ SPDX-License-Identifier: GPL-3.0-or-later # 0064 — The agent layer coordinates writers, not reviews -- **Status:** accepted; supersedes [ADR-0062](0062-draft-first-review-lane-with-metered-providers.md) and [ADR-0063](0063-review-evidence-is-read-not-inferred.md), and the review gate, round cap, launcher and advisory scope guard of [ADR-0057](0057-github-native-swarm-coordination.md) +- **Status:** accepted; supersedes [ADR-0062](0062-draft-first-review-lane-with-metered-providers.md) and [ADR-0063](0063-review-evidence-is-read-not-inferred.md), and the review gate, round cap, launcher and advisory scope guard of [ADR-0057](0057-github-native-swarm-coordination.md); its review-gate terminus is in turn superseded by [ADR-0065](0065-a-spent-metered-cap-closes-on-the-unmetered-provider.md), the rest governing unchanged - **Date:** 2026-08-07 - **Deciders:** bioedca - **PRD anchor:** §12 (development & version-control protocol) diff --git a/docs/adr/0065-a-spent-metered-cap-closes-on-the-unmetered-provider.md b/docs/adr/0065-a-spent-metered-cap-closes-on-the-unmetered-provider.md new file mode 100644 index 00000000..fac0cd71 --- /dev/null +++ b/docs/adr/0065-a-spent-metered-cap-closes-on-the-unmetered-provider.md @@ -0,0 +1,484 @@ + + +# 0065 — A spent metered cap closes on the unmetered provider, not on a maintainer + +- **Status:** accepted; supersedes the review-gate terminus of [ADR-0064](0064-the-agent-layer-coordinates-writers-not-reviews.md), whose other decisions govern unchanged +- **Date:** 2026-08-10 +- **Deciders:** bioedca +- **PRD anchor:** §12 (development & version-control protocol) +- **Milestone:** M11 - Agent-swarm infrastructure + +## Context and problem statement + +> **What of ADR-0064 survives.** Everything except one clause. Its four coordination controls, its +> drop rule, its feature-complete boundary and its removals **govern unchanged**; this record +> supersedes only the fourth of the four review rules it kept — *"one CodeRabbit review with no +> actionable comments is the last gate before merge"* (ADR-0064:311–312). That distinction is stated +> here rather than compressed into the `Status` bullet because `scripts/gen_adr_index.py` extracts +> that field with a single-line pattern and copies it verbatim into the index. + +Three rules in `AGENTS.md` §Review were individually sound and jointly unsatisfiable: + +| line | rule | +|---|---| +| `:188` | CodeRabbit is the last gate — **at least one review with no actionable comments** | +| `:211` | **Two completed reviews per METERED provider**, then stop | +| `:221` | If a third pass would be needed, **hand the PR to the maintainer** | + +If both permitted metered reviews found anything at all, the zero-finding review the gate demanded +required a third read, which the cap forbade. The PR was then stuck behind a human indefinitely, +with nothing wrong with it. + +### The rule priced diligence as failure + +This is the defect, and it is sharper than the deadlock it produced. The old gate bundled two +different questions into one test: + +1. *Were the findings dealt with?* +2. *Did a provider read the head being merged?* + +and then assigned both to the **most expensive** provider in the lane. The arithmetic that falls out +runs backwards. A review that finds three real problems costs two credits to close — one to find +them, one to confirm the fixes — while a review that finds nothing costs one. A pull request was +charged for having been reviewed usefully, and the charge was paid in the currency ADR-0064 was +most careful about: the maintainer's metered budget. + +That inversion is why the deadlock is not an edge case. It is reached by exactly the PRs the lane is +working correctly on. + +### It was reached + +[PR #434](https://github.com/bioedca/tether/pull/434) is the first pull request to arrive there, on +2026-08-10, and the record is complete because the state was documented as it happened: + +- 15/15 required checks green, `mergeStateStatus: CLEAN`, every commit signed. +- Codex reviewed five times and converged with nothing blocking outstanding. +- CodeRabbit review 1 at `454eebf` — `Actionable comments posted: 2`. Both fixed. On the replies + CodeRabbit **confirmed one and withdrew the other** in its own words: *"I withdraw the finding."* +- CodeRabbit review 2 at `359675e` — `Actionable comments posted: 1`, labelled 🟡 Minor. Fixed in + `02fb081`, a **three-line comment edit** — squarely on the non-material exception list. +- All three review threads resolved. Nothing outstanding. + +Two completed metered reviews, every finding disposed of, and the pull request could not merge. + +### Two things already anticipated this + +**`claim.py doctor` detects the class and cannot remedy it.** Its `unarmed` section reports *"open +pull requests that are finished and that nothing will merge"* — precisely this state — but `doctor` +reports and never writes, by design and with a test asserting it issues no write verb. Detection +without an agent-reachable remedy is what produces a stranded pull request rather than a fixed one. + +**ADR-0064 booked the loss.** Its §Consequences lists *"automatic resumption of a stranded pull +request"* among what the cut gave up, on the evidence that the empty `refs/lane-advances/` namespace +showed it had never once occurred. That evidence was true when written. It is no longer. + +### The section already handled the same situation correctly one provider over + +`AGENTS.md:183–187` disposes of an exhausted **metered** provider in words: *"if the seat is empty +record 'Greptile: no credits this month' and move on; **exhaustion never blocks**."* A spent +CodeRabbit cap is the same situation and blocked. This record makes CodeRabbit consistent with +Greptile rather than inventing a principle for it. + +## Decision drivers + +- **The unmetered provider is already trusted and already uncapped.** `AGENTS.md:165` calls Codex + *"unmetered and so uncapped"* and `:168` calls it *"not optional: it is what makes the metered + providers affordable."* Nothing new is being extended to it; it is being allowed to finish. +- **The safety property must not move.** *"You are never the only reviewer of your own diff"* must + hold identically after the change. +- **No new machinery.** ADR-0064 established that review state has no concurrency content and needs + no ledger, label state machine or launcher. The fix has to be prose an agent evaluates. +- **The branch must not become the preferred path.** An agent must not be able to reach a cheaper + route by deliberately spending the cap. + +## Considered options + +1. **Keep the escalation.** Rejected: it is the human gate this record exists to remove, and #434 + shows it fires on well-reviewed work rather than on risky work. +2. **Raise the cap to three.** Rejected: it moves the deadlock rather than removing it, and spends + more of the budget ADR-0064 was protecting. A fourth review would be demanded next. +3. **Reframe the gate for every PR** as *"every finding disposed"* rather than *"the review found + nothing."* Rejected, though it is the most intellectually appealing of the three. It swaps a + third-party attestation for a self-attestation on the **default** path, where the deadlock is + rare; it collides with `CONTRIBUTING.md`'s rule that a credit-funded review must not be + answerable entirely by deferral; and since *serious* is the provider's own labelling, it would + let every 🟡 Minor exit through deferral with no re-read. A rare deadlock does not justify + loosening the common case. +4. **A narrow branch that opens only on a spent cap.** Adopted. + +## Decision outcome + +**When the metered cap is genuinely spent and every finding is genuinely disposed of, a fresh Codex +review of the final head closes the gate in the metered provider's place. The maintainer escalation is +deleted rather than supplemented.** + +### What makes it narrow + +Four conditions shut the branch, and each is **readable off the pull request** rather than out of +an agent's account of its own reasoning: + +1. **A refusal is not a spent cap.** A throttle, a quota refusal or a failed run reviewed nothing, + so it is a wait, and waiting is still what you do. This also means an *unavailable* CodeRabbit + still freezes the pull request — nothing reviewed, so nothing opens the close. +2. **A clean review is the gate, and it has already closed** — when it read the head being merged. + If either completed review came back clean, its evidence still stands under the non-material + rule, **and** it named the merging commit, the branch never opens. + + All three conditions, and the third exposes a tension older than this record. A clean review at + commit A closes the gate; a permitted non-material push to B leaves that closure intact — + `AGENTS.md` says review evidence survives such a push — while `--match-head-commit` still demands + a head no metered provider has named, and the cap forbids asking for a third to name it. Stated + without the third condition, this branch shut there, and a clean review followed by a formatting + commit **stranded the pull request outright**. + + Four review rounds were spent trying to fix that with a second mechanism — a *rebinding* read + that supplied a head without closing anything — and each round found a new defect in it: the + pull-request template had no state for it, `CONTRIBUTING.md` contradicted it, a rebinding read + that found something serious deadlocked, and the arming rule still sourced the SHA from "the + clean review". The mechanism was the problem. Narrowing this condition instead deletes it: the + case simply takes the **ordinary close**, since the cap is genuinely spent and a fresh posted Codex review + of the final head closes and names it under every condition here. That is strictly *more* work + than the clean review it follows, never less, so widening the branch this way opens nothing — + and the rule that must not bend is untouched: the close may never stand in for a metered read + that never happened. +3. **The second review must have been asked after the first one's findings were disposed of** — by + commits that answer them, or, where the disposition is a deferral or a sub-floor drop, by the + replies and resolutions that record it. Asking twice at one head with nothing answered in + between is one review asked twice. + + **The test is the disposal, not a new commit**, and an earlier draft got this wrong in a way + worth recording. It required the two reviews to sit at *different `commit_id`s*. Codex's review + of this record's own pull request found the hole: when a review's findings are all non-serious, + the prescribed disposition is deferral or a sub-floor drop, which moves **no head**. A second + review at that same head would then have been locked out of the close while the cap forbade a + third — **the deadlock re-created by the rule written to remove it**. A rule that fixes a + deadlock must be checked against its own failure mode, and this one was not until a provider + checked it. + +4. **Nothing but disposal may land after the cap is spent.** Every change added after **the commit + the second completed review actually read** — its `commit_id`, never its `submitted_at` — must + answer something already recorded on the pull request **that the worker was required to address** + — a review finding from any provider, a CodeQL or `secret-scan` alert, a condition a human + sign-off attached, a closing review's own finding — or be one of the existing non-material + exceptions, or be the resolution of a conflict in the `main` merge this contract requires. + Anchored at the reviewed commit and applied per change, both for the reasons below, and stated as + a test on the change rather than on its source for the reason after those. + + This is the condition that makes the coverage claim below true rather than merely asserted, and + it was missing from the first two drafts. A second Codex review + of this record's own pull request found it: a material push **after** the cap is spent leaves the + cap spent, so the close still applied — and the new code would then be read by the closing + provider and by nobody else. Material pushes re-arm review but do **not** raise the two-review + ceiling, so there was no path by which a metered provider could ever see that scope. It also made + the branch reachable by choice, which conditions 1–3 were written to prevent: push the risky part + last. A safety property that holds only when the author does not think to break it is not a + safety property. + + The same condition also has to be read **per change, not per commit**. A commit that answers a + recorded finding and carries an unrelated hunk alongside it satisfies any per-commit phrasing + while smuggling in exactly the scope the condition excludes. + + It also has to be anchored at the commit the second review **read**, not at the clock. An earlier + phrasing here said *"between the second completed review and the closing review"*, which sounds + equivalent and is not: a material push landing while that review is still running is after its + `commit_id` but before it completed, so a clock-anchored window waves through the one change the + provider demonstrably never saw. This record is a rule-stating file and was accepted carrying the + weaker wording, which is its own small lesson — an ADR can drift from the contract it records. + + And the set is stated as a **test on the change rather than on its source**, because every + version that named sources omitted one. Four drafts, four omissions, one failure mode: + + 1. only the two CodeRabbit reviews' findings — which shut the close against any closing review that + found something; + 2. plus the closing review's own — which still omitted **Greptile**, whose findings a worker is + equally obliged to fix; + 3. plus *any provider* — which still omitted **CodeQL and `secret-scan` alerts, and conditions a + human sign-off attaches**, none of which come from a review provider and all of which are + mandatory; + 4. and finally the test that has no source dimension at all. + + Each omission produced the identical deadlock — the fix was compulsory, it answered nobody on the + list, the cap forbade another metered read — and each was found only by the next review round, + never by the drafting. That is the general lesson and it is worth more than the rule: **an + enumeration inside a safety condition is a latent deadlock**, because the condition fails closed + and the enumeration is always incomplete. The line the condition is actually drawing is between + work you were *obliged* to do and scope you *chose* to add, and that is what it should say. + + And the set has to admit **the resolution of a conflict in the required `main` merge**. This + contract obliges a worker to merge a freshly fetched `origin/main` before merging; the + non-material list covers that merge only when it is *clean*; so a conflicted one is a material + push the contract itself ordered. Excluding it strands any pull request that `main` happened to + touch after the cap was spent — a deadlock triggered entirely by other people's merges. The + reconciliation is admitted because both sides were already read, yours by the metered reviews and + `main`'s on its own pull request; a resolution carrying new logic of its own is new scope like any + other and shuts the close. + + And the allowed set has to include **the closing review's own findings**, which the first three + drafts of this condition did not. A sixth Codex review of this record's pull request found it, + and it is the third time a draft here re-created the deadlock it removes: the bullet above + requires the closing review to dispose of whatever it surfaces *before* it closes, and that fix is + a material push answering no CodeRabbit finding — so a set holding only *their* findings shut the + close against every closing review that found anything, while the cap forbade asking the metered + provider again. The branch was reachable only by the closing reviews doing their job, which is the + worst possible selection. The remedy is not a narrower set but another stamped read of the head + the fix produced: each round is still read by the provider that closes it, and unread scope stays + excluded, because a closing review cannot raise a finding about a hunk it never saw. + +### The closing review must carry the head it closes + +The SHA that reaches `--match-head-commit` must not be one the author asserted after the fact. This +is a fifth condition in substance, and it took three review rounds pulling in different directions +to land, which is the part worth recording. + +Round one found that demanding `commit_id` / `submitted_at` / `COMMENTED` from the closer made the +path **unsatisfiable in the ordinary clean case**: Codex's clean result is often a bare 👍 carrying +no commit, and a reaction has none of those fields. Round two found that accepting the reaction made +the head **author-asserted** — a push landing while the read is in flight would let a pull request +name a head the provider never saw, which is precisely what binding the merge exists to prevent. The +draft that came out of those two required an artifact *the provider itself stamps with the commit*, +and said the gate stays shut until one exists. + +Round three concluded that no such artifact existed, on the evidence that the GitHub Codex bot's only +two appearances in this repository — #427 and #428, both 2026-08-07 — were usage-limit refusals, and +that the CLI's rollout record carries `cwd`, `cli_version` and a session id but **not the commit it +read**. From that it built a *procedural pin*: `git rev-parse HEAD` before and after the run, equal +to the PR head at arming time, standing in for an attestation nothing could give. + +**Round seven then measured what the clean result actually is**, and it is neither of the things +five rounds had argued about. Every Codex run *with findings* on this pull request posted a **review +object** carrying a full 40-hex `commit_id`. The clean run at `ef797b2` posted **no review object at +all** — an issue comment, reading `Codex Review: Didn't find any major issues.` with +`Reviewed commit: ef797b2336` on its own line. So the closer's evidence is provider-written and does +name the commit, but a rule demanding a `commit_id` *field* accepts only the finding case and +strands the clean one, which is the case the close exists for. The rule now asks for **a posted +artifact that names the commit it read**, which both shapes satisfy, with the short SHA expanded by +`git rev-parse` — mechanical, redoable by anyone, and therefore still not an author's assertion. + +The *"bare 👍 carrying no commit"* that rounds one and two argued over is Codex's own documented +description of its no-suggestions result. It is not what this repository observes, and five rounds +of rule-writing rested on it without anyone looking. + +**The earlier detour was wrong too, and the way it was wrong is the more useful record.** The refusals were five days +stale, and no one had asked the bot on this pull request. When it finally was asked, it **posted a +review in nine minutes**, carrying `commit_id c26a683b843bf12361d6dbcabe4dbdadfe103bc3` — the +provider-attested head the whole detour existed to substitute for. The pin is deleted, and the +closing review is a **posted** one. + +Three things had to be true at once for that error to survive as long as it did: an observation was +turned into a rule (*the bot declines*), the rule was **load-bearing** (it forced the close onto the +weaker path and pushed a rewrite of `AGENTS.md:156`, which #439 explicitly put out of scope), and it +was never re-tested, because it explained the evidence well enough that re-testing felt unnecessary. +**Availability is determined by asking.** A provider that refused last week has not declined today, +and the cost of finding out is one comment. + +### A CLI read must not run under the rules the branch is proposing + +**The close itself is a posted review, so it is not the case this guards.** What it guards is the +ordinary lane read: the CLI discovers `AGENTS.md`, `AGENTS.override.md` anywhere and `CLAUDE.md` from the +checkout it runs in and injects repository skills, so a pull request editing any of them supplies +the instructions to a provider reading it — the branch graded by its own unmerged contract, which +*"only agent instructions on the default branch govern; unmerged edits are inert"* refuses in the +first paragraph of that file. The **posted** review's loading is not ours to configure at all, and +**#451** covers that half. + +So on a diff touching `AGENTS.md`, **`AGENTS.override.md` anywhere**, `CLAUDE.md` **or +`.agents/skills/**`** — the two routes by which the checkout reaches the model, discovered files and +injected skills, the override included because it takes precedence and the skills because a +skill-only diff edits none of the three files — a CLI read runs +`codex review --strict-config -c project_doc_max_bytes=0 -c skills.include_instructions=false --base origin/main`. +The overrides do different jobs and all are load-bearing: `project_doc_max_bytes` turns off the +`AGENTS.md` family, `skills.include_instructions` turns off repository **skills**, which are injected +through a switch of their own that defaults to on — so the first draft of this command left a +branch-modified `SKILL.md` model-visible through a read that reported as isolated — and +`--strict-config` makes a mistyped key **fail** rather than be ignored, which matters because the +failure mode of a silently-dropped override is a read that looks isolated and is not. Both were +verified against the installed CLI (0.147.0) — a deliberately bogus key is rejected under +`--strict-config`, and `project_doc_max_bytes` is accepted. + +**That trigger over-approximates on purpose, and the four attempts it took to get there are the +reason.** Each named a set; each was wrong, in both directions; each was caught only by the next +review round: + +1. *any rule-stating file* — too wide: a `CONTRIBUTING.md`- or template-only diff was never at risk, + and the flag costs the reviewer `main`'s contract for nothing; +2. *`AGENTS.md` or `CLAUDE.md`* — too narrow by one name: the CLI also discovers + **`AGENTS.override.md`** and gives it *precedence*, so a pull request adding one kept the exact + hole the rule closes while appearing to satisfy it; +3. *those three* — still too narrow: the CLI injects repository **skill** metadata from + `.agents/skills/**`, so a changed skill description can pull an unmerged `SKILL.md` into the + review without touching any of the three; +4. *and `CLAUDE.md` may not belong at all*, since Codex reportedly does not load it as a project + document when a root `AGENTS.md` is present. + +What every attempt had in common is that **the contract was asserting how Codex resolves +instructions**, and reading filename literals out of a binary does not establish that — precedence, +fallback and skill injection are behaviour, not strings, and they move between versions. So the +trigger stopped being a claim about Codex and became a **policy choice**: fire on the agent-layer +paths, accept the over-approximation, and record why. **#451** is where the real set gets +established, by observing what a review actually receives rather than by inference. + +This is the same correction condition 4 needed when it enumerated sources, and the disposition list +needed when it enumerated dispositions — **inside a safety condition, a list is a hole or a deadlock +waiting for the next case** — with one addition this instance makes plain: when the true list is a +fact about someone else's tool, do not encode a guess at it. Over-approximate and say so. + +**The switch is blunt.** It is all-or-nothing: +turning discovery off denies the reviewer `main`'s contract as well as the branch's, so the read is +less informed than an ordinary one. Over-firing therefore costs review *quality* on a narrow class +of pull requests, while under-firing leaves a *self-grading* path open — which is why the +over-approximation is the right way round rather than merely the safe-sounding one. There is no +narrower switch available: `--base` selects the diff and does not substitute +`origin/main`'s copy of the instructions, so "review under the default-branch contract" is not +something the CLI can be asked for. + +This is worth stating plainly: **every review round on this record's own pull request ran without +that isolation**, in a worktree carrying the modified `AGENTS.md`. Those rounds were adversarial +throughout and found twenty-odd defects in the text feeding them, so there is no sign it mattered +here — but "no sign it mattered" is not the property the trust boundary asks for, and the last round +of this pull request was re-run with the flag. + +### What condition 4 does not buy + +It is applied by the worker, and **nothing checks it mechanically.** Every hunk after the cap has to +trace to a recorded finding, a non-material exception, or the required `main` merge's conflict +resolution — and whether a given hunk really does is a judgement, made by the same worker the +condition constrains. A determined author can present unrelated logic as finding disposal and the +closing review would be the first and only read of it. + +That is stated rather than closed, for two reasons. A machine check is **out of scope by +construction**: #439 rules out restoring the machinery ADR-0064 removed, and ADR-0064's own finding +was that review state has no concurrency content and needs no ledger. And the alternative on offer +is not a stronger check but a *different* unchecked rule, since any mapping from hunk to finding is +prose either way. + +What the condition does buy is narrower and real: it is **per hunk rather than per commit**, so +bundling an unrelated change into a fix commit does not launder it; it is anchored at a commit rather +than a clock, so a push mid-review cannot slip through; and it puts the claim on the record, where +the merged history is auditable. Against a careless worker it works. Against a deliberate one it is a +statement of intent, and this record does not pretend otherwise — the same honesty the procedural-pin +section above required before that mechanism was deleted. + +**Motive is deliberately not a test.** An earlier draft closed the gaming path with *"an ask made to +spend the cap is not one of the two."* That was rejected on review for two reasons: a motive is not +checkable by anyone, including the agent itself; and read strictly it **restores the deadlock in a +new form**, since an agent that suspects its own motive concludes the cap is unspent, so the close is +shut, so it must ask again — which it cannot, because two asks have been made. A rule that fixes a +deadlock must not be able to re-create one. + +The remaining deterrent is structural rather than moral: spending an ask to reach the close **buys +nothing even if it works**, because the close costs the disposal of every finding *plus* a further +review on top. It is strictly more work than the clean pass it replaces. + +### The close is a substitute, not a discount + +Anything the closing review surfaces is cleared the same way — **left not outstanding**, by being +fixed, deferred-and-tracked, dropped sub-floor, or withdrawn by the provider that raised it — before +it closes. That last one was missing from three drafts, and it is not hypothetical: a provider +retracting a false positive is how #434's own record reads, so the close was shut on its own +motivating example unless a worker mislabelled the disposition. The test is *nothing left open*; +the four are the known ways of getting there, the same way condition 4 tests the change rather than +naming who may raise a finding. An earlier draft held the closing review +to *"nothing blocking"*, which would have silently dropped two severity bands relative to the +zero-actionable-comments bar it replaces, and on agent-layer paths those findings are not tracked at +all. The bar does not move; only who holds it does. + +### Clearing the gate is still not authority to merge + +Stated as its own rule rather than as a tail clause, because it is the most damaging available +misreading. `AGENTS.md:32–33` — *"Do not infer merge authority"* — is untouched. + +### What the merge binds to + +`--match-head-commit` still names *"the 40-hex head the clean review read."* When the close applies, +the Codex closing review **is** that clean review. The merge bullet is left byte-identical: it is the +only place in `AGENTS.md` satisfying both mechanical guards in +`tests/test_agent_contract_is_runnable.py` — the `_SHA_DEFINED` window and the arming-page floor — +and its referent was always provider-neutral in words. Only context had pointed it at CodeRabbit. + +## Consequences + +**Good.** A pull request that has been reviewed, has had its findings fixed and its threads resolved +can finish without a human. The escalation is deleted from all **three** files that carried it — +`AGENTS.md`, `CONTRIBUTING.md` and +[`docs/PRD.md`](https://github.com/bioedca/tether/blob/main/docs/PRD.md) §12.4. An earlier draft of +this record said *two*, and the third was found by review rather than by the sweep that looked for +it: the PRD phrases it as *"the lane stops for the maintainer"* rather than *"hand the PR to the +maintainer"*, so a +phrase-matched search missed it. **A rule stated in five files is found by reading all five, not by +grepping the wording you happen to remember** — and this record is the third place in this pull +request where a fix landed in one file and not its mirrors. Deleting it **repairs** `AGENTS.md:227`, *"Human sign-off: releases, +tags, signing, any new scientific claim or citation. Nothing else waits,"* which was false while a +capped review gate waited on a human. That sentence becoming true again is affirmative evidence the +escalation was the anomaly rather than the design. + +**Bad, and named rather than minimised.** The terminal verdict on a capped PR now comes from the +provider the repository does **not** pay for, and Codex's reliability is therefore load-bearing in a +way it was not before. Two things bound that. It is a fresh read of the exact head being merged, not +a re-quoted earlier pass. And it is reached only after two completed metered reviews, with condition +4 above refusing the close to any scope that landed after the cap was spent — so **every substantive +part of the merging diff has had at least one external read**, metered up to the commit the second +review read and the closing review after it, and the close is a further opinion on it rather than a +first opinion on an unread one. + +This paragraph has now been wrong twice in the same direction, which is worth keeping visible because +it *is* the argument that the close is safe. It first said *"a third opinion on a twice-read diff"*, +and a review of this record's own pull request showed it false: if review 1 comes back clean and an +in-scope +material push then draws review 2, the added part carries **one** metered read, not two. The +correction then claimed one *metered* read of every substantive part, and the next round showed that +false too: a fix answering review 2 lands after that review's `commit_id` by design, as do a +permitted conflict resolution and anything the closing review raises, and no metered provider ever sees +them. What the conditions actually buy is **external** coverage throughout — which is what §Review's +first bullet asks for, and it is worth noticing that the honest version of this claim turned out to +be exactly the property that bullet already states, rather than something stronger the close was +smuggling in. An argument that overstates its own premise is worth less than the weaker true one, +and this one took two rounds of being caught to stop doing it. **If Codex's review quality degrades, this paragraph is the part of the record that +stops holding**, and nothing in this repository would detect that. + +A second cost, and it is the larger of the two: **§Review grows from 80 lines to 163** — it doubles — in a file ADR-0064 deliberately shrank, and whose resident-context driver, *"`AGENTS.md` is +read on every model call by every agent"*, argues against every addition. An earlier draft of this +paragraph said *"roughly twenty lines"*, and a later one said 152; both were measured once and then left to rot as the section kept changing. The figures above are for the merge base and for the revision this record ships with — a count stated without the revision it was taken at is the same defect as an ADR citing a line number. The +understatement is recorded rather than quietly corrected, because a decision record that +under-reports its own cost is how a cost stops being weighed. + +**§Review is now at its practical ceiling, and that is a finding rather than a footnote.** Answering +CodeRabbit's third review pushed `AGENTS.md` to 30,120 bytes — over the guard — and the fix was not +to raise the budget but to cut ~430 bytes of rationale out of §Review and into this record. It now +sits within a handful of bytes of the limit. The next rule added to that section has to displace +something, and the honest reading is that **§Review has no room left for prose that is not +operative**. That is the byte guard doing exactly what it was added for, one commit after it was +added, on the author who added it. + +What the length buys is the four shutting conditions and the stamped-head rule, which cannot be +compressed without turning the branch into a judgment call. What it also carries is the *reasoning* +behind each condition — most of it added one review round at a time, as each draft was shown to +re-create the deadlock it removed. That reasoning belongs here, in a file nobody loads on every call, +and a later pass moving it out of §Review would be a straightforward win. It is not attempted in this +pull request: the same review rounds that produced the prose also showed that large edits to this +text reliably introduce new defects, and a compression pass is exactly such an edit. It is named as +follow-up work rather than left for someone to notice. + +**Reversible.** Prose only. No script, no workflow, no label, no ref namespace, no test fixture. A +`git revert` restores the escalation exactly. + +## Adoption status + +Landed with this record. The six surfaces that restated the retired rule move in the same pull +request, because `AGENTS.md:177–182` puts every rule-stating file on the material list precisely so +that a push changing what the gate requires cannot leave a stale copy behind: +`.agents/skills/tether-worker/SKILL.md`, `.agents/skills/tether-worker/agents/openai.yaml`, +`CONTRIBUTING.md`, `.github/pull_request_template.md`, +[`docs/PRD.md`](https://github.com/bioedca/tether/blob/main/docs/PRD.md) §12 and +`.greptile/README.md`. + +`openai.yaml` is called out because it is the likeliest omission and the most consequential one: +four lines, read by no test, and injected as the Codex lane's **default prompt**, so a stale copy +there briefs every future Codex-lane worker under a contract that no longer exists. + +**This pull request cannot use the branch it introduces.** `AGENTS.md:6` says only instructions on +the default branch govern and unmerged edits are inert, and every file it touches is material, so it +re-arms its own review and is judged under the old gate. That is the correct order and not an +oversight: the rule earns its way in under the regime it replaces. diff --git a/docs/adr/README.md b/docs/adr/README.md index e9cefa88..8c1df737 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -103,7 +103,8 @@ else would make it a worse historical record in exchange for a working link. | [0061](0061-transport-failures-and-the-strict-x509-opt-out.md) | A transport failure is not a scope verdict, and strict X.509 conformance is opt-out | accepted; reverses a non-goal of [#315](https://github.com/bioedca/tether/issues/315) | §12.2–§12.5 | | [0062](0062-draft-first-review-lane-with-metered-providers.md) | Draft-first review lane: spend the free provider before the metered ones | superseded by [ADR-0064](0064-the-agent-layer-coordinates-writers-not-reviews.md) | §12 | | [0063](0063-review-evidence-is-read-not-inferred.md) | Review evidence is read from the payload, not inferred from its shape | superseded by [ADR-0064](0064-the-agent-layer-coordinates-writers-not-reviews.md); its measurements stand as evidence | §12 | -| [0064](0064-the-agent-layer-coordinates-writers-not-reviews.md) | The agent layer coordinates writers, not reviews | accepted; supersedes [ADR-0062](0062-draft-first-review-lane-with-metered-providers.md) and [ADR-0063](0063-review-evidence-is-read-not-inferred.md), and the review gate, round cap, launcher and advisory scope guard of [ADR-0057](0057-github-native-swarm-coordination.md) | §12 (development & version-control protocol) | +| [0064](0064-the-agent-layer-coordinates-writers-not-reviews.md) | The agent layer coordinates writers, not reviews | accepted; supersedes [ADR-0062](0062-draft-first-review-lane-with-metered-providers.md) and [ADR-0063](0063-review-evidence-is-read-not-inferred.md), and the review gate, round cap, launcher and advisory scope guard of [ADR-0057](0057-github-native-swarm-coordination.md); its review-gate terminus is in turn superseded by [ADR-0065](0065-a-spent-metered-cap-closes-on-the-unmetered-provider.md), the rest governing unchanged | §12 (development & version-control protocol) | +| [0065](0065-a-spent-metered-cap-closes-on-the-unmetered-provider.md) | A spent metered cap closes on the unmetered provider, not on a maintainer | accepted; supersedes the review-gate terminus of [ADR-0064](0064-the-agent-layer-coordinates-writers-not-reviews.md), whose other decisions govern unchanged | §12 (development & version-control protocol) | diff --git a/tests/test_agent_contract_is_runnable.py b/tests/test_agent_contract_is_runnable.py index fbf89f25..818bd583 100644 --- a/tests/test_agent_contract_is_runnable.py +++ b/tests/test_agent_contract_is_runnable.py @@ -847,3 +847,39 @@ def test_a_page_that_arms_the_merge_says_what_sha_to_supply() -> None: "reaches the merge command as a literal and the binding guard is a guess: " f"{undefined}. State that it is the 40-hex head the clean review read." ) + + +# --- AGENTS.md has to fit in the window the tools that read it actually load --- +# A contract nobody finishes reading is worse than a shorter one, and the failure is SILENT: the +# reader gets a prefix and no error. Codex's CLI loads project documents up to a default cap of +# 32,768 bytes and simply stops there, mid-word. PR #441 grew this file from 21,361 to 35,178 bytes +# and cut the load off inside the cluster bullet, so every Codex worker lost the rest of §WSL +# clusters and Slurm, the whole of §Handoff and cleanup, and the whole of §This machine -- including +# `TETHER_ALLOW_NONSTRICT_X509`, without which `claim.py` cannot reach the GitHub API on the +# maintainer's machine at all. Nothing in the repository would have reported that. +# +# The bound is the tool's, not ours, so this guard keeps real headroom under it rather than sitting +# on the line: a later version could lower the cap, and a file at 32,700 bytes would start +# truncating on an unrelated commit. +_PROJECT_DOC_CAP_BYTES = 32_768 +_HEADROOM_BYTES = 3_072 + + +def test_the_contract_fits_inside_the_project_document_cap() -> None: + """`AGENTS.md` stays far enough under the loader's cap that its tail is never dropped. + + Asserted on **bytes**, matching how the cap is applied -- a character count would pass while a + file full of the em dashes and typographic quotes this contract uses truncated anyway. + """ + size = len((_REPO / "AGENTS.md").read_bytes()) + budget = _PROJECT_DOC_CAP_BYTES - _HEADROOM_BYTES + assert size <= budget, ( + f"AGENTS.md is {size:,} bytes and the working budget is {budget:,} " + f"({_PROJECT_DOC_CAP_BYTES:,}-byte loader cap minus {_HEADROOM_BYTES:,} of headroom). " + "This is the working budget, not the cap itself -- the file may still be under the " + f"{_PROJECT_DOC_CAP_BYTES:,}-byte loader limit and this still fails, deliberately, so the " + "headroom is not spent. Past the CAP the tail is dropped silently and workers lose " + "whatever sections sit at the end -- Handoff and This machine, including the TLS " + "workaround. Move " + "rationale into the ADR that records the decision and leave the operative rule here." + )