From ad6f7a9f08d0403ccb831c2e75fd241436bdd12b Mon Sep 17 00:00:00 2001 From: William Aaron Cheung Date: Tue, 28 Jul 2026 16:28:41 +0800 Subject: [PATCH 1/2] feat(pr-review): announce the round before analysis starts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sticky status comment was only written by the publisher, so a PR stayed silent for the several minutes a round takes and then showed the review and the status comment at once. Preparation now posts or updates the comment to "Review in progress" before analysis, so the status is there first and the review lands into an already-visible comment. Any path that ends without publishing retires the phase itself โ€” a failure diagnostic, or output discarded because the head moved โ€” so the comment never sits at "in progress" after the job ends. Skip-mode rounds publish nothing and are not announced. Announcing is best effort: a GitHub failure while posting it leaves the round unaffected. --- .github/actions/README.md | 9 +- .github/actions/claude-pr-review/action.yml | 3 + .../claude-pr-review/review_pipeline.py | 133 ++++++++++++++++-- .../claude-pr-review/test_review_pipeline.py | 96 ++++++++++++- 4 files changed, 226 insertions(+), 15 deletions(-) diff --git a/.github/actions/README.md b/.github/actions/README.md index 4d4cf8d..3c65593 100644 --- a/.github/actions/README.md +++ b/.github/actions/README.md @@ -60,7 +60,9 @@ The PR reviewer is an explicit staged pipeline: 1. A deterministic preparation step freezes the base and head SHAs, loads the durable review manifest, fetches prior automated threads, computes the full or incremental diff, and - selects the model tier. + selects the model tier. It then posts or updates the sticky status comment to + `๐Ÿ”„ Review in progress`, so the PR shows the round has started instead of staying silent + until the review lands minutes later. 2. Claude performs semantic analysis and verification with read-only tools. It returns schema-constrained data and cannot publish comments or resolve threads. 3. A deterministic compiler validates findings, enforces severity budgets, checks RIGHT-side @@ -124,6 +126,11 @@ every run, so a reader who meets it mid-thread can tell it describes the current than the moment it first appeared. It carries the reviewed range, an update timestamp, this round's counts, and a roll-up of the questions still awaiting an answer with a link to the review that asked each one. +It moves through three phases: `๐Ÿ”„ Review in progress` from preparation, then either the +finished verdict or `๐Ÿ› ๏ธ Review did not finish` for a round that ends without publishing. +Every non-publishing path โ€” a failure, a discarded stale head โ€” retires the in-progress phase +itself, so the comment never sits at "in progress" after the job ends. A skip-mode round +publishes nothing and is never announced. When old automated review threads are addressed, the deterministic publisher resolves them without adding confirmation replies. A consumer repo's `REVIEW.md` may override or extend the semantic severity guidance. diff --git a/.github/actions/claude-pr-review/action.yml b/.github/actions/claude-pr-review/action.yml index f9aad5e..4bc19d6 100644 --- a/.github/actions/claude-pr-review/action.yml +++ b/.github/actions/claude-pr-review/action.yml @@ -299,6 +299,9 @@ runs: continue-on-error: true shell: bash env: + # Needed because this step retires the sticky status comment when a + # round ends without publishing. + GH_TOKEN: ${{ inputs.github_identity_token || github.token }} STATE_DIR: ${{ github.workspace }}/.pr-review PREPARE_OUTCOME: ${{ steps.prepare.outcome }} COMPOSE_OUTCOME: ${{ steps.compose.outcome }} diff --git a/.github/actions/claude-pr-review/review_pipeline.py b/.github/actions/claude-pr-review/review_pipeline.py index de7ed11..c2fe15b 100644 --- a/.github/actions/claude-pr-review/review_pipeline.py +++ b/.github/actions/claude-pr-review/review_pipeline.py @@ -1285,6 +1285,19 @@ def prepare(args: argparse.Namespace) -> None: "pipeline_version": pipeline_version, "rubric_version": rubric_version, } + if mode != "skip": + # Announce the round before analysis so the PR carries a status comment + # from the start rather than staying silent until the review lands. + # Skip mode publishes nothing, so announcing it would strand the + # comment at "in progress". + input_value["sticky_comment_id"] = set_sticky_phase( + repository=args.repository, + pull_request=args.pull_request, + manifest=manifest, + sticky_comment_id=sticky_comment_id, + scope_text=scope_label(input_value), + phase="in_progress", + ) (state_dir / "review-input.json").write_text( json.dumps(input_value, indent=2, sort_keys=True) + "\n", encoding="utf-8", @@ -2317,6 +2330,8 @@ def render_status_body( current head or the moment it first appeared. """ summary = payload.get("status_summary") or {} + phase = str(summary.get("phase") or "complete") + scope_text = summary.get("scope_text", "the current head") findings = (manifest.get("findings") or {}).values() open_findings = [ item @@ -2324,14 +2339,39 @@ def render_status_body( if isinstance(item, dict) and item.get("status") == "open" ] pending_questions = open_questions(manifest) - if open_findings: - status_title = f"โš ๏ธ {len(open_findings)} open finding(s)" - if pending_questions: - status_title += f" ยท {len(pending_questions)} open question(s)" - elif pending_questions: - status_title = f"โ“ {len(pending_questions)} open question(s)" + if phase == "in_progress": + status_title = "๐Ÿ”„ Review in progress" + progress_line = f"Reviewing {scope_text} ยท started {utc_now()}" + detail_line = ( + "Findings and questions from this round appear here, and in a" + " review below, once the round finishes. Anything listed below is" + " carried over from earlier rounds." + ) + elif phase == "failed": + status_title = "๐Ÿ› ๏ธ Review did not finish" + reason = public_text(summary.get("reason"), maximum=200) + progress_line = f"Attempted {scope_text} ยท updated {utc_now()}" + detail_line = ( + f"This round did not publish{': ' + reason if reason else ''}." + " Anything listed below is from the last round that did. Re-run" + " the workflow or push a new commit to try again." + ) else: - status_title = "โœ… Review clean" + if open_findings: + status_title = f"โš ๏ธ {len(open_findings)} open finding(s)" + if pending_questions: + status_title += f" ยท {len(pending_questions)} open question(s)" + elif pending_questions: + status_title = f"โ“ {len(pending_questions)} open question(s)" + else: + status_title = "โœ… Review clean" + progress_line = f"Last reviewed: {scope_text} ยท updated {utc_now()}" + detail_line = ( + f"New this round: {summary.get('new_findings', 0)} finding(s)," + f" {summary.get('new_questions', 0)} question(s)" + f" ยท Resolved this round: {summary.get('resolved_findings', 0)}" + f" ยท Open questions: {len(pending_questions)}" + ) lines = [ "## Claude review status", "", @@ -2343,13 +2383,9 @@ def render_status_body( "", status_title, "", - f"Last reviewed: {summary.get('scope_text', 'the current head')}" - f" ยท updated {utc_now()}", + progress_line, "", - f"New this round: {summary.get('new_findings', 0)} finding(s)," - f" {summary.get('new_questions', 0)} question(s)" - f" ยท Resolved this round: {summary.get('resolved_findings', 0)}" - f" ยท Open questions: {len(pending_questions)}", + detail_line, ] if pending_questions: lines.extend(["", "Open questions awaiting an answer:"]) @@ -2361,6 +2397,38 @@ def render_status_body( return "\n".join(lines) +def set_sticky_phase( + *, + repository: str, + pull_request: int, + manifest: dict[str, Any], + sticky_comment_id: int | None, + scope_text: str, + phase: str, + reason: str | None = None, +) -> int | None: + """Move the sticky status comment to a non-publishing phase. + + Used to announce a round before analysis starts and to close it out when a + round ends without publishing. Best effort: the status comment is a + courtesy, so a GitHub failure here must never fail the review. + """ + payload = { + "repository": repository, + "pull_request": pull_request, + "sticky_comment_id": sticky_comment_id, + "status_summary": { + "scope_text": scope_text, + "phase": phase, + "reason": reason, + }, + } + try: + return upsert_sticky(payload, manifest) + except PipelineError: + return sticky_comment_id + + def annotate_questions( payload: dict[str, Any], manifest: dict[str, Any], @@ -2609,6 +2677,30 @@ def render_outcomes(outcomes: dict[str, str]) -> str: return "\n".join(rows) +def close_out_sticky(state_dir: Path, *, reason: str | None = None) -> None: + """Move an announced-but-unpublished round out of the in-progress phase. + + `prepare` announces every round, so any path that ends without publishing + has to retire the comment itself; otherwise it reads as a review that is + still running hours later. + """ + review_input = read_json_file(state_dir / "review-input.json") + if not review_input: + return + manifest = review_input.get("manifest") + if not isinstance(manifest, dict): + return + set_sticky_phase( + repository=str(review_input["repository"]), + pull_request=int(review_input["pull_request"]), + manifest=manifest, + sticky_comment_id=review_input.get("sticky_comment_id"), + scope_text=scope_label(review_input), + phase="failed", + reason=reason, + ) + + def report(args: argparse.Namespace) -> None: state_dir = Path(args.state_dir) outcomes = step_outcomes() @@ -2647,6 +2739,10 @@ def report(args: argparse.Namespace) -> None: + details ) write_job_summary(body) + close_out_sticky( + state_dir, + reason=f"{diagnostic['code']} in phase {diagnostic['phase']}", + ) print( "Claude PR review: FAILED " f"[{diagnostic['code']}] phase={diagnostic['phase']}: " @@ -2668,6 +2764,17 @@ def report(args: argparse.Namespace) -> None: stale = os.environ.get("PUBLISH_STALE", "") == "true" published = os.environ.get("PUBLISH_PUBLISHED", "") == "true" retry_used = outcomes.get("review_retry") not in {"", "skipped"} + if not published and mode != "skip": + # prepare left the comment at "in progress"; nothing published, so no + # later step will move it. Close it out here or it stays there. + close_out_sticky( + state_dir, + reason=( + "the PR head changed while the review was running" + if stale + else None + ), + ) if stale: status = "โš ๏ธ Review output was discarded because the PR head changed." elif mode == "skip": diff --git a/.github/actions/claude-pr-review/test_review_pipeline.py b/.github/actions/claude-pr-review/test_review_pipeline.py index 0696ffe..7e58e35 100644 --- a/.github/actions/claude-pr-review/test_review_pipeline.py +++ b/.github/actions/claude-pr-review/test_review_pipeline.py @@ -125,7 +125,8 @@ def test_action_exposes_optional_github_identity_token(self): "GH_TOKEN: " "${{ inputs.github_identity_token || github.token }}" ) - self.assertEqual(action.count(identity_env), 2) + # prepare (announce), publish, and report (retire on failure). + self.assertEqual(action.count(identity_env), 3) self.assertIn( "GH_RESOLVE_THREADS: " "${{ inputs.github_identity_token != '' }}", @@ -1414,6 +1415,99 @@ def test_answered_question_produces_a_pending_annotation(self): payload["question_annotations"][0]["headline"], ) + def test_in_progress_status_announces_the_round(self): + payload = { + "repository": "megaeth-labs/example", + "pull_request": 7, + "status_summary": { + "scope_text": "head `bbbbbbbb`", + "phase": "in_progress", + }, + } + + body = pipeline.render_status_body(payload, {"findings": {}}) + + self.assertIn("๐Ÿ”„ Review in progress", body) + self.assertIn("Reviewing head `bbbbbbbb`", body) + self.assertIn("Living comment โ€” rewritten in place", body) + self.assertNotIn("New this round", body) + + def test_in_progress_status_keeps_prior_open_items_visible(self): + payload = { + "repository": "megaeth-labs/example", + "pull_request": 7, + "status_summary": { + "scope_text": "head `bbbbbbbb`", + "phase": "in_progress", + }, + } + manifest = { + "findings": {}, + "questions": { + "Q-1": { + "status": "open", + "question": "Is the upstream unique?", + "review_id": 42, + } + }, + } + + body = pipeline.render_status_body(payload, manifest) + + self.assertIn("Open questions awaiting an answer:", body) + self.assertIn("carried over from earlier rounds", body) + + def test_failed_status_retires_the_in_progress_phase(self): + payload = { + "repository": "megaeth-labs/example", + "pull_request": 7, + "status_summary": { + "scope_text": "head `bbbbbbbb`", + "phase": "failed", + "reason": "MODEL_NO_OUTPUT in phase compile", + }, + } + + body = pipeline.render_status_body(payload, {"findings": {}}) + + self.assertIn("๐Ÿ› ๏ธ Review did not finish", body) + self.assertIn("MODEL_NO_OUTPUT in phase compile", body) + self.assertNotIn("๐Ÿ”„", body) + + @mock.patch.object(pipeline, "upsert_sticky") + def test_set_sticky_phase_survives_a_github_failure(self, upsert_mock): + upsert_mock.side_effect = pipeline.PipelineError("gh api failed") + + result = pipeline.set_sticky_phase( + repository="megaeth-labs/example", + pull_request=7, + manifest={"findings": {}}, + sticky_comment_id=11, + scope_text="head `bbbbbbbb`", + phase="in_progress", + ) + + # Announcing is a courtesy; failing to announce must not fail the run. + self.assertEqual(result, 11) + + @mock.patch.object(pipeline, "set_sticky_phase") + def test_close_out_sticky_needs_prepared_state(self, phase_mock): + with tempfile.TemporaryDirectory() as directory: + pipeline.close_out_sticky(Path(directory)) + phase_mock.assert_not_called() + + @mock.patch.object(pipeline, "set_sticky_phase") + def test_close_out_sticky_retires_the_comment(self, phase_mock): + with tempfile.TemporaryDirectory() as directory: + Path(directory, "review-input.json").write_text( + json.dumps(review_input()), + encoding="utf-8", + ) + pipeline.close_out_sticky(Path(directory), reason="head moved") + + self.assertEqual(phase_mock.call_args.kwargs["phase"], "failed") + self.assertEqual(phase_mock.call_args.kwargs["reason"], "head moved") + def test_closed_question_without_a_review_stops_being_retried(self): value = review_input() first = pipeline.compile_review(value, question_output()) From e4cc8c01174fef7a8e566ee5f247918acf5a70eb Mon Sep 17 00:00:00 2001 From: William Aaron Cheung Date: Tue, 28 Jul 2026 16:55:51 +0800 Subject: [PATCH 2/2] fix(pr-review): raise the strong turn budget and escalate it on retry Strong-tier reviews ran out of turns at 24. The driver is not diff size: the prompt mandates six pipeline files plus repo guidance before the diff is read, roughly ten turns, and a small diff inside a large file spends many more paging through surrounding code. A 226-line change across four files exhausted the budget while an 81-line new file finished comfortably. Raise strong to 36 and deep to 56 so the ordering between them stays meaningful. Also stop replaying the failure: the retry reused the first attempt's budget, so an exhausted-turns failure burned a second full model run that could not succeed. It now gets half again as many turns, and its prompt says to favour returning a well-supported result over exhaustive exploration. --- .github/actions/README.md | 8 ++++++ .github/actions/claude-pr-review/action.yml | 25 ++++++++++++++++--- .../claude-pr-review/test_review_pipeline.py | 18 +++++++++++++ 3 files changed, 47 insertions(+), 4 deletions(-) diff --git a/.github/actions/README.md b/.github/actions/README.md index 3c65593..7dffcd8 100644 --- a/.github/actions/README.md +++ b/.github/actions/README.md @@ -44,6 +44,14 @@ The `pr-review` action additionally accepts: reviews, but skips it for ordinary incremental updates. `on` always enables it and `off` disables it. +The semantic-analysis stage runs under a turn budget: 12 for a low-risk incremental review, +36 for a strong-tier one, and 56 for `deep`. +Roughly ten turns go on mandated context โ€” six pipeline files plus repo guidance โ€” before the +diff is read, and a small diff inside a large file spends many more paging through it, so the +budget tracks files to understand rather than lines changed. +The retry gets half again as many turns as the first attempt, because exhausting the budget is +deterministic and replaying it with the same budget cannot succeed. + Consumers that already create a GitHub App token can opt into the unified identity with: ```yaml diff --git a/.github/actions/claude-pr-review/action.yml b/.github/actions/claude-pr-review/action.yml index 4bc19d6..2b9c484 100644 --- a/.github/actions/claude-pr-review/action.yml +++ b/.github/actions/claude-pr-review/action.yml @@ -106,18 +106,30 @@ runs: allowed_tools="${allowed_tools},Task" fi + # Turn budgets. The prompt mandates reading six pipeline files plus + # repo guidance before analysis begins, so roughly ten turns are spent + # on context before the diff is even read; a review whose diff sits in + # a large file spends many more paging through it. 24 proved too tight + # for exactly that shape (a small diff inside a big file) even though + # it is ample for a typical PR. selected_model="$STRONG_MODEL" - max_turns=24 + max_turns=36 if [[ "$MODEL_TIER" == "fast" ]]; then selected_model="$INCREMENTAL_MODEL" max_turns=12 fi if [[ "$REVIEW_DEPTH" == "deep" ]]; then - max_turns=40 + max_turns=56 fi + # Exhausting the budget is deterministic: retrying with the same one + # burns a second full model run that cannot succeed. Give the retry + # half again as many turns so it can actually finish. + retry_turns=$(( (max_turns * 3 + 1) / 2 )) runtime_flags="--max-turns $max_turns" + retry_runtime_flags="--max-turns $retry_turns" if [[ -n "$selected_model" ]]; then runtime_flags="--model $selected_model $runtime_flags" + retry_runtime_flags="--model $selected_model $retry_runtime_flags" fi cp "$GITHUB_ACTION_PATH/rubric.md" "$GITHUB_WORKSPACE/.pr-review/rubric.md" @@ -126,6 +138,7 @@ runs: echo "allowed_tools=${allowed_tools}" >> "$GITHUB_OUTPUT" echo "runtime_flags=${runtime_flags}" >> "$GITHUB_OUTPUT" + echo "retry_runtime_flags=${retry_runtime_flags}" >> "$GITHUB_OUTPUT" - name: Analyze and verify findings id: review @@ -249,10 +262,14 @@ runs: --allowedTools "${{ steps.compose.outputs.allowed_tools }}" --disallowedTools "mcp__github_inline_comment__create_inline_comment" --json-schema '${{ steps.prepare.outputs.output_schema }}' - ${{ steps.compose.outputs.runtime_flags }} + ${{ steps.compose.outputs.retry_runtime_flags }} prompt: | Produce the schema-bound pull request review result now. The prior attempt - did not return structured output. + did not return structured output. One cause is running out of turns before + answering, so favour returning a well-supported result over exhaustive + exploration: read what you need, stop widening the search once the changed + lines are understood, and keep turns in reserve for the final response. A + smaller set of high-confidence findings beats no answer at all. Read `.pr-review/review-input.json`, `.pr-review/review.diff`, `.pr-review/full.diff`, `.pr-review/rubric.md`, and diff --git a/.github/actions/claude-pr-review/test_review_pipeline.py b/.github/actions/claude-pr-review/test_review_pipeline.py index 7e58e35..f9b7ab4 100644 --- a/.github/actions/claude-pr-review/test_review_pipeline.py +++ b/.github/actions/claude-pr-review/test_review_pipeline.py @@ -1415,6 +1415,24 @@ def test_answered_question_produces_a_pending_annotation(self): payload["question_annotations"][0]["headline"], ) + def test_retry_gets_a_larger_turn_budget_than_the_first_attempt(self): + action = Path(pipeline.__file__).with_name("action.yml").read_text( + encoding="utf-8" + ) + + self.assertIn("max_turns=36", action) + self.assertIn("retry_turns=$(( (max_turns * 3 + 1) / 2 ))", action) + # The first attempt and the retry must not share a budget: exhausting + # turns is deterministic, so replaying it cannot succeed. + self.assertIn( + "${{ steps.compose.outputs.retry_runtime_flags }}", + action, + ) + self.assertEqual( + action.count("${{ steps.compose.outputs.runtime_flags }}"), + 1, + ) + def test_in_progress_status_announces_the_round(self): payload = { "repository": "megaeth-labs/example",