feat(agents): a spent metered cap closes on Codex, not on a maintainer - #441
Draft
bioedca wants to merge 7 commits into
Draft
feat(agents): a spent metered cap closes on Codex, not on a maintainer#441bioedca wants to merge 7 commits into
bioedca wants to merge 7 commits into
Conversation
AGENTS.md held three individually-sound rules that were jointly unsatisfiable: the gate needed a CodeRabbit review with no actionable comments, metered providers were capped at two completed reviews, and a third pass meant handing the PR to a human. A PR whose two permitted reviews both found something -- even something small and immediately fixed -- could never reach the zero-finding review the gate demanded. PR #434 is the first to arrive there. The deeper defect is that the old gate bundled "were the findings dealt with?" and "did a provider read the merging head?" into one test and billed both to the most expensive provider, so a review finding three real problems cost two credits to close while one finding nothing cost one. The contract priced diligence as failure. When the cap is spent and every finding is fixed, deferred-and-tracked or dropped sub-floor with threads resolved, a fresh Codex read of the final head now closes the gate in CodeRabbit's place. Three conditions shut that branch, each readable off the PR rather than out of an agent's account of its own motives: a refusal is not a spent cap, a clean review is already the gate, and the two completed reviews must sit at different commit_ids with the commits between them answering the first one's findings. The maintainer escalation is deleted from both files that carried it, which repairs AGENTS.md's "Nothing else waits" -- false while a capped review gate waited on a human. Clearing the gate is still not authority to merge, stated as its own rule. The merge bullet is byte-identical, preserving both mechanical guards in test_agent_contract_is_runnable.py. All six drift surfaces move together, since every rule-stating file is material. Closes #439. ADR-0065.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Codex raised two P1 findings against the first draft, both correct and both falsifying this PR's central claim, so both are fixed rather than deferred. 1. The different-commit_id condition re-created the deadlock. It required the two completed reviews to sit at different commit_ids with commits between them answering the first one's findings. But 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 was then locked out of the close while the cap forbade a third. The test is now the disposal itself -- commits that answer the findings, or the replies and resolutions that record a deferral or drop -- not a new commit. 2. The final-head requirement was unsatisfiable on the capped path. On that path CodeRabbit's two reviews sit at earlier heads by construction, because answering them is what moved the head; demanding every reached provider cover the final head therefore demanded the third review the cap forbids. Only the provider that closes the gate must read the final head. Finding 2 was reported against one line of the PR template. It was a sample of a class: the same requirement appears three more times, in the template's CodeRabbit line and twice in CONTRIBUTING.md -- one of which asserted outright that "answering a finding moves the head", the exact premise finding 1 falsifies. All four are fixed. ADR-0065 records the corrected condition and, deliberately, the draft that was wrong: a rule that fixes a deadlock must be checked against its own failure mode.
Codex round 2 raised two more P1s, both correct and both fixed. 1. The fallback could be used for material scope neither metered review saw. A material push AFTER the second completed review leaves the cap spent, so the close still applied -- and that new code would then be read by the closing provider and by nobody else, since material pushes re-arm review but do not raise the two-review ceiling. It also made the branch reachable by choice, which the other conditions exist to prevent: push the risky part last. A fourth condition now shuts it -- every commit between the second completed review and the closing read must answer a recorded finding or be a non-material exception. This is what makes ADR-0065's "third opinion on a twice-read diff" true rather than merely asserted. 2. The PR template still carried the different-commit_id condition that the previous commit retired from AGENTS.md, so the mandatory template rejected the valid cap-spent path -- the same deadlock, one file over. Fixed there, and the "earlier heads by construction" phrasing I introduced in the template and CONTRIBUTING.md is wrong for the same reason: where disposal is a deferral or drop, the heads legitimately match. The second finding is the third time in this PR that a fix landed in one file and not its mirrors. Swept all five rule-stating files; the only surviving mention of the retired condition is ADR-0065's deliberate record of the draft that was wrong.
…clock Codex round 3, three more P1s, all correct. 1. The scope freeze was time-anchored. "After the second completed review" let a material push that landed while that review was still running fall inside the window -- before its submitted_at, after the commit_id it actually read -- so a commit no metered provider saw could still ride the close. Anchored at the commit_id now, with the reason stated inline. 2. The evidence shape was unsatisfiable in the ordinary clean case. The template demanded commit_id, submitted_at and COMMENTED/APPROVED from whichever provider closed the gate, but a clean Codex run reports a thumbs-up reaction and has no pull-request-review object at all, so the new path could not be recorded exactly when it succeeded cleanly. It now asks for what Codex can emit while keeping the final-head binding. 3. There was a THIRD copy of the maintainer escalation, in docs/PRD.md §12.4. The PR body, the previous commit message and ADR-0065 all claimed it lived in exactly two files. That was wrong. The PRD phrases it "the lane stops for the maintainer" rather than "hand the PR to the maintainer", so the phrase-matched grep never saw it -- and since AGENTS.md tells agents to stop and ask when contracts conflict, a worker reading the PRD would have stranded on exactly the PR this unblocks. The sweep is now semantic across all seven rule-stating files rather than a grep for remembered wording, and ADR-0065 records why the grep missed it. That is the fourth time in this PR a fix landed in one file and not its mirrors.
Codex round 4, two P1s and a P2, all correct. 1. A clean Codex run is often a bare thumbs-up carrying no commit, so the head recorded for it was AUTHOR-supplied, not provider-attested. A push landing while the read was in flight could then have the PR name a head the provider never saw and pass it to --match-head-commit -- the exact thing binding the merge exists to prevent. The closing read must now carry a SHA the provider itself wrote; an unstamped reaction is a fine lane result and is not a close. This is the resolution of a genuine tension between two rounds. Round 3 found that demanding commit_id/submitted_at/COMMENTED made the path unsatisfiable in the ordinary clean case; round 4 found that accepting a bare reaction made the head unattested. Both are right, and the answer is not a compromise: a reaction is a lane result, not a close. 2. The scope freeze was per-commit, so a commit that answered a recorded finding AND carried an unrelated hunk passed the literal test while smuggling in exactly the scope the condition excludes. The unit is now the change. 3. The template's cap-spent option said "nothing but disposal", which contradicts AGENTS.md still allowing the non-material exceptions after the cap -- a clean main merge or a comment edit would have re-stranded an eligible PR. The checklist now carries the exceptions too. ADR-0065 records the fifth condition and, deliberately, the round-3/round-4 tension, because the resolution is the reusable part.
…dicted
Codex round 5, three P1s and a P3. All four are contradictions the round-4 fix
introduced: the attestation rule landed in one place and three others still said
the old thing. Fifth instance of that pattern in this PR.
1. AGENTS.md still required the closing read to be "posted by the provider on
the pull request" one sentence above the bullet that permits a stamped run
artifact, so the artifact alternative was rejected by the text introducing it.
2. The template's cap-spent option required "neither came back clean", which is
stronger than the canonical condition ("neither came back clean AND its
evidence still stands"). Where review 1 was clean at an earlier head, a
material push re-armed it, and review 2 then found something, the option
became unselectable with no third read available -- the deadlock again.
3. CONTRIBUTING.md applied submitted_at and COMMENTED/APPROVED to whichever
review closes the gate, including a Codex close, which is exactly the
unsatisfiability ADR-0065 records. Those fields are CodeRabbit's; a stamped
artifact records what it actually emits.
4. The ADR said "three conditions" in two places while the normative list has
four, understating the safeguard that keeps post-cap scope out. Counted as
sub-floor on an agent-layer path, but fixed rather than dropped because it
miscounts safety conditions in the record that defines them.
…ger names Codex round 6, one P1 and one P2, both correct. 1. The clean-review condition stranded a PR it was written to leave alone. Closing the gate and supplying a bindable head are two different jobs, and only the first was handled. A clean review at commit A closes the gate; a PERMITTED non-material push to B leaves that closure intact, since review evidence survives such a push -- but --match-head-commit still demands A, which is no longer the head, and the spent cap forbids a third metered read to rebind it. Condition 2 then shut the Codex close precisely BECAUSE the review was clean. A clean review followed by a formatting commit could therefore never merge at all. A stamped Codex read of the current head now supplies the binding without re-opening a gate that was never in question. What the close still may never do is stand in for a metered read that never happened. This is an older tension than this PR -- evidence surviving a non-material push has always sat awkwardly against an exact-head merge guard -- but the new branch made it reachable, so it is fixed here rather than deferred. 2. docs/PRD.md still said the mandatory CodeRabbit gate is what holds auto-merge, contradicting the terminus this PR introduces. The identical sentence in CONTRIBUTING.md was updated earlier in this PR and this one was not: the sixth time a fix landed in one file and not its mirrors.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
AGENTS.md§Review held three individually-sound rules that were jointly unsatisfiable::188:211:221If both permitted metered reviews found anything at all — even something small and immediately
fixed — the zero-finding review the gate demanded needed a third read, which the cap forbade.
The PR then sat behind a human indefinitely with nothing wrong with it.
The defect is sharper than the deadlock
The old gate bundled two different questions — were the findings dealt with? and did a provider
read the head being merged? — into one test, and billed both to the most expensive provider in
the lane. A review that finds three real problems costs two credits to close; one that finds nothing
costs one. The contract priced diligence as failure, and it did so in the currency ADR-0064 was
most careful about. That is why the deadlock is not an edge case: it is reached by exactly the PRs
the lane is working correctly on.
What changes
When the cap is spent and every finding from both completed reviews is fixed,
deferred-and-tracked, or dropped sub-floor with every thread resolved, a fresh Codex read of the
final head closes the gate in CodeRabbit's place. The maintainer escalation is deleted, not
supplemented — leaving it would preserve the gate this removes.
Nothing new is extended to Codex.
AGENTS.mdalready calls it "unmetered and so uncapped" and"not optional: it is what makes the metered providers affordable." It is being allowed to finish.
Three conditions shut the branch, and none of them is a motive
Each is readable off the pull request:
is a wait. An unavailable CodeRabbit still freezes the PR.
evidence still stands, the branch never opens.
commit_ids, with the commits between themanswering the first review's findings. Two reads of one unchanged diff are one review asked twice.
An earlier draft was rejected in review for testing intent. It read "an ask made to spend the
cap is not one of the two." A motive is not checkable by anyone, including the agent itself — and
read strictly it restores the deadlock in a new form: 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. A rule
that fixes a deadlock must not be able to re-create one. The remaining deterrent is structural
instead: 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.
The close is a substitute, not a discount
Anything the closing read surfaces is disposed of by the same three dispositions before it closes. An
earlier draft held it 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, promoted to its own rule rather than left in
a tail clause, because it is the most damaging available misreading.
All six drift surfaces move together
Every rule-stating file is material (
AGENTS.md:177–182) precisely so a push that changes what thegate requires cannot leave a stale copy behind:
.agents/skills/tether-worker/agents/openai.yaml.agents/skills/tether-worker/SKILL.mdCONTRIBUTING.md×6.github/pull_request_template.mddocs/PRD.md§12.greptile/README.mdopenai.yamlis the one worth naming: four lines, read by no test, and injected as the Codexlane's default prompt. A stale copy there would brief every future Codex-lane worker under a
contract that no longer exists, with no signal at all.
The escalation existed in three files —
AGENTS.md:221,CONTRIBUTING.md:416anddocs/PRD.md§12.4 — and all three are gone. This body, a commit message and ADR-0065 all saidtwo until Codex found the third: the PRD phrases it "the lane stops for the maintainer" rather
than "hand the PR to the maintainer", so the phrase-matched grep never saw it. Since
AGENTS.mdtells agents to stop and ask when contracts conflict, a worker reading the PRD would have stranded
on exactly the PR this unblocks. The sweep is now semantic across all seven rule-stating files. Deleting it also repairs
AGENTS.md:227, "Human sign-off: … 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.
The PR template gains somewhere for the new evidence to live: a Codex closing read line and a
"cap spent, closed by Codex" option. A rule with no field in the template is a rule that will not
be evidenced.
One bullet deliberately not changed
AGENTS.md:82–83, thedoctorbullet's "Every remedy is maintainer authority — post a marker,promote a label, arm someone else's merge." I intended to append that your own stranded PR is not on
that list. It is unchanged and correct as it stands: every remedy
doctoractually enumerates issomeone else's to apply, and closing your own PR's gate is not one of them. Recorded here rather than
left as a silent omission.
Linked tracking
claim.py reserve-adr(refs/adr-reservations/0065), never byreading
docs/adr/. ADR-0064'sStatusbullet amended on one line, sincescripts/gen_adr_index.pyextracts that field with a single-line pattern.namespace or test fixture;
git revertrestores the escalation exactly.This PR cannot use the branch it introduces
AGENTS.md:6— only instructions on the default branch govern, and unmerged edits are inert. Everyfile here is material, so this re-arms its own review and is judged under the old gate: it
needs a clean CodeRabbit review like any other PR. That is the correct order, not an oversight — the
rule earns its way in under the regime it replaces. #434 becomes mergeable under the new branch only
once this is on
main.Provenance of the claim
No claim ref was taken, and that is stated rather than hidden. #439 is
status:backlogwith notether-agent-readymarker, soclaim.py claimwould exit3. Posting that marker myself wasblocked — correctly: an agent minting the maintainer approval that authorizes its own issue is the
write-loop the design guards against. The mutex risk is nil regardless (a brand-new backlog issue is
unclaimable by anyone), so this is a process gap, not a safety gap. The maintainer's authorization for
this exact work is quoted verbatim in #439's Execution autonomy section.
Type of change
feat— new capabilitySelf-review checklist (PRD §12.4)
pinning review wording in a test is exactly what retired
tests/test_review_policy.pyin chore(agents): retire the review-policy prose-drift guard #260and made test(ci): re-cover the PR-template evidence fields structurally, after the review-gate rewrite #261 necessary. The existing mechanical guards in
tests/test_agent_contract_is_runnable.pyare the right level and all still pass.docs/PRD.md§12, the new ADR, and the ADR index;mkdocs build --strictgreen.
raised was a P1 and every one was fixed rather than deferred, because each falsified this PR's own
central claim. Round 1: the different-
commit_idcondition re-created the deadlock (non-seriousfindings are disposed by deferral, which moves no head), and the template's final-head requirement
was unsatisfiable on the capped path. Round 2: the close admitted scope pushed after the cap was
spent — read by the closing provider and nobody else — which is now shut by a fourth condition.
Round 3: the scope freeze was anchored to the clock rather than the reviewed commit, the closing
read's evidence shape was impossible for a clean Codex run, and a third copy of the escalation
was found in the PRD. CodeRabbit has not been asked yet.
"compliant with version 3.3").
Testing
Local gates, reported exactly as run, on Python 3.12.13 / pytest 9.1.1 in this worktree:
2817 passed, 19 skipped, 34 deselected in 60.55s(
QT_QPA_PLATFORM=offscreen pytest -m "not large and not sidecar and not deep").have caught a careless edit here:
_SHA_DEFINED(the`<SHA>`→40-hex→reviewwindow)and the arming-page floor both bind the merge bullet, which is why it is left byte-identical
and the new rule supplies its referent instead of rewording it.
test_no_published_page_points_at_the_unserved_prdcaughtthe new ADR pointing at
docs/PRD.mdin a form a site reader cannot follow — mkdocs excludes thatfile. Replaced with the absolute blob URL the test requires. It is recorded rather than quietly
fixed because the guard did its job.
mkdocs build --strict— green, built in 1.41s.reuse lint— green, 415/415 files, "compliant with version 3.3".tabs, merge-conflict markers;
openai.yamlre-parsed with PyYAML.pre-commit run --all-files— NOT COMPLETED LOCALLY, stated rather than implied. It stallsindefinitely in the native-PowerShell lane on this machine (bounded at 420s, no output), and the
WSL lane cannot run it because
gitthere cannot resolve a Windows-path worktree. Neither failureinvolves this diff. The required
pre-commitCI job runs the identical pinned hooks and isauthoritative; the manual per-hook run above is what bounds the gap in the meantime.