From f74b5e39f587b151c0ec3642f7cc63e4cd9ac865 Mon Sep 17 00:00:00 2001 From: hathach Date: Tue, 3 Nov 2026 17:15:51 +0700 Subject: [PATCH 1/6] pr-review: prepare names the submitted review an auto-fix on the same head repairs from --- skills/pr-review/scripts/prepare.py | 30 +++++++++++-- tests/test_pr_review.py | 65 +++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+), 3 deletions(-) diff --git a/skills/pr-review/scripts/prepare.py b/skills/pr-review/scripts/prepare.py index 2fa99716..86174f72 100644 --- a/skills/pr-review/scripts/prepare.py +++ b/skills/pr-review/scripts/prepare.py @@ -38,7 +38,10 @@ edits from maintainers, push access to the base; `unavailable` when this account has neither; `unknown` when gh cannot tell. Only when available, it binds the review branch to the head branch through a remote of the head repository (head_remote, bind_upstream) and adds {remote, addedRemote}; a binding it cannot make refuses. Without --auto-fix -nothing of this runs. +nothing of this runs. On a `same` head it also adds `submittedReview` {head, reviewId}, our review submitted on that +head (by the human included), which the repair then takes its findings from, else null with `submittedWhy`; one unsent +answer bundle an earlier auto-fix deferred on that review no longer refuses the run, and `deferredAnswers` counts its +answers, which wait for the repair to end. --release unlocks the pr-review- sibling whose lock carries the token, once the run that took it has ended; nothing locked by it is a no-op, and while @@ -63,7 +66,8 @@ from pathlib import Path sys.path.insert(0, str(Path(__file__).resolve().parent)) -from ledger import ONLINE, last, ledger_dir, ledger_path, load, locked, publications, refuse_unsettled, repo_of, store # noqa: E402 +from ledger import (ONLINE, last, ledger_dir, ledger_path, load, locked, publications, refuse_unsettled, repo_of, store, # noqa: E402 + submitted_last) from post import sync # noqa: E402 sys.path.insert(0, str(Path(__file__).resolve().parents[2] / 'pr-babysit' / 'scripts')) @@ -223,6 +227,21 @@ def repair_target(repo, head_repo, info, branch, head): return {'state': state, 'why': why, 'remote': remote, 'addedRemote': added} +def reused_review(led, head): + """auto-fix on a head we reviewed already: ({submittedReview, submittedWhy}, the deferred answer bundle or None). + The repair takes its findings from our review submitted on the head, by the human included. One unsent bundle of + answers an earlier auto-fix deferred on that review (post.py --defer-answers) waits for the repair to end instead + of refusing the run; any other pending draft on the head still refuses it.""" + try: + rev = submitted_last(led, head) + except Unusable as e: + return {'submittedReview': None, 'submittedWhy': str(e)}, None + reuse = {'submittedReview': {'head': head, 'reviewId': rev['receipts']['review'].get('reviewId')}, 'submittedWhy': None} + pending = [p for p, _ in publications(led) if p['head'] == head and p['status'] == 'pending'] + bundles = [p for p in rev.get('answerPublications', []) if p in pending and p.get('auto') and not p.get('receipts')] + return reuse, (bundles[0] if len(pending) == 1 and bundles else None) + + def remote_for(repo): """The remote whose URL is this repository: fetching another one would pin someone else's refs.""" names = [] @@ -396,7 +415,6 @@ def prepare(a): with locked(path): led = load(path, repo, a.pr) synced = sync(repo, a.pr, led, lambda: store(path, led)) - refuse_unsettled(led, head, ('pending',)) online = [r for r, _ in publications(led) if r['status'] in ONLINE] if online and online[-1]['status'] == 'drafted': raise Unusable(f"your pending review of {online[-1]['head']} is still open on the PR: submit or delete it on GitHub first") @@ -405,8 +423,13 @@ def prepare(a): raise Unusable(f"a review POST for {online[-1]['head']} may have landed unseen: reconcile it first with " f"post.py --pr {a.pr} --expected-head {online[-1]['head']}{auto}, which only looks for it by its marker") mode, scope_base, prior = mode_of(led, head, merge_base, a.full) + reuse, deferred = reused_review(led, head) if a.auto_fix and mode == 'same' else (None, None) + if not deferred: + refuse_unsettled(led, head, ('pending',)) wt, branch = worktree(top, a.pr, head, token) repair = repair_target(repo, head_repo_of(info), info, branch, head) if a.auto_fix else None + if reuse is not None: + repair = {**repair, **reuse} pr_paths = changed(merge_base, head) scope = pr_paths if mode != 'incremental' else changed(scope_base, head) groups, over = groups_of(scope, a.max_groups) @@ -434,6 +457,7 @@ def prepare(a): 'tooling': [p for p in pr_paths if TOOLING.search(p)], 'ci': ci, 'ledger': path.as_posix(), 'facts': facts.as_posix(), 'synced': synced, 'changedFile': changed_file and changed_file.as_posix(), **({'repair': repair} if a.auto_fix else {}), + **({'deferredAnswers': len(deferred['answers'])} if deferred else {}), } diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index 28924f5f..dfec86ad 100644 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -2051,6 +2051,71 @@ def test_auto_fix_defers_the_answers_so_the_repair_may_list_its_findings_then_pl with self.assertRaisesRegex(facts.Unusable, '--defer-answers goes with --auto'): self.post('--defer-answers') + def deferred(self): + """Our review submitted on the head by an auto-fix run, its answer to the contributor's pushback deferred.""" + rec = self.dispute('upheld', {'body': 'Still stands.', 'resolve': False}) + self.comment(950, 'contrib', 'intentional') + self.save(self.result_for(self.p, findings=[{'id': 'pr7-f1', 'status': 'upheld', 'disputes': [rec]}, + {'file': 'src/core/a.c', 'line': 1, 'why': 'a shadows a global', 'severity': 'medium', + 'dimension': 'correctness', 'status': 'open'}], + draft={'event': 'COMMENT', 'body': 'Again.', 'replies': [], + 'comments': [{'path': 'src/core/a.c', 'line': 1, 'body': 'a shadows a global', 'finding': 1}]}), + reason='again') + self.assertEqual(self.post('--auto', '--defer-answers')['answers'], {'status': 'deferred'}) + + def test_auto_fix_on_the_same_head_reuses_our_submitted_review_and_lets_its_deferred_answers_wait(self): + self.deferred() + p = self.prepare('--auto-fix') + rid = self.review()['receipts']['review']['reviewId'] + self.assertEqual((p['mode'], p['repair']['submittedReview'], p['repair']['submittedWhy'], p['deferredAnswers']), + ('same', {'head': self.head, 'reviewId': rid}, None, 1)) + with self.assertRaisesRegex(facts.Unusable, 'a pending draft'): + self.prepare() + led = self.led() + led['reviews'][-1]['answerPublications'].append({**led['reviews'][-1]['answerPublications'][0], 'auto': False}) + Path(self.p['ledger']).write_text(json.dumps(led)) + with self.assertRaisesRegex(facts.Unusable, 'a pending draft'): + self.prepare('--auto-fix') + + def test_a_reply_after_the_deferred_judgment_keeps_its_finding_held_and_its_stale_answer_out(self): + self.deferred() + self.comment(951, 'contrib', 'still intentional') + snap = self.tmp / 'threads.json' + self.call(threads, ['--pr', str(PR), '--repo', REPO, '--out', str(snap)]) + got = self.call(ledger, ['show', '--pr', str(PR), '--repo', REPO, '--fixable', '--threads', str(snap), '--head', self.head])['fixable'] + self.assertEqual(([f['id'] for f in got['fix']], [(h['findingId'], h['why']) for h in got['held']]), (['pr7-f2'], [('pr7-f1', 'replied')])) + out = self.post() + self.assertEqual([(e['findingId'], e['state']) for e in out['staged']], [('pr7-f1', 'skipped')], 'the stale answer is never sent') + self.gh.submit(out['review']['reviewId']) + snap = self.snapshot((950, 'contrib', 'intentional', False), (951, 'contrib', 'still intentional', False)) + self.assertEqual([d['findingId'] for d in self.disputes(snap)], ['pr7-f1'], 'the newer reply waits for its judgment') + + def test_auto_fix_reuses_a_handed_over_review_once_the_human_submitted_it(self): + rec = self.dispute('upheld', {'body': 'Still stands.', 'resolve': False}) + self.comment(950, 'contrib', 'intentional') + self.save(self.result_for(self.p, findings=[{'id': 'pr7-f1', 'status': 'upheld', 'disputes': [rec]}], + draft={'event': 'COMMENT', 'body': 'Again.', 'comments': [], 'replies': []}), reason='again') + self.crash_at('POST') + with self.assertRaises(KeyboardInterrupt): + self.post('--auto', '--defer-answers') + out = self.post('--auto', '--defer-answers') + self.assertEqual(out['status'], 'drafted', 'handed over') + with self.assertRaisesRegex(facts.Unusable, 'still open on the PR'): + self.prepare('--auto-fix') + self.gh.submit(self.review()['receipts']['review']['reviewId']) + p = self.prepare('--auto-fix') + self.assertEqual((p['mode'], p['repair']['submittedReview']['head'], p['deferredAnswers']), ('same', self.head, 1)) + out = self.post() + self.assertEqual([(e['kind'], e['state']) for e in out['staged']], [('answer', 'staged')], 'plain post.py still publishes them') + + def test_auto_fix_on_the_same_head_without_a_submitted_review_names_why(self): + self.save(self.result_for(self.p, draft={'event': 'COMMENT', 'body': 'Again.', 'comments': [], 'replies': []}), reason='again') + self.mark_posted(self.p) + p = self.prepare('--auto-fix') + self.assertEqual((p['mode'], p['repair']['submittedReview']), ('same', None)) + self.assertRegex(p['repair']['submittedWhy'], 'not one submitted on') + self.assertNotIn('deferredAnswers', p) + def test_deferred_answers_leave_the_recheck_free_and_are_judged_again_on_the_repaired_head(self): rec = self.dispute('upheld', {'body': 'Still stands.', 'resolve': False}) self.comment(950, 'contrib', 'intentional') From 3e325f55ea2362dd6d8c71bd4f6f9f620b1d1d35 Mon Sep 17 00:00:00 2001 From: hathach Date: Tue, 3 Nov 2026 17:17:29 +0700 Subject: [PATCH 2/6] pr-review: each guard of the deferred-bundle exemption refuses on its own --- tests/test_pr_review.py | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index dfec86ad..d8467fbe 100644 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -2071,11 +2071,18 @@ def test_auto_fix_on_the_same_head_reuses_our_submitted_review_and_lets_its_defe ('same', {'head': self.head, 'reviewId': rid}, None, 1)) with self.assertRaisesRegex(facts.Unusable, 'a pending draft'): self.prepare() - led = self.led() - led['reviews'][-1]['answerPublications'].append({**led['reviews'][-1]['answerPublications'][0], 'auto': False}) - Path(self.p['ledger']).write_text(json.dumps(led)) - with self.assertRaisesRegex(facts.Unusable, 'a pending draft'): - self.prepare('--auto-fix') + good = self.led() + bundle = good['reviews'][-1]['answerPublications'][0] + not_auto, sent, two = (json.loads(json.dumps(good)) for _ in range(3)) + not_auto['reviews'][-1]['answerPublications'][0]['auto'] = False + sent['reviews'][-1]['answerPublications'][0]['receipts'] = {'answer': {'sent': True}} + two['reviews'][-1]['answerPublications'].append({**bundle, 'auto': False}) + other = json.loads(json.dumps(good)) + other['reviews'][0].setdefault('answerPublications', []).append(other['reviews'][-1]['answerPublications'].pop()) + for name, led in (('not auto', not_auto), ('sent', sent), ('two bundles', two), ("another review's", other)): + Path(self.p['ledger']).write_text(json.dumps(led)) + with self.subTest(name), self.assertRaisesRegex(facts.Unusable, 'a pending draft'): + self.prepare('--auto-fix') def test_a_reply_after_the_deferred_judgment_keeps_its_finding_held_and_its_stale_answer_out(self): self.deferred() From 43130dfb585d64cc6be03ec00541964b08accabd Mon Sep 17 00:00:00 2001 From: hathach Date: Tue, 3 Nov 2026 17:18:41 +0700 Subject: [PATCH 3/6] pr-review: auto-fix on a reviewed head repairs from our submitted review --- agents/chief.md | 2 +- skills/pr-review/SKILL.md | 38 ++++++++++++++++++++++++++------------ 2 files changed, 27 insertions(+), 13 deletions(-) diff --git a/agents/chief.md b/agents/chief.md index 44907aae..e4d9dcd8 100644 --- a/agents/chief.md +++ b/agents/chief.md @@ -74,7 +74,7 @@ Ungranted actions are collected, never dispatched, and asked once at the end wit ## Review a PR -Run pr-review's launch task in its order. Its review phases have no writer batches, so the commit check, the simplification challenge and the completion sequence do not apply to them. An auto-fix repair phase is writer work: its repairs get the commit check, one simplification pass, validation and a whole-task review from the reviewed head to the repair tip before pr-babysit adopts them, and pr-babysit's own repairs follow its paragraph above. Build each HIL row the workflow takes from the report the operator run wrote, its tested head included, never from a summary. A discussion run's answers go only into a pending review. +Run pr-review's launch task in its order. Its review phases have no writer batches, so the commit check, the simplification challenge and the completion sequence do not apply to them. An auto-fix repair phase is writer work: its repairs get the commit check, one simplification pass, validation and a whole-task review from the reviewed head to the repair tip before pr-babysit adopts them, and pr-babysit's own repairs follow its paragraph above. Build each HIL row the workflow takes from the report the operator run wrote, its tested head included, never from a summary. A discussion run's answers go only into a pending review; under auto-fix on a head reviewed already, they wait for the repair's end, as the launch task's sequence says. ## Fix issue N diff --git a/skills/pr-review/SKILL.md b/skills/pr-review/SKILL.md index 34923d4a..9aa1229a 100644 --- a/skills/pr-review/SKILL.md +++ b/skills/pr-review/SKILL.md @@ -46,9 +46,13 @@ the mode: `full` on a first review or after a force-push or rebase, unchanged, `same` when this head was reviewed already. On `same`, launch in `discussion` mode, which only judges new replies on our threads and returns `nothing-new` when there are none; `full` reviews the head again instead, saved -with the request's words as `--reason`. Under `auto-fix`, a discussion run -repairs nothing, since step 6 of §4 needs a review this run submitted: a repair -of an already-reviewed head takes `auto-fix full`. An `overCap` answer merged +with the request's words as `--reason`. Under `auto-fix`, prepare's +`repair.submittedReview` names our review submitted on this head, the human's +submit included: the run repairs from it (§4 step 6) without reviewing the head +again. With `deferredAnswers`, the answers an earlier auto-fix deferred on it, +the discussion is skipped, since that run judged its pushback. A null +`submittedReview` repairs nothing, for `repair.submittedWhy`; `auto-fix full` +reviews the head again and repairs from that review. An `overCap` answer merged everything into one group: report it and launch. A sibling tree is locked for the run, so worktree-remove's cleanup of the @@ -111,8 +115,10 @@ since the same head otherwise gets only a discussion run. Under `auto-fix`, also the head repository and branch, said plainly to be the contributor's branch for a fork, prepare's `repair` state and why (`unknown` is not `unavailable`), and that the run reviews, repairs, validates and rechecks the PR within -pr-babysit's cycle accounting, or, on `same`, that it repairs nothing and -`auto-fix full` would. +pr-babysit's cycle accounting. On `same`, say instead that it repairs from +submitted review `submittedReview.reviewId` without a whole-head review, with +any `deferredAnswers` waiting for the repair's end, or that it repairs nothing, +for `submittedWhy`. ## 3. Publishing @@ -145,8 +151,8 @@ finds still standing is raised again on a new comment, unless someone reopened its thread since, which then still states it. A recheck review the ledger cannot vouch for (`ledger.py show --fixable`'s `autoPost` false) stays pending for the human. One repair round: what the recheck finds new is -reported, and repairing it needs a new request, with `full` while the head is -still the rechecked one (§1). +reported, and repairing it needs a new request, which on the rechecked head +repairs from the recheck review once that is submitted (§1). ## 4. Launch @@ -182,15 +188,21 @@ sequence: or {choice: 'boards', boards: [rows]}, hardwareRelevant, autoPost, dimensions?}` (`mode: 'discussion'` for prepare's `same`). `dimensions` comes from the project's instruction file when it names review dimensions. + Under auto-fix with prepare's `deferredAnswers`, steps 3 to 5 are skipped + and step 6 starts from `submittedReview`. 4. `result.py --output `; when its `status` is `reviewed`, `ledger.py save --pr N --output `. Any other status saves - and posts nothing: step 7 reports it with its reason. + and posts nothing: step 7 reports it with its reason, except that under + auto-fix with `submittedReview` a `nothing-new` discussion goes on to step 6. 5. The session leading the review runs `post.py` (pending), `post.py --auto` (auto-post) or `post.py --auto --defer-answers` (auto-fix) itself, never through a unit, with `--expected-head`; a - `discussion` run always takes plain `post.py`, since its answers only go - into a pending review. Its exit 1 names what it could not confirm. -6. Auto-fix, only once step 5 reports `posted`; a failed, partial, uncertain + `discussion` run takes plain `post.py`, since its answers only go into a + pending review, except under auto-fix with `submittedReview`: nothing is + posted, and its answers wait on the ledger as deferred ones do. Its exit 1 + names what it could not confirm. +6. Auto-fix, only once step 5 reports `posted`, or on `same` with prepare's + `submittedReview`, whose head is then the reviewed head; a failed, partial, uncertain or handed-over post ends it there. After a handed-over one, its deferred answers wait for the human's submit (§5): plain `post.py` refuses them until then, so it is not run. Its answers to pushback, `answers` @@ -245,6 +257,7 @@ sequence: 7. Report the verdict and its reasons, the counts, coverage lost, the CI and HIL rows, the receipts, `post.py`'s `firstLineUnchecked` and the result's `heldThreads`; never the draft's text. Under auto-fix also the repair state, + whether the repair took this run's review or reused submitted review `submittedReview.reviewId`, the held findings, the repair and pr-babysit receipts, the `--fixed` receipts, the recheck's post, and the review state GitHub shows (`gh pr view N --json @@ -274,7 +287,8 @@ rules). `post.py` reports `uncertain` or `partial` when it cannot prove what lan it never sends again blind, and the next run looks for it; reconcile by hand what it names. A `handedOver` auto review is the human's pending review, with its reason; once they submit it, `post.py --pr N --expected-head SHA` publishes -its answers. A thread in `heldThreads` is one we resolved and someone reopened, +its answers, or a new `auto-fix` request repairs from it first and publishes +them at the repair's end (§1). A thread in `heldThreads` is one we resolved and someone reopened, whose resolve is unconfirmed, or whose repair note was deleted or edited before its resolve: it is never resolved again by itself, so name it for the human. From 9432c70d18bc6deca72f5099829c0ed9f309f0d9 Mon Sep 17 00:00:00 2001 From: hathach Date: Tue, 3 Nov 2026 17:19:52 +0700 Subject: [PATCH 4/6] pr-review: report the stale answers and the pending review the closing post leaves --- skills/pr-review/SKILL.md | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/skills/pr-review/SKILL.md b/skills/pr-review/SKILL.md index 9aa1229a..56e1768f 100644 --- a/skills/pr-review/SKILL.md +++ b/skills/pr-review/SKILL.md @@ -49,8 +49,8 @@ unchanged, `same` when this head was reviewed already. On `same`, launch in with the request's words as `--reason`. Under `auto-fix`, prepare's `repair.submittedReview` names our review submitted on this head, the human's submit included: the run repairs from it (§4 step 6) without reviewing the head -again. With `deferredAnswers`, the answers an earlier auto-fix deferred on it, -the discussion is skipped, since that run judged its pushback. A null +again. When prepare's line has `deferredAnswers`, the answers an earlier +auto-fix deferred on it, the discussion is skipped, since that run judged its pushback. A null `submittedReview` repairs nothing, for `repair.submittedWhy`; `auto-fix full` reviews the head again and repairs from that review. An `overCap` answer merged everything into one group: report it and launch. @@ -188,7 +188,7 @@ sequence: or {choice: 'boards', boards: [rows]}, hardwareRelevant, autoPost, dimensions?}` (`mode: 'discussion'` for prepare's `same`). `dimensions` comes from the project's instruction file when it names review dimensions. - Under auto-fix with prepare's `deferredAnswers`, steps 3 to 5 are skipped + Under auto-fix, when prepare's line has `deferredAnswers`, steps 3 to 5 are skipped and step 6 starts from `submittedReview`. 4. `result.py --output `; when its `status` is `reviewed`, `ledger.py save --pr N --output `. Any other status saves @@ -258,6 +258,8 @@ sequence: HIL rows, the receipts, `post.py`'s `firstLineUnchecked` and the result's `heldThreads`; never the draft's text. Under auto-fix also the repair state, whether the repair took this run's review or reused submitted review `submittedReview.reviewId`, + the answers the closing plain `post.py` skipped as stale and the pending review it left, which blocks + the next run until the human settles it, the held findings, the repair and pr-babysit receipts, the `--fixed` receipts, the recheck's post, and the review state GitHub shows (`gh pr view N --json From fbd8bb8fa47eeb420422e9110606215de896ae2c Mon Sep 17 00:00:00 2001 From: hathach Date: Tue, 3 Nov 2026 17:25:06 +0700 Subject: [PATCH 5/6] pr-review: skip the discussion when the repair reuses our review; fold its fields into repair --- agents/chief.md | 4 +-- skills/pr-review/SKILL.md | 41 ++++++++++++-------------- skills/pr-review/scripts/prepare.py | 31 +++++++++----------- tests/test_agents.py | 2 +- tests/test_pr_review.py | 45 +++++++++++++---------------- 5 files changed, 55 insertions(+), 68 deletions(-) diff --git a/agents/chief.md b/agents/chief.md index e4d9dcd8..440e2421 100644 --- a/agents/chief.md +++ b/agents/chief.md @@ -58,7 +58,7 @@ Follow-up issue rule: a push grant for the task's PR, the interactive default or Reply takeover rule: a push grant for the task's PR, the interactive default or a headless PR launch that does not withhold it, also permits finishing on that PR the reply obligations a launch returns in `handoffs`: posting the answer, settling on an adequate reply of ours already there, resolving its thread once the reply is verified, and editing a reply of ours on that comment through `reply.py --edit`, whichever task posted it, to an evidence-backed correction when it is stale or wrong, never left for the user to accept or authorize, or a second answer where `reply.py`'s `secondAnswer` permits one. It permits no delete, no reply to another comment, no review and no SonarCloud action by its units, and a dry run withholds it. Answer only when every point the comment raises as it stands now is fixed on the PR branch, refuted with evidence or deferred by a passed deferral; a valid point not yet fixed is fixed first, by the workflow or by your own writer through `adoptHead`. For all of a launch's handoffs, one `general-purpose` prep unit on `model: sonnet` loads `pr-reply`, works from the launch reading and the threads through `reply.py --inspect`, opening the output file for evidence the reading omits or cuts, and proposes per comment reuse, post, edit or blocked with the exact body and its evidence, posting nothing. One `finding-verifier` then judges, one verdict per comment, each proposal against its whole current comment, except a comment whose prep shows all of: its digest and the PR head are the ones the launch judged, the launch's final findings after its challenger cover every point of the comment with none held, and the body states those dispositions without changing their substance; a missing, duplicate or inconclusive verdict leaves that comment blocked. You then load `pr-reply` and run each `reply.py` operation yourself with the exact operations, bodies and digests, keeping its receipts verbatim. Answer each comment once per digest, and rerun its operation once after an uncertain receipt; an unchanged unresolved one is a blocker. Hand each verified reply to the next launch, a `reviews` or `both` one, as `replySettlements: [{ commentId, commentDigest, replyId, bodyDigest, headSha }]`, `headSha` the PR head its answer was judged on: pr-babysit re-reads both and settles the comment, or names why not in its `settlements`. For a code-scanning comment whose answer fixes or refutes rather than defers, add `sonar: { how, note }`, `how` `fixNote` when any point was fixed, else `refutation`, and `note` the exact reply body: pr-babysit then marks its SonarCloud issue under `markSonar`. -Exception for a headless PR review launch: the human's own message asking pr-review to review a PR with auto-post or auto-fix, carried and accepted under the headless PR launch exception's provenance and bound to its expected head, grants this chief invocation one review on that PR, as `COMMENT` or `REQUEST_CHANGES` with its inline comments and never `APPROVE`, and replies to and resolves of only our own review threads whose fix a recheck verified, solely through pr-review's `post.py --auto`, which you run. A relaunch of the same unfinished review on the same head first reconciles any earlier post by `post.py`'s `uncertain` and `partial` rules, never submitting a second review. A moved head, a finished review or a withdrawal ends it; `post.py` refuses a moved head, which stops publishing. It submits no answer to a contributor's pushback: `post.py --auto`, or under auto-fix a plain `post.py` once the repair ends, puts those in a pending review under the standing exception below. With auto-fix, the same message, under the headless PR launch exception above, also grants pr-babysit on that PR for the repairs of that review's findings and the bots' review, a fix note and a resolve on each of our own threads whose repair that pr-babysit pushed and a fix check verified on the PR head, solely through pr-review's `post.py --fixed`, which you run, and one more review, the recheck on the head those repairs leave, submitted the same way only while `ledger.py show --fixable` allows it, else created pending; a further repair of newly found findings needs a new request. Without auto-fix it permits no push; it never permits approval, dismissal, another PR or issue beyond the follow-up issue rule, and no onward delegation of the quoted message. +Exception for a headless PR review launch: the human's own message asking pr-review to review a PR with auto-post or auto-fix, carried and accepted under the headless PR launch exception's provenance and bound to its expected head, grants this chief invocation one review on that PR, as `COMMENT` or `REQUEST_CHANGES` with its inline comments and never `APPROVE`, and replies to and resolves of only our own review threads whose fix a recheck verified, solely through pr-review's `post.py --auto`, which you run. A relaunch of the same unfinished review on the same head first reconciles any earlier post by `post.py`'s `uncertain` and `partial` rules, never submitting a second review. A moved head, a finished review or a withdrawal ends it; `post.py` refuses a moved head, which stops publishing. It submits no answer to a contributor's pushback: `post.py --auto`, or under auto-fix a plain `post.py` once the repair ends, puts those in a pending review under the standing exception below. With auto-fix, the same message, under the headless PR launch exception above, also grants pr-babysit on that PR for the repairs of that review's findings, or, on a head already reviewed, of our review submitted on it that prepare's `repair.submittedReview` names, and the bots' review, a fix note and a resolve on each of our own threads whose repair that pr-babysit pushed and a fix check verified on the PR head, solely through pr-review's `post.py --fixed`, which you run, and one more review, the recheck on the head those repairs leave, submitted the same way only while `ledger.py show --fixable` allows it, else created pending; a further repair of newly found findings needs a new request. Without auto-fix it permits no push; it never permits approval, dismissal, another PR or issue beyond the follow-up issue rule, and no onward delegation of the quoted message. Standing exception for pr-review pending reviews: under the user's standing grant in their instructions, a pr-review task may, with no exchange, create one pending review on the task's PR through pr-review's `post.py`, holding the saved draft's body, inline comments, fix notes and thread answers, and resolve, through `post.py` or `prepare.py`, only our own threads whose reply the human submitted. It never submits, edits or deletes a review or comment, and permits no other publishing. @@ -74,7 +74,7 @@ Ungranted actions are collected, never dispatched, and asked once at the end wit ## Review a PR -Run pr-review's launch task in its order. Its review phases have no writer batches, so the commit check, the simplification challenge and the completion sequence do not apply to them. An auto-fix repair phase is writer work: its repairs get the commit check, one simplification pass, validation and a whole-task review from the reviewed head to the repair tip before pr-babysit adopts them, and pr-babysit's own repairs follow its paragraph above. Build each HIL row the workflow takes from the report the operator run wrote, its tested head included, never from a summary. A discussion run's answers go only into a pending review; under auto-fix on a head reviewed already, they wait for the repair's end, as the launch task's sequence says. +Run pr-review's launch task in its order. Its review phases have no writer batches, so the commit check, the simplification challenge and the completion sequence do not apply to them. An auto-fix repair phase is writer work: its repairs get the commit check, one simplification pass, validation and a whole-task review from the reviewed head to the repair tip before pr-babysit adopts them, and pr-babysit's own repairs follow its paragraph above. Build each HIL row the workflow takes from the report the operator run wrote, its tested head included, never from a summary. A discussion run's answers go only into a pending review. ## Fix issue N diff --git a/skills/pr-review/SKILL.md b/skills/pr-review/SKILL.md index 56e1768f..682162ce 100644 --- a/skills/pr-review/SKILL.md +++ b/skills/pr-review/SKILL.md @@ -48,11 +48,9 @@ unchanged, `same` when this head was reviewed already. On `same`, launch in `nothing-new` when there are none; `full` reviews the head again instead, saved with the request's words as `--reason`. Under `auto-fix`, prepare's `repair.submittedReview` names our review submitted on this head, the human's -submit included: the run repairs from it (§4 step 6) without reviewing the head -again. When prepare's line has `deferredAnswers`, the answers an earlier -auto-fix deferred on it, the discussion is skipped, since that run judged its pushback. A null -`submittedReview` repairs nothing, for `repair.submittedWhy`; `auto-fix full` -reviews the head again and repairs from that review. An `overCap` answer merged +submit included, and the run repairs from it (§4 step 6) without reviewing the +head again; a null one repairs nothing, for `repair.submittedWhy`, and +`auto-fix full` reviews the head again and repairs from that review. An `overCap` answer merged everything into one group: report it and launch. A sibling tree is locked for the run, so worktree-remove's cleanup of the @@ -116,9 +114,8 @@ the head repository and branch, said plainly to be the contributor's branch for a fork, prepare's `repair` state and why (`unknown` is not `unavailable`), and that the run reviews, repairs, validates and rechecks the PR within pr-babysit's cycle accounting. On `same`, say instead that it repairs from -submitted review `submittedReview.reviewId` without a whole-head review, with -any `deferredAnswers` waiting for the repair's end, or that it repairs nothing, -for `submittedWhy`. +submitted review `repair.submittedReview` without a whole-head review, or that +it repairs nothing, for `submittedWhy`. ## 3. Publishing @@ -151,8 +148,7 @@ finds still standing is raised again on a new comment, unless someone reopened its thread since, which then still states it. A recheck review the ledger cannot vouch for (`ledger.py show --fixable`'s `autoPost` false) stays pending for the human. One repair round: what the recheck finds new is -reported, and repairing it needs a new request, which on the rechecked head -repairs from the recheck review once that is submitted (§1). +reported, and repairing it needs a new request (§1). ## 4. Launch @@ -188,21 +184,21 @@ sequence: or {choice: 'boards', boards: [rows]}, hardwareRelevant, autoPost, dimensions?}` (`mode: 'discussion'` for prepare's `same`). `dimensions` comes from the project's instruction file when it names review dimensions. - Under auto-fix, when prepare's line has `deferredAnswers`, steps 3 to 5 are skipped - and step 6 starts from `submittedReview`. + Under auto-fix with `repair.submittedReview`, steps 3 to 5 are skipped when + `repair.state` is `available` or `repair` has `deferredAnswers` (the answers + an earlier auto-fix deferred on that review): a discussion cannot change what + step 6 repairs, whose findings with pushback stay held, and step 6.4 judges + that pushback. Otherwise the discussion runs as without auto-fix. 4. `result.py --output `; when its `status` is `reviewed`, `ledger.py save --pr N --output `. Any other status saves - and posts nothing: step 7 reports it with its reason, except that under - auto-fix with `submittedReview` a `nothing-new` discussion goes on to step 6. + and posts nothing: step 7 reports it with its reason. 5. The session leading the review runs `post.py` (pending), `post.py --auto` (auto-post) or `post.py --auto --defer-answers` (auto-fix) itself, never through a unit, with `--expected-head`; a - `discussion` run takes plain `post.py`, since its answers only go into a - pending review, except under auto-fix with `submittedReview`: nothing is - posted, and its answers wait on the ledger as deferred ones do. Its exit 1 - names what it could not confirm. -6. Auto-fix, only once step 5 reports `posted`, or on `same` with prepare's - `submittedReview`, whose head is then the reviewed head; a failed, partial, uncertain + `discussion` run always takes plain `post.py`, since its answers only go + into a pending review. Its exit 1 names what it could not confirm. +6. Auto-fix, only once step 5 reports `posted`, or on `same` with + `repair.submittedReview`, whose head is then the reviewed head; a failed, partial, uncertain or handed-over post ends it there. After a handed-over one, its deferred answers wait for the human's submit (§5): plain `post.py` refuses them until then, so it is not run. Its answers to pushback, `answers` @@ -257,7 +253,7 @@ sequence: 7. Report the verdict and its reasons, the counts, coverage lost, the CI and HIL rows, the receipts, `post.py`'s `firstLineUnchecked` and the result's `heldThreads`; never the draft's text. Under auto-fix also the repair state, - whether the repair took this run's review or reused submitted review `submittedReview.reviewId`, + whether the repair took this run's review or reused submitted review `repair.submittedReview`, the answers the closing plain `post.py` skipped as stale and the pending review it left, which blocks the next run until the human settles it, the held findings, the repair and pr-babysit receipts, the `--fixed` @@ -289,8 +285,7 @@ rules). `post.py` reports `uncertain` or `partial` when it cannot prove what lan it never sends again blind, and the next run looks for it; reconcile by hand what it names. A `handedOver` auto review is the human's pending review, with its reason; once they submit it, `post.py --pr N --expected-head SHA` publishes -its answers, or a new `auto-fix` request repairs from it first and publishes -them at the repair's end (§1). A thread in `heldThreads` is one we resolved and someone reopened, +its answers; a new `auto-fix` request instead repairs from it first (§1). A thread in `heldThreads` is one we resolved and someone reopened, whose resolve is unconfirmed, or whose repair note was deleted or edited before its resolve: it is never resolved again by itself, so name it for the human. diff --git a/skills/pr-review/scripts/prepare.py b/skills/pr-review/scripts/prepare.py index 86174f72..034ed2d2 100644 --- a/skills/pr-review/scripts/prepare.py +++ b/skills/pr-review/scripts/prepare.py @@ -38,9 +38,9 @@ edits from maintainers, push access to the base; `unavailable` when this account has neither; `unknown` when gh cannot tell. Only when available, it binds the review branch to the head branch through a remote of the head repository (head_remote, bind_upstream) and adds {remote, addedRemote}; a binding it cannot make refuses. Without --auto-fix -nothing of this runs. On a `same` head it also adds `submittedReview` {head, reviewId}, our review submitted on that -head (by the human included), which the repair then takes its findings from, else null with `submittedWhy`; one unsent -answer bundle an earlier auto-fix deferred on that review no longer refuses the run, and `deferredAnswers` counts its +nothing of this runs. On a `same` head `repair` also has `submittedReview`, the id of our review submitted on that +head (by the human included), which the repair takes its findings from, else null with `submittedWhy`; one unsent +answer bundle an earlier auto-fix deferred on that review no longer refuses the run: `deferredAnswers` counts its answers, which wait for the repair to end. --release unlocks the pr-review- sibling whose lock carries the token, once @@ -228,18 +228,18 @@ def repair_target(repo, head_repo, info, branch, head): def reused_review(led, head): - """auto-fix on a head we reviewed already: ({submittedReview, submittedWhy}, the deferred answer bundle or None). - The repair takes its findings from our review submitted on the head, by the human included. One unsent bundle of - answers an earlier auto-fix deferred on that review (post.py --defer-answers) waits for the repair to end instead - of refusing the run; any other pending draft on the head still refuses it.""" + """repair's submittedReview or submittedWhy, and deferredAnswers when the one pending publication on `head` is + an unsent bundle an auto-fix deferred on that review (post.py --defer-answers); any other pending draft refuses.""" try: rev = submitted_last(led, head) except Unusable as e: - return {'submittedReview': None, 'submittedWhy': str(e)}, None - reuse = {'submittedReview': {'head': head, 'reviewId': rev['receipts']['review'].get('reviewId')}, 'submittedWhy': None} + return {'submittedReview': None, 'submittedWhy': str(e)} + reuse = {'submittedReview': rev['receipts']['review'].get('reviewId')} pending = [p for p, _ in publications(led) if p['head'] == head and p['status'] == 'pending'] - bundles = [p for p in rev.get('answerPublications', []) if p in pending and p.get('auto') and not p.get('receipts')] - return reuse, (bundles[0] if len(pending) == 1 and bundles else None) + one = pending[0] if len(pending) == 1 else None + if one and one.get('auto') and not one.get('receipts') and any(one is p for p in rev.get('answerPublications', [])): + reuse['deferredAnswers'] = len(one['answers']) + return reuse def remote_for(repo): @@ -423,13 +423,11 @@ def prepare(a): raise Unusable(f"a review POST for {online[-1]['head']} may have landed unseen: reconcile it first with " f"post.py --pr {a.pr} --expected-head {online[-1]['head']}{auto}, which only looks for it by its marker") mode, scope_base, prior = mode_of(led, head, merge_base, a.full) - reuse, deferred = reused_review(led, head) if a.auto_fix and mode == 'same' else (None, None) - if not deferred: + reuse = reused_review(led, head) if a.auto_fix and mode == 'same' else {} + if 'deferredAnswers' not in reuse: refuse_unsettled(led, head, ('pending',)) wt, branch = worktree(top, a.pr, head, token) - repair = repair_target(repo, head_repo_of(info), info, branch, head) if a.auto_fix else None - if reuse is not None: - repair = {**repair, **reuse} + repair = {**repair_target(repo, head_repo_of(info), info, branch, head), **reuse} if a.auto_fix else None pr_paths = changed(merge_base, head) scope = pr_paths if mode != 'incremental' else changed(scope_base, head) groups, over = groups_of(scope, a.max_groups) @@ -457,7 +455,6 @@ def prepare(a): 'tooling': [p for p in pr_paths if TOOLING.search(p)], 'ci': ci, 'ledger': path.as_posix(), 'facts': facts.as_posix(), 'synced': synced, 'changedFile': changed_file and changed_file.as_posix(), **({'repair': repair} if a.auto_fix else {}), - **({'deferredAnswers': len(deferred['answers'])} if deferred else {}), } diff --git a/tests/test_agents.py b/tests/test_agents.py index 27aac90c..38b0f5d4 100644 --- a/tests/test_agents.py +++ b/tests/test_agents.py @@ -189,6 +189,7 @@ def test_headless_pr_exception_is_bounded(self): 'Without auto-fix it permits no push; it never permits approval, dismissal, another PR or issue beyond the follow-up issue rule'): self.assertIn(phrase, review) self.assertIn('their request that pr-review review a PR with auto-fix', exception) + self.assertIn("of our review submitted on it that prepare's `repair.submittedReview` names", body) self.assertIn('An auto-fix repair phase is writer work: its repairs get the commit check, one simplification pass, validation ' 'and a whole-task review from the reviewed head to the repair tip before pr-babysit adopts them', body) self.assertNotIn('fresh exchange', review) @@ -203,7 +204,6 @@ def test_headless_pr_exception_is_bounded(self): 'a failed, partial, uncertain or handed-over post ends it there', 'report "review submitted; auto-fix blocked"', 'never a `held` one', 'A crash with repairs unpublished resumes this task with them, never a new prepare', - 'Under `auto-fix`, a discussion run repairs nothing', 'a repair of an already-reviewed head takes `auto-fix full`', '`post.py --auto --defer-answers` (auto-fix)', 'whatever becomes of the recheck review; one the recheck finds still standing is raised again on a new comment, ' 'unless someone reopened its thread since, which then still states it', diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index d8467fbe..95a5282f 100644 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -2051,33 +2051,33 @@ def test_auto_fix_defers_the_answers_so_the_repair_may_list_its_findings_then_pl with self.assertRaisesRegex(facts.Unusable, '--defer-answers goes with --auto'): self.post('--defer-answers') - def deferred(self): - """Our review submitted on the head by an auto-fix run, its answer to the contributor's pushback deferred.""" + def upheld_again(self, *extra, comments=()): + """A second review of the head, pr7-f1 upheld on the contributor's pushback with an answer, plus `extra` findings + and their `comments`.""" rec = self.dispute('upheld', {'body': 'Still stands.', 'resolve': False}) self.comment(950, 'contrib', 'intentional') - self.save(self.result_for(self.p, findings=[{'id': 'pr7-f1', 'status': 'upheld', 'disputes': [rec]}, - {'file': 'src/core/a.c', 'line': 1, 'why': 'a shadows a global', 'severity': 'medium', - 'dimension': 'correctness', 'status': 'open'}], - draft={'event': 'COMMENT', 'body': 'Again.', 'replies': [], - 'comments': [{'path': 'src/core/a.c', 'line': 1, 'body': 'a shadows a global', 'finding': 1}]}), - reason='again') + self.save(self.result_for(self.p, findings=[{'id': 'pr7-f1', 'status': 'upheld', 'disputes': [rec]}, *extra], + draft={'event': 'COMMENT', 'body': 'Again.', 'comments': list(comments), 'replies': []}), reason='again') + + def deferred(self): + """Our review submitted on the head by an auto-fix run, its answer to the contributor's pushback deferred.""" + self.upheld_again({'file': 'src/core/a.c', 'line': 1, 'why': 'a shadows a global', 'severity': 'medium', + 'dimension': 'correctness', 'status': 'open'}, + comments=[{'path': 'src/core/a.c', 'line': 1, 'body': 'a shadows a global', 'finding': 1}]) self.assertEqual(self.post('--auto', '--defer-answers')['answers'], {'status': 'deferred'}) def test_auto_fix_on_the_same_head_reuses_our_submitted_review_and_lets_its_deferred_answers_wait(self): self.deferred() p = self.prepare('--auto-fix') rid = self.review()['receipts']['review']['reviewId'] - self.assertEqual((p['mode'], p['repair']['submittedReview'], p['repair']['submittedWhy'], p['deferredAnswers']), - ('same', {'head': self.head, 'reviewId': rid}, None, 1)) + self.assertEqual((p['mode'], p['repair']['submittedReview'], p['repair']['deferredAnswers']), ('same', rid, 1)) + self.assertNotIn('submittedWhy', p['repair']) with self.assertRaisesRegex(facts.Unusable, 'a pending draft'): self.prepare() - good = self.led() - bundle = good['reviews'][-1]['answerPublications'][0] - not_auto, sent, two = (json.loads(json.dumps(good)) for _ in range(3)) + not_auto, sent, two, other = (self.led() for _ in range(4)) not_auto['reviews'][-1]['answerPublications'][0]['auto'] = False sent['reviews'][-1]['answerPublications'][0]['receipts'] = {'answer': {'sent': True}} - two['reviews'][-1]['answerPublications'].append({**bundle, 'auto': False}) - other = json.loads(json.dumps(good)) + two['reviews'][-1]['answerPublications'].append({**two['reviews'][-1]['answerPublications'][0], 'auto': False}) other['reviews'][0].setdefault('answerPublications', []).append(other['reviews'][-1]['answerPublications'].pop()) for name, led in (('not auto', not_auto), ('sent', sent), ('two bundles', two), ("another review's", other)): Path(self.p['ledger']).write_text(json.dumps(led)) @@ -2098,10 +2098,7 @@ def test_a_reply_after_the_deferred_judgment_keeps_its_finding_held_and_its_stal self.assertEqual([d['findingId'] for d in self.disputes(snap)], ['pr7-f1'], 'the newer reply waits for its judgment') def test_auto_fix_reuses_a_handed_over_review_once_the_human_submitted_it(self): - rec = self.dispute('upheld', {'body': 'Still stands.', 'resolve': False}) - self.comment(950, 'contrib', 'intentional') - self.save(self.result_for(self.p, findings=[{'id': 'pr7-f1', 'status': 'upheld', 'disputes': [rec]}], - draft={'event': 'COMMENT', 'body': 'Again.', 'comments': [], 'replies': []}), reason='again') + self.upheld_again() self.crash_at('POST') with self.assertRaises(KeyboardInterrupt): self.post('--auto', '--defer-answers') @@ -2111,7 +2108,8 @@ def test_auto_fix_reuses_a_handed_over_review_once_the_human_submitted_it(self): self.prepare('--auto-fix') self.gh.submit(self.review()['receipts']['review']['reviewId']) p = self.prepare('--auto-fix') - self.assertEqual((p['mode'], p['repair']['submittedReview']['head'], p['deferredAnswers']), ('same', self.head, 1)) + self.assertEqual((p['mode'], p['repair']['submittedReview'], p['repair']['deferredAnswers']), + ('same', self.review()['receipts']['review']['reviewId'], 1)) out = self.post() self.assertEqual([(e['kind'], e['state']) for e in out['staged']], [('answer', 'staged')], 'plain post.py still publishes them') @@ -2121,13 +2119,10 @@ def test_auto_fix_on_the_same_head_without_a_submitted_review_names_why(self): p = self.prepare('--auto-fix') self.assertEqual((p['mode'], p['repair']['submittedReview']), ('same', None)) self.assertRegex(p['repair']['submittedWhy'], 'not one submitted on') - self.assertNotIn('deferredAnswers', p) + self.assertNotIn('deferredAnswers', p['repair']) def test_deferred_answers_leave_the_recheck_free_and_are_judged_again_on_the_repaired_head(self): - rec = self.dispute('upheld', {'body': 'Still stands.', 'resolve': False}) - self.comment(950, 'contrib', 'intentional') - self.save(self.result_for(self.p, findings=[{'id': 'pr7-f1', 'status': 'upheld', 'disputes': [rec]}], - draft={'event': 'COMMENT', 'body': 'Again.', 'comments': [], 'replies': []}), reason='again') + self.upheld_again() self.assertEqual(self.post('--auto', '--defer-answers')['answers'], {'status': 'deferred'}) self.commit('src/core/c.c', 'int c;\n', 'repair') self.head = self.push_pr() From 403de12f262413c15d8a4b5acadf2681643d4b06 Mon Sep 17 00:00:00 2001 From: hathach Date: Tue, 3 Nov 2026 17:26:24 +0700 Subject: [PATCH 6/6] pr-review: the pre-launch report names the deferred answers --- skills/pr-review/SKILL.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/skills/pr-review/SKILL.md b/skills/pr-review/SKILL.md index 682162ce..ad57b88c 100644 --- a/skills/pr-review/SKILL.md +++ b/skills/pr-review/SKILL.md @@ -114,8 +114,9 @@ the head repository and branch, said plainly to be the contributor's branch for a fork, prepare's `repair` state and why (`unknown` is not `unavailable`), and that the run reviews, repairs, validates and rechecks the PR within pr-babysit's cycle accounting. On `same`, say instead that it repairs from -submitted review `repair.submittedReview` without a whole-head review, or that -it repairs nothing, for `submittedWhy`. +submitted review `repair.submittedReview` without a whole-head review, with +the `repair.deferredAnswers` answers waiting for the repair to end, or that it +repairs nothing, for `submittedWhy`. ## 3. Publishing