diff --git a/.github/workflows/pr-review.yaml b/.github/workflows/pr-review.yaml index 2a53314..d2fdca5 100644 --- a/.github/workflows/pr-review.yaml +++ b/.github/workflows/pr-review.yaml @@ -3,78 +3,51 @@ name: AI PR Review on: issue_comment: types: [created] + workflow_dispatch: + inputs: + pr_number: + description: Pull request to review with the workflow on this branch. + required: true + type: string + review_mode: + description: default or deep. + required: true + default: default + type: choice + options: [default, deep] permissions: contents: read + issues: write pull-requests: write -concurrency: - group: pr-review-${{ github.repository }}-${{ github.event.issue.number }} - cancel-in-progress: true - jobs: review: - # Restrict to the repository owner account. The review runner is checked - # out from main and never executes code from the PR, but the job holds an - # API key, so the trigger keeps a permission gate beyond the body check. if: >- - github.event.comment.user.login == 'luckyPipewrench' && - github.event.comment.author_association == 'OWNER' && - github.event.issue.pull_request && - (github.event.comment.body == '/review' || - github.event.comment.body == '/review deep' || - github.event.comment.body == '/review tests' || - github.event.comment.body == '/review docs') - runs-on: ubuntu-latest - timeout-minutes: 10 - steps: - - name: Check out trusted review runner - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 - with: - fetch-depth: 0 - # The runner must come from the default branch, never the pull - # request, since this job holds an API key. Bound to the repository's - # own default branch rather than a hard-coded name so a rename does - # not silently break the workflow. Still repository-controlled, not - # pull-request-controlled, so the trust property is unchanged. - ref: ${{ github.event.repository.default_branch }} - persist-credentials: false - - - name: Set up Python - uses: actions/setup-python@a309ff8b426b58ec0e2a45f0f869d46889d02405 # v6.2.0 - with: - python-version: '3.12' - - - name: Install dependencies - run: pip install --require-hashes -r .github/requirements-pr-review.txt - - - name: Test trusted review runner - run: python -m unittest tests/test_pr_review_routing.py - - - name: Determine review mode - id: mode - env: - COMMENT_BODY: ${{ github.event.comment.body }} - run: | - case "$COMMENT_BODY" in - "/review deep") echo "mode=deep" >> "$GITHUB_OUTPUT" ;; - "/review tests") echo "mode=tests" >> "$GITHUB_OUTPUT" ;; - "/review docs") echo "mode=docs" >> "$GITHUB_OUTPUT" ;; - *) echo "mode=default" >> "$GITHUB_OUTPUT" ;; - esac - - - name: Run PR review - env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - LITELLM_BASE_URL: ${{ secrets.LITELLM_BASE_URL }} - LITELLM_API_KEY: ${{ secrets.LITELLM_API_KEY }} - OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} - REVIEW_MODE: ${{ steps.mode.outputs.mode }} - PR_NUMBER: ${{ github.event.issue.number }} - REPO: ${{ github.repository }} - # Empty/unset repository variables deliberately fall back to the - # Python defaults in scripts/pr-review.py. Keep defaults there so - # manual and workflow runs cannot drift. - PR_REVIEW_MODEL_FAST: ${{ vars.PR_REVIEW_MODEL_FAST }} - PR_REVIEW_MODEL_DEEP: ${{ vars.PR_REVIEW_MODEL_DEEP }} - run: python scripts/pr-review.py + github.actor == 'luckyPipewrench' && + github.triggering_actor == 'luckyPipewrench' && + ((github.event_name == 'issue_comment' && + github.event.comment.user.login == 'luckyPipewrench' && + github.event.comment.author_association == 'OWNER' && + github.event.issue.pull_request && + (github.event.comment.body == '/review' || + github.event.comment.body == '/review deep')) || + github.event_name == 'workflow_dispatch') + # Pinned to an immutable Pipelock commit in both positions. A branch or tag + # here would run reviewer code that can change under the pin. + uses: luckyPipewrench/pipelock/.github/workflows/pr-review-reusable.yaml@74b3b3f1099d8d6d8ffeb67407ba7e99d1bd3119 + with: + pr_number: >- + ${{ github.event_name == 'issue_comment' && + github.event.issue.number || inputs.pr_number }} + review_mode: >- + ${{ github.event_name == 'issue_comment' && + (github.event.comment.body == '/review deep' && 'deep' || + 'default') || + inputs.review_mode }} + reviewer_sha: 74b3b3f1099d8d6d8ffeb67407ba7e99d1bd3119 + # Personal-account repositories cannot use secrets: inherit with a reusable + # workflow, so every secret the reviewer needs is mapped by name. + secrets: + review_token: ${{ secrets.GITHUB_TOKEN }} + openai_api_key: ${{ secrets.OPENAI_API_KEY }} diff --git a/tests/test_pr_review_routing.py b/tests/test_pr_review_routing.py index a8f534b..6d466a8 100644 --- a/tests/test_pr_review_routing.py +++ b/tests/test_pr_review_routing.py @@ -91,28 +91,44 @@ def test_empty_repository_variable_overrides_use_python_defaults(self): self.assertEqual(module.model_for_mode("default"), module.DEFAULT_MODEL_FAST) self.assertEqual(module.model_for_mode("deep"), module.DEFAULT_MODEL_DEEP) - def test_workflow_delegates_model_defaults_to_python(self): + def test_workflow_leaves_model_selection_to_the_shared_reviewer(self): + # The caller used to choose models through repository variables. The + # shared reviewer owns that decision now, so a model name appearing + # here would mean this repository had started diverging from the + # reviewer it delegates to, which is the drift this change removes. workflow = WORKFLOW_PATH.read_text(encoding="utf-8") - self.assertIn("PR_REVIEW_MODEL_FAST: ${{ vars.PR_REVIEW_MODEL_FAST }}", workflow) - self.assertIn("PR_REVIEW_MODEL_DEEP: ${{ vars.PR_REVIEW_MODEL_DEEP }}", workflow) - self.assertIsNone(re.search(r"PR_REVIEW_MODEL_(?:FAST|DEEP): gpt-", workflow)) + self.assertIsNone(re.search(r"\bgpt-[0-9]", workflow)) + self.assertNotIn("PR_REVIEW_MODEL_FAST", workflow) + self.assertNotIn("PR_REVIEW_MODEL_DEEP", workflow) - def test_workflow_keeps_trusted_runner_and_owner_gate(self): + def test_workflow_pins_one_immutable_reviewer_and_gates_on_owner(self): workflow = WORKFLOW_PATH.read_text(encoding="utf-8") self.assertIn("github.event.comment.user.login == 'luckyPipewrench'", workflow) self.assertIn("github.event.comment.author_association == 'OWNER'", workflow) - self.assertIn("ref: ${{ github.event.repository.default_branch }}", workflow) - self.assertIn("persist-credentials: false", workflow) - self.assertIn("LITELLM_BASE_URL: ${{ secrets.LITELLM_BASE_URL }}", workflow) - self.assertIn("LITELLM_API_KEY: ${{ secrets.LITELLM_API_KEY }}", workflow) - self.assertIn("OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }}", workflow) - self.assertIn( - "group: pr-review-${{ github.repository }}-${{ github.event.issue.number }}", workflow - ) - self.assertIn("cancel-in-progress: true", workflow) - self.assertIn("python -m unittest tests/test_pr_review_routing.py", workflow) + + # The pin appears twice and selects two different things: which + # workflow runs, and which reviewer source it runs. A mismatch runs one + # version's workflow against another version's code and reports nothing + # wrong, so equality is the property worth asserting, not presence. + used = re.search(r"pr-review-reusable\.yaml@([0-9a-f]{40})\b", workflow) + declared = re.search(r"reviewer_sha:\s*([0-9a-f]{40})\b", workflow) + self.assertIsNotNone(used, "the reusable workflow must be pinned to a full commit sha") + self.assertIsNotNone(declared, "reviewer_sha must be a full commit sha") + self.assertEqual(used.group(1), declared.group(1)) + + # A branch or tag can move the reviewer code under the pin, so neither + # position may carry one. + self.assertIsNone(re.search(r"pr-review-reusable\.yaml@(?![0-9a-f]{40}\b)\S+", workflow)) + + # A caller may only pass secrets the reusable workflow declares. Passing + # an undeclared one fails at workflow load rather than at review time, + # which presents as the review simply never running. + secrets_block = workflow.split(" secrets:", 1) + self.assertEqual(len(secrets_block), 2, "the caller must map secrets explicitly") + mapped = set(re.findall(r"^ ([a-z_]+):", secrets_block[1], re.MULTILINE)) + self.assertEqual(mapped, {"review_token", "openai_api_key"}) runner = SCRIPT_PATH.read_text(encoding="utf-8") self.assertNotIn("resp.text[:500]", runner)