diff --git a/.github/actions/README.md b/.github/actions/README.md index 4d4cf8d..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 @@ -60,7 +68,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 +134,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..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 @@ -299,6 +316,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..f9b7ab4 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,117 @@ 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", + "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())