From a5e5364141f1527d9377f91a13078c6ae83efb44 Mon Sep 17 00:00:00 2001 From: Ildar Nurislamov Date: Fri, 16 Oct 2026 12:32:55 +0400 Subject: [PATCH] feat(ai-review): isolate analysis from validated GitHub publishing --- .github/workflows/ai-review.yml | 409 ++++++++++++++++++++++++-------- README.md | 41 +++- tests/test_review_context.py | 2 +- tests/test_review_publisher.py | 253 ++++++++++++++++++++ 4 files changed, 605 insertions(+), 100 deletions(-) create mode 100644 tests/test_review_publisher.py diff --git a/.github/workflows/ai-review.yml b/.github/workflows/ai-review.yml index 1816e53..a1bb57d 100644 --- a/.github/workflows/ai-review.yml +++ b/.github/workflows/ai-review.yml @@ -44,6 +44,11 @@ on: env: REVIEW_INSTRUCTIONS: ${{ inputs.review_instructions || '' }} +# Serialize the complete analysis/publication pipeline, including duplicate checks. +concurrency: + group: ai-review-pipeline-${{ github.repository }}-${{ github.event.pull_request.number || inputs.pr_number || inputs.commit_sha || github.sha }} + cancel-in-progress: false + jobs: ai-review: if: | @@ -52,17 +57,16 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 25 - # Concurrency key uses the actual subject being reviewed (PR number or commit SHA), - # not github.sha (which is the same for all dispatch runs on the same branch). - # cancel-in-progress: false means a second dispatch for the same subject queues - # rather than cancelling the running review. - concurrency: - group: ai-review-${{ github.event.pull_request.number || inputs.pr_number || inputs.commit_sha || github.sha }} - cancel-in-progress: false - permissions: - contents: write + contents: read pull-requests: read + outputs: + analysis_artifact: ai-review-log-${{ github.run_id }}-${{ github.run_attempt }} + review_mode: ${{ steps.resolve-target.outputs.review_mode }} + pr_number: ${{ steps.resolve-target.outputs.pr_number }} + compare_head: ${{ steps.resolve-target.outputs.compare_head }} + compare_base: ${{ steps.prepare-context.outputs.compare_base }} + subject_url: ${{ steps.resolve-target.outputs.subject_url }} steps: - name: "๐Ÿงญ Resolve review target" @@ -159,8 +163,10 @@ jobs: with: fetch-depth: 0 ref: ${{ steps.resolve-target.outputs.compare_head }} + persist-credentials: false - name: "๐Ÿงพ Prepare review context" + id: prepare-context if: ${{ steps.resolve-target.outputs.review_mode != 'skip' }} env: GH_TOKEN: ${{ github.token }} @@ -175,12 +181,13 @@ jobs: set -euo pipefail mkdir -p .github/codex/context .github/codex/output - git fetch --no-tags origin "$COMPARE_BASE" "$COMPARE_HEAD" + git -c credential.helper= -c 'credential.helper=!gh auth git-credential' fetch --no-tags origin "$COMPARE_BASE" "$COMPARE_HEAD" test "$(git rev-parse HEAD)" = "$COMPARE_HEAD" BASE_TIP="$COMPARE_BASE" if [ "$REVIEW_MODE" = "pr" ]; then COMPARE_BASE=$(git merge-base "$BASE_TIP" "$COMPARE_HEAD") fi + echo "compare_base=$COMPARE_BASE" >> "$GITHUB_OUTPUT" git diff --stat "$COMPARE_BASE" "$COMPARE_HEAD" | tee .github/codex/output/diff-stat.txt git diff --name-only "$COMPARE_BASE" "$COMPARE_HEAD" | tee .github/codex/output/diff-files.txt @@ -247,14 +254,13 @@ jobs: - Intentional tradeoffs are context, not immunity: distinguish an intended behavior change from a defect in its implementation. Execution mode: - - If `review_mode` is `pr`, post a native PR review directly to GitHub. - - If `review_mode` is `commit`, do not post to GitHub; write one markdown file for the workflow to post as a consolidated commit comment. - - GitHub communication: - - Authentication is pre-configured with `GH_TOKEN`. - - You may use GitHub CLI (`gh`) or direct HTTPS requests to GitHub API. - - If using direct HTTPS requests, use `Authorization: Bearer $GH_TOKEN`. - - Choose the method that is most reliable for completing the task. + - Analyze only. Do not post reviews or comments, call GitHub write APIs, or edit code. + - Return one JSON object matching the supplied output schema as your final answer. The action saves it; do not write the output file yourself. + - Set version to 1 and reviewed_sha to the recorded compare_head. + - Set completion_status to complete only after the entire supplied diff has been reviewed. Use incomplete if a timeout, missing context, or other limitation prevented completion, and explain in summary. + - findings may be empty after a completed review. Never use an empty list to conceal an incomplete review. + - Each finding needs priority (P0/P1/P2), title, body, path, side (LEFT for deleted lines or RIGHT for added lines), start_line, and end_line. Use a minimal range of at most 10 changed lines in a single diff hunk. Describe evidence and impact in body. + - Do not include @mentions or HTML publisher markers in text. Review process: @@ -265,63 +271,11 @@ jobs: 5. An empty findings list is valid. If no high-confidence defects remain, say so briefly. Keep feedback concise; include priority, a changed-line location, and an explanation of the failure and impact for each finding. 6. Never call this review "WIP", "draft", or "preliminary". Do not edit source code. - If `review_mode` is `pr`: - 1. Use the prepared thread history to avoid repeating unresolved findings or previously dismissed findings. Re-raise a dismissed finding only if new changes invalidate its dismissal, and explain that new evidence. - 2. Always post the review with `event=APPROVE`. Use this event regardless of how many findings you have โ€” even when you surface bugs, security issues, or regressions. The approval is not a judgment that the code is flawless; it only means the review is not blocking the merge. - 3. Surface all of your findings as inline comments on the relevant lines so the author sees them. Post as many comments as the change warrants โ€” do not hold back findings just because the verdict is APPROVE. Say plainly in each comment when something looks like a bug and why. - 4. Never post `event=REQUEST_CHANGES` or `event=COMMENT`. The verdict is always APPROVE; the findings live in the inline comments and summary, not in the event type. - 5. Do not post questions as a substitute for validating a finding. Apply the same high-confidence evidence standard to every inline comment. - 6. Post the PR review directly to GitHub (do not delegate posting to workflow wrapper logic). Include a short summary and inline comments. The summary should describe what you looked at and list the findings briefly โ€” do not frame it as "blocking" or "approving the merge". - - If `review_mode` is `commit`: - 1. Write `.github/codex/output/review.md`. - 2. Use valid, readable markdown. - 3. Keep it concise and actionable. + Use the prepared thread history to avoid repeating unresolved findings or previously dismissed findings. Re-raise a dismissed finding only if new changes invalidate its dismissal, and explain that new evidence. Never use or mention "WIP". PROMPT_EOF - # PR mode requires a GitHub App with repo access and Pull requests: Read & write. - # Create app: https://github.com/organizations/jitsucom/settings/apps/new - # Install app to org/repo: https://github.com/settings/installations - # App settings page (replace APP_SLUG): https://github.com/settings/apps/APP_SLUG - # Generate private key in app settings, then store these repo/org secrets: - # - AI_CODE_REVIEW_APP_ID (App ID from app settings) - # - AI_CODE_REVIEW_PRIVATE_KEY (contents of downloaded .pem private key) - # Example CLI (org secrets, grant access to selected repos): - # gh secret set AI_CODE_REVIEW_APP_ID --org jitsucom --repos jitsu --body "" - # gh secret set AI_CODE_REVIEW_PRIVATE_KEY --org jitsucom --repos jitsu < app-private-key.pem - - name: "๐Ÿ” Check required secrets" - if: ${{ steps.resolve-target.outputs.review_mode != 'skip' }} - env: - REVIEW_MODE: ${{ steps.resolve-target.outputs.review_mode }} - OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} - AI_CODE_REVIEW_APP_ID: ${{ secrets.AI_CODE_REVIEW_APP_ID }} - AI_CODE_REVIEW_PRIVATE_KEY: ${{ secrets.AI_CODE_REVIEW_PRIVATE_KEY }} - run: | - set -euo pipefail - if [ -z "${OPENAI_API_KEY:-}" ]; then - echo "OPENAI_API_KEY secret is empty or unavailable to this workflow run." >&2 - exit 1 - fi - if [ "$REVIEW_MODE" = "pr" ]; then - if [ -z "${AI_CODE_REVIEW_APP_ID:-}" ] || [ -z "${AI_CODE_REVIEW_PRIVATE_KEY:-}" ]; then - echo "PR mode requires AI_CODE_REVIEW_APP_ID and AI_CODE_REVIEW_PRIVATE_KEY secrets." >&2 - exit 1 - fi - fi - echo "Required secrets are present for mode: $REVIEW_MODE" - - - name: "๐Ÿ”‘ Generate GitHub App token for PR review" - if: ${{ steps.resolve-target.outputs.review_mode == 'pr' }} - id: app-token - uses: actions/create-github-app-token@v2 - with: - app-id: ${{ secrets.AI_CODE_REVIEW_APP_ID }} - private-key: ${{ secrets.AI_CODE_REVIEW_PRIVATE_KEY }} - owner: ${{ github.repository_owner }} - repositories: ${{ github.event.repository.name }} - - name: "๐Ÿค– Run Codex analysis" if: ${{ steps.resolve-target.outputs.review_mode != 'skip' }} id: run-codex @@ -330,7 +284,6 @@ jobs: REVIEW_MODE: ${{ steps.resolve-target.outputs.review_mode }} REPO: ${{ github.repository }} PR_NUMBER: ${{ steps.resolve-target.outputs.pr_number }} - GH_TOKEN: ${{ steps.app-token.outputs.token }} # Pinned to the v1.11 commit, not the floating v1 tag: v1.12 (2026-08-20) # rewrote the Linux launch path and the step never returns after a heavy # turn โ€” every caller's review died on the job timeout with the result @@ -340,7 +293,9 @@ jobs: with: openai-api-key: ${{ secrets.OPENAI_API_KEY }} prompt-file: .github/workflows/ai-review.prompt.md - output-file: .github/codex/output/codex-final-message.md + output-file: .github/codex/output/review.json + output-schema: >- + {"type": "object", "additionalProperties": false, "required": ["version", "reviewed_sha", "completion_status", "summary", "findings"], "properties": {"version": {"type": "integer", "enum": [1]}, "reviewed_sha": {"type": "string"}, "completion_status": {"type": "string", "enum": ["complete", "incomplete"]}, "summary": {"type": "string"}, "findings": {"type": "array", "items": {"type": "object", "additionalProperties": false, "required": ["priority", "title", "body", "path", "side", "start_line", "end_line"], "properties": {"priority": {"type": "string", "enum": ["P0", "P1", "P2"]}, "title": {"type": "string"}, "body": {"type": "string"}, "path": {"type": "string"}, "side": {"type": "string", "enum": ["LEFT", "RIGHT"]}, "start_line": {"type": "integer"}, "end_line": {"type": "integer"}}}}}} model: gpt-5.6-terra effort: high # the CLI floated to npm latest too; pinned to the release verified @@ -357,16 +312,8 @@ jobs: # token, so it is repo-internal automation, not an external actor. allow-bots: true - - name: "โœ… Validate review output" - if: ${{ steps.resolve-target.outputs.review_mode == 'commit' }} - id: validate-review - run: | - set -euo pipefail - test -f .github/codex/output/review.md - test -s .github/codex/output/review.md - - name: "๐Ÿ’ฐ Estimate token usage and cost" - if: ${{ steps.resolve-target.outputs.review_mode != 'skip' }} + if: ${{ always() && steps.resolve-target.outputs.review_mode != 'skip' }} id: estimate-cost env: CODEX_HOME: .github/codex/home @@ -413,7 +360,7 @@ jobs: COST_USD=$(awk -v i="$INPUT_TOKENS" -v ci="$CACHED_INPUT_TOKENS" -v o="$OUTPUT_TOKENS" \ -v pi="$PRICE_INPUT_PER_M" -v pci="$PRICE_CACHED_INPUT_PER_M" -v po="$PRICE_OUTPUT_PER_M" \ 'BEGIN { - cost = (i/1000000.0)*pi + (ci/1000000.0)*pci + (o/1000000.0)*po; + cost = ((i-ci)/1000000.0)*pi + (ci/1000000.0)*pci + (o/1000000.0)*po; printf "%.6f", cost }') @@ -427,18 +374,23 @@ jobs: echo "Token usage: input=$INPUT_TOKENS cached_input=$CACHED_INPUT_TOKENS output=$OUTPUT_TOKENS total=$TOTAL_TOKENS" echo "Estimated run cost (USD): $COST_USD" - - name: "๐Ÿ’ฌ Post commit review comments" - if: ${{ steps.resolve-target.outputs.review_mode == 'commit' }} + - name: "๐Ÿงพ Record analysis status" + if: ${{ always() && steps.resolve-target.outputs.review_mode != 'skip' }} env: - GH_TOKEN: ${{ github.token }} - REPO: ${{ github.repository }} - COMMIT_SHA: ${{ steps.resolve-target.outputs.commit_sha }} + REVIEWED_SHA: ${{ steps.resolve-target.outputs.compare_head }} + ANALYSIS_OUTCOME: ${{ steps.run-codex.outcome }} + USAGE_FOUND: ${{ steps.estimate-cost.outputs.usage_found }} + INPUT_TOKENS: ${{ steps.estimate-cost.outputs.input_tokens }} + CACHED_INPUT_TOKENS: ${{ steps.estimate-cost.outputs.cached_input_tokens }} + OUTPUT_TOKENS: ${{ steps.estimate-cost.outputs.output_tokens }} + ESTIMATED_COST: ${{ steps.estimate-cost.outputs.estimated_cost_usd }} run: | - set -euo pipefail - jq -n --rawfile body .github/codex/output/review.md '{body: $body}' > /tmp/commit-comment.json - gh api "repos/$REPO/commits/$COMMIT_SHA/comments" \ - --method POST \ - --input /tmp/commit-comment.json + mkdir -p .github/codex/output + jq -n '{reviewed_sha:env.REVIEWED_SHA,analysis_outcome:env.ANALYSIS_OUTCOME, + model:"gpt-5.6-terra",effort:"high",codex_version:"0.149.0", + usage_found:env.USAGE_FOUND,input_tokens:env.INPUT_TOKENS, + cached_input_tokens:env.CACHED_INPUT_TOKENS,output_tokens:env.OUTPUT_TOKENS, + estimated_cost_usd:env.ESTIMATED_COST}' > .github/codex/output/analysis-status.json - name: "๐Ÿ“ Write workflow summary" if: always() @@ -449,7 +401,8 @@ jobs: echo "- Subject: ${{ steps.resolve-target.outputs.subject_url }}" echo "- Mode: ${{ steps.resolve-target.outputs.review_mode }}" [ -n "${{ steps.resolve-target.outputs.skip_reason }}" ] && echo "- Skip reason: ${{ steps.resolve-target.outputs.skip_reason }}" - echo "- Review output: review.md" + echo "- Analysis outcome: ${{ steps.run-codex.outcome }}" + echo "- Review output: review.json (validated and published in a separate job)" echo "- Model: ${{ steps.estimate-cost.outputs.model || 'n/a' }}" if [ "${{ steps.estimate-cost.outputs.usage_found || 'false' }}" = "true" ]; then echo "- Token usage: input=${{ steps.estimate-cost.outputs.input_tokens }}, cached_input=${{ steps.estimate-cost.outputs.cached_input_tokens }}, output=${{ steps.estimate-cost.outputs.output_tokens }}, total=${{ steps.estimate-cost.outputs.total_tokens }}" @@ -460,15 +413,275 @@ jobs: fi [ -n "${{ steps.estimate-cost.outputs.price_source }}" ] && echo "- Pricing source: ${{ steps.estimate-cost.outputs.price_source }}" echo - if [ -f .github/codex/output/review.md ]; then - cat .github/codex/output/review.md - fi } >> "$GITHUB_STEP_SUMMARY" - name: "๐Ÿ“ฆ Upload review log" if: ${{ always() && steps.resolve-target.outputs.review_mode != 'skip' }} uses: actions/upload-artifact@v4 with: - name: ai-review-log-${{ github.run_id }} + name: ai-review-log-${{ github.run_id }}-${{ github.run_attempt }} path: .github/codex/output retention-days: 30 + + publish-review: + needs: ai-review + if: ${{ needs.ai-review.result == 'success' && needs.ai-review.outputs.review_mode != 'skip' }} + runs-on: ubuntu-latest + timeout-minutes: 10 + permissions: + contents: write # Commit comments only; analysis has no write permission. + pull-requests: read + env: + REPO: ${{ github.repository }} + REVIEW_MODE: ${{ needs.ai-review.outputs.review_mode }} + PR_NUMBER: ${{ needs.ai-review.outputs.pr_number }} + REVIEW_HEAD: ${{ needs.ai-review.outputs.compare_head }} + REVIEW_BASE: ${{ needs.ai-review.outputs.compare_base }} + REVIEW_REPO: ${{ github.workspace }}/reviewed-repo + REVIEW_FILE: ${{ github.workspace }}/review-artifact/review.json + STATUS_FILE: ${{ github.workspace }}/publication/status.json + + steps: + # Never run scripts from the PR checkout or from the downloaded artifact. + - name: "๐Ÿ“ฅ Checkout reviewed tree for line validation" + uses: actions/checkout@v5.0.0 + with: + ref: ${{ needs.ai-review.outputs.compare_head }} + fetch-depth: 0 + persist-credentials: false + path: reviewed-repo + + - name: "๐Ÿ“ฆ Download analysis output" + uses: actions/download-artifact@v4 + with: + name: ${{ needs.ai-review.outputs.analysis_artifact }} + path: review-artifact + + - name: "๐Ÿ› ๏ธ Write trusted publisher" + run: | + mkdir -p publication + cat > publication/publish.py <<'PYTHON_EOF' + """Validate untrusted review data and publish it; never execute artifact contents.""" + import ast + import hashlib + import json + import os + from pathlib import Path + import re + import subprocess + import sys + + + def api(endpoint, payload=None, paginate=False): + args = ['gh', 'api', endpoint] + if paginate: + args += ['--paginate', '--slurp'] + if payload is not None: + args += ['--method', 'POST', '--input', '-'] + result = subprocess.run(args, input=json.dumps(payload) if payload is not None else None, + text=True, capture_output=True, check=True) + value = json.loads(result.stdout) + return [item for page in value for item in page] if paginate else value + + + def git(*args): + return subprocess.check_output(['git', '--no-pager', *args], cwd=os.environ['REVIEW_REPO'], text=True) + + + def text(value, limit): + if not isinstance(value, str) or not value.strip() or len(value) > limit or '\x00' in value: + raise ValueError('Missing, empty, or oversized review text') + # Prevent review-generated notifications and forged publisher metadata. + if '' + + + def comment_body(f): + return f"[{f['priority']}] {f['title']}\n\n{f['body']}\n\n{marker(f)}" + + + def publish(data, findings, repo, mode, subject, head): + digest = hashlib.sha256(json.dumps(data, sort_keys=True).encode()).hexdigest() + review_marker = f'' + summary = f"Review of `{head}`\n\n{data['summary']}\n\n{review_marker}" + if mode == 'pr': + endpoint = f'repos/{repo}/pulls/{subject}' + comments = api(endpoint + '/comments?per_page=100', paginate=True) + reviews = api(endpoint + '/reviews?per_page=100', paginate=True) + login = os.environ['REVIEW_BOT_LOGIN'] + own_comments = [c for c in comments if (c.get('user') or {}).get('login') == login] + own_reviews = [r for r in reviews if (r.get('user') or {}).get('login') == login] + fresh = [f for f in findings if not any(marker(f) in c['body'] for c in own_comments)] + # Check after fetching history, immediately before any publication. + current = api(endpoint) + if current['state'] != 'open' or current['draft'] or current['head']['sha'] != head: + raise ValueError('PR closed, draft, or changed head; stale review discarded') + if any(review_marker in (r.get('body') or '') for r in own_reviews) or (findings and not fresh): + return 'duplicate', 0 + inline = [] + for f in fresh: + c = {'path': f['path'], 'side': f['side'], 'line': f['end_line'], 'body': comment_body(f)} + if f['start_line'] != f['end_line']: + c.update(start_line=f['start_line'], start_side=f['side']) + inline.append(c) + result = api(endpoint + '/reviews', {'commit_id': head, 'event': 'APPROVE', 'body': summary, 'comments': inline}) + return 'published', len(inline) + endpoint = f'repos/{repo}/commits/{head}/comments' + comments = api(endpoint + '?per_page=100', paginate=True) + if any(review_marker in c['body'] and c['user']['login'] == 'github-actions[bot]' for c in comments): + return 'duplicate', 0 + body = summary + ''.join('\n\n' + comment_body(f) + f"\n\n`{f['path']}:{f['start_line']}-{f['end_line']}` ({f['side']})" for f in findings) + if len(body) > 60000: + raise ValueError('Commit review exceeds comment size limit') + api(endpoint, {'body': body}) + return 'published', len(findings) + + + def main(): + status = {'completion_status': 'failed', 'publication_status': 'not_posted', + 'reviewed_sha': os.environ['REVIEW_HEAD'], 'model': 'gpt-5.6-terra', 'effort': 'high'} + try: + head, base = os.environ['REVIEW_HEAD'], os.environ['REVIEW_BASE'] + if not re.fullmatch(r'[0-9a-f]{40}', head) or not re.fullmatch(r'[0-9a-f]{40}', base): + raise ValueError('Expected resolved commit SHAs') + if git('rev-parse', 'HEAD').strip() != head: + raise ValueError('Publisher checkout differs from reviewed head') + data = json.loads(Path(os.environ['REVIEW_FILE']).read_text()) + findings = validate(data, head, base) + status['completion_status'] = 'complete' + status['validated_findings'] = len(findings) + if os.environ.get('VALIDATE_ONLY') == 'true': + status['publication_status'] = 'validated' + else: + # A failed POST may have reached GitHub; a rerun checks markers. + status['publication_status'] = 'unconfirmed' + state, count = publish(data, findings, os.environ['REPO'], os.environ['REVIEW_MODE'], + os.environ.get('PR_NUMBER', ''), head) + status.update(publication_status=state, posted_findings=count) + except (ValueError, KeyError, TypeError, OSError, subprocess.CalledProcessError) as exc: + # Do not copy untrusted artifact text or raw authenticated subprocess output into logs. + status['error'] = str(exc) if isinstance(exc, ValueError) else type(exc).__name__ + raise + finally: + Path(os.environ['STATUS_FILE']).write_text(json.dumps(status, indent=2) + '\n') + + + if __name__ == '__main__': + main() + PYTHON_EOF + + - name: "โœ… Validate completed review and changed-line locations" + env: + VALIDATE_ONLY: "true" + run: python3 publication/publish.py + + # Mint posting credentials only after validation, on a runner that never ran Codex. + - name: "๐Ÿ”‘ Generate GitHub App token for PR review" + if: ${{ env.REVIEW_MODE == 'pr' }} + id: app-token + uses: actions/create-github-app-token@v2 + with: + app-id: ${{ secrets.AI_CODE_REVIEW_APP_ID }} + private-key: ${{ secrets.AI_CODE_REVIEW_PRIVATE_KEY }} + owner: ${{ github.repository_owner }} + repositories: ${{ github.event.repository.name }} + + - name: "๐Ÿ’ฌ Publish validated review" + env: + GH_TOKEN: ${{ steps.app-token.outputs.token || github.token }} + REVIEW_BOT_LOGIN: ${{ steps.app-token.outputs.app-slug }}[bot] + run: python3 publication/publish.py + + - name: "๐Ÿ“ Write publication summary" + if: always() + run: | + { + echo "## Review publication" + if [ -f publication/status.json ]; then + echo '```json' + cat publication/status.json + echo '```' + else + echo "Publication did not complete; see failed step." + fi + } >> "$GITHUB_STEP_SUMMARY" + + - name: "๐Ÿ“ฆ Upload publication status" + if: always() + uses: actions/upload-artifact@v4 + with: + name: ai-review-publication-${{ github.run_id }}-${{ github.run_attempt }} + path: publication/status.json + retention-days: 30 diff --git a/README.md b/README.md index eb12a5e..228aba0 100644 --- a/README.md +++ b/README.md @@ -12,7 +12,7 @@ using [OpenAI Codex](https://openai.com/codex). - On **pull requests**: posts a native PR review with inline comments via a GitHub App - On **push to main** (commits not part of any PR): posts a review as a commit comment - Skips commits that already belong to an open PR (reviewed there instead) -- Reports token usage and estimated cost in the workflow summary +- Reports reviewed SHAs, completion/publication status, token usage, and estimated cost #### Secrets required @@ -108,6 +108,45 @@ To run the context regression checks locally (Python 3, Git, Bash, and jq requir python3 -m unittest discover -s tests -v ``` +#### Analysis and publication + +Review runs use two separate jobs. `ai-review` has read-only GitHub permissions and +returns a schema-constrained `review.json`; it never receives the GitHub App key or a +posting token. Checkout credentials are not persisted. `publish-review` runs on a fresh +runner and validates the JSON before minting the App token or posting anything. + +The publisher requires an explicit `complete` result for the exact reviewed SHA. Each +finding must have a P0/P1/P2 priority and a path/side/line range within the actual Git diff. +Deleted lines use `LEFT`; added lines use `RIGHT`. Invalid JSON, missing output, incomplete +analysis, invalid locations, and GitHub API failures fail the workflow. A completed empty +review is valid and is posted with `APPROVE`, matching the existing review convention. + +Before posting, the publisher checks that the PR is still open, ready for review, and at +the reviewed head. Stale results are discarded and the publication job fails. Reviews +also specify `commit_id` so any change in the narrow interval between the check and the +POST cannot attach findings to a different commit; GitHub does not offer an atomic +head-check-and-post operation. + +Per-subject concurrency covers both jobs so duplicate checks and posting are serialized. +Finding fingerprints prevent exact duplicate comments from this review bot, including +when lines move. Identical review results are idempotent on reruns. Existing legacy +comments and semantic duplicates are handled by the analysis prompt's thread history; +fingerprint checks do not attempt to infer whether differently worded findings describe +the same bug. Commit mode posts one consolidated comment, with rerun deduplication. + +Artifacts are named with both run ID and run attempt. The publisher uses the artifact +name exported by analysis, including when only failed jobs are rerun: + +- `ai-review-log-*`: `review.json` when available, diff files/statistics, and + `analysis-status.json` with the analysis outcome, pinned model/effort/runtime, and usage. +- `ai-review-publication-*`: `status.json` recording validation/publication outcome and + reviewed SHA, including failures. An unsuccessful API request leaves publication + unconfirmed, since a failed POST may still have reached GitHub; reruns check markers + before posting again. Publication is never inferred from an APPROVE badge. + +Only data is read from the analysis artifact; publisher code comes from the workflow. +The model, reasoning effort, and CLI/action pins remain unchanged by this separation. + #### Versioning Every merge to `main` is tagged `1..` by diff --git a/tests/test_review_context.py b/tests/test_review_context.py index c856dd7..c1c94d4 100644 --- a/tests/test_review_context.py +++ b/tests/test_review_context.py @@ -72,7 +72,7 @@ def prepare(self, mode='pr', **extra): COMPARE_BASE=self.base if mode == 'pr' else self.common, COMPARE_HEAD=self.head, SUBJECT_URL='https://example.com/pr/7', REVIEW_INSTRUCTIONS='Focus on routing.\nDo not invent requirements.', - REPO='owner/repo', **extra) + REPO='owner/repo', GITHUB_OUTPUT=str(self.path / 'outputs'), **extra) return subprocess.run(['bash', '-e', '-c', run_block('๐Ÿงพ Prepare review context')], cwd=self.repo, env=env, capture_output=True, text=True) diff --git a/tests/test_review_publisher.py b/tests/test_review_publisher.py new file mode 100644 index 0000000..7e36401 --- /dev/null +++ b/tests/test_review_publisher.py @@ -0,0 +1,253 @@ +"""Validate the actual inline publisher against Git fixtures and mocked GitHub APIs.""" +import importlib.util +import json +import os +from pathlib import Path +import subprocess +import tempfile +import unittest +from unittest.mock import patch + +from test_review_context import run_block, WORKFLOW + + +def load_publisher(path): + script = run_block('๐Ÿ› ๏ธ Write trusted publisher') + subprocess.run(['bash', '-e', '-c', script], cwd=path, check=True) + spec = importlib.util.spec_from_file_location('publisher', path / 'publication/publish.py') + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +class PublisherTest(unittest.TestCase): + def setUp(self): + self.temp = tempfile.TemporaryDirectory() + self.addCleanup(self.temp.cleanup) + self.path = Path(self.temp.name) + self.p = load_publisher(self.path) + self.repo = self.path / 'repo' + self.repo.mkdir() + self.git('init', '-b', 'main') + self.git('config', 'user.name', 'Test') + self.git('config', 'user.email', 'test@example.com') + (self.repo / 'file with spaces.txt').write_text('keep\nold\nlast\n') + self.git('add', '.') + self.git('commit', '-m', 'base') + self.base = self.git('rev-parse', 'HEAD') + (self.repo / 'file with spaces.txt').write_text('keep\nnew\nadded\nlast\n') + self.git('add', '.') + self.git('commit', '-m', 'change') + self.head = self.git('rev-parse', 'HEAD') + self.env = patch.dict(os.environ, REVIEW_REPO=str(self.repo), REVIEW_HEAD=self.head, + REVIEW_BASE=self.base, REPO='owner/repo', REVIEW_MODE='pr', + PR_NUMBER='7', REVIEW_FILE=str(self.path / 'review.json'), + STATUS_FILE=str(self.path / 'publication/status.json'), + REVIEW_BOT_LOGIN='review-bot[bot]') + self.env.start() + self.addCleanup(self.env.stop) + self.data = {'version': 1, 'reviewed_sha': self.head, 'completion_status': 'complete', + 'summary': 'Reviewed the changed path.', 'findings': []} + self.f = {'priority': 'P1', 'title': 'Preserve validation', 'body': 'Bad input now reaches the destination.', + 'path': 'file with spaces.txt', 'side': 'RIGHT', 'start_line': 2, 'end_line': 3} + self.posts = [] + self.history = [] + self.reviews = [] + self.live = {'head': {'sha': self.head}, 'state': 'open', 'draft': False} + self.calls = [] + + def git(self, *args): + return subprocess.check_output(['git', *args], cwd=self.repo, text=True, + stderr=subprocess.DEVNULL).strip() + + def api(self, endpoint, payload=None, paginate=False): + self.calls.append(endpoint) + if payload is not None: + self.posts.append((endpoint, payload)) + return {'id': 1} + if '/comments?' in endpoint: + return self.history + if '/reviews?' in endpoint: + return self.reviews + return self.live + + def findings(self): + self.data['findings'] = [dict(self.f)] + return self.p.validate(self.data, self.head, self.base) + + def publish(self, mode='pr'): + return self.p.publish(self.data, self.p.validate(self.data, self.head, self.base), + 'owner/repo', mode, '7', self.head) + + def test_valid_right_and_deleted_left_lines(self): + self.assertEqual(len(self.findings()), 1) + self.data['findings'][0].update(side='LEFT', start_line=2, end_line=2) + self.assertEqual(len(self.p.validate(self.data, self.head, self.base)), 1) + + def test_rename_keeps_old_and_new_line_coordinates(self): + (self.repo / 'old.txt').write_text(''.join(f'line {n}\n' for n in range(30))) + self.git('add', '.') + self.git('commit', '-m', 'before rename') + base = self.git('rev-parse', 'HEAD') + self.git('mv', 'old.txt', 'new.txt') + path = self.repo / 'new.txt' + path.write_text(path.read_text().replace('line 10\n', 'changed\n')) + self.git('add', '.') + self.git('commit', '-m', 'rename and edit') + head = self.git('rev-parse', 'HEAD') + self.data.update(reviewed_sha=head, findings=[dict(self.f, path='new.txt', side='LEFT', start_line=11, end_line=11)]) + self.assertEqual(len(self.p.validate(self.data, head, base)), 1) + self.data['findings'][0].update(side='RIGHT', start_line=1, end_line=1) + with self.assertRaises(ValueError): + self.p.validate(self.data, head, base) + + def test_quoted_paths_and_header_like_source_text(self): + path = self.repo / 'quoted"name.txt' + path.write_text('old\n') + self.git('add', '.') + self.git('commit', '-m', 'quoted path') + base = self.git('rev-parse', 'HEAD') + path.write_text('++ header-like source\n') + self.git('add', '.') + self.git('commit', '-m', 'edit quoted path') + head = self.git('rev-parse', 'HEAD') + self.data.update(reviewed_sha=head, findings=[dict(self.f, path=path.name, start_line=1, end_line=1)]) + self.assertEqual(len(self.p.validate(self.data, head, base)), 1) + + def test_invalid_output_is_never_published(self): + cases = [None, {}, dict(self.data, completion_status='incomplete'), + dict(self.data, reviewed_sha=self.base), dict(self.data, version=True)] + for data in cases: + with self.subTest(data=data): + Path(os.environ['REVIEW_FILE']).write_text(json.dumps(data)) + with patch.object(self.p, 'api', self.api), self.assertRaises(ValueError): + self.p.main() + self.assertEqual(self.posts, []) + self.assertEqual(json.loads(Path(os.environ['STATUS_FILE']).read_text())['publication_status'], 'not_posted') + + def test_invalid_paths_ranges_mentions_and_extra_fields(self): + changes = [{'path': '../secrets'}, {'path': 'unchanged.txt'}, {'start_line': 1}, + {'end_line': 4}, {'start_line': True}, {'side': 'OTHER'}, + {'priority': 'P3'}, {'body': 'Please notify @maintainer'}, + {'body': ''}, {'extra': 'unknown'}] + for change in changes: + with self.subTest(change=change), self.assertRaises(ValueError): + self.data['findings'] = [dict(self.f, **change)] + self.p.validate(self.data, self.head, self.base) + + def test_complete_empty_review_posts_approve_bound_to_sha(self): + with patch.object(self.p, 'api', self.api): + self.assertEqual(self.publish(), ('published', 0)) + payload = self.posts[0][1] + self.assertEqual(payload['event'], 'APPROVE') + self.assertEqual(payload['commit_id'], self.head) + self.assertEqual(payload['comments'], []) + self.assertIn(self.head, payload['body']) + self.assertTrue(self.calls[-2].endswith('/pulls/7')) + + def test_inline_multiline_and_single_line_comments(self): + self.findings() + with patch.object(self.p, 'api', self.api): + self.publish() + comment = self.posts[-1][1]['comments'][0] + self.assertEqual(comment['start_line'], 2) + self.assertEqual(comment['line'], 3) + self.assertIn('ai-review-finding:', comment['body']) + self.data['findings'][0]['end_line'] = 2 + with patch.object(self.p, 'api', self.api): + self.publish() + self.assertNotIn('start_line', self.posts[-1][1]['comments'][0]) + + def test_duplicates_with_shifted_lines_and_other_authors(self): + f = self.findings()[0] + self.data['findings'].append(dict(self.f)) + self.assertEqual(len(self.p.validate(self.data, self.head, self.base)), 1) + self.history = [{'user': {'login': 'review-bot[bot]'}, 'body': self.p.comment_body(f), 'line': 999}] + with patch.object(self.p, 'api', self.api): + self.assertEqual(self.publish(), ('duplicate', 0)) + self.assertFalse(self.posts) + self.history[0]['user']['login'] = 'another-user' + with patch.object(self.p, 'api', self.api): + self.assertEqual(self.publish(), ('published', 1)) + + def test_rerun_of_identical_empty_review_is_idempotent(self): + with patch.object(self.p, 'api', self.api): + self.publish() + self.reviews = [{'user': {'login': 'review-bot[bot]'}, 'body': self.posts[-1][1]['body']}] + self.assertEqual(self.publish(), ('duplicate', 0)) + self.assertEqual(len(self.posts), 1) + + def test_stale_closed_or_draft_pr_never_posts(self): + for update in [{'head': {'sha': self.base}}, {'state': 'closed'}, {'draft': True}]: + with self.subTest(update=update), patch.object(self.p, 'api', self.api), self.assertRaises(ValueError): + self.live = dict(head={'sha': self.head}, state='open', draft=False) + self.live.update(update) + self.publish() + self.assertFalse(self.posts) + + def test_api_failure_and_ambiguous_post_fail_closed(self): + Path(os.environ['REVIEW_FILE']).write_text(json.dumps(self.data)) + for endpoint in ['history', 'post']: + def failing_api(url, payload=None, paginate=False): + if endpoint == 'history' or payload is not None: + raise subprocess.CalledProcessError(1, ['gh', 'api']) + return self.api(url, payload, paginate) + with self.subTest(endpoint=endpoint), patch.object(self.p, 'api', failing_api), self.assertRaises(subprocess.CalledProcessError): + self.p.main() + status = json.loads(Path(os.environ['STATUS_FILE']).read_text()) + self.assertEqual(status['publication_status'], 'unconfirmed') + + def test_validation_only_needs_no_github_token_or_api(self): + Path(os.environ['REVIEW_FILE']).write_text(json.dumps(self.data)) + with patch.dict(os.environ, VALIDATE_ONLY='true'), patch.object(self.p, 'api') as api: + self.p.main() + api.assert_not_called() + self.assertEqual(json.loads(Path(os.environ['STATUS_FILE']).read_text())['publication_status'], 'validated') + + def test_commit_review_is_consolidated_and_idempotent(self): + self.findings() + with patch.object(self.p, 'api', self.api): + self.assertEqual(self.publish('commit'), ('published', 1)) + body = self.posts[-1][1]['body'] + self.history = [{'body': body, 'user': {'login': 'github-actions[bot]'}}] + self.assertEqual(self.publish('commit'), ('duplicate', 0)) + self.assertIn('/commits/' + self.head + '/comments', self.posts[0][0]) + + def test_missing_and_malformed_output_fail_without_posting(self): + for contents in (None, '{broken'): + if contents is not None: + Path(os.environ['REVIEW_FILE']).write_text(contents) + with self.subTest(contents=contents), patch.object(self.p, 'api', self.api), self.assertRaises((OSError, ValueError)): + self.p.main() + self.assertFalse(self.posts) + self.assertEqual(json.loads(Path(os.environ['STATUS_FILE']).read_text())['completion_status'], 'failed') + + def test_api_pagination_is_flattened_and_post_uses_stdin(self): + response = subprocess.CompletedProcess([], 0, stdout='[[{"id":1}],[{"id":2}]]') + with patch.object(self.p.subprocess, 'run', return_value=response) as run: + self.assertEqual(self.p.api('repos/owner/repo/comments', paginate=True), [{'id': 1}, {'id': 2}]) + self.assertIn('--paginate', run.call_args.args[0]) + response.stdout = '{"id":3}' + with patch.object(self.p.subprocess, 'run', return_value=response) as run: + self.p.api('repos/owner/repo/comments', {'body': 'literal $(text)'}) + self.assertEqual(json.loads(run.call_args.kwargs['input']), {'body': 'literal $(text)'}) + self.assertEqual(run.call_args.args[0][-2:], ['--input', '-']) + + def test_posting_credentials_are_isolated_from_analysis(self): + text = WORKFLOW.read_text() + analysis, publisher = text.split('\n publish-review:', 1) + self.assertNotIn('contents: write', analysis) + self.assertNotIn('steps.app-token.outputs.token', analysis) + # Secret declarations are interface-only; no analysis step consumes them. + self.assertNotIn('${{ secrets.AI_CODE_REVIEW_PRIVATE_KEY }}', analysis) + self.assertIn('persist-credentials: false', analysis) + self.assertIn('output-schema:', analysis) + self.assertIn('name: ${{ needs.ai-review.outputs.analysis_artifact }}', publisher) + self.assertIn('analysis_artifact: ai-review-log-${{ github.run_id }}-${{ github.run_attempt }}', analysis) + self.assertIn('\nconcurrency:', text) + self.assertLess(publisher.index('โœ… Validate completed review'), publisher.index('uses: actions/create-github-app-token')) + self.assertNotIn('run: ./', publisher) + + +if __name__ == '__main__': + unittest.main()