From 5c0bb871d5cec6219feda08c80614b3d27ffecd3 Mon Sep 17 00:00:00 2001 From: mega-putin Date: Thu, 8 Oct 2026 08:34:23 +0000 Subject: [PATCH] fix: surface Claude session-limit errors on the review sticky Classify the first analysis transcript for quota/rate-limit failures, skip the immediate model retry, and rewrite the existing sticky status with the provider message and workflow run URL. Do not post a new conversation comment. --- .github/actions/CLAUDE-CI.md | 7 + .github/actions/claude-pr-review/README.md | 3 +- .github/actions/claude-pr-review/action.yml | 14 ++ .../claude-pr-review/review_pipeline.py | 205 +++++++++++++++++- .../claude-pr-review/test_review_pipeline.py | 164 ++++++++++++++ 5 files changed, 387 insertions(+), 6 deletions(-) diff --git a/.github/actions/CLAUDE-CI.md b/.github/actions/CLAUDE-CI.md index 453cd9e..82c37fb 100644 --- a/.github/actions/CLAUDE-CI.md +++ b/.github/actions/CLAUDE-CI.md @@ -292,6 +292,10 @@ diff is read, and a small diff inside a large file spends many more paging throu 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. +The retry is skipped when the first analysis attempt is a provider quota or rate-limit +failure (Claude 5-hour session limit, HTTP 429). Immediate replay cannot succeed. +The existing sticky status comment then records the provider's own message and a link to +the workflow run. The job still fails. No extra PR comment is posted. Consumers that already create a GitHub App token can opt into the unified identity with: @@ -361,6 +365,9 @@ It carries the reviewed range, an update timestamp, this round's counts, and a r 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. +A quota or rate-limit failure puts the provider sentence (for example the session-limit +reset time) on that sticky comment, with a link to the workflow run, instead of only an +internal `MODEL_NO_OUTPUT` code. 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. diff --git a/.github/actions/claude-pr-review/README.md b/.github/actions/claude-pr-review/README.md index 9d228ae..19777a0 100644 --- a/.github/actions/claude-pr-review/README.md +++ b/.github/actions/claude-pr-review/README.md @@ -45,7 +45,8 @@ Family: [Claude CI](../CLAUDE-CI.md). 1. Prepare immutable review context 1. Compose bounded LLM analysis 1. Analyze and verify findings โ€” `anthropics/claude-code-action` *(only if `steps.prepare.outputs.mode != 'skip'`)* -1. Retry missing structured review output โ€” `anthropics/claude-code-action` *(only if `steps.prepare.outputs.mode != 'skip' && (steps.review.outcome == 'failure' || steps.review.outputs.structured_output == '')`)* +1. Classify first analysis attempt *(only if `steps.prepare.outputs.mode != 'skip'`)* +1. Retry missing structured review output โ€” `anthropics/claude-code-action` *(only if `steps.prepare.outputs.mode != 'skip' && steps.classify.outputs.retryable != 'false' && (steps.review.outcome == 'failure' || steps.review.outputs.structured_output == '')`)* 1. Capture analysis session transcript *(only if `always()`)* 1. Compile and validate review 1. Publish atomically and persist state diff --git a/.github/actions/claude-pr-review/action.yml b/.github/actions/claude-pr-review/action.yml index 4b67a25..3020751 100644 --- a/.github/actions/claude-pr-review/action.yml +++ b/.github/actions/claude-pr-review/action.yml @@ -348,10 +348,24 @@ runs: ${{ inputs.extra_prompt }} + - name: Classify first analysis attempt + id: classify + if: steps.prepare.outputs.mode != 'skip' + shell: bash + env: + EXECUTION_FILE: ${{ steps.review.outputs.execution_file }} + STATE_DIR: ${{ github.workspace }}/.pr-review + run: | + set -euo pipefail + python3 "$GITHUB_ACTION_PATH/review_pipeline.py" classify \ + --state-dir "$STATE_DIR" \ + --execution-file "${EXECUTION_FILE:-}" + - name: Retry missing structured review output id: review_retry if: >- steps.prepare.outputs.mode != 'skip' && + steps.classify.outputs.retryable != 'false' && (steps.review.outcome == 'failure' || steps.review.outputs.structured_output == '') uses: anthropics/claude-code-action@5ef2e550a465a721f4f45e4a7d3c340c873e1dcc # v1 diff --git a/.github/actions/claude-pr-review/review_pipeline.py b/.github/actions/claude-pr-review/review_pipeline.py index 15e2d61..52903e9 100644 --- a/.github/actions/claude-pr-review/review_pipeline.py +++ b/.github/actions/claude-pr-review/review_pipeline.py @@ -78,6 +78,7 @@ FAILURE_FILENAME = "failure.json" TRACE_FILENAME = "trace.log" MODEL_OUTPUT_FILENAME = "model-output.json" +MODEL_FAILURE_FILENAME = "model-failure.json" TRACE_LIST_LIMIT = 40 DEFAULT_REMEDIATIONS = { @@ -1135,6 +1136,165 @@ def sync_manifest_threads( finding["thread_resolution"] = "unresolved" + +def workflow_run_url() -> str | None: + """Public Actions URL for this job, when the runner exported the usual vars.""" + server = (os.environ.get("GITHUB_SERVER_URL") or "").rstrip("/") + repo = os.environ.get("GITHUB_REPOSITORY") or "" + run_id = os.environ.get("GITHUB_RUN_ID") or "" + if server and repo and run_id: + return f"{server}/{repo}/actions/runs/{run_id}" + return None + + +def iter_json_strings(value: Any): + """Yield every string in a JSON-like value, depth first.""" + if isinstance(value, str): + if value: + yield value + return + if isinstance(value, dict): + for item in value.values(): + yield from iter_json_strings(item) + return + if isinstance(value, list): + for item in value: + yield from iter_json_strings(item) + + +def load_json_value(path: Path) -> Any: + try: + return json.loads(path.read_text(encoding="utf-8")) + except (OSError, json.JSONDecodeError): + return None + + +def classify_model_text(text: str) -> dict[str, Any] | None: + """Map provider quota / rate-limit prose onto a non-retryable failure. + + Bare HTTP 429 is not enough: SHAs and line numbers contain that digit + sequence. Require session-limit or rate-limit wording. + """ + blob = text or "" + if not blob.strip(): + return None + sessionish = bool( + re.search(r"session limit", blob, re.I) + or re.search(r"five[_-]?hour", blob, re.I) + or re.search(r"rateLimitType", blob) + ) + rate_limited = bool( + sessionish + or re.search(r"rate_limit", blob, re.I) + or re.search(r"rate limit", blob, re.I) + ) + if not rate_limited: + return None + match = re.search( + r"You've hit your session limit[^\n]*", + blob, + re.I, + ) + if sessionish: + message = ( + match.group(0).strip() + if match + else "Claude session limit reached (5-hour quota)." + ) + return { + "code": "MODEL_SESSION_LIMIT", + "retryable": False, + "message": public_text(message, maximum=300), + "remediation": ( + "Wait until the quoted reset time, then rerun the workflow. " + "An immediate retry hits the same quota." + ), + } + return { + "code": "MODEL_RATE_LIMITED", + "retryable": False, + "message": "Claude API rate limit (HTTP 429).", + "remediation": ( + "Wait and rerun the workflow. An immediate retry hits the same limit." + ), + } + + +def classify_model_failure(*, execution_file: str = "") -> dict[str, Any]: + """Read the first analysis transcript and decide whether retry can help.""" + chunks: list[str] = [] + path = Path(execution_file) if execution_file else None + if path is not None and path.is_file(): + value = load_json_value(path) + if value is not None: + chunks.extend(iter_json_strings(value)) + else: + try: + chunks.append(path.read_text(encoding="utf-8", errors="replace")) + except OSError: + pass + classified = classify_model_text("\n".join(chunks)) + if classified is None: + return { + "retryable": True, + "code": "", + "message": "", + "remediation": "", + } + return classified + + +def write_model_failure(state_dir: Path, classified: dict[str, Any]) -> None: + state_dir.mkdir(parents=True, exist_ok=True) + (state_dir / MODEL_FAILURE_FILENAME).write_text( + json.dumps(classified, indent=2, sort_keys=True) + "\n", + encoding="utf-8", + ) + + +def classify_command(args: argparse.Namespace) -> None: + """Classify the first Claude attempt so retry and compile can see quota hits. + + Always exits successfully: a classifier bug must not skip a useful retry + by failing the step. Missing transcripts are retryable. + """ + state_dir = Path(args.state_dir) + try: + classified = classify_model_failure( + execution_file=args.execution_file or "", + ) + write_model_failure(state_dir, classified) + write_github_output( + { + "retryable": ( + "true" if classified.get("retryable", True) else "false" + ), + "code": classified.get("code") or "", + } + ) + if classified.get("code"): + log( + f"classified first analysis as {classified['code']} " + f"retryable={classified.get('retryable')}: " + f"{classified.get('message')}" + ) + else: + log("classified first analysis as retryable (no quota signature)") + except Exception as error: # noqa: BLE001 โ€” fail open toward retry + log(f"classify failed open toward retry: {error}") + fallback = { + "retryable": True, + "code": "", + "message": "", + "remediation": "", + } + try: + write_model_failure(state_dir, fallback) + except OSError: + pass + write_github_output({"retryable": "true", "code": ""}) + + def write_github_output(values: dict[str, Any]) -> None: output_path = os.environ.get("GITHUB_OUTPUT") if not output_path: @@ -2388,6 +2548,16 @@ def compile_command(args: argparse.Namespace) -> None: else: raw = os.environ.get("REVIEW_STRUCTURED_OUTPUT", "") if not raw: + classified = read_json_file(state_dir / MODEL_FAILURE_FILENAME) or {} + if classified.get("code") and classified.get("retryable") is False: + raise PipelineError( + str( + classified.get("message") + or "Claude returned no structured review output" + ), + code=str(classified["code"]), + remediation=classified.get("remediation"), + ) raise PipelineError( "Claude returned no structured review output", code="MODEL_NO_OUTPUT", @@ -2817,13 +2987,20 @@ def render_status_body( ) elif phase == "failed": status_title = "๐Ÿ› ๏ธ Review did not finish" - reason = public_text(summary.get("reason"), maximum=200) + reason = public_text(summary.get("reason"), maximum=400) 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." + " Anything listed below is from the last round that did." ) + run_url = str(summary.get("run_url") or "").strip() + if run_url: + detail_line += f" Workflow run: {run_url}" + else: + detail_line += ( + " Re-run the workflow after the limit resets, or push a new" + " commit to try again." + ) else: if open_findings: status_title = f"โš ๏ธ {len(open_findings)} open finding(s)" @@ -2875,6 +3052,7 @@ def set_sticky_phase( scope_text: str, phase: str, reason: str | None = None, + run_url: str | None = None, ) -> int | None: """Move the sticky status comment to a non-publishing phase. @@ -2890,6 +3068,7 @@ def set_sticky_phase( "scope_text": scope_text, "phase": phase, "reason": reason, + "run_url": run_url, }, } try: @@ -3216,7 +3395,12 @@ def report_status_line( return "โœ… Review completed." -def close_out_sticky(state_dir: Path, *, reason: str | None = None) -> None: +def close_out_sticky( + state_dir: Path, + *, + reason: str | None = None, + run_url: 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 @@ -3237,6 +3421,7 @@ def close_out_sticky(state_dir: Path, *, reason: str | None = None) -> None: scope_text=scope_label(review_input), phase="failed", reason=reason, + run_url=run_url, ) @@ -3283,9 +3468,14 @@ def report(args: argparse.Namespace) -> None: + details_block("Pipeline trace", read_trace(state_dir)) ) write_job_summary(body) + reason = diagnostic["message"] + code = diagnostic.get("code") or "" + if code and code not in reason: + reason = f"{code}: {reason}" close_out_sticky( state_dir, - reason=f"{diagnostic['code']} in phase {diagnostic['phase']}", + reason=reason, + run_url=workflow_run_url(), ) print( "Claude PR review: FAILED " @@ -3567,6 +3757,11 @@ def parser() -> argparse.ArgumentParser: ) prepare_parser.set_defaults(func=prepare) + classify_parser = subparsers.add_parser("classify") + classify_parser.add_argument("--state-dir", required=True) + classify_parser.add_argument("--execution-file", default="") + classify_parser.set_defaults(func=classify_command) + compile_parser = subparsers.add_parser("compile") compile_parser.add_argument("--state-dir", required=True) compile_parser.set_defaults(func=compile_command) diff --git a/.github/actions/claude-pr-review/test_review_pipeline.py b/.github/actions/claude-pr-review/test_review_pipeline.py index 627df12..5f90798 100644 --- a/.github/actions/claude-pr-review/test_review_pipeline.py +++ b/.github/actions/claude-pr-review/test_review_pipeline.py @@ -2425,6 +2425,170 @@ def test_success_report_embeds_the_trace_and_model_output(self, _print): self.assertIn("Model output (before compilation)", summary) self.assertIn("Reviewed the delta.", summary) + + + def test_action_skips_retry_when_classify_says_not_retryable(self): + action = Path(pipeline.__file__).with_name("action.yml").read_text( + encoding="utf-8" + ) + self.assertIn("id: classify", action) + self.assertIn("review_pipeline.py\" classify", action) + self.assertIn( + "steps.classify.outputs.retryable != 'false'", + action, + ) + # Still the same two Claude invocations; classify is local Python. + self.assertEqual(action.count("show_full_output: true"), 2) + + def test_classify_session_limit_from_transcript(self): + transcript = [ + { + "type": "result", + "subtype": "success", + "is_error": True, + "result": ( + "You've hit your session limit ยท resets 11:30am (UTC)" + ), + "errors": [ + { + "rate_limit_event": True, + "status": "rejected", + "rateLimitType": "five_hour", + "api_error_status": 429, + } + ], + } + ] + with tempfile.TemporaryDirectory() as directory: + path = Path(directory, "analysis-transcript.json") + path.write_text(json.dumps(transcript), encoding="utf-8") + classified = pipeline.classify_model_failure( + execution_file=str(path) + ) + + self.assertEqual(classified["code"], "MODEL_SESSION_LIMIT") + self.assertFalse(classified["retryable"]) + self.assertIn("session limit", classified["message"].lower()) + self.assertIn("11:30am", classified["message"]) + + def test_classify_missing_file_is_retryable(self): + classified = pipeline.classify_model_failure( + execution_file="/no/such/transcript.json" + ) + self.assertTrue(classified["retryable"]) + self.assertEqual(classified["code"], "") + + def test_classify_ignores_bare_429_digits(self): + classified = pipeline.classify_model_text( + "checked line 429 in src/example.py" + ) + self.assertIsNone(classified) + + def test_compile_uses_classified_session_limit(self): + with tempfile.TemporaryDirectory() as directory: + state = Path(directory) + (state / "review-input.json").write_text( + json.dumps(review_input()), + encoding="utf-8", + ) + pipeline.write_model_failure( + state, + { + "code": "MODEL_SESSION_LIMIT", + "retryable": False, + "message": ( + "You've hit your session limit ยท resets 11:30am (UTC)" + ), + "remediation": "Wait until the quoted reset time.", + }, + ) + with mock.patch.dict("os.environ", {"REVIEW_STRUCTURED_OUTPUT": ""}): + with self.assertRaises(pipeline.PipelineError) as raised: + pipeline.compile_command( + SimpleNamespace(state_dir=str(state)) + ) + self.assertEqual(raised.exception.code, "MODEL_SESSION_LIMIT") + self.assertIn("session limit", str(raised.exception)) + + def test_failed_status_includes_provider_message_and_run_url(self): + payload = { + "repository": "megaeth-labs/example", + "pull_request": 7, + "status_summary": { + "scope_text": "head `bbbbbbbb`", + "phase": "failed", + "reason": ( + "MODEL_SESSION_LIMIT: You've hit your session limit " + "ยท resets 11:30am (UTC)" + ), + "run_url": ( + "https://github.com/megaeth-labs/example/actions/runs/1" + ), + }, + } + body = pipeline.render_status_body(payload, {"findings": {}}) + self.assertIn("๐Ÿ› ๏ธ Review did not finish", body) + self.assertIn("You've hit your session limit", body) + self.assertIn("11:30am", body) + self.assertIn( + "https://github.com/megaeth-labs/example/actions/runs/1", + body, + ) + self.assertNotIn("๐Ÿ”„", body) + + def test_report_quota_failure_rewrites_sticky_not_new_comment(self): + with tempfile.TemporaryDirectory() as directory: + state = Path(directory) + (state / "review-input.json").write_text( + json.dumps(review_input()), + encoding="utf-8", + ) + (state / "failure.json").write_text( + json.dumps( + { + "schema_version": 1, + "status": "failed", + "phase": "compile", + "code": "MODEL_SESSION_LIMIT", + "message": ( + "You've hit your session limit ยท resets 11:30am (UTC)" + ), + "remediation": "Wait until the quoted reset time.", + } + ), + encoding="utf-8", + ) + with mock.patch.object( + pipeline, "set_sticky_phase" + ) as phase_mock, mock.patch.object( + pipeline, "gh_json" + ) as gh_mock, mock.patch.dict( + "os.environ", + { + "GITHUB_SERVER_URL": "https://github.com", + "GITHUB_REPOSITORY": "megaeth-labs/example", + "GITHUB_RUN_ID": "99", + "PREPARE_OUTCOME": "success", + "REVIEW_OUTCOME": "failure", + "REVIEW_RETRY_OUTCOME": "skipped", + "COMPILE_OUTCOME": "failure", + "PUBLISH_OUTCOME": "skipped", + }, + clear=False, + ): + pipeline.report(SimpleNamespace(state_dir=str(state))) + + gh_mock.assert_not_called() + self.assertEqual(phase_mock.call_args.kwargs["phase"], "failed") + self.assertIn( + "You've hit your session limit", + phase_mock.call_args.kwargs["reason"], + ) + self.assertEqual( + phase_mock.call_args.kwargs["run_url"], + "https://github.com/megaeth-labs/example/actions/runs/99", + ) + def test_action_uploads_the_review_state_artifact(self): action = Path(pipeline.__file__).with_name("action.yml").read_text(