diff --git a/.github/actions/claude-pr-review/README.md b/.github/actions/claude-pr-review/README.md index 9d228ae..b018959 100644 --- a/.github/actions/claude-pr-review/README.md +++ b/.github/actions/claude-pr-review/README.md @@ -14,7 +14,7 @@ Family: [Claude CI](../CLAUDE-CI.md). | Input | Required | Default | Description | |---|---|---|---| | `claude_code_oauth_token` | yes | | OAuth token for Claude Code. | -| `github_identity_token` | no | | Optional GitHub token used for review publication and state updates. When supplied, it is also used to resolve addressed automated review threads. Empty uses the job token and skips thread resolution. | +| `github_identity_token` | no | | Optional GitHub token used for review publication and state updates. When supplied, it also authenticates model analysis and resolves addressed automated review threads. Empty uses OIDC for model analysis and the job token for publication, and skips thread resolution. | | `allowed_bots` | no | `mega-putin` | Bot accounts allowed to trigger Claude Code. | | `extra_allowed_tools` | no | | Additional read-only tools to append to the canonical allowedTools list. | | `extra_prompt` | no | | Additional analysis instructions appended after the canonical prompt. | diff --git a/.github/actions/claude-pr-review/action.yml b/.github/actions/claude-pr-review/action.yml index 4b67a25..a9c47d8 100644 --- a/.github/actions/claude-pr-review/action.yml +++ b/.github/actions/claude-pr-review/action.yml @@ -10,8 +10,9 @@ inputs: default: "" description: > Optional GitHub token used for review publication and state updates. - When supplied, it is also used to resolve addressed automated review - threads. Empty uses the job token and skips thread resolution. + When supplied, it also authenticates model analysis and resolves addressed + automated review threads. Empty uses OIDC for model analysis and the job + token for publication, and skips thread resolution. allowed_bots: required: false default: mega-putin @@ -213,6 +214,7 @@ runs: uses: anthropics/claude-code-action@5ef2e550a465a721f4f45e4a7d3c340c873e1dcc # v1 with: claude_code_oauth_token: ${{ inputs.claude_code_oauth_token }} + github_token: ${{ inputs.github_identity_token }} allowed_bots: ${{ inputs.allowed_bots }} display_report: false # Always on. The review body says what the model concluded; only this @@ -357,6 +359,7 @@ runs: uses: anthropics/claude-code-action@5ef2e550a465a721f4f45e4a7d3c340c873e1dcc # v1 with: claude_code_oauth_token: ${{ inputs.claude_code_oauth_token }} + github_token: ${{ inputs.github_identity_token }} allowed_bots: ${{ inputs.allowed_bots }} display_report: false show_full_output: true diff --git a/.github/actions/claude-pr-review/review_pipeline.py b/.github/actions/claude-pr-review/review_pipeline.py index 15e2d61..30655c9 100644 --- a/.github/actions/claude-pr-review/review_pipeline.py +++ b/.github/actions/claude-pr-review/review_pipeline.py @@ -1275,6 +1275,53 @@ def prepare_trace( return lines +def pull_request_diff( + repository: str, pull_request: int, base_sha: str, head_sha: str +) -> str: + try: + return run( + ["gh", "pr", "diff", str(pull_request), "--repo", repository] + ).stdout + except PipelineError as error: + if ( + error.code != "GITHUB_API_FAILED" + or "HTTP 406" not in str(error) + or "maximum number of files" not in str(error) + ): + raise + + # GitHub's diff endpoint rejects PRs with more than 300 files. Resolve the + # frozen merge base through the compare API, then diff the complete trees. + # Fetching only those two snapshots also works with a depth-one checkout. + comparison = gh_json( + ["api", f"repos/{repository}/compare/{base_sha}...{head_sha}"] + ) + merge_base = str((comparison.get("merge_base_commit") or {}).get("sha") or "") + if not re.fullmatch(r"[0-9a-f]{40}", merge_base): + raise PipelineError("GitHub compare did not return a valid merge base") + missing = [ + sha + for sha in (merge_base, head_sha) + if run(["git", "cat-file", "-e", f"{sha}^{{commit}}"], check=False).returncode + ] + if missing: + # checkout may remove its credentials before the shared action runs. + # gh's helper reads GH_TOKEN from the environment; no token enters argv. + run( + [ + "git", "-c", "credential.helper=", + "-c", "credential.helper=!gh auth git-credential", + "fetch", "--no-tags", "--depth=1", "origin", merge_base, head_sha, + ] + ) + return run( + [ + "git", "diff", "--no-ext-diff", "--no-textconv", "--no-color", + "--binary", merge_base, head_sha, "--", + ] + ).stdout + + def prepare(args: argparse.Namespace) -> None: action_dir = Path(__file__).resolve().parent @@ -1487,16 +1534,9 @@ def prepare(args: argparse.Namespace) -> None: else: run_premortem = mode == "full" or high_risk - full_diff = run( - [ - "gh", - "pr", - "diff", - str(args.pull_request), - "--repo", - args.repository, - ] - ).stdout + full_diff = pull_request_diff( + args.repository, args.pull_request, expected_base, expected_head + ) require_expected_pull( gh_json( ["api", f"repos/{args.repository}/pulls/{args.pull_request}"] diff --git a/.github/actions/claude-pr-review/test_review_pipeline.py b/.github/actions/claude-pr-review/test_review_pipeline.py index 627df12..2ac55b2 100644 --- a/.github/actions/claude-pr-review/test_review_pipeline.py +++ b/.github/actions/claude-pr-review/test_review_pipeline.py @@ -2,6 +2,7 @@ import copy import json +import subprocess import tempfile import unittest from pathlib import Path @@ -220,6 +221,13 @@ def test_action_exposes_optional_github_identity_token(self): ) # prepare (announce), publish, and report (retire on failure). self.assertEqual(action.count(identity_env), 3) + model_steps = [ + step for step in action.split("\n - name:") + if "uses: anthropics/claude-code-action@" in step + ] + self.assertEqual(len(model_steps), 2) + for step in model_steps: + self.assertIn("github_token: ${{ inputs.github_identity_token }}", step) self.assertIn( "GH_RESOLVE_THREADS: " "${{ inputs.github_identity_token != '' }}", @@ -354,6 +362,112 @@ def test_failed_github_command_hides_verbose_arguments(self, run_mock): "gh api graphql failed: permission denied", ) + @mock.patch.object(pipeline, "gh_json") + @mock.patch.object(pipeline, "run") + def test_large_pr_diff_uses_frozen_merge_base_and_head(self, run_mock, gh_mock): + base, head, merge_base = "a" * 40, "b" * 40, "c" * 40 + gh_mock.return_value = {"merge_base_commit": {"sha": merge_base}} + run_mock.side_effect = [ + pipeline.PipelineError( + "gh pr diff failed: HTTP 406: diff exceeded the maximum number of files (300)", + code="GITHUB_API_FAILED", + ), + SimpleNamespace(returncode=1), # Missing merge base in a shallow checkout. + SimpleNamespace(returncode=1), # Missing PR head. + SimpleNamespace(stdout=""), + SimpleNamespace(stdout="complete diff including all files"), + ] + + result = pipeline.pull_request_diff("megaeth-labs/example", 7, base, head) + + self.assertEqual(result, "complete diff including all files") + gh_mock.assert_called_once_with( + ["api", f"repos/megaeth-labs/example/compare/{base}...{head}"] + ) + fetch = run_mock.call_args_list[-2].args[0] + self.assertIn("fetch", fetch) + self.assertEqual(fetch[-2:], [merge_base, head]) + self.assertEqual( + run_mock.call_args_list[-1].args[0], + ["git", "diff", "--no-ext-diff", "--no-textconv", "--no-color", "--binary", merge_base, head, "--"], + ) + + @mock.patch.object(pipeline, "gh_json") + @mock.patch.object(pipeline, "run") + def test_normal_pr_diff_uses_github_without_fetching(self, run_mock, gh_mock): + run_mock.return_value = SimpleNamespace(stdout="github diff") + self.assertEqual( + pipeline.pull_request_diff("megaeth-labs/example", 7, "a" * 40, "b" * 40), + "github diff", + ) + run_mock.assert_called_once() + gh_mock.assert_not_called() + + @mock.patch.object(pipeline, "gh_json") + @mock.patch.object(pipeline, "run") + def test_diff_server_errors_are_not_hidden_by_git_fallback(self, run_mock, gh_mock): + failure = pipeline.PipelineError("gh pr diff failed: HTTP 500", code="GITHUB_API_FAILED") + run_mock.side_effect = failure + with self.assertRaises(pipeline.PipelineError) as context: + pipeline.pull_request_diff("megaeth-labs/example", 7, "a" * 40, "b" * 40) + self.assertIs(context.exception, failure) + run_mock.assert_called_once() + gh_mock.assert_not_called() + + def test_large_pr_diff_is_complete_from_a_shallow_checkout(self): + real_run = subprocess.run + with tempfile.TemporaryDirectory() as directory: + source, checkout = Path(directory) / "source", Path(directory) / "checkout" + source.mkdir() + + def git(*args): + return real_run( + ["git", *args], cwd=source, text=True, capture_output=True, check=True + ).stdout.strip() + + git("init", "--initial-branch=base") + git("config", "user.name", "Test") + git("config", "user.email", "test@example.invalid") + names = [f"file-{index}.txt" for index in range(301)] + for name in names: + (source / name).write_text("before\n", encoding="utf-8") + (source / "binary.dat").write_bytes(b"\x00before") + git("add", "--", *names, "binary.dat") + git("commit", "-m", "merge base") + merge_base = git("rev-parse", "HEAD") + git("checkout", "-b", "head") + (source / names[0]).rename(source / "renamed.txt") + for name in names[1:]: + (source / name).write_text("after\n", encoding="utf-8") + (source / "binary.dat").write_bytes(b"\x00after") + git("add", "--", *names, "renamed.txt", "binary.dat") + git("commit", "-m", "large PR") + head = git("rev-parse", "HEAD") + git("checkout", "base") + (source / "base-only.txt").write_text("unrelated base change\n", encoding="utf-8") + git("add", "base-only.txt") + git("commit", "-m", "base advanced independently") + base = git("rev-parse", "HEAD") + git("clone", "--depth=1", "--branch=base", source.as_uri(), str(checkout)) + + def run_in_checkout(args, **kwargs): + if args[0] == "gh": + return subprocess.CompletedProcess( + args, 1, stdout="", + stderr="HTTP 406: diff exceeded the maximum number of files (300)", + ) + return real_run(args, cwd=checkout, **kwargs) + + with mock.patch.object(pipeline.subprocess, "run", side_effect=run_in_checkout), mock.patch.object( + pipeline, "gh_json", return_value={"merge_base_commit": {"sha": merge_base}} + ): + result = pipeline.pull_request_diff("megaeth-labs/example", 7, base, head) + + self.assertEqual(sum(line.startswith("diff --git ") for line in result.splitlines()), 302) + self.assertIn("rename from file-0.txt", result) + self.assertIn("GIT binary patch", result) + self.assertNotIn("base-only.txt", result) + @mock.patch("builtins.print") def test_failure_report_is_human_readable(self, print_mock): with tempfile.TemporaryDirectory() as directory: @@ -2440,4 +2554,3 @@ def test_action_uploads_the_review_state_artifact(self): if __name__ == "__main__": unittest.main() -